From be2bf29e452e706ae47d042e227a463df36e28e4 Mon Sep 17 00:00:00 2001 From: Kevin Date: Mon, 20 Jul 2026 10:22:28 -0400 Subject: [PATCH] PROD-2309: guard against unresolved content references before push, preserve server exception type in errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Linked/nested content refs with no source→target mapping were shipped with the stale source contentID, which the server dereferences as null and throws a NullReferenceException surfaced to operators as an opaque "Object reference not set to an instance of an object." Skip such items before the API call with a precise reason (STEP 3.5 in content-batch-processor), and stop stripping the exception type/server method out of batch error messages so a server-side crash is distinguishable from a plain validation failure. --- src/lib/pushers/batch-polling.ts | 71 +++++++++++++++--- .../content-pusher/content-batch-processor.ts | 26 ++++++- .../util/has-unresolved-content-references.ts | 62 ++++++++++++++++ .../has-unresolved-content-references.test.ts | 54 +++++++++++++- src/lib/pushers/tests/batch-polling.test.ts | 73 ++++++++++++++++++- 5 files changed, 271 insertions(+), 15 deletions(-) diff --git a/src/lib/pushers/batch-polling.ts b/src/lib/pushers/batch-polling.ts index 005aa2f8..98e67718 100644 --- a/src/lib/pushers/batch-polling.ts +++ b/src/lib/pushers/batch-polling.ts @@ -37,11 +37,11 @@ function extractErrorMessage(errorText: string): string { } return errorText; } catch { - // Not JSON, return as-is but truncate if too long - // Also extract message from exception format: "ExceptionType: Message" - const exceptionMatch = errorText.match(/^[\w.]+Exception:\s*(.+?)(?:\r?\n|$)/); - if (exceptionMatch) { - return exceptionMatch[1].trim(); + // Not JSON. If it's a .NET exception dump, keep the exception type + server method + // (prettyException) instead of stripping them; otherwise return as-is, truncated if long. + const pretty = prettyException(errorText); + if (pretty) { + return pretty; } return errorText.length > 200 ? errorText.substring(0, 200) + "..." : errorText; } @@ -75,7 +75,9 @@ function logBatchErrors(batchStatus: CompletedBatch, originalPayloads?: any[]): referenceName = payload?.properties?.referenceName || payload?.referenceName || "unknown"; } - console.error(ansiColors.red(` ✗ ${itemType} ${batchItemId} (${referenceName}): ${errorMessage}`)); + console.error( + ansiColors.red(` ✗ ${itemType} ${batchItemId} (${referenceName}): ${formatBatchItemError(errorType, errorMessage)}`) + ); }); return; } @@ -325,7 +327,8 @@ function extractBatchResultsImpl(batch: CompletedBatch, originalItems: T[]): batch.failedItems.length > 0 && batch.failedItems.find((fi) => fi.batchItemId == item.batchItemID); if (newApiFailedItem) { - errorMsg = newApiFailedItem.errorMessage; + // Preserve the exception type (e.g. NullReferenceException) alongside the message. + errorMsg = formatBatchItemError(newApiFailedItem.errorType, newApiFailedItem.errorMessage); } failedItems.push({ originalItem, newItem: null, error: errorMsg, index }); @@ -349,11 +352,55 @@ export function extractPageBatchResults( return extractBatchResultsImpl(batch, originalItems); } -export function prettyException(error: string) { - // TODO: regex out the exception type and message - // Item -1 failed with error: Agility.Shared.Exceptions.ManagementValidationException: The maximum length for the Message field is 1500 characters. - // at Agility.Shared.Engines.BatchProcessing.BatchInsertContentitem(String languageCode, BatchImportContentItem batchImportContentItem) in D:\a\_work\1\s\Agility CMS 2014\Agility.Shared\Engines\BatchProcessing\BatchProcessing_InsertContentItem.cs:line 398 - // at Agility.Shared.Engines.BatchProcessing.BatchInsertContent(Batch batch) in D:\a\_work\1\s\Agility CMS 2014\Agility.Shared\Engines\BatchProcessing\BatchProcessing.cs:line 1212 +/** + * Turn a raw .NET exception dump into a concise, operator-facing one-liner that PRESERVES the + * exception type (so a server-side crash is distinguishable from a validation message) and, when + * available, the server method that threw. Returns "" when the text isn't a recognizable + * ".NET exception" so callers can fall back to their own handling. + * + * Example input: + * "System.NullReferenceException: Object reference not set to an instance of an object. + * at Agility.Shared.Engines.BatchProcessing.BatchInsertContentitem(String languageCode, ...) in D:\...:line 398 + * at Agility.Shared.Engines.BatchProcessing.BatchInsertContent(Batch batch) in D:\...:line 1212" + * Example output: + * "NullReferenceException: Object reference not set to an instance of an object. [server: BatchInsertContentitem]" + */ +export function prettyException(error: string): string { + if (!error) return ""; + + // ": " — message may span to the first newline. + const exceptionMatch = error.match(/([\w.]*Exception):\s*([\s\S]+?)(?:\r?\n|$)/); + if (!exceptionMatch) return ""; + + const shortType = exceptionMatch[1].split(".").pop() || exceptionMatch[1]; + const message = exceptionMatch[2].trim(); + + // First stack frame's method name, e.g. "...BatchProcessing.BatchInsertContentitem(" → "BatchInsertContentitem". + const frameMatch = error.match(/\bat\s+[\w.]+\.([A-Za-z_][\w`]*)\s*\(/); + const serverMethod = frameMatch ? frameMatch[1] : null; + + return serverMethod ? `${shortType}: ${message} [server: ${serverMethod}]` : `${shortType}: ${message}`; +} + +/** + * Combine a batch item's structured errorType + errorMessage (from the new API) into a single + * operator-facing string. Keeps the exception type visible so a server-side null-dereference + * ("Object reference not set to an instance of an object.") is no longer indistinguishable from a + * plain validation failure. Falls back gracefully when either field is missing. (PROD-2309) + */ +export function formatBatchItemError(errorType?: string, errorMessage?: string): string { + const raw = (errorMessage || "").trim(); + + // If the message itself is a full .NET exception dump, let prettyException surface type + method. + const pretty = prettyException(raw); + if (pretty) return pretty; + + const shortType = (errorType || "").split(".").pop()?.trim() || ""; + // Prefix with the exception type when it adds signal (not a generic "Error", not already in the message). + if (shortType && shortType.toLowerCase() !== "error" && !raw.toLowerCase().includes(shortType.toLowerCase())) { + return raw ? `${shortType}: ${raw}` : shortType; + } + return raw || shortType || "Unknown error"; } /** diff --git a/src/lib/pushers/content-pusher/content-batch-processor.ts b/src/lib/pushers/content-pusher/content-batch-processor.ts index 4744e9b7..da431522 100644 --- a/src/lib/pushers/content-pusher/content-batch-processor.ts +++ b/src/lib/pushers/content-pusher/content-batch-processor.ts @@ -17,6 +17,7 @@ import { buildDeletedLinkedListMessage, } from "./util/detect-deleted-linked-list-references"; import { fileOperations } from "core/fileOperations"; +import { collectUnresolvedContentReferences } from "./util/has-unresolved-content-references"; import { Logs } from "core/logs"; import { state } from "core/state"; /****** @@ -376,6 +377,29 @@ export class ContentBatchProcessor { const targetContainer = containerMapper.getMappedEntity(containerMapping, "target"); + // STEP 3.5: Guard against unresolved content references (PROD-2309). + // If a linked/nested content reference has no source→target mapping (e.g. a stale or + // incomplete mapping), the field mapper leaves the SOURCE contentID in the payload; the + // server's batch engine then dereferences a non-existent item and throws a + // NullReferenceException, which reaches the operator as an opaque + // "Object reference not set to an instance of an object." Skip the item here with a + // precise, actionable reason instead of shipping a payload that is guaranteed to fail. + const unresolvedRefs = collectUnresolvedContentReferences( + contentItem.fields || {}, + this.config.referenceMapper + ); + if (unresolvedRefs.length > 0) { + const detail = unresolvedRefs + .slice(0, 5) + .map((r) => `${r.path} → source contentID ${r.contentID}`) + .join("; "); + const more = unresolvedRefs.length > 5 ? ` (+${unresolvedRefs.length - 5} more)` : ""; + throw new Error( + `Unresolved content reference(s) not present in the target mapping: ${detail}${more}. ` + + `Skipping to avoid a server-side NullReferenceException.` + ); + } + // STEP 4: Check if content already exists using reference mapper (since filtering already happened) const existingMapping = this.config.referenceMapper.getContentItemMappingByContentID( contentItem.contentID, @@ -471,7 +495,7 @@ export class ContentBatchProcessor { } catch (error: any) { console.error( ansiColors.yellow( - `✗ Orphaned content item ${contentItem.contentID}, skipping - ${error.message || "payload preparation failed"}.` + `✗ Skipping content item ${contentItem.contentID} (${contentItem.properties?.referenceName ?? "unknown"}) - ${error.message || "payload preparation failed"}` ) ); diff --git a/src/lib/pushers/content-pusher/util/has-unresolved-content-references.ts b/src/lib/pushers/content-pusher/util/has-unresolved-content-references.ts index f65d2f77..cebb906b 100644 --- a/src/lib/pushers/content-pusher/util/has-unresolved-content-references.ts +++ b/src/lib/pushers/content-pusher/util/has-unresolved-content-references.ts @@ -43,3 +43,65 @@ export function hasUnresolvedContentReferences(obj: any, referenceMapper: Conten return false; } + +export interface UnresolvedContentReference { + /** Dotted/indexed path to the offending value within the item's fields (e.g. "menuItems[0].contentid"). */ + path: string; + /** The source contentID that has no source→target mapping. */ + contentID: number; +} + +/** + * Recursively collect every unresolved content reference (a contentID / sortids value with + * no source→target mapping) along with the field path where it occurs. + * + * Unlike hasUnresolvedContentReferences (which early-exits with a boolean), this walks the + * whole structure so callers can report exactly which field/reference is unmapped. Only + * positive IDs are considered — 0 / -1 mean "no reference selected" and are ignored so we + * don't over-skip items with intentionally empty linked-content fields. + */ +export function collectUnresolvedContentReferences( + obj: any, + referenceMapper: ContentItemMapper, + path = "" +): UnresolvedContentReference[] { + const results: UnresolvedContentReference[] = []; + if (typeof obj !== "object" || obj === null) { + return results; + } + + if (Array.isArray(obj)) { + obj.forEach((item, index) => { + results.push(...collectUnresolvedContentReferences(item, referenceMapper, `${path}[${index}]`)); + }); + return results; + } + + for (const [key, value] of Object.entries(obj)) { + const childPath = path ? `${path}.${key}` : key; + + // Direct content reference (contentid / contentID) + if ((key === "contentid" || key === "contentID") && typeof value === "number") { + if (value > 0 && !referenceMapper.getContentItemMappingByContentID(value, "source")) { + results.push({ path: childPath, contentID: value }); + } + continue; + } + + // Comma-separated content IDs in sortids fields + if (key === "sortids" && typeof value === "string") { + for (const contentIdStr of value.split(",")) { + const contentId = parseInt(contentIdStr.trim()); + if (!isNaN(contentId) && contentId > 0 && !referenceMapper.getContentItemMappingByContentID(contentId, "source")) { + results.push({ path: childPath, contentID: contentId }); + } + } + continue; + } + + // Recurse into nested objects/arrays + results.push(...collectUnresolvedContentReferences(value, referenceMapper, childPath)); + } + + return results; +} diff --git a/src/lib/pushers/content-pusher/util/tests/has-unresolved-content-references.test.ts b/src/lib/pushers/content-pusher/util/tests/has-unresolved-content-references.test.ts index fe038137..d077ccdf 100644 --- a/src/lib/pushers/content-pusher/util/tests/has-unresolved-content-references.test.ts +++ b/src/lib/pushers/content-pusher/util/tests/has-unresolved-content-references.test.ts @@ -1,5 +1,8 @@ import { resetState } from "core/state"; -import { hasUnresolvedContentReferences } from "../has-unresolved-content-references"; +import { + hasUnresolvedContentReferences, + collectUnresolvedContentReferences, +} from "../has-unresolved-content-references"; beforeEach(() => { resetState(); @@ -170,3 +173,52 @@ describe("hasUnresolvedContentReferences — combination cases", () => { expect(mapper.getContentItemMappingByContentID).toHaveBeenCalledTimes(1); }); }); + +// ─── collectUnresolvedContentReferences ──────────────────────────────────────── + +describe("collectUnresolvedContentReferences", () => { + it("returns an empty array when there are no references", () => { + expect(collectUnresolvedContentReferences({ title: "Hello" }, makeMapper(false))).toEqual([]); + }); + + it("returns an empty array for non-object input", () => { + expect(collectUnresolvedContentReferences(null, makeMapper(false))).toEqual([]); + expect(collectUnresolvedContentReferences("x", makeMapper(false))).toEqual([]); + }); + + it("collects an unresolved contentid with its field path", () => { + const result = collectUnresolvedContentReferences({ link: { contentid: 5 } }, makeMapper(false)); + expect(result).toEqual([{ path: "link.contentid", contentID: 5 }]); + }); + + it("collects a nested contentID with a dotted path", () => { + const result = collectUnresolvedContentReferences({ nested: { deeper: { contentID: 77 } } }, makeMapper(false)); + expect(result).toEqual([{ path: "nested.deeper.contentID", contentID: 77 }]); + }); + + it("uses array index notation in the path", () => { + const mapper = makePartialMapper([1]); + const result = collectUnresolvedContentReferences({ items: [{ contentid: 1 }, { contentid: 99 }] }, mapper); + expect(result).toEqual([{ path: "items[1].contentid", contentID: 99 }]); + }); + + it("collects every unresolved sortid (does not early-exit)", () => { + const mapper = makePartialMapper([2]); + const result = collectUnresolvedContentReferences({ list: { sortids: "1,2,3" } }, mapper); + expect(result).toEqual([ + { path: "list.sortids", contentID: 1 }, + { path: "list.sortids", contentID: 3 }, + ]); + }); + + it("ignores non-positive IDs (0 / -1 = no reference selected)", () => { + const mapper = makeMapper(false); + expect(collectUnresolvedContentReferences({ a: { contentid: 0 }, b: { contentID: -1 } }, mapper)).toEqual([]); + expect(mapper.getContentItemMappingByContentID).not.toHaveBeenCalled(); + }); + + it("returns an empty array when all references resolve", () => { + const result = collectUnresolvedContentReferences({ link: { contentid: 5 }, list: { sortids: "1,2" } }, makeMapper(true)); + expect(result).toEqual([]); + }); +}); diff --git a/src/lib/pushers/tests/batch-polling.test.ts b/src/lib/pushers/tests/batch-polling.test.ts index 0e7b6225..30e748d4 100644 --- a/src/lib/pushers/tests/batch-polling.test.ts +++ b/src/lib/pushers/tests/batch-polling.test.ts @@ -1,5 +1,12 @@ import { resetState } from "core/state"; -import { extractContentBatchResults, extractPageBatchResults, logBatchError, CompletedBatch } from "../batch-polling"; +import { + extractContentBatchResults, + extractPageBatchResults, + logBatchError, + prettyException, + formatBatchItemError, + CompletedBatch, +} from "../batch-polling"; import * as mgmtApi from "@agility/management-sdk"; const asBatch = (obj: Record): CompletedBatch => obj as CompletedBatch; @@ -302,3 +309,67 @@ describe("logBatchError", () => { expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining("Original Payload")); }); }); + +// ─── prettyException ─────────────────────────────────────────────────────────── + +describe("prettyException", () => { + it("returns '' for empty/non-exception text", () => { + expect(prettyException("")).toBe(""); + expect(prettyException("just a plain message")).toBe(""); + }); + + it("keeps the (short) exception type and message", () => { + expect(prettyException("System.NullReferenceException: Object reference not set to an instance of an object.")).toBe( + "NullReferenceException: Object reference not set to an instance of an object." + ); + }); + + it("appends the server method from the first stack frame", () => { + const dump = [ + "System.NullReferenceException: Object reference not set to an instance of an object.", + " at Agility.Shared.Engines.BatchProcessing.BatchInsertContentitem(String languageCode, BatchImportContentItem b) in D:\\x.cs:line 398", + " at Agility.Shared.Engines.BatchProcessing.BatchInsertContent(Batch batch) in D:\\y.cs:line 1212", + ].join("\n"); + expect(prettyException(dump)).toBe( + "NullReferenceException: Object reference not set to an instance of an object. [server: BatchInsertContentitem]" + ); + }); + + it("strips the namespace from the exception type", () => { + expect(prettyException("Agility.Shared.Exceptions.ManagementValidationException: too long")).toBe( + "ManagementValidationException: too long" + ); + }); +}); + +// ─── formatBatchItemError ────────────────────────────────────────────────────── + +describe("formatBatchItemError", () => { + it("prefixes the exception type when the message lacks it", () => { + expect(formatBatchItemError("NullReferenceException", "Object reference not set to an instance of an object.")).toBe( + "NullReferenceException: Object reference not set to an instance of an object." + ); + }); + + it("does not duplicate a type already present in the message", () => { + expect(formatBatchItemError("ValidationException", "ValidationException: bad field")).toBe( + "ValidationException: bad field" + ); + }); + + it("ignores a generic 'Error' type", () => { + expect(formatBatchItemError("Error", "something broke")).toBe("something broke"); + }); + + it("prefers a full exception dump in the message via prettyException", () => { + const dump = + "System.NullReferenceException: Object reference not set to an instance of an object.\n at Foo.Bar.Baz(String x)"; + expect(formatBatchItemError("NullReferenceException", dump)).toBe( + "NullReferenceException: Object reference not set to an instance of an object. [server: Baz]" + ); + }); + + it("falls back to 'Unknown error' when nothing is provided", () => { + expect(formatBatchItemError(undefined, undefined)).toBe("Unknown error"); + }); +});