diff --git a/src/core/json-summary.ts b/src/core/json-summary.ts index 0969a6ee..d170bc3b 100644 --- a/src/core/json-summary.ts +++ b/src/core/json-summary.ts @@ -40,6 +40,10 @@ export interface JsonSummaryDetail { mediaID?: number; guid?: string; locale?: string; + /** PROD-2542: model of a content item that failed client-side validation. */ + model?: string; + /** PROD-2542: the required fields it was missing, as "name (Type)". */ + fields?: string[]; } /** @@ -71,16 +75,18 @@ export interface JsonSummaryPreflightPhase { update: number; skip: number; conflict: number; + /** PROD-2542: would-be writes that fail validation against the target model. */ + invalid: number; } export interface JsonSummaryPreflight { - totals: { create: number; update: number; skip: number; conflict: number }; + totals: { create: number; update: number; skip: number; conflict: number; invalid: number }; hasConflicts: boolean; phases: JsonSummaryPreflightPhase[]; /** Every recorded action, in the order the pushers recorded them. */ entries: Array<{ phase: string; - action: "create" | "update" | "skip" | "conflict"; + action: "create" | "update" | "skip" | "conflict" | "invalid"; name: string; locale?: string; detail?: string; diff --git a/src/core/push.ts b/src/core/push.ts index c0b48bc3..19e544c9 100644 --- a/src/core/push.ts +++ b/src/core/push.ts @@ -5,6 +5,7 @@ import { state, clearFailedContentRegistry, clearHeldBackTemplateRegistry, + clearEffectiveTargetModels, getPageCmsLink, getContentCmsLink, } from "./state"; @@ -56,6 +57,8 @@ export class Push { // Clear failed content and held-back template registries from any previous sync clearFailedContentRegistry(); clearHeldBackTemplateRegistry(); + // PROD-2542: models saved by a previous run must not stand in for this run's target models + clearEffectiveTargetModels(); // Preflight (PROD-2203): start each run with a clean report of planned actions preflightReport.reset(); diff --git a/src/core/state.ts b/src/core/state.ts index 68ecf282..6367c63b 100644 --- a/src/core/state.ts +++ b/src/core/state.ts @@ -107,8 +107,15 @@ export interface State { mappingPair?: { sourceGuid: string; targetGuid: string }; // Failed content registry - tracks content items that failed during sync - // Used by page pusher to provide better error messages when content mappings are missing - failedContentRegistry: Map; + // Used by page pusher to provide better error messages when content mappings are missing. + // Keyed by `${locale}:${contentID}` (PROD-2542) so a failure in one locale never labels another. + failedContentRegistry: Map; + + // Effective target models (PROD-2542) - the model definition content will actually be validated + // against on the target, keyed by lower-cased SOURCE model referenceName. Filled by the model + // pusher on a saved create/update (and with the source model in preflight), because the target + // models/.json cache is not rewritten after saveModel and is stale for the rest of the run. + effectiveTargetModels: Map; // Held-back template registry - source template names whose mapped target template is gone and // was not recreated (no --overwrite). Used by page pusher to skip pages that depend on them. @@ -187,6 +194,9 @@ export const state: State = { // Failed content registry - tracks content items that failed during sync failedContentRegistry: new Map(), + // Effective target models - models saved (or, in preflight, planned) during this run + effectiveTargetModels: new Map(), + // Held-back template registry - templates whose mapped target is gone and was not recreated heldBackTemplateRegistry: new Map(), }; @@ -490,6 +500,9 @@ export function resetState() { // Reverse sync state.reverseSync = false; state.mappingPair = undefined; + + // Run-scoped model registry (PROD-2542) + state.effectiveTargetModels = new Map(); } /** @@ -751,26 +764,32 @@ export function clearLogger(): void { // better error messages when content mappings are missing // ============================================================================ +function failedContentKey(contentID: number, locale: string): string { + return `${(locale || "").toLowerCase()}:${contentID}`; +} + /** * Register a failed content item * @param contentID - The source content ID that failed * @param referenceName - The reference name of the content item * @param error - The error message - * @param locale - The locale being processed + * @param locale - The locale being processed (part of the key, PROD-2542) */ export function registerFailedContent(contentID: number, referenceName: string, error: string, locale: string): void { - state.failedContentRegistry.set(contentID, { referenceName, error, locale }); + state.failedContentRegistry.set(failedContentKey(contentID, locale), { referenceName, error, locale }); } /** - * Look up a failed content item by its source content ID + * Look up a failed content item by its source content ID in a locale * @param contentID - The source content ID to look up + * @param locale - The locale being processed; a failure registered for another locale is not returned * @returns The failure info if found, or undefined */ export function getFailedContent( - contentID: number + contentID: number, + locale: string ): { referenceName: string; error: string; locale: string } | undefined { - return state.failedContentRegistry.get(contentID); + return state.failedContentRegistry.get(failedContentKey(contentID, locale)); } /** @@ -780,6 +799,30 @@ export function clearFailedContentRegistry(): void { state.failedContentRegistry.clear(); } +// ============================================================================ +// Effective target model registry (PROD-2542) +// ============================================================================ + +/** + * Record the model definition content of `sourceReferenceName` will be validated against on the + * target for the rest of this run. + */ +export function registerEffectiveTargetModel(sourceReferenceName: string, model: any): void { + if (!sourceReferenceName || !model) return; + state.effectiveTargetModels.set(sourceReferenceName.toLowerCase(), model); +} + +/** Look up a model registered this run by its source referenceName (case-insensitive). */ +export function getEffectiveTargetModel(sourceReferenceName: string): any | undefined { + if (!sourceReferenceName) return undefined; + return state.effectiveTargetModels.get(sourceReferenceName.toLowerCase()); +} + +/** Clear the effective target model registry (called at start of each sync). */ +export function clearEffectiveTargetModels(): void { + state.effectiveTargetModels.clear(); +} + /** * Register a source template that was held back because its mapped target template no longer * exists and --overwrite was not set diff --git a/src/core/tests/json-summary.test.ts b/src/core/tests/json-summary.test.ts index 7aaffd09..90347698 100644 --- a/src/core/tests/json-summary.test.ts +++ b/src/core/tests/json-summary.test.ts @@ -180,9 +180,9 @@ describe("buildJsonSummary — shape", () => { const summary = buildJsonSummary( makeInput({ preflight: { - totals: { create: 2, update: 1, skip: 0, conflict: 1 }, + totals: { create: 2, update: 1, skip: 0, conflict: 1, invalid: 0 }, hasConflicts: true, - phases: [{ phase: "Models", create: 2, update: 1, skip: 0, conflict: 1 }], + phases: [{ phase: "Models", create: 2, update: 1, skip: 0, conflict: 1, invalid: 0 }], entries: [{ phase: "Models", action: "conflict", name: "LinkCard" }], }, }) @@ -372,3 +372,38 @@ describe("buildAbortSummary", () => { }); }); }); + +// ─── PROD-2542: client-side validation failures ────────────────────────────── + +describe("buildJsonSummary — required-field validation failures (PROD-2542)", () => { + it("carries the model and missing fields of a content failure through to the JSON", () => { + const failure = { + name: "home_promobanner", + error: + "Missing required field(s) on model PromoBanner: promoTitle (Text), promoSubtitle (Text) — skipped client-side", + type: "content" as const, + contentID: 12942, + guid: "source-u", + locale: "en-us", + model: "PromoBanner", + fields: ["promoTitle (Text)", "promoSubtitle (Text)"], + }; + const summary = buildJsonSummary(makeInput({ failures: [failure] })); + expect(JSON.parse(JSON.stringify(summary)).failures).toEqual([failure]); + }); + + it("carries the preflight invalid count additively", () => { + const preflight = { + totals: { create: 1, update: 0, skip: 0, conflict: 0, invalid: 1 }, + hasConflicts: false, + phases: [{ phase: "Content", create: 1, update: 0, skip: 0, conflict: 0, invalid: 1 }], + entries: [ + { phase: "Content", action: "invalid" as const, name: "home_promobanner", locale: "en-us", detail: "x" }, + { phase: "Content", action: "create" as const, name: "ok", locale: "en-us" }, + ], + }; + const summary = buildJsonSummary(makeInput({ preflight })); + expect(summary.preflight?.totals.invalid).toBe(1); + expect(summary.preflight?.hasConflicts).toBe(false); + }); +}); diff --git a/src/core/tests/state.test.ts b/src/core/tests/state.test.ts index 39be04d6..5f75e3fe 100644 --- a/src/core/tests/state.test.ts +++ b/src/core/tests/state.test.ts @@ -14,6 +14,9 @@ import { registerFailedContent, getFailedContent, clearFailedContentRegistry, + registerEffectiveTargetModel, + getEffectiveTargetModel, + clearEffectiveTargetModels, initializeLogger, initializeGuidLogger, getLoggerForGuid, @@ -304,18 +307,50 @@ describe("getApiKeysForGuid / getAllApiKeys", () => { describe("failed content registry", () => { it("registers and retrieves a failed content item", () => { registerFailedContent(123, "my-ref", "some error", "en-us"); - const result = getFailedContent(123); + const result = getFailedContent(123, "en-us"); expect(result).toEqual({ referenceName: "my-ref", error: "some error", locale: "en-us" }); }); it("returns undefined for unknown content ID", () => { - expect(getFailedContent(999)).toBeUndefined(); + expect(getFailedContent(999, "en-us")).toBeUndefined(); }); it("clears the registry", () => { registerFailedContent(1, "ref", "err", "en-us"); clearFailedContentRegistry(); - expect(getFailedContent(1)).toBeUndefined(); + expect(getFailedContent(1, "en-us")).toBeUndefined(); + }); + + // PROD-2542: the same source contentID failing in es-us must not label the en-us item. + it("scopes failures by locale", () => { + registerFailedContent(42, "ref-es", "es error", "es-us"); + expect(getFailedContent(42, "en-us")).toBeUndefined(); + expect(getFailedContent(42, "es-us")).toEqual({ referenceName: "ref-es", error: "es error", locale: "es-us" }); + registerFailedContent(42, "ref-en", "en error", "en-us"); + expect(getFailedContent(42, "en-us")?.error).toBe("en error"); + expect(getFailedContent(42, "es-us")?.error).toBe("es error"); + }); + + it("matches locale case-insensitively", () => { + registerFailedContent(7, "ref", "err", "en-US"); + expect(getFailedContent(7, "en-us")).toBeDefined(); + }); +}); + +describe("effective target model registry (PROD-2542)", () => { + it("registers and looks up by source referenceName, case-insensitively", () => { + const model = { referenceName: "PromoBanner", fields: [] }; + registerEffectiveTargetModel("PromoBanner", model); + expect(getEffectiveTargetModel("promobanner")).toBe(model); + }); + + it("is cleared by clearEffectiveTargetModels and resetState", () => { + registerEffectiveTargetModel("A", { fields: [] }); + clearEffectiveTargetModels(); + expect(getEffectiveTargetModel("A")).toBeUndefined(); + registerEffectiveTargetModel("B", { fields: [] }); + resetState(); + expect(getEffectiveTargetModel("B")).toBeUndefined(); }); }); diff --git a/src/lib/preflight/preflight-report.ts b/src/lib/preflight/preflight-report.ts index a3c4ebf0..c736b48a 100644 --- a/src/lib/preflight/preflight-report.ts +++ b/src/lib/preflight/preflight-report.ts @@ -13,7 +13,12 @@ import ansiColors from "ansi-colors"; import { state } from "../../core/state"; -export type PreflightAction = "create" | "update" | "skip" | "conflict"; +/** + * "invalid" (PROD-2542): the item would be created/updated, but fails client-side validation against + * the target model (e.g. a missing required field), so a real run skips it and reports a failure. + * It is a warning in the plan: not a write, and — unlike "conflict" — it doesn't set the exit code. + */ +export type PreflightAction = "create" | "update" | "skip" | "conflict" | "invalid"; export interface PreflightEntry { /** Phase / element type, e.g. "Models", "Containers", "Content". */ @@ -34,16 +39,18 @@ export interface PreflightPhaseSummary { update: number; skip: number; conflict: number; + invalid: number; entries: PreflightEntry[]; } -const ACTION_ORDER: PreflightAction[] = ["create", "update", "skip", "conflict"]; +const ACTION_ORDER: PreflightAction[] = ["create", "update", "skip", "conflict", "invalid"]; const ACTION_COLOR: Record string> = { create: ansiColors.green, update: ansiColors.cyan, skip: ansiColors.gray, conflict: ansiColors.red, + invalid: ansiColors.yellow, }; const ACTION_GLYPH: Record = { @@ -51,6 +58,7 @@ const ACTION_GLYPH: Record = { update: "~", skip: "·", conflict: "✗", + invalid: "!", }; class PreflightReport { @@ -90,7 +98,7 @@ class PreflightReport { for (const entry of this.entries) { let summary = byPhase.get(entry.phase); if (!summary) { - summary = { phase: entry.phase, create: 0, update: 0, skip: 0, conflict: 0, entries: [] }; + summary = { phase: entry.phase, create: 0, update: 0, skip: 0, conflict: 0, invalid: 0, entries: [] }; byPhase.set(entry.phase, summary); } summary[entry.action]++; @@ -101,7 +109,7 @@ class PreflightReport { /** Aggregate totals across all phases. */ getTotals(): Record { - const totals: Record = { create: 0, update: 0, skip: 0, conflict: 0 }; + const totals: Record = { create: 0, update: 0, skip: 0, conflict: 0, invalid: 0 }; for (const entry of this.entries) { totals[entry.action]++; } @@ -122,18 +130,26 @@ class PreflightReport { toJSON(): { totals: Record; hasConflicts: boolean; - phases: Array<{ phase: string; create: number; update: number; skip: number; conflict: number }>; + phases: Array<{ + phase: string; + create: number; + update: number; + skip: number; + conflict: number; + invalid: number; + }>; entries: PreflightEntry[]; } { return { totals: this.getTotals(), hasConflicts: this.hasConflicts(), - phases: this.getPhaseSummaries().map(({ phase, create, update, skip, conflict }) => ({ + phases: this.getPhaseSummaries().map(({ phase, create, update, skip, conflict, invalid }) => ({ phase, create, update, skip, conflict, + invalid, })), entries: this.getEntries(), }; @@ -169,7 +185,8 @@ class PreflightReport { ACTION_COLOR.create(`${phase.create} create, `) + ACTION_COLOR.update(`${phase.update} update, `) + ACTION_COLOR.skip(`${phase.skip} skip, `) + - ACTION_COLOR.conflict(`${phase.conflict} conflict`); + ACTION_COLOR.conflict(`${phase.conflict} conflict`) + + (phase.invalid > 0 ? ACTION_COLOR.invalid(`, ${phase.invalid} invalid`) : ""); lines.push(header); // List entries grouped by action so creates/updates/conflicts are easy to scan. @@ -191,8 +208,16 @@ class PreflightReport { ACTION_COLOR.create(`${totals.create} create, `) + ACTION_COLOR.update(`${totals.update} update, `) + ACTION_COLOR.skip(`${totals.skip} skip, `) + - ACTION_COLOR.conflict(`${totals.conflict} conflict`) + ACTION_COLOR.conflict(`${totals.conflict} conflict`) + + (totals.invalid > 0 ? ACTION_COLOR.invalid(`, ${totals.invalid} invalid`) : "") ); + if (totals.invalid > 0) { + lines.push( + ansiColors.yellow( + `\n⚠️ ${totals.invalid} item(s) fail validation against the target model — a real sync would skip them and report them as failures.` + ) + ); + } if (this.hasConflicts()) { lines.push(ansiColors.red("\n⚠️ Conflicts detected — a real sync would require --overwrite (exit code 1).")); } diff --git a/src/lib/preflight/tests/preflight-report.test.ts b/src/lib/preflight/tests/preflight-report.test.ts index e4ca4d20..ac2318a3 100644 --- a/src/lib/preflight/tests/preflight-report.test.ts +++ b/src/lib/preflight/tests/preflight-report.test.ts @@ -121,7 +121,7 @@ describe("hasConflicts", () => { describe("getTotals", () => { it("returns all-zeros totals when there are no entries", () => { setState({ preflight: true }); - expect(preflightReport.getTotals()).toEqual({ create: 0, update: 0, skip: 0, conflict: 0 }); + expect(preflightReport.getTotals()).toEqual({ create: 0, update: 0, skip: 0, conflict: 0, invalid: 0 }); }); it("correctly counts each action type", () => { @@ -133,14 +133,14 @@ describe("getTotals", () => { preflightReport.record({ phase: "Content", action: "skip", name: "c3" }); preflightReport.record({ phase: "Pages", action: "conflict", name: "p1" }); - expect(preflightReport.getTotals()).toEqual({ create: 2, update: 1, skip: 2, conflict: 1 }); + expect(preflightReport.getTotals()).toEqual({ create: 2, update: 1, skip: 2, conflict: 1, invalid: 0 }); }); it("counts only the actions that appear", () => { setState({ preflight: true }); preflightReport.record({ phase: "Models", action: "update", name: "m1" }); - expect(preflightReport.getTotals()).toEqual({ create: 0, update: 1, skip: 0, conflict: 0 }); + expect(preflightReport.getTotals()).toEqual({ create: 0, update: 1, skip: 0, conflict: 0, invalid: 0 }); }); }); @@ -292,7 +292,7 @@ describe("toJSON", () => { const json = preflightReport.toJSON(); - expect(json.totals).toEqual({ create: 1, update: 1, skip: 1, conflict: 1 }); + expect(json.totals).toEqual({ create: 1, update: 1, skip: 1, conflict: 1, invalid: 0 }); expect(json.hasConflicts).toBe(true); }); @@ -306,7 +306,7 @@ describe("toJSON", () => { const json = preflightReport.toJSON(); expect(json.phases.map((p) => p.phase)).toEqual(["Models", "Containers"]); - expect(json.phases[0]).toEqual({ phase: "Models", create: 2, update: 0, skip: 0, conflict: 0 }); + expect(json.phases[0]).toEqual({ phase: "Models", create: 2, update: 0, skip: 0, conflict: 0, invalid: 0 }); }); it("carries every entry through, with locale and detail intact", () => { @@ -326,7 +326,7 @@ describe("toJSON", () => { const json = preflightReport.toJSON(); - expect(json.totals).toEqual({ create: 0, update: 0, skip: 0, conflict: 0 }); + expect(json.totals).toEqual({ create: 0, update: 0, skip: 0, conflict: 0, invalid: 0 }); expect(json.hasConflicts).toBe(false); expect(json.phases).toEqual([]); expect(json.entries).toEqual([]); @@ -376,3 +376,47 @@ describe("renderTable — --containers scope", () => { expect(preflightReport.renderTable()).not.toContain("Scoped to --containers"); }); }); + +// ─── PROD-2542: invalid (would fail client-side validation) ────────────────── + +describe("invalid action (PROD-2542)", () => { + beforeEach(() => setState({ preflight: true })); + + it("is counted separately and does not count as a create/update or a conflict", () => { + preflightReport.record({ phase: "Content", action: "create", name: "ok", locale: "en-us" }); + preflightReport.record({ + phase: "Content", + action: "invalid", + name: "banner", + locale: "en-us", + detail: "would create but Missing required field(s) on model PromoBanner: promoTitle (Text)", + }); + + expect(preflightReport.getTotals()).toEqual({ create: 1, update: 0, skip: 0, conflict: 0, invalid: 1 }); + expect(preflightReport.hasConflicts()).toBe(false); + expect(preflightReport.toJSON().phases).toEqual([ + { phase: "Content", create: 1, update: 0, skip: 0, conflict: 0, invalid: 1 }, + ]); + }); + + it("renders the item with its reason and a warning footer, without the conflict exit-code notice", () => { + preflightReport.record({ + phase: "Content", + action: "invalid", + name: "banner", + locale: "en-us", + detail: "would create but Missing required field(s) on model PromoBanner: promoTitle (Text)", + }); + const table = preflightReport.renderTable(); + expect(table).toContain("1 invalid"); + expect(table).toContain("banner"); + expect(table).toContain("promoTitle (Text)"); + expect(table).toContain("would skip them and report them as failures"); + expect(table).not.toContain("exit code 1"); + }); + + it("omits the invalid count from headers when there are none", () => { + preflightReport.record({ phase: "Content", action: "create", name: "ok" }); + expect(preflightReport.renderTable()).not.toContain("invalid"); + }); +}); diff --git a/src/lib/pushers/batch-polling.ts b/src/lib/pushers/batch-polling.ts index 98e67718..67bd4dd5 100644 --- a/src/lib/pushers/batch-polling.ts +++ b/src/lib/pushers/batch-polling.ts @@ -56,7 +56,24 @@ function createProgressBar(percent: number, width: number = 20): string { return `[${"█".repeat(filled)}${"░".repeat(empty)}]`; } -function logBatchErrors(batchStatus: CompletedBatch, originalPayloads?: any[]): void { +/** + * Find the payload that produced a batch item, by its batchItemId. The batch's `items` are returned in + * the order the payloads were sent, so the item's position is the payload's position — but only when + * the two lists have the same length; otherwise alignment is unverifiable and nothing is returned. + */ +export function resolvePayloadForBatchItem( + batchStatus: CompletedBatch, + batchItemId: any, + originalPayloads?: any[] +): any { + if (!originalPayloads || batchItemId === undefined || batchItemId === null || batchItemId === "?") return undefined; + const items: any[] = Array.isArray(batchStatus?.items) ? batchStatus.items : []; + if (items.length !== originalPayloads.length) return undefined; + const index = items.findIndex((item) => (item?.batchItemID ?? item?.batchItemId) == batchItemId); + return index >= 0 ? originalPayloads[index] : undefined; +} + +export function logBatchErrors(batchStatus: CompletedBatch, originalPayloads?: any[]): void { // Prefer structured failedItems array from new API if (Array.isArray(batchStatus.failedItems) && batchStatus.failedItems.length > 0) { batchStatus.failedItems.forEach((failed: any) => { @@ -65,18 +82,16 @@ function logBatchErrors(batchStatus: CompletedBatch, originalPayloads?: any[]): const errorMessage = failed.errorMessage || "Unknown error"; const itemType = failed.itemType || "Item"; - // Try to find the original payload by batchItemId to get referenceName - let referenceName = "unknown"; - if (originalPayloads) { - // batchItemId typically corresponds to index in the batch - const payload = - originalPayloads.find((p, idx) => p?.batchItemId === batchItemId) || - originalPayloads[batchStatus.failedItems.indexOf(failed)]; - referenceName = payload?.properties?.referenceName || payload?.referenceName || "unknown"; - } + // PROD-2542: resolve the payload via batchItemId → position in batch.items → payload at that + // position. (The old fallback used the index within the FAILED list, which named the wrong item + // whenever an earlier item in the batch had succeeded.) Unresolvable → "unknown", never a guess. + const payload = resolvePayloadForBatchItem(batchStatus, batchItemId, originalPayloads); + const referenceName = payload?.properties?.referenceName || payload?.referenceName || payload?.name || "unknown"; console.error( - ansiColors.red(` ✗ ${itemType} ${batchItemId} (${referenceName}): ${formatBatchItemError(errorType, errorMessage)}`) + ansiColors.red( + ` ✗ ${itemType} ${batchItemId} (${referenceName}): ${formatBatchItemError(errorType, errorMessage)}` + ) ); }); return; @@ -88,7 +103,9 @@ function logBatchErrors(batchStatus: CompletedBatch, originalPayloads?: any[]): if (item.errorMessage) { const errorMessage = extractErrorMessage(item.errorMessage); const batchItemId = item.batchItemID || item.batchItemId || `idx:${index}`; - const referenceName = originalPayloads?.[index]?.properties?.referenceName || "unknown"; + // Positional match is only trustworthy when every payload has exactly one item. + const aligned = originalPayloads?.length === batchStatus.items.length; + const referenceName = (aligned && originalPayloads?.[index]?.properties?.referenceName) || "unknown"; console.error(ansiColors.red(` ✗ Batch item ${batchItemId} (${referenceName}): ${errorMessage}`)); } @@ -280,7 +297,21 @@ interface BatchExtractResult { summary?: BatchSummary; } -function extractBatchResultsImpl(batch: CompletedBatch, originalItems: T[]): BatchExtractResult { +/** + * Pair each returned batch item with the source item at the same position. + * + * PROD-2542: pairing is positional, so it is only trusted when it can be verified: + * - batch.items must have exactly one entry per original item; on a length mismatch EVERY item is + * failed ("unverifiable batch result alignment") rather than guessing which result is whose — + * a wrong guess writes a source→target mapping to the wrong item. + * - when `expectedIds` is given (the ID each payload was sent with, e.g. contentID for an update), + * a returned itemID that differs from a positive expected ID fails that item. + */ +function extractBatchResultsImpl( + batch: CompletedBatch, + originalItems: T[], + expectedIds?: Array +): BatchExtractResult { const successfulItems: BatchSuccessItem[] = []; const failedItems: BatchFailureItem[] = []; @@ -308,10 +339,38 @@ function extractBatchResultsImpl(batch: CompletedBatch, originalItems: T[]): }; } + if (batch.items.length !== originalItems.length) { + const error = + `Unverifiable batch result alignment: batch ${batch.batchID ?? "?"} returned ${batch.items.length} item(s) ` + + `for ${originalItems.length} sent — not recording any result from it`; + return { + successfulItems: [], + failedItems: originalItems.map((item, index) => ({ originalItem: item, newItem: null, error, index })), + summary, + }; + } + batch.items.forEach((item, index: number) => { const originalItem = originalItems[index]; - - if (item.itemID > 0 && !item.itemNull) { + const expectedId = expectedIds?.[index]; + + if ( + item.itemID > 0 && + !item.itemNull && + typeof expectedId === "number" && + expectedId > 0 && + item.itemID !== expectedId + ) { + failedItems.push({ + originalItem, + newItem: null, + error: + `Batch result mismatch: item was sent as ID ${expectedId} but the batch returned itemID ${item.itemID} ` + + `at the same position — not recording this result`, + batchItemId: item.batchItemID, + index, + }); + } else if (item.itemID > 0 && !item.itemNull) { successfulItems.push({ originalItem, newId: item.itemID, newItem: item, index }); } else { let errorMsg = "Failed to create item"; @@ -338,11 +397,17 @@ function extractBatchResultsImpl(batch: CompletedBatch, originalItems: T[]): return { successfulItems, failedItems, summary }; } +/** + * @param expectedIds - per original item (same order), the payload contentID the batch must return + * for it — i.e. for an update of an existing target item. A returned itemID that differs from a + * positive expected ID fails the item. Leave an entry undefined where a new ID is legitimate. + */ export function extractContentBatchResults( batch: CompletedBatch, - originalItems: mgmtApi.ContentItem[] + originalItems: mgmtApi.ContentItem[], + expectedIds?: Array ): BatchExtractResult { - return extractBatchResultsImpl(batch, originalItems); + return extractBatchResultsImpl(batch, originalItems, expectedIds); } export function extractPageBatchResults( diff --git a/src/lib/pushers/content-pusher/content-batch-processor.ts b/src/lib/pushers/content-pusher/content-batch-processor.ts index b459884f..58047e28 100644 --- a/src/lib/pushers/content-pusher/content-batch-processor.ts +++ b/src/lib/pushers/content-pusher/content-batch-processor.ts @@ -19,7 +19,22 @@ import { import { fileOperations } from "core/fileOperations"; import { collectUnresolvedContentReferences } from "./util/has-unresolved-content-references"; import { Logs } from "core/logs"; -import { state } from "core/state"; +import { state, getFailedContent } from "core/state"; + +/** + * PROD-2542: one source item and the payload built from it. Prepared items travel as pairs so the + * payload list sent to the API and the source list results are matched against are derived from + * the same array and can never drift out of step (a skipped item used to shift every later result). + */ +interface PreparedContentPayload { + source: mgmtApi.ContentItem; + payload: any; + // The target contentID the batch must return for this item, when that is known for certain (an + // update of a target item that exists). Left unset for creates and for a re-sent ID whose target + // item is gone (--overwrite recreate), where the server may legitimately assign a new ID. + expectedId?: number; +} + /****** * USAGE PATTERN: * 1. Filter content items BEFORE creating the batch processor using filterContentItemsForProcessing() @@ -126,15 +141,21 @@ export class ContentBatchProcessor { `[${progress}%] Bulk batch ${batchNumber}/${contentBatches.length}: Processing ${contentBatch.length} ${batchType} content items (ETA: ${etaMinutes}m)...` ); + // PROD-2542: the items actually sent in this batch (null until prepared), and whether their + // results were already counted — so the catch path below fails only what was in flight and + // never double-counts prep failures or already-recorded results. + let prepared: PreparedContentPayload[] | null = null; + let resultsCounted = false; + try { // Prepare content payloads for bulk upload - - const { - payloads: contentPayloads, - skippedCount: batchSkippedCount, - includedItems, - failedItems: prepFailedItems, - } = await this.prepareContentPayloads(contentBatch, this.config.sourceGuid, this.config.targetGuid, logger); + const prep = await this.prepareContentPayloads( + contentBatch, + this.config.sourceGuid, + this.config.targetGuid, + logger + ); + const { skippedCount: batchSkippedCount, failedItems: prepFailedItems } = prep; // Track skipped items from this batch totalSkippedCount += batchSkippedCount; @@ -144,6 +165,15 @@ export class ContentBatchProcessor { totalFailureCount += prepFailedItems.length; allFailedItems.push(...prepFailedItems); + prepared = prep.prepared; + if (prepared.length === 0) { + // Nothing left to send (every item skipped or failed preparation) — no API call. + continue; + } + // Both lists derive from the same paired array, so index i of one is index i of the other. + const contentPayloads = prepared.map((p) => p.payload); + const includedItems = prepared.map((p) => p.source); + // Execute bulk upload using saveContentItems API with returnBatchID flag const batchIDResult = await this.config.apiClient.contentMethods.saveContentItems( contentPayloads, @@ -171,7 +201,12 @@ export class ContentBatchProcessor { // contentBatch may include skipped items (orphaned content, missing model, etc.) that were // never added to contentPayloads; using it here would shift every result after a skip onto // the wrong source item, corrupting the source→target ID mappings. - const { successfulItems, failedItems } = extractContentBatchResults(completedBatch, includedItems); + // The expected IDs let extraction cross-check each returned itemID against the contentID sent. + const { successfulItems, failedItems } = extractContentBatchResults( + completedBatch, + includedItems, + prepared.map((p) => p.expectedId) + ); // Convert to expected format // Filter publishableIds to only include items that are Published (state === 2) in source @@ -211,6 +246,7 @@ export class ContentBatchProcessor { totalFailureCount += batchResult.failureCount; allSuccessfulItems.push(...batchResult.successfulItems); allFailedItems.push(...batchResult.failedItems); + resultsCounted = true; // Update ID mappings for successful uploads if (batchResult.successfulItems.length > 0) { @@ -255,9 +291,15 @@ export class ContentBatchProcessor { } catch (error: any) { console.error(`❌ Bulk batch ${batchNumber} failed:`, error.message); - // Batch pusher only handles batches - mark entire batch as failed + // Results already recorded for this batch — the error came from post-processing. + if (resultsCounted) continue; + + // Batch pusher only handles batches - mark the items that were in flight as failed. + // PROD-2542: only the PREPARED items; prep failures were already counted above. If + // preparation itself threw, nothing was counted yet, so the whole batch fails. // Individual processing fallbacks should be handled at the sync level - const failedBatchItems: BatchFailedItem[] = contentBatch.map((item) => ({ + const inFlight = prepared ? prepared.map((p) => p.source) : contentBatch; + const failedBatchItems: BatchFailedItem[] = inFlight.map((item) => ({ originalContent: item, error: `Batch processing failed: ${error.message}`, })); @@ -305,13 +347,11 @@ export class ContentBatchProcessor { // PROD-2533: needed so an item skipped here is written to the push log, not just the console. logger: Logs ): Promise<{ - payloads: any[]; + prepared: PreparedContentPayload[]; skippedCount: number; - includedItems: mgmtApi.ContentItem[]; failedItems: BatchFailedItem[]; }> { - const payloads: any[] = []; - const includedItems: mgmtApi.ContentItem[] = []; + const prepared: PreparedContentPayload[] = []; let skippedCount = 0; const failedItems: BatchFailedItem[] = []; @@ -335,8 +375,13 @@ export class ContentBatchProcessor { contentID: existingMapping ? existingMapping.targetContentID : -1, }; - payloads.push(payload); - includedItems.push(contentItem); + const targetExists = + !!existingMapping && !!this.config.referenceMapper.getMappedEntity(existingMapping, "target"); + prepared.push({ + source: contentItem, + payload, + expectedId: targetExists ? existingMapping.targetContentID : undefined, + }); } else { //map the content item to the target instance const modelMapping = modelMapper.getModelMappingByReferenceName( @@ -424,9 +469,15 @@ export class ContentBatchProcessor { sourceModel ); if (unresolvedRefs.length > 0) { + // PROD-2542: when a referenced child failed earlier in this locale (e.g. skipped + // client-side for a missing required field), say so — that's the actual cause. const detail = unresolvedRefs .slice(0, 5) - .map((r) => `${r.path} → source contentID ${r.contentID}`) + .map((r) => { + const child = getFailedContent(r.contentID, this.config.locale); + const cause = child ? ` (child '${child.referenceName}' skipped: ${child.error})` : ""; + return `${r.path} → source contentID ${r.contentID}${cause}`; + }) .join("; "); const more = unresolvedRefs.length > 5 ? ` (+${unresolvedRefs.length - 5} more)` : ""; throw new Error( @@ -528,8 +579,14 @@ export class ContentBatchProcessor { scripts: contentItem.scripts ?? defaultScripts, }; - payloads.push(payload); - includedItems.push(contentItem); + prepared.push({ + source: contentItem, + payload, + // Only an update of a target item known to exist (or a locale version of one found in + // another locale) must come back with the same ID. + expectedId: + existingTargetContentItem || (!existingMapping && existingContentID > 0) ? existingContentID : undefined, + }); } catch (error: any) { const prepareError = error.message || "payload preparation failed"; @@ -557,7 +614,7 @@ export class ContentBatchProcessor { } } - return { payloads, skippedCount, includedItems, failedItems }; + return { prepared, skippedCount, failedItems }; } /** diff --git a/src/lib/pushers/content-pusher/content-pusher.ts b/src/lib/pushers/content-pusher/content-pusher.ts index e1ab92ac..40f47690 100644 --- a/src/lib/pushers/content-pusher/content-pusher.ts +++ b/src/lib/pushers/content-pusher/content-pusher.ts @@ -3,7 +3,7 @@ import ansiColors from "ansi-colors"; // Removed ContentBatchProcessor import - individual pusher only handles individual processing import { getLoggerForGuid, state, registerFailedContent } from "core/state"; import { ContentItemMapper } from "lib/mappers/content-item-mapper"; -import { filterContentItemsForProcessing } from "./util/filter-content-items-for-processing"; +import { ContentValidationFailure, filterContentItemsForProcessing } from "./util/filter-content-items-for-processing"; import { getContentItemTypes } from "./util/get-content-item-types"; import { ModelMapper } from "lib/mappers/model-mapper"; import { ContentItem, Model } from "@agility/management-sdk"; @@ -51,8 +51,40 @@ export async function pushContent(sourceData: ContentItem[], targetData: Content contentID?: number; guid?: string; locale?: string; + model?: string; + fields?: string[]; }> = []; + // PROD-2542: items the filter rejected client-side (missing required field on the target model). + // Register each in the failed-content registry right away, so the next pass's parents and the + // page pusher can say why their reference is unresolved. In preflight that is all — the filter + // already recorded them as "invalid" — so they count as skips and never touch the exit code; in a + // real run they are failures with full detail (console/log, ERROR SUMMARY, --jsonSummary). + const reportValidationFailures = (failures: ContentValidationFailure[]): void => { + for (const { item, error, model, missing } of failures) { + const name = item.properties?.referenceName || "Unknown"; + if (item.contentID) { + registerFailedContent(item.contentID, name, error, locale); + } + if (state.preflight) { + totalSkipped++; + continue; + } + logger.content.error(item, error, locale, targetGuidStr); + totalFailed++; + allFailureDetails.push({ + name, + error, + type: "content", + contentID: item.contentID, + guid: sourceGuidStr, + locale, + model, + fields: missing.map((m) => `${m.name} (${m.type})`), + }); + } + }; + try { // Import getApiClient for both batch configurations @@ -82,7 +114,9 @@ export async function pushContent(sourceData: ContentItem[], targetData: Content referenceMapper, targetData, logger, + sourceGuid: sourceGuidStr, }); + reportValidationFailures(filteredLinkedContentItems.itemsToFail); // Preflight (PROD-2203): the filter has already recorded create/update/skip/conflict. // Skip the batch write entirely so nothing is sent to the target, but keep @@ -146,7 +180,9 @@ export async function pushContent(sourceData: ContentItem[], targetData: Content referenceMapper, targetData, logger, + sourceGuid: sourceGuidStr, }); + reportValidationFailures(filteredNormalContentItems.itemsToFail); // Preflight (PROD-2203): filter already recorded the actions; skip the write. if (state.preflight) { totalSuccessful += filteredNormalContentItems.itemsToProcess.length; diff --git a/src/lib/pushers/content-pusher/tests/content-batch-processor.test.ts b/src/lib/pushers/content-pusher/tests/content-batch-processor.test.ts index ed0ec4bd..38b26e02 100644 --- a/src/lib/pushers/content-pusher/tests/content-batch-processor.test.ts +++ b/src/lib/pushers/content-pusher/tests/content-batch-processor.test.ts @@ -1,7 +1,7 @@ import * as fs from "fs"; import * as os from "os"; import * as path from "path"; -import { resetState, setState } from "core/state"; +import { resetState, setState, registerFailedContent } from "core/state"; import { ContentBatchProcessor } from "../content-batch-processor"; import { ContentItemMapper } from "lib/mappers/content-item-mapper"; @@ -91,6 +91,11 @@ function makeContentItem(id: number, stateVal = 2): any { }; } +// prepareContentPayloads' result shape: source items paired with their payloads (PROD-2542). +function pairs(items: any[]): Array<{ source: any; payload: any }> { + return items.map((source) => ({ source, payload: { contentID: -1 } })); +} + function makeLogger(): any { return { content: { @@ -157,9 +162,8 @@ describe("ContentBatchProcessor.processBatches — batch-level API failure", () const processor = new ContentBatchProcessor(makeConfig({ apiClient: makeApiClient(saveFn) })); jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ - payloads: [{}, {}], + prepared: pairs([makeContentItem(1), makeContentItem(2)]), skippedCount: 0, - includedItems: [makeContentItem(1), makeContentItem(2)], failedItems: [], }); @@ -179,9 +183,8 @@ describe("ContentBatchProcessor.processBatches — batch-level API failure", () const processor = new ContentBatchProcessor(makeConfig({ apiClient: makeApiClient(saveFn) })); jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ - payloads: [{}], + prepared: pairs([makeContentItem(1)]), skippedCount: 0, - includedItems: [makeContentItem(1)], failedItems: [], }); @@ -199,9 +202,8 @@ describe("ContentBatchProcessor.processBatches — successful batch", () => { const item2 = makeContentItem(2); jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ - payloads: [{}, {}], + prepared: pairs([item1, item2]), skippedCount: 0, - includedItems: [item1, item2], failedItems: [], }); @@ -226,9 +228,8 @@ describe("ContentBatchProcessor.processBatches — successful batch", () => { const item2 = makeContentItem(2); jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ - payloads: [{}, {}], + prepared: pairs([item1, item2]), skippedCount: 0, - includedItems: [item1, item2], failedItems: [], }); @@ -254,9 +255,8 @@ describe("ContentBatchProcessor.processBatches — publishableIds", () => { const processor = new ContentBatchProcessor(makeConfig()); jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ - payloads: [{}, {}], + prepared: pairs([publishedItem, stagingItem]), skippedCount: 0, - includedItems: [publishedItem, stagingItem], failedItems: [], }); @@ -282,9 +282,8 @@ describe("ContentBatchProcessor.processBatches — publishableIds", () => { const processor = new ContentBatchProcessor(makeConfig()); jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ - payloads: [{}], + prepared: pairs([stagingItem]), skippedCount: 0, - includedItems: [stagingItem], failedItems: [], }); @@ -305,9 +304,8 @@ describe("ContentBatchProcessor.processBatches — publishableIds", () => { const processor = new ContentBatchProcessor(makeConfig()); jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ - payloads: [{}], + prepared: pairs([stagingItem]), skippedCount: 0, - includedItems: [stagingItem], failedItems: [], }); @@ -326,9 +324,8 @@ describe("ContentBatchProcessor.processBatches — publishableIds", () => { const processor = new ContentBatchProcessor(makeConfig()); jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ - payloads: [{}, {}], + prepared: pairs(stagingItems), skippedCount: 0, - includedItems: stagingItems, failedItems: [], }); @@ -355,9 +352,8 @@ describe("ContentBatchProcessor.processBatches — onBatchComplete", () => { const item = makeContentItem(1); jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ - payloads: [{}], + prepared: pairs([item]), skippedCount: 0, - includedItems: [item], failedItems: [], }); @@ -374,9 +370,8 @@ describe("ContentBatchProcessor.processBatches — onBatchComplete", () => { const item = makeContentItem(1); jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ - payloads: [{}], + prepared: pairs([item]), skippedCount: 0, - includedItems: [item], failedItems: [], }); @@ -394,9 +389,8 @@ describe("ContentBatchProcessor.processBatches — batch splitting", () => { const processor = new ContentBatchProcessor(makeConfig({ batchSize: 2 })); const prepSpy = jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ - payloads: [], + prepared: pairs([]), skippedCount: 0, - includedItems: [], failedItems: [], }); @@ -413,9 +407,8 @@ describe("ContentBatchProcessor.processBatches — batch splitting", () => { const processor = new ContentBatchProcessor(makeConfig({ batchSize: 2 })); jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ - payloads: [], - skippedCount: 1, // 1 skip per batch - includedItems: [], + prepared: pairs([]), + skippedCount: 1, failedItems: [], }); @@ -440,9 +433,8 @@ describe("ContentBatchProcessor — referenceMapper.addMapping side effect", () const item = makeContentItem(10); jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ - payloads: [{}], + prepared: pairs([item]), skippedCount: 0, - includedItems: [item], failedItems: [], }); @@ -463,9 +455,8 @@ describe("ContentBatchProcessor — referenceMapper.addMapping side effect", () const item = makeContentItem(10); jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ - payloads: [{}], + prepared: pairs([item]), skippedCount: 0, - includedItems: [item], failedItems: [], }); @@ -486,9 +477,8 @@ describe("ContentBatchProcessor.processBatches — failed item logging", () => { const processor = new ContentBatchProcessor(makeConfig()); jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ - payloads: [{}], + prepared: pairs([item]), skippedCount: 0, - includedItems: [item], failedItems: [], }); @@ -508,9 +498,8 @@ describe("ContentBatchProcessor.processBatches — failed item logging", () => { const processor = new ContentBatchProcessor(makeConfig()); jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ - payloads: [{}], + prepared: pairs([item]), skippedCount: 0, - includedItems: [item], failedItems: [], }); @@ -605,8 +594,7 @@ describe("ContentBatchProcessor.prepareContentPayloads — stale container mappi const consoleErrorSpy = jest.spyOn(console, "error"); const result = await (processor as any).prepareContentPayloads([item], sourceGuid, targetGuid, makeLogger()); - expect(result.payloads).toHaveLength(0); - expect(result.includedItems).toHaveLength(0); + expect(result.prepared).toHaveLength(0); // PROD-2310: this item never reaches the target, so it must count as a failure — // not a skip — so it fails the sync exit code. expect(result.skippedCount).toBe(0); @@ -631,8 +619,238 @@ describe("ContentBatchProcessor.prepareContentPayloads — stale container mappi const result = await (processor as any).prepareContentPayloads([item], sourceGuid, targetGuid, makeLogger()); expect(result.skippedCount).toBe(0); - expect(result.payloads).toHaveLength(1); - expect(result.includedItems).toHaveLength(1); - expect(result.payloads[0].properties.referenceName).toBe("ref-1"); + expect(result.prepared).toHaveLength(1); + expect(result.prepared[0].source).toBe(item); + expect(result.prepared[0].payload.properties.referenceName).toBe("ref-1"); + }); +}); + +// ─── PROD-2542: payload/source alignment ────────────────────────────────────── +// +// These use the REAL extractContentBatchResults (the module is mocked above for the other tests), so +// the pairing between what was sent and what is mapped is exercised end to end through the processor. + +const realBatchPolling = jest.requireActual("lib/pushers/batch-polling"); + +describe("ContentBatchProcessor — alignment (PROD-2542)", () => { + beforeEach(() => { + mockExtract.mockImplementation(realBatchPolling.extractContentBatchResults); + }); + + it("maps the right target IDs when item 2 of 4 drops out before the batch", async () => { + const referenceMapper = makeMapper(); + const addMappingSpy = jest.spyOn(referenceMapper, "addMapping").mockImplementation(() => {}); + const saveFn = jest.fn().mockResolvedValue([77]); + const processor = new ContentBatchProcessor(makeConfig({ referenceMapper, apiClient: makeApiClient(saveFn) })); + const [item1, item2, item3, item4] = [1, 2, 3, 4].map((id) => makeContentItem(id)); + + jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ + prepared: [item1, item3, item4].map((source) => ({ source, payload: { contentID: -1, tag: source.contentID } })), + skippedCount: 0, + failedItems: [{ originalContent: item2, error: "Missing required field(s)" }], + }); + mockPoll.mockResolvedValue({ + batchID: 77, + items: [ + { batchItemID: 1, itemID: 101, processedItemVersionID: 1 }, + { batchItemID: 2, itemID: 103, processedItemVersionID: 1 }, + { batchItemID: 3, itemID: 104, processedItemVersionID: 1 }, + ], + }); + + const result = await processor.processBatches([item1, item2, item3, item4], makeLogger(), "Test"); + + // Payloads went out in the same order as their sources. + expect(saveFn.mock.calls[0][0].map((p: any) => p.tag)).toEqual([1, 3, 4]); + const mapped = addMappingSpy.mock.calls.map(([source, target]: any[]) => [source.contentID, target.contentID]); + expect(mapped).toEqual([ + [1, 101], + [3, 103], + [4, 104], + ]); + expect(result.successCount).toBe(3); + expect(result.failureCount).toBe(1); + expect(result.failedItems[0].originalContent).toBe(item2); + }); + + it("makes no API call when every item in the batch drops out", async () => { + mockPoll.mockClear(); + const saveFn = jest.fn().mockResolvedValue([1]); + const processor = new ContentBatchProcessor(makeConfig({ apiClient: makeApiClient(saveFn) })); + const items = [makeContentItem(1), makeContentItem(2)]; + jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ + prepared: [], + skippedCount: 0, + failedItems: items.map((item) => ({ originalContent: item, error: "prep failed" })), + }); + + const result = await processor.processBatches(items, makeLogger(), "Test"); + + expect(saveFn).not.toHaveBeenCalled(); + expect(mockPoll).not.toHaveBeenCalled(); + expect(result.failureCount).toBe(2); + expect(result.successCount).toBe(0); + }); + + it("fails only the prepared items when the API call throws (no double count of prep failures)", async () => { + const saveFn = jest.fn().mockRejectedValue(new Error("API down")); + const processor = new ContentBatchProcessor(makeConfig({ apiClient: makeApiClient(saveFn) })); + const [item1, item2, item3] = [1, 2, 3].map((id) => makeContentItem(id)); + jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ + prepared: pairs([item1, item3]), + skippedCount: 0, + failedItems: [{ originalContent: item2, error: "prep failed" }], + }); + + const result = await processor.processBatches([item1, item2, item3], makeLogger(), "Test"); + + expect(result.failureCount).toBe(3); + expect(result.failedItems).toHaveLength(3); + const byId = new Map(result.failedItems.map((f: any) => [f.originalContent.contentID, f.error])); + expect(byId.get(2)).toBe("prep failed"); + expect(byId.get(1)).toContain("Batch processing failed"); + expect(byId.get(3)).toContain("Batch processing failed"); + }); + + it("does not re-fail items whose results were already counted when post-processing throws", async () => { + const processor = new ContentBatchProcessor(makeConfig()); + const item = makeContentItem(1); + jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ + prepared: pairs([item]), + skippedCount: 0, + failedItems: [], + }); + mockPoll.mockResolvedValue({ items: [{ batchItemID: 1, itemID: 500, processedItemVersionID: 1 }] }); + const logger = makeLogger(); + logger.content.created.mockImplementation(() => { + throw new Error("log write failed"); + }); + + const result = await processor.processBatches([item], logger, "Test"); + + expect(result.successCount).toBe(1); + expect(result.failureCount).toBe(0); + expect(result.failedItems).toHaveLength(0); + }); + + it("fails every item (no mappings) when the batch returns a different number of items", async () => { + const referenceMapper = makeMapper(); + const addMappingSpy = jest.spyOn(referenceMapper, "addMapping").mockImplementation(() => {}); + const processor = new ContentBatchProcessor(makeConfig({ referenceMapper })); + const items = [makeContentItem(1), makeContentItem(2)]; + jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ + prepared: pairs(items), + skippedCount: 0, + failedItems: [], + }); + mockPoll.mockResolvedValue({ batchID: 9, items: [{ batchItemID: 1, itemID: 101 }] }); + + const result = await processor.processBatches(items, makeLogger(), "Test"); + + expect(addMappingSpy).not.toHaveBeenCalled(); + expect(result.successCount).toBe(0); + expect(result.failureCount).toBe(2); + result.failedItems.forEach((f: any) => expect(f.error).toContain("Unverifiable batch result alignment")); + }); + + it("fails an update whose returned itemID differs from the contentID it was sent with", async () => { + const referenceMapper = makeMapper(); + const addMappingSpy = jest.spyOn(referenceMapper, "addMapping").mockImplementation(() => {}); + const processor = new ContentBatchProcessor(makeConfig({ referenceMapper })); + const [item1, item2] = [makeContentItem(1), makeContentItem(2)]; + jest.spyOn(processor as any, "prepareContentPayloads").mockResolvedValue({ + prepared: [ + { source: item1, payload: { contentID: 500 }, expectedId: 500 }, + { source: item2, payload: { contentID: 600 }, expectedId: 600 }, + ], + skippedCount: 0, + failedItems: [], + }); + // Returned in swapped order — exactly what positional matching would have mis-mapped. + mockPoll.mockResolvedValue({ + items: [ + { batchItemID: 1, itemID: 600, processedItemVersionID: 1 }, + { batchItemID: 2, itemID: 500, processedItemVersionID: 1 }, + ], + }); + + const result = await processor.processBatches([item1, item2], makeLogger(), "Test"); + + expect(addMappingSpy).not.toHaveBeenCalled(); + expect(result.failureCount).toBe(2); + expect(result.failedItems[0].error).toContain("sent as ID 500 but the batch returned itemID 600"); + }); +}); + +// ─── PROD-2542: cascade — name the child that was skipped ───────────────────── + +describe("ContentBatchProcessor.prepareContentPayloads — unresolved child cascade (PROD-2542)", () => { + function seed(sourceGuid: string, targetGuid: string): void { + writeEntityFile(tmpDir, sourceGuid, "models", 1, { id: 1, referenceName: "TestModel", fields: [] }); + writeMappingFile(tmpDir, sourceGuid, targetGuid, "models", [ + { + sourceGuid, + targetGuid, + sourceID: 1, + targetID: 1, + sourceReferenceName: "TestModel", + targetReferenceName: "TestModel", + }, + ]); + writeMappingFile(tmpDir, sourceGuid, targetGuid, "containers", [ + { + sourceGuid, + targetGuid, + sourceContentViewID: 10, + targetContentViewID: 999, + sourceReferenceName: "ref-1", + targetReferenceName: "ref-1", + }, + ]); + writeEntityFile(tmpDir, targetGuid, "containers", 999, { contentViewID: 999, referenceName: "ref-1" }); + } + + function parentWithChild(childId: number): any { + const item = makeContentItem(1); + item.fields = { title: "Parent", banner: { contentid: childId } }; + return item; + } + + it("appends the child's skip reason, from the same locale only", async () => { + const sourceGuid = "cascade-src"; + const targetGuid = "cascade-tgt"; + seed(sourceGuid, targetGuid); + registerFailedContent( + 12942, + "home_promobanner", + "Missing required field(s) on model PromoBanner: promoTitle (Text)", + "en-us" + ); + registerFailedContent(777, "es-only", "es failure", "es-us"); + + const referenceMapper = new ContentItemMapper(sourceGuid, targetGuid, "en-us"); + const processor = new ContentBatchProcessor(makeConfig({ sourceGuid, targetGuid, referenceMapper })); + + const result = await (processor as any).prepareContentPayloads( + [parentWithChild(12942)], + sourceGuid, + targetGuid, + makeLogger() + ); + expect(result.prepared).toHaveLength(0); + expect(result.failedItems[0].error).toContain("Unresolved content reference(s)"); + expect(result.failedItems[0].error).toContain( + "(child 'home_promobanner' skipped: Missing required field(s) on model PromoBanner: promoTitle (Text))" + ); + + // A failure registered for es-us must not explain an en-us parent's unresolved reference. + const other = await (processor as any).prepareContentPayloads( + [parentWithChild(777)], + sourceGuid, + targetGuid, + makeLogger() + ); + expect(other.failedItems[0].error).toContain("Unresolved content reference(s)"); + expect(other.failedItems[0].error).not.toContain("child 'es-only'"); }); }); diff --git a/src/lib/pushers/content-pusher/tests/content-pusher-validation.test.ts b/src/lib/pushers/content-pusher/tests/content-pusher-validation.test.ts new file mode 100644 index 00000000..f76f8149 --- /dev/null +++ b/src/lib/pushers/content-pusher/tests/content-pusher-validation.test.ts @@ -0,0 +1,197 @@ +import * as fs from "fs"; +import * as os from "os"; +import * as path from "path"; +import { resetState, setState, state, getFailedContent } from "core/state"; + +// PROD-2542: how pushContent reports items the filter rejected for a missing required field. +// Classification, filtering and batching are mocked so only content-pusher's own bookkeeping runs. + +jest.mock("../util/get-content-item-types", () => ({ + getContentItemTypes: jest.fn(), +})); + +jest.mock("../util/filter-content-items-for-processing", () => ({ + filterContentItemsForProcessing: jest.fn(), +})); + +const mockProcessBatches = jest.fn(); +jest.mock("../content-batch-processor", () => ({ + ContentBatchProcessor: jest.fn().mockImplementation(() => ({ processBatches: mockProcessBatches })), +})); + +import { getContentItemTypes } from "../util/get-content-item-types"; +import { filterContentItemsForProcessing } from "../util/filter-content-items-for-processing"; +import { pushContent } from "../content-pusher"; + +const mockTypes = getContentItemTypes as jest.Mock; +const mockFilter = filterContentItemsForProcessing as jest.Mock; + +let tmpDir: string; +let logger: any; + +beforeAll(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "agility-cpv-")); +}); + +afterAll(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); +}); + +beforeEach(() => { + resetState(); + state.failedContentRegistry.clear(); + setState({ rootPath: tmpDir, sourceGuid: "src-guid", targetGuid: "tgt-guid" }); + logger = { + content: { error: jest.fn(), created: jest.fn(), skipped: jest.fn() }, + warning: jest.fn(), + getGuid: () => "src-guid", + setGuid: jest.fn(), + }; + state.loggerRegistry = new Map([["src-guid", logger]]); + state.cachedApiClient = {} as any; + mockProcessBatches.mockReset(); + mockProcessBatches.mockResolvedValue({ + successCount: 0, + failureCount: 0, + skippedCount: 0, + successfulItems: [], + failedItems: [], + publishableIds: [], + }); + jest.spyOn(console, "log").mockImplementation(() => {}); + jest.spyOn(console, "warn").mockImplementation(() => {}); + jest.spyOn(console, "error").mockImplementation(() => {}); +}); + +afterEach(() => { + jest.restoreAllMocks(); +}); + +function item(id: number, referenceName = `ref-${id}`): any { + return { + contentID: id, + properties: { referenceName, definitionName: "PromoBanner", versionID: 1, state: 2 }, + fields: { enabled: "true" }, + }; +} + +function invalid(contentItem: any): any { + return { + item: contentItem, + error: + "Missing required field(s) on model PromoBanner: promoTitle (Text), promoSubtitle (Text) — skipped client-side", + model: "PromoBanner", + missing: [ + { name: "promoTitle", type: "Text" }, + { name: "promoSubtitle", type: "Text" }, + ], + }; +} + +function filterResult(itemsToProcess: any[], itemsToFail: any[]): any { + return { itemsToProcess, itemsToSkip: [], skippedCount: 0, itemsToFail }; +} + +describe("pushContent — client-side validation failures (PROD-2542)", () => { + it("counts them as failures with full detail, in both the linked and normal passes", async () => { + const linkedBad = item(10, "linked-bad"); + const normalBad = item(12942, "home_promobanner"); + const normalGood = item(2); + mockTypes.mockReturnValue({ + linkedContentItems: [linkedBad], + normalContentItems: [normalBad, normalGood], + skippedItems: [], + }); + mockFilter + .mockResolvedValueOnce(filterResult([], [invalid(linkedBad)])) + .mockResolvedValueOnce(filterResult([normalGood], [invalid(normalBad)])); + mockProcessBatches.mockResolvedValueOnce({ + successCount: 0, + failureCount: 0, + skippedCount: 0, + successfulItems: [], + failedItems: [], + publishableIds: [], + }); + mockProcessBatches.mockResolvedValueOnce({ + successCount: 1, + failureCount: 0, + skippedCount: 0, + successfulItems: [], + failedItems: [], + publishableIds: [], + }); + + const result = await pushContent([linkedBad, normalBad, normalGood], [], "en-us"); + + expect(result.status).toBe("error"); + expect(result.failed).toBe(2); + expect(result.successful).toBe(1); + expect(result.failureDetails).toEqual([ + expect.objectContaining({ name: "linked-bad", contentID: 10, type: "content", locale: "en-us" }), + { + name: "home_promobanner", + error: + "Missing required field(s) on model PromoBanner: promoTitle (Text), promoSubtitle (Text) — skipped client-side", + type: "content", + contentID: 12942, + guid: "src-guid", + locale: "en-us", + model: "PromoBanner", + fields: ["promoTitle (Text)", "promoSubtitle (Text)"], + }, + ]); + // Console/log line for each, like any other content failure. + expect(logger.content.error).toHaveBeenCalledTimes(2); + expect(logger.content.error).toHaveBeenCalledWith( + normalBad, + expect.stringContaining("promoTitle (Text)"), + "en-us", + "tgt-guid" + ); + // The invalid items were never handed to the batch processor. + const batched = mockProcessBatches.mock.calls.flatMap((c) => c[0]); + expect(batched).toEqual([normalGood]); + }); + + it("registers a failure (locale-scoped) before the next pass runs", async () => { + const linkedBad = item(10, "linked-bad"); + const parent = item(20, "parent"); + mockTypes.mockReturnValue({ linkedContentItems: [linkedBad], normalContentItems: [parent], skippedItems: [] }); + let seenDuringNormalPass: any; + mockFilter.mockResolvedValueOnce(filterResult([], [invalid(linkedBad)])).mockImplementationOnce(async () => { + seenDuringNormalPass = getFailedContent(10, "en-us"); + return filterResult([parent], []); + }); + + await pushContent([linkedBad, parent], [], "en-us"); + + expect(seenDuringNormalPass).toEqual( + expect.objectContaining({ + referenceName: "linked-bad", + error: expect.stringContaining("promoTitle"), + locale: "en-us", + }) + ); + expect(getFailedContent(10, "es-us")).toBeUndefined(); + }); + + it("in preflight, counts them as skips, registers them, and adds no failures", async () => { + setState({ preflight: true }); + const bad = item(12942); + const good = item(2); + mockTypes.mockReturnValue({ linkedContentItems: [], normalContentItems: [bad, good], skippedItems: [] }); + mockFilter.mockResolvedValueOnce(filterResult([good], [invalid(bad)])); + + const result = await pushContent([bad, good], [], "en-us"); + + expect(result.status).toBe("success"); + expect(result.failed).toBe(0); + expect(result.failureDetails).toEqual([]); + expect(result.successful).toBe(1); + expect(result.skipped).toBe(1); + expect(mockProcessBatches).not.toHaveBeenCalled(); + expect(logger.content.error).not.toHaveBeenCalled(); + expect(getFailedContent(12942, "en-us")).toBeDefined(); + }); +}); diff --git a/src/lib/pushers/content-pusher/util/filter-content-items-for-processing.ts b/src/lib/pushers/content-pusher/util/filter-content-items-for-processing.ts index 746a1e77..21a2b106 100644 --- a/src/lib/pushers/content-pusher/util/filter-content-items-for-processing.ts +++ b/src/lib/pushers/content-pusher/util/filter-content-items-for-processing.ts @@ -5,6 +5,12 @@ import { ApiClient, ContentItem } from "@agility/management-sdk"; import { Logs } from "core/logs"; import { state } from "core"; import { preflightReport } from "../../../preflight/preflight-report"; +import { + RequiredFieldIssue, + formatMissingRequiredFields, + validateContentItemAgainstModel, +} from "./validate-required-fields"; +import { createEffectiveTargetModelResolver } from "./resolve-effective-target-model"; /** * Filter content items for processing @@ -14,6 +20,16 @@ export interface ContentFilterResult { itemsToProcess: any[]; itemsToSkip: any[]; skippedCount: number; + // PROD-2542: items that would be written but are missing a required field on the target model. + // They never reach itemsToProcess (so never a batch); the caller reports them as failures. + itemsToFail: ContentValidationFailure[]; +} + +export interface ContentValidationFailure { + item: ContentItem; + error: string; + model: string; + missing: RequiredFieldIssue[]; } interface FilterProp { @@ -24,6 +40,8 @@ interface FilterProp { referenceMapper: ContentItemMapper; targetData: ContentItem[]; logger: Logs; + // Source instance guid, for resolving the item's model (defaults to state.sourceGuid). + sourceGuid?: string; } export async function filterContentItemsForProcessing({ @@ -34,15 +52,66 @@ export async function filterContentItemsForProcessing({ referenceMapper, targetData = [], logger, + sourceGuid = state.sourceGuid, }: FilterProp): Promise { const itemsToProcess: any[] = []; const itemsToSkip: any[] = []; + const itemsToFail: ContentValidationFailure[] = []; + const resolveModel = createEffectiveTargetModelResolver(sourceGuid, targetGuid); + + // PROD-2542: validate the SOURCE fields (the field mapper preserves keys and nulls) against the + // effective target model. Returns the failure when the item must not be sent; warnings are + // logged and the item proceeds. + const validateForWrite = (contentItem: ContentItem, itemName: string): ContentValidationFailure | null => { + const definitionName = contentItem.properties?.definitionName || ""; + const model = resolveModel(definitionName); + if (!model) return null; + + const { missing, warnings } = validateContentItemAgainstModel(contentItem.fields, model); + const modelName = model.referenceName || definitionName; + for (const warning of warnings) { + logger.warning?.(`[${locale}] Content ${itemName} (contentID: ${contentItem.contentID}): ${warning}`); + } + if (missing.length === 0) return null; + + return { + item: contentItem, + error: `${formatMissingRequiredFields(modelName, missing)} — skipped client-side`, + model: modelName, + missing, + }; + }; + + // Route an item that would be written (create/update) either to processing or to failure. + const queueForWrite = (contentItem: ContentItem, itemName: string, action: "create" | "update"): boolean => { + const failure = validateForWrite(contentItem, itemName); + if (failure) { + itemsToFail.push(failure); + invalidCount++; + if (state.preflight) { + // A real run would fail this item without sending it — warn, but don't count it as a write. + console.warn(ansiColors.yellow(`⚠️ Content ${itemName} [${locale}] would be skipped: ${failure.error}`)); + } + preflightReport.record({ + phase: "Content", + action: "invalid", + name: itemName, + locale, + detail: `would ${action} but ${failure.error}`, + }); + return false; + } + itemsToProcess.push(contentItem); + preflightReport.record({ phase: "Content", action, name: itemName, locale }); + return true; + }; // Track decision stats for summary logging let createCount = 0; let updateCount = 0; let skipCount = 0; let conflictCount = 0; + let invalidCount = 0; for (const contentItem of contentItems) { const itemName = contentItem.properties.referenceName || "Unknown"; @@ -78,15 +147,11 @@ export async function filterContentItemsForProcessing({ }); continue; } else if (shouldCreate) { - // Content doesn't exist - include it for creation - itemsToProcess.push(contentItem); - createCount++; - preflightReport.record({ phase: "Content", action: "create", name: itemName, locale }); + // Content doesn't exist - include it for creation (unless it fails model validation) + if (queueForWrite(contentItem, itemName, "create")) createCount++; } else if (shouldUpdate) { - // Content exists but needs updating - itemsToProcess.push(contentItem); - updateCount++; - preflightReport.record({ phase: "Content", action: "update", name: itemName, locale }); + // Content exists but needs updating (unless it fails model validation) + if (queueForWrite(contentItem, itemName, "update")) updateCount++; } else if (shouldSkip) { // Content exists and is up to date - skip logger.content.skipped(contentItem, "up to date, skipping", locale, targetGuid); @@ -105,7 +170,7 @@ export async function filterContentItemsForProcessing({ if (state.verbose && contentItems.length > 0) { console.log( ansiColors.gray( - `[FilterContent] Decision summary: ${createCount} create, ${updateCount} update, ${skipCount} skip, ${conflictCount} conflict` + `[FilterContent] Decision summary: ${createCount} create, ${updateCount} update, ${skipCount} skip, ${conflictCount} conflict, ${invalidCount} invalid` ) ); } @@ -114,5 +179,6 @@ export async function filterContentItemsForProcessing({ itemsToProcess, itemsToSkip, skippedCount: itemsToSkip.length, + itemsToFail, }; } diff --git a/src/lib/pushers/content-pusher/util/resolve-effective-target-model.ts b/src/lib/pushers/content-pusher/util/resolve-effective-target-model.ts new file mode 100644 index 00000000..cfdefe7b --- /dev/null +++ b/src/lib/pushers/content-pusher/util/resolve-effective-target-model.ts @@ -0,0 +1,55 @@ +import { getEffectiveTargetModel } from "core/state"; +import { ModelMapper } from "lib/mappers/model-mapper"; + +/** + * PROD-2542: resolve the model a content item will be validated against on the target, by its + * source definitionName. Lookup order: + * 1. the run-scoped registry (a model saved — or, in preflight, planned — earlier in this run; + * the target models/.json cache is NOT rewritten after saveModel, so it would be stale), + * 2. the target model cache via the model mapping, + * 3. the source model via the model mapping, + * 4. none — the caller skips validation. + * + * Results are memoized per resolver instance; build one per filter pass, not per item. + */ +export function createEffectiveTargetModelResolver( + sourceGuid: string, + targetGuid: string +): (definitionName: string) => any | null { + const cache = new Map(); + let modelMapper: ModelMapper | null | undefined; + + const getModelMapper = (): ModelMapper | null => { + if (modelMapper === undefined) { + try { + modelMapper = sourceGuid && targetGuid ? new ModelMapper(sourceGuid, targetGuid) : null; + } catch { + modelMapper = null; + } + } + return modelMapper; + }; + + return (definitionName: string) => { + if (!definitionName) return null; + const key = definitionName.toLowerCase(); + if (cache.has(key)) return cache.get(key); + + let model: any | null = getEffectiveTargetModel(definitionName) || null; + if (!model) { + try { + const mapper = getModelMapper(); + const mapping = mapper?.getModelMappingByReferenceName(definitionName, "source") || null; + if (mapper && mapping) { + model = mapper.getMappedEntity(mapping, "target") || mapper.getMappedEntity(mapping, "source") || null; + } + } catch { + // Best-effort: an unreadable cache just means no validation for this model. + model = null; + } + } + + cache.set(key, model); + return model; + }; +} diff --git a/src/lib/pushers/content-pusher/util/tests/filter-content-items-for-processing.test.ts b/src/lib/pushers/content-pusher/util/tests/filter-content-items-for-processing.test.ts index 99b69d41..6f0a2a18 100644 --- a/src/lib/pushers/content-pusher/util/tests/filter-content-items-for-processing.test.ts +++ b/src/lib/pushers/content-pusher/util/tests/filter-content-items-for-processing.test.ts @@ -1,4 +1,5 @@ -import { resetState, setState } from "core/state"; +import { resetState, setState, registerEffectiveTargetModel } from "core/state"; +import { preflightReport } from "lib/preflight/preflight-report"; // Mock findContentInTargetInstance so we can control its return value jest.mock("../find-content-in-target-instance", () => ({ @@ -293,3 +294,128 @@ describe("filterContentItemsForProcessing — verbose logging", () => { expect(logSpy).not.toHaveBeenCalled(); }); }); + +// ─── PROD-2542: required-field validation against the effective target model ─── + +describe("filterContentItemsForProcessing — required field validation (PROD-2542)", () => { + const promoBanner = { + referenceName: "PromoBanner", + fields: [ + { name: "enabled", type: "Boolean", settings: {} }, + { name: "promoTitle", type: "Text", settings: { Required: "True" } }, + { name: "promoSubtitle", type: "Text", settings: { Required: "True" } }, + { name: "promoImage", type: "ImageAttachment", settings: { Required: "True" } }, + ], + }; + + function bannerItem(id: number, fields: Record): any { + return { + contentID: id, + properties: { referenceName: `banner-${id}`, definitionName: "PromoBanner", versionID: 1 }, + fields, + }; + } + + const wouldCreate = { + content: null, + shouldCreate: true, + shouldUpdate: false, + shouldSkip: false, + isConflict: false, + }; + + beforeEach(() => { + preflightReport.reset(); + registerEffectiveTargetModel("PromoBanner", promoBanner); + }); + + it("routes an item missing a required scalar to itemsToFail, never itemsToProcess", async () => { + mockFind.mockReturnValue(wouldCreate); + const bad = bannerItem(12942, { enabled: "true" }); + const result = await filterContentItemsForProcessing(makeBaseProps([bad])); + + expect(result.itemsToProcess).toHaveLength(0); + expect(result.itemsToSkip).toHaveLength(0); + expect(result.itemsToFail).toHaveLength(1); + expect(result.itemsToFail[0].item).toBe(bad); + expect(result.itemsToFail[0].model).toBe("PromoBanner"); + expect(result.itemsToFail[0].missing).toEqual([ + { name: "promoTitle", type: "Text" }, + { name: "promoSubtitle", type: "Text" }, + ]); + expect(result.itemsToFail[0].error).toBe( + "Missing required field(s) on model PromoBanner: promoTitle (Text), promoSubtitle (Text) — skipped client-side" + ); + }); + + it("validates updates too", async () => { + mockFind.mockReturnValue({ ...wouldCreate, shouldCreate: false, shouldUpdate: true }); + const result = await filterContentItemsForProcessing(makeBaseProps([bannerItem(1, { promoTitle: null })])); + expect(result.itemsToFail).toHaveLength(1); + expect(result.itemsToProcess).toHaveLength(0); + }); + + it("does not validate items that are skipped as up to date", async () => { + mockFind.mockReturnValue({ ...wouldCreate, shouldCreate: false, shouldSkip: true }); + const result = await filterContentItemsForProcessing(makeBaseProps([bannerItem(1, {})])); + expect(result.itemsToFail).toHaveLength(0); + expect(result.itemsToSkip).toHaveLength(1); + }); + + it("keeps valid items (in order) and logs warnings without blocking", async () => { + mockFind.mockReturnValue(wouldCreate); + const logger = { ...makeLogger(), warning: jest.fn() }; + const items = [ + bannerItem(1, { promoTitle: "a", promoSubtitle: "b" }), + bannerItem(2, { enabled: "true" }), + bannerItem(3, { PROMOTITLE: "c", promosubtitle: "" }), + bannerItem(4, { promoTitle: "d", promoSubtitle: "e" }), + ]; + const result = await filterContentItemsForProcessing(makeBaseProps(items, { logger })); + + expect(result.itemsToProcess.map((i: any) => i.contentID)).toEqual([1, 3, 4]); + expect(result.itemsToFail.map((f) => f.item.contentID)).toEqual([2]); + // promoImage (ImageAttachment) missing on every item + the empty promoSubtitle on item 3 + expect(logger.warning).toHaveBeenCalledWith(expect.stringContaining("promoSubtitle (Text) is empty")); + expect(logger.warning).toHaveBeenCalledWith(expect.stringContaining("promoImage (ImageAttachment) has no value")); + }); + + it("skips validation when no model can be resolved", async () => { + mockFind.mockReturnValue(wouldCreate); + const item = makeContentItem(1); // definitionName "Model" — not registered, no mapping on disk + const result = await filterContentItemsForProcessing(makeBaseProps([item])); + expect(result.itemsToProcess).toEqual([item]); + expect(result.itemsToFail).toHaveLength(0); + }); + + it("returns an empty itemsToFail for empty input", async () => { + const result = await filterContentItemsForProcessing(makeBaseProps([])); + expect(result.itemsToFail).toEqual([]); + }); + + it("records an invalid (not create) preflight entry with the reason", async () => { + setState({ preflight: true }); + registerEffectiveTargetModel("PromoBanner", promoBanner); + mockFind.mockReturnValue(wouldCreate); + await filterContentItemsForProcessing( + makeBaseProps([bannerItem(1, { enabled: "true" }), bannerItem(2, { promoTitle: "a", promoSubtitle: "b" })]) + ); + + const entries = preflightReport.getEntries(); + expect(entries).toEqual([ + expect.objectContaining({ + phase: "Content", + action: "invalid", + name: "banner-1", + locale: "en-us", + detail: expect.stringContaining("would create but Missing required field(s) on model PromoBanner: promoTitle"), + }), + expect.objectContaining({ phase: "Content", action: "create", name: "banner-2" }), + ]); + const totals = preflightReport.getTotals(); + expect(totals.create).toBe(1); + expect(totals.invalid).toBe(1); + // An invalid item is a warning in the plan — it must not set the conflict exit code. + expect(preflightReport.hasConflicts()).toBe(false); + }); +}); diff --git a/src/lib/pushers/content-pusher/util/tests/resolve-effective-target-model.test.ts b/src/lib/pushers/content-pusher/util/tests/resolve-effective-target-model.test.ts new file mode 100644 index 00000000..4563c56c --- /dev/null +++ b/src/lib/pushers/content-pusher/util/tests/resolve-effective-target-model.test.ts @@ -0,0 +1,78 @@ +import * as fs from "fs"; +import * as os from "os"; +import * as path from "path"; +import { resetState, setState, registerEffectiveTargetModel } from "core/state"; +import { createEffectiveTargetModelResolver } from "../resolve-effective-target-model"; + +let tmpDir: string; + +beforeAll(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "agility-etm-")); +}); + +afterAll(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); +}); + +beforeEach(() => { + resetState(); + setState({ rootPath: tmpDir }); +}); + +function writeJson(rel: string, data: any): void { + const file = path.join(tmpDir, rel); + fs.mkdirSync(path.dirname(file), { recursive: true }); + fs.writeFileSync(file, JSON.stringify(data)); +} + +function seed(sourceGuid: string, targetGuid: string, opts: { targetFile: boolean }): void { + writeJson(`${sourceGuid}/models/10.json`, { id: 10, referenceName: "Post", fields: [{ name: "source" }] }); + if (opts.targetFile) { + writeJson(`${targetGuid}/models/50.json`, { id: 50, referenceName: "Post", fields: [{ name: "target" }] }); + } + writeJson(`mappings/${sourceGuid}-${targetGuid}/models/mappings.json`, [ + { + sourceGuid, + targetGuid, + sourceID: 10, + targetID: 50, + sourceReferenceName: "Post", + targetReferenceName: "Post", + sourceLastModifiedDate: "2025-01-01T00:00:00.000", + targetLastModifiedDate: "2025-01-01T00:00:00.000", + }, + ]); +} + +describe("createEffectiveTargetModelResolver (PROD-2542)", () => { + it("prefers the run-scoped registry over the (stale) target cache", () => { + seed("etm-src-1", "etm-tgt-1", { targetFile: true }); + const saved = { referenceName: "Post", fields: [{ name: "saved" }] }; + registerEffectiveTargetModel("Post", saved); + const resolve = createEffectiveTargetModelResolver("etm-src-1", "etm-tgt-1"); + expect(resolve("post")).toBe(saved); + }); + + it("falls back to the target model cache via the model mapping", () => { + seed("etm-src-2", "etm-tgt-2", { targetFile: true }); + const resolve = createEffectiveTargetModelResolver("etm-src-2", "etm-tgt-2"); + expect(resolve("Post").fields).toEqual([{ name: "target" }]); + }); + + it("falls back to the source model when the target cache file is missing", () => { + seed("etm-src-3", "etm-tgt-3", { targetFile: false }); + const resolve = createEffectiveTargetModelResolver("etm-src-3", "etm-tgt-3"); + expect(resolve("Post").fields).toEqual([{ name: "source" }]); + }); + + it("returns null when nothing is known about the model", () => { + const resolve = createEffectiveTargetModelResolver("etm-src-4", "etm-tgt-4"); + expect(resolve("Unknown")).toBeNull(); + expect(resolve("")).toBeNull(); + }); + + it("returns null without guids instead of throwing", () => { + const resolve = createEffectiveTargetModelResolver("", ""); + expect(resolve("Post")).toBeNull(); + }); +}); diff --git a/src/lib/pushers/content-pusher/util/tests/validate-required-fields.test.ts b/src/lib/pushers/content-pusher/util/tests/validate-required-fields.test.ts new file mode 100644 index 00000000..fac3ea2e --- /dev/null +++ b/src/lib/pushers/content-pusher/util/tests/validate-required-fields.test.ts @@ -0,0 +1,224 @@ +import { + validateContentItemAgainstModel, + parseDropdownChoices, + formatMissingRequiredFields, +} from "../validate-required-fields"; + +// Field definitions shaped like real pulled models (settings values are strings). +function field(name: string, type: string, settings: Record = {}): any { + return { name, type, label: name, settings }; +} + +// PromoBanner (model 362), trimmed: the item that tripped "Column 'promoTitle' does not allow nulls." +const promoBanner = { + referenceName: "PromoBanner", + fields: [ + field("tab_General", "Tab"), + field("enabled", "Boolean"), + field("promoTitle", "Text", { Required: "True", Length: "" }), + field("promoDescription", "HTML", { Required: "False" }), + field("promoVisibility", "DropdownList", { + Required: "True", + Choices: "Static|static\nDynamic|dynamic\nNone|none", + }), + field("promoSubtitle", "Text", { Required: "True", Length: "" }), + field("promoActionButton", "Link", { Required: "True" }), + field("balanceThreshold", "Decimal", { Required: "True" }), + ], +}; + +describe("validateContentItemAgainstModel — no model", () => { + it("returns no issues when the model is null/undefined", () => { + expect(validateContentItemAgainstModel({}, null)).toEqual({ missing: [], warnings: [] }); + expect(validateContentItemAgainstModel({}, undefined)).toEqual({ missing: [], warnings: [] }); + }); + + it("returns no issues when the model has no fields", () => { + expect(validateContentItemAgainstModel({}, { referenceName: "X" })).toEqual({ missing: [], warnings: [] }); + expect(validateContentItemAgainstModel({}, { referenceName: "X", fields: [] })).toEqual({ + missing: [], + warnings: [], + }); + }); + + it("tolerates null/undefined fields", () => { + const model = { fields: [field("title", "Text", { Required: "True" })] }; + expect(validateContentItemAgainstModel(null, model).missing).toEqual([{ name: "title", type: "Text" }]); + expect(validateContentItemAgainstModel(undefined, model).missing).toEqual([{ name: "title", type: "Text" }]); + }); +}); + +describe("validateContentItemAgainstModel — required scalar fields", () => { + it("fails the real item 12942 shape (only {enabled}) on every required scalar", () => { + const result = validateContentItemAgainstModel({ enabled: "true" }, promoBanner); + expect(result.missing).toEqual([ + { name: "promoTitle", type: "Text" }, + { name: "promoVisibility", type: "DropdownList" }, + { name: "promoSubtitle", type: "Text" }, + { name: "balanceThreshold", type: "Decimal" }, + ]); + // Link is not confirmed to be rejected server-side — a warning, not a failure. + expect(result.warnings).toEqual([expect.stringContaining("promoActionButton (Link)")]); + }); + + it("fails when the value is explicitly null", () => { + const model = { fields: [field("title", "Text", { Required: "True" })] }; + expect(validateContentItemAgainstModel({ title: null }, model).missing).toHaveLength(1); + }); + + it.each(["Text", "LongText", "HTML", "Html", "Integer", "Decimal", "Date", "DropdownList", "Boolean"])( + "treats %s as a scalar (missing → failure)", + (type) => { + const model = { fields: [field("f", type, { Required: "True" })] }; + const result = validateContentItemAgainstModel({}, model); + expect(result.missing).toEqual([{ name: "f", type }]); + expect(result.warnings).toHaveLength(0); + } + ); + + it.each(["ImageAttachment", "FileAttachment", "PhotoGallery", "CustomField", "Content", "Link"])( + "only warns for a missing required %s", + (type) => { + const model = { fields: [field("f", type, { Required: "True" })] }; + const result = validateContentItemAgainstModel({}, model); + expect(result.missing).toHaveLength(0); + expect(result.warnings).toEqual([`Required field f (${type}) has no value`]); + } + ); + + it.each(["True", "true", "TRUE", " true "])("reads Required %p as required", (required) => { + const model = { fields: [field("title", "Text", { Required: required })] }; + expect(validateContentItemAgainstModel({}, model).missing).toHaveLength(1); + }); + + it.each(["False", "false", "", undefined])("reads Required %p as not required", (required) => { + const settings = required === undefined ? {} : { Required: required }; + const model = { fields: [field("title", "Text", settings)] }; + expect(validateContentItemAgainstModel({}, model)).toEqual({ missing: [], warnings: [] }); + }); + + it("handles a field with no settings object", () => { + const model = { fields: [{ name: "title", type: "Text" }] }; + expect(validateContentItemAgainstModel({}, model)).toEqual({ missing: [], warnings: [] }); + }); + + it("matches item keys to model field names case-insensitively", () => { + const model = { + fields: [field("Title", "Text", { Required: "True" }), field("promoTitle", "Text", { Required: "True" })], + }; + expect(validateContentItemAgainstModel({ title: "x", PROMOTITLE: "y" }, model).missing).toHaveLength(0); + }); + + it("accepts falsy-but-present scalar values (0, false)", () => { + const model = { + fields: [field("count", "Integer", { Required: "True" }), field("flag", "Boolean", { Required: "True" })], + }; + expect(validateContentItemAgainstModel({ count: 0, flag: false }, model)).toEqual({ missing: [], warnings: [] }); + }); + + it("skips Tab fields even if marked required", () => { + const model = { fields: [field("tab_Main", "Tab", { Required: "True" })] }; + expect(validateContentItemAgainstModel({}, model)).toEqual({ missing: [], warnings: [] }); + }); + + it("ignores malformed field definitions", () => { + const model = { fields: [null, {}, { type: "Text" }, field("", "Text", { Required: "True" })] }; + expect(validateContentItemAgainstModel({}, model)).toEqual({ missing: [], warnings: [] }); + }); +}); + +describe("validateContentItemAgainstModel — warnings (never failures)", () => { + it("warns on an empty or whitespace-only required value", () => { + const model = { + fields: [field("title", "Text", { Required: "True" }), field("body", "LongText", { Required: "true" })], + }; + const result = validateContentItemAgainstModel({ title: "", body: " " }, model); + expect(result.missing).toHaveLength(0); + expect(result.warnings).toEqual([ + "Required field title (Text) is empty", + "Required field body (LongText) is empty", + ]); + }); + + it("warns when Text exceeds its Length", () => { + const model = { fields: [field("title", "Text", { Length: "5" })] }; + const result = validateContentItemAgainstModel({ title: "toolong" }, model); + expect(result.missing).toHaveLength(0); + expect(result.warnings).toEqual(["Field title (Text) is 7 characters, over its Length of 5"]); + }); + + it.each(["", "0", undefined])("treats Length %p as unlimited", (length) => { + const model = { fields: [field("title", "Text", { Length: length })] }; + expect(validateContentItemAgainstModel({ title: "x".repeat(5000) }, model).warnings).toHaveLength(0); + }); + + it("does not warn at exactly the Length", () => { + const model = { fields: [field("title", "Text", { Length: "5" })] }; + expect(validateContentItemAgainstModel({ title: "12345" }, model).warnings).toHaveLength(0); + }); + + it("warns when a DropdownList value is not among its Choices", () => { + const result = validateContentItemAgainstModel( + { + promoVisibility: "sometimes", + promoTitle: "a", + promoSubtitle: "b", + balanceThreshold: 1, + promoActionButton: "x", + }, + promoBanner + ); + expect(result.missing).toHaveLength(0); + expect(result.warnings).toEqual([ + 'Field promoVisibility (DropdownList) value "sometimes" is not one of its Choices', + ]); + }); + + it("accepts a DropdownList value matching a choice value, case-insensitively", () => { + const model = { fields: [field("vis", "DropdownList", { Choices: "Static|static\r\nDynamic|dynamic" })] }; + expect(validateContentItemAgainstModel({ vis: "Dynamic" }, model).warnings).toHaveLength(0); + }); + + it("does not check a DropdownList without Choices, or an empty optional value", () => { + const model = { + fields: [field("a", "DropdownList", { Choices: "" }), field("b", "DropdownList", { Choices: "x|x" })], + }; + expect(validateContentItemAgainstModel({ a: "anything", b: "" }, model).warnings).toHaveLength(0); + }); + + it("leaves optional missing fields alone", () => { + const model = { fields: [field("title", "Text", { Required: "False", Length: "3" })] }; + expect(validateContentItemAgainstModel({}, model)).toEqual({ missing: [], warnings: [] }); + }); +}); + +describe("parseDropdownChoices", () => { + it("parses the Label|value-per-line format from real models", () => { + expect(parseDropdownChoices("Standard|standard\nExplicit|explicit\nImage Only|image_only")).toEqual([ + "standard", + "explicit", + "image_only", + ]); + }); + + it("uses the whole line when there is no pipe, and drops blank lines", () => { + expect(parseDropdownChoices("one\r\n\ntwo|2\n")).toEqual(["one", "2"]); + }); + + it("returns [] for non-strings and empty strings", () => { + expect(parseDropdownChoices(undefined)).toEqual([]); + expect(parseDropdownChoices("")).toEqual([]); + expect(parseDropdownChoices(42)).toEqual([]); + }); +}); + +describe("formatMissingRequiredFields", () => { + it("lists each field with its type", () => { + expect( + formatMissingRequiredFields("PromoBanner", [ + { name: "promoTitle", type: "Text" }, + { name: "promoSubtitle", type: "Text" }, + ]) + ).toBe("Missing required field(s) on model PromoBanner: promoTitle (Text), promoSubtitle (Text)"); + }); +}); diff --git a/src/lib/pushers/content-pusher/util/validate-required-fields.ts b/src/lib/pushers/content-pusher/util/validate-required-fields.ts new file mode 100644 index 00000000..be9dc468 --- /dev/null +++ b/src/lib/pushers/content-pusher/util/validate-required-fields.ts @@ -0,0 +1,134 @@ +/** + * PROD-2542: validate a content item's fields against its (target) model before the payload is + * sent to the Management API. + * + * The batch engine enforces a Required field at the database ("Column 'promoTitle' does not allow + * nulls."), so an item missing one fails server-side with an opaque error — and, inside a batch, + * used to be reported against whichever item happened to share its index. Catching it here lets the + * CLI skip the item client-side with a precise reason. + * + * Deliberately conservative: only a missing/null value for a SCALAR required field is a failure (the + * DB rejection is confirmed for those). Attachments, galleries, linked content and custom fields are + * only warned about, since it isn't confirmed the API rejects them. Everything else (empty strings, + * over-length text, unknown dropdown values) is a warning and never blocks the item. + * + * Pure: no state, no I/O. Keep the exported signatures stable — another branch copies this file. + */ + +export interface RequiredFieldIssue { + name: string; + type: string; +} + +export interface FieldValidationResult { + missing: RequiredFieldIssue[]; + warnings: string[]; +} + +/** Field types whose missing required value is a confirmed server-side (DB) failure. Lower-cased. */ +const SCALAR_FIELD_TYPES = new Set([ + "text", + "longtext", + "html", + "integer", + "decimal", + "date", + "dropdownlist", + "boolean", +]); + +/** Model settings are strings ("True"/"true"/"False"/"") or absent — compare case-insensitively. */ +function isTruthySetting(value: unknown): boolean { + if (value === true) return true; + return typeof value === "string" && value.trim().toLowerCase() === "true"; +} + +/** + * Parse a DropdownList `settings.Choices` value. Real model data stores one choice per line as + * "Label|value" (e.g. "Static|static\nDynamic|dynamic"); a line without "|" is its own value. + */ +export function parseDropdownChoices(choices: unknown): string[] { + if (typeof choices !== "string" || choices.trim() === "") return []; + return choices + .split(/\r?\n/) + .map((line) => { + const pipe = line.indexOf("|"); + return (pipe >= 0 ? line.substring(pipe + 1) : line).trim(); + }) + .filter((value) => value !== ""); +} + +/** + * Check `fields` (a content item's field map, as pulled from the source) against `model`. + * Item field keys match model field names case-insensitively (model "Title" ↔ item "title"). + */ +export function validateContentItemAgainstModel( + fields: Record | null | undefined, + model: { referenceName?: string; fields?: any[] } | null | undefined +): FieldValidationResult { + const result: FieldValidationResult = { missing: [], warnings: [] }; + if (!model || !Array.isArray(model.fields) || model.fields.length === 0) return result; + + // Case-insensitive view of the item's fields (an absent key and an explicit null both count as missing). + const itemFields = new Map(); + for (const [key, value] of Object.entries(fields || {})) { + itemFields.set(key.toLowerCase(), value); + } + + for (const fieldDef of model.fields) { + if (!fieldDef || typeof fieldDef.name !== "string" || fieldDef.name === "") continue; + const type = String(fieldDef.type || ""); + const typeKey = type.toLowerCase(); + if (typeKey === "tab") continue; + + const settings = fieldDef.settings || {}; + const name: string = fieldDef.name; + const key = name.toLowerCase(); + const value = itemFields.get(key); + + if (isTruthySetting(settings.Required)) { + if (value === null || value === undefined) { + if (SCALAR_FIELD_TYPES.has(typeKey)) { + result.missing.push({ name, type }); + } else { + result.warnings.push(`Required field ${name} (${type}) has no value`); + } + continue; + } + if (typeof value === "string" && value.trim() === "") { + result.warnings.push(`Required field ${name} (${type}) is empty`); + continue; + } + } + + if (value === null || value === undefined) continue; + + // Text over its configured Length ("" / "0" mean unlimited). + if (typeKey === "text" && typeof value === "string") { + const maxLength = parseInt(String(settings.Length ?? ""), 10); + if (maxLength > 0 && value.length > maxLength) { + result.warnings.push(`Field ${name} (${type}) is ${value.length} characters, over its Length of ${maxLength}`); + } + } + + // DropdownList value not among the configured Choices (compared case-insensitively). + if (typeKey === "dropdownlist" && (typeof value === "string" || typeof value === "number")) { + const text = String(value).trim(); + const choices = parseDropdownChoices(settings.Choices); + if (text !== "" && choices.length > 0) { + const allowed = new Set(choices.map((c) => c.toLowerCase())); + if (!allowed.has(text.toLowerCase())) { + result.warnings.push(`Field ${name} (${type}) value "${text}" is not one of its Choices`); + } + } + } + } + + return result; +} + +/** "Missing required field(s) on model PromoBanner: promoTitle (Text), promoSubtitle (Text)" */ +export function formatMissingRequiredFields(modelName: string, missing: RequiredFieldIssue[]): string { + const list = missing.map((m) => `${m.name} (${m.type})`).join(", "); + return `Missing required field(s) on model ${modelName}: ${list}`; +} diff --git a/src/lib/pushers/model-pusher.ts b/src/lib/pushers/model-pusher.ts index 12c13945..9439fdce 100644 --- a/src/lib/pushers/model-pusher.ts +++ b/src/lib/pushers/model-pusher.ts @@ -1,5 +1,5 @@ import * as mgmtApi from "@agility/management-sdk"; -import { getApiClient, state, getLoggerForGuid } from "../../core/state"; +import { getApiClient, state, getLoggerForGuid, registerEffectiveTargetModel } from "../../core/state"; import { PusherResult, FailureDetail } from "../../types/sourceData"; import { ModelMapper } from "lib/mappers/model-mapper"; import { Logs } from "core/logs"; @@ -405,11 +405,14 @@ export async function pushModels(sourceData: mgmtApi.Model[], targetData: mgmtAp // A "target has changed" skip is surfaced as a conflict since a real sync would // need --overwrite to proceed. if (state.preflight) { + // PROD-2542: a real run would save the source definition, so content is validated against it. for (const model of shouldCreateStub) { preflightReport.record({ phase: "Models", action: "create", name: model.referenceName }); + registerEffectiveTargetModel(model.referenceName, model); } for (const model of shouldUpdateFields) { preflightReport.record({ phase: "Models", action: "update", name: model.referenceName }); + registerEffectiveTargetModel(model.referenceName, model); } for (const { model, reason } of shouldSkip) { const isConflict = /target model has changed|no longer exists on the target/i.test(reason); @@ -568,6 +571,12 @@ async function updateExistingModel( const updatedModel = await apiClient.modelMethods.saveModel(updatePayload, targetGuid); logger.model.updated(sourceModel, "updated", targetGuid); referenceMapper.addMapping(sourceModel, updatedModel); + // PROD-2542: the target models/.json cache isn't rewritten here, so record what was saved + // for content validation later in this run (the payload, if the response omits the fields). + registerEffectiveTargetModel( + sourceModel.referenceName, + Array.isArray(updatedModel?.fields) ? updatedModel : updatePayload + ); return "updated"; } catch (error: any) { // PROD-2211: saveModel can reject even though the field update was persisted server-side. Re-query @@ -578,6 +587,7 @@ async function updateExistingModel( if (recovered && (recovered.fields?.length ?? 0) === (sourceModel.fields?.length ?? 0)) { logger.model.updated(sourceModel, "updated", targetGuid); referenceMapper.addMapping(sourceModel, recovered); + registerEffectiveTargetModel(sourceModel.referenceName, recovered); return "updated"; } logger.model.error(sourceModel, error, targetGuid); diff --git a/src/lib/pushers/page-pusher/process-page.ts b/src/lib/pushers/page-pusher/process-page.ts index 593ce288..927c19db 100644 --- a/src/lib/pushers/page-pusher/process-page.ts +++ b/src/lib/pushers/page-pusher/process-page.ts @@ -195,11 +195,15 @@ export async function processPage({ // Preflight (PROD-2203): we've decided this page would be created or updated. // Record the planned action and skip the actual write to the target. if (state.preflight) { + // PROD-2542: content the content phase would fail (e.g. skipped for a missing required field) + // is in the failed-content registry; a real run drops the modules that point at it. + const wouldDrop = countModulesReferencingFailedContent(page, locale); preflightReport.record({ phase: "Pages", action: createRequired ? "create" : "update", name: page.name, locale, + ...(wouldDrop > 0 ? { detail: `${wouldDrop} component(s) would be dropped (content would fail)` } : {}), }); return { status: "success" }; } @@ -249,7 +253,7 @@ export async function processPage({ newZoneContent.push(newModule); } else { // Content mapping failed - check why (in priority order) - const failedContent = getFailedContent(sourceContentId); + const failedContent = getFailedContent(sourceContentId, locale); const isPageUnpublished = page.properties?.state !== 2; const existsInSource = contentExistsInSourceData(sourceGuid, locale, sourceContentId); const otherLocale = !existsInSource @@ -600,3 +604,19 @@ export async function processPage({ return { status: "failure", error: error.message }; } } + +/** + * PROD-2542: number of the page's modules whose source content is registered as failed in `locale`. + * Used by preflight to preview the modules a real run would drop. + */ +export function countModulesReferencingFailedContent(page: mgmtApi.PageItem, locale: string): number { + let count = 0; + for (const zoneModules of Object.values(page?.zones || {})) { + if (!Array.isArray(zoneModules)) continue; + for (const module of zoneModules as any[]) { + const sourceContentId = module?.item?.contentid || module?.item?.contentId; + if (sourceContentId > 0 && getFailedContent(sourceContentId, locale)) count++; + } + } + return count; +} diff --git a/src/lib/pushers/page-pusher/tests/process-page.test.ts b/src/lib/pushers/page-pusher/tests/process-page.test.ts index 07945f47..d1af91bb 100644 --- a/src/lib/pushers/page-pusher/tests/process-page.test.ts +++ b/src/lib/pushers/page-pusher/tests/process-page.test.ts @@ -1,8 +1,9 @@ import * as fs from "fs"; import * as os from "os"; import * as path from "path"; -import { resetState, setState } from "core/state"; -import { processPage } from "../process-page"; +import { resetState, setState, registerFailedContent, clearFailedContentRegistry } from "core/state"; +import { processPage, countModulesReferencingFailedContent } from "../process-page"; +import { preflightReport } from "lib/preflight/preflight-report"; // Mock all modules that make real network or disk calls from within processPage jest.mock("../find-page-in-other-locale", () => ({ @@ -705,3 +706,103 @@ describe("processPage — channel resolution", () => { expect(["success", "failure", "skip"]).toContain(result.status); }); }); + +// ─── PROD-2542: locale-scoped failed-content lookups ───────────────────────── + +describe("processPage — failed content is looked up per locale (PROD-2542)", () => { + beforeEach(() => clearFailedContentRegistry()); + + function setup() { + const { TemplateMapper } = require("lib/mappers/template-mapper"); + TemplateMapper.mockImplementation(() => ({ + getTemplateMappingByPageTemplateName: jest.fn().mockReturnValue({ ref: "Main" }), + getMappedEntity: jest.fn().mockReturnValue({ + contentSectionDefinitions: [{ pageItemTemplateReferenceName: "Main", itemOrder: 0 }], + }), + })); + const { ContentItemMapper } = require("lib/mappers/content-item-mapper"); + ContentItemMapper.mockImplementation(() => ({ + getContentItemMappingByContentID: jest.fn((id: number) => (id === 55 ? { targetContentID: 955 } : null)), + })); + mockExtract.mockReturnValue({ + successfulItems: [{ newId: 400, newItem: { processedItemVersionID: 7 } }], + failedItems: [], + }); + } + + const page = () => + makePage({ + zones: { + Main: [ + { module: "Hero", item: { contentid: 55 } }, + { module: "PromoBanner", item: { contentid: 12942 } }, + ], + }, + }); + + it("does not label an en-us module with an es-us failure", async () => { + setup(); + registerFailedContent(12942, "es-banner", "es-us only failure", "es-us"); + + const result = await processPage(makeProps({ page: page(), locale: "en-us" })); + + expect(result.warnings).toHaveLength(1); + expect(result.warnings![0].error).not.toContain("failed earlier"); + expect(result.warnings![0].error).not.toContain("es-us only failure"); + }); + + it("explains the dropped module with the same-locale failure", async () => { + setup(); + registerFailedContent( + 12942, + "home_promobanner", + "Missing required field(s) on model PromoBanner: promoTitle (Text) — skipped client-side", + "en-us" + ); + + const result = await processPage(makeProps({ page: page(), locale: "en-us" })); + + expect(result.warnings![0].error).toContain( + "content 'home_promobanner' failed earlier: Missing required field(s) on model PromoBanner: promoTitle (Text)" + ); + }); + + it("notes in preflight how many components would be dropped", async () => { + setup(); + setState({ preflight: true }); + preflightReport.reset(); + registerFailedContent(12942, "home_promobanner", "Missing required field(s)", "en-us"); + + const apiClient = makeApiClient(); + const result = await processPage(makeProps({ page: page(), locale: "en-us", apiClient })); + + expect(result.status).toBe("success"); + expect(apiClient.pageMethods.savePage).not.toHaveBeenCalled(); + expect(preflightReport.getEntries()).toEqual([ + expect.objectContaining({ + phase: "Pages", + name: "Test Page", + detail: "1 component(s) would be dropped (content would fail)", + }), + ]); + }); +}); + +describe("countModulesReferencingFailedContent (PROD-2542)", () => { + beforeEach(() => clearFailedContentRegistry()); + + it("counts modules (contentid or contentId) whose content failed in the given locale", () => { + registerFailedContent(1, "a", "err", "en-us"); + registerFailedContent(2, "b", "err", "en-us"); + registerFailedContent(3, "c", "err", "es-us"); + const page = makePage({ + zones: { + Main: [{ item: { contentid: 1 } }, { item: { contentId: 2 } }, { item: { contentid: 3 } }, { item: null }], + Side: "not-an-array", + }, + }); + expect(countModulesReferencingFailedContent(page, "en-us")).toBe(2); + expect(countModulesReferencingFailedContent(page, "es-us")).toBe(1); + expect(countModulesReferencingFailedContent(makePage({ zones: undefined }), "en-us")).toBe(0); + }); +}); diff --git a/src/lib/pushers/tests/batch-polling.test.ts b/src/lib/pushers/tests/batch-polling.test.ts index 30e748d4..0c4e30b0 100644 --- a/src/lib/pushers/tests/batch-polling.test.ts +++ b/src/lib/pushers/tests/batch-polling.test.ts @@ -5,6 +5,8 @@ import { logBatchError, prettyException, formatBatchItemError, + logBatchErrors, + resolvePayloadForBatchItem, CompletedBatch, } from "../batch-polling"; import * as mgmtApi from "@agility/management-sdk"; @@ -373,3 +375,129 @@ describe("formatBatchItemError", () => { expect(formatBatchItemError(undefined, undefined)).toBe("Unknown error"); }); }); + +// ─── PROD-2542: verified alignment ─────────────────────────────────────────── + +describe("extractContentBatchResults — alignment guard (PROD-2542)", () => { + it("fails every item when batch.items is shorter than the items sent", () => { + const originals = [asContent({ contentID: 1 }), asContent({ contentID: 2 }), asContent({ contentID: 3 })]; + const batch = { batchID: 55, items: [{ itemID: 101 }, { itemID: 102 }] }; + const result = extractContentBatchResults(asBatch(batch), originals); + + expect(result.successfulItems).toHaveLength(0); + expect(result.failedItems.map((f) => f.originalItem)).toEqual(originals); + result.failedItems.forEach((f) => { + expect(f.error).toContain("Unverifiable batch result alignment"); + expect(f.error).toContain("returned 2 item(s) for 3 sent"); + }); + }); + + it("fails every item when batch.items is longer than the items sent", () => { + const batch = { items: [{ itemID: 101 }, { itemID: 102 }] }; + const result = extractContentBatchResults(asBatch(batch), [asContent({ contentID: 1 })]); + expect(result.successfulItems).toHaveLength(0); + expect(result.failedItems).toHaveLength(1); + }); + + it("applies to page batches too", () => { + const batch = { items: [] as any[] }; + const result = extractPageBatchResults(asBatch(batch), [{ pageID: 1 }] as mgmtApi.PageItem[]); + expect(result.failedItems[0].error).toContain("Unverifiable batch result alignment"); + }); + + it("fails an item whose returned itemID differs from the expected (update) ID", () => { + const originals = [asContent({ contentID: 1 }), asContent({ contentID: 2 })]; + const batch = { + items: [ + { batchItemID: 1, itemID: 600 }, + { batchItemID: 2, itemID: 700 }, + ], + }; + const result = extractContentBatchResults(asBatch(batch), originals, [500, 700]); + + expect(result.successfulItems).toHaveLength(1); + expect(result.successfulItems[0].originalItem).toBe(originals[1]); + expect(result.failedItems).toHaveLength(1); + expect(result.failedItems[0].originalItem).toBe(originals[0]); + expect(result.failedItems[0].error).toContain("sent as ID 500 but the batch returned itemID 600"); + }); + + it("does not cross-check creates (no/negative expected ID)", () => { + const batch = { + items: [ + { batchItemID: 1, itemID: 900 }, + { batchItemID: 2, itemID: 901 }, + ], + }; + const result = extractContentBatchResults( + asBatch(batch), + [asContent({ contentID: 1 }), asContent({ contentID: 2 })], + [-1, undefined] + ); + expect(result.successfulItems.map((s) => s.newId)).toEqual([900, 901]); + }); + + it("leaves a failed item's own error alone when the expected ID can't be compared", () => { + const batch = { items: [{ batchItemID: 1, itemID: 0, errorMessage: "Column 'promoTitle' does not allow nulls." }] }; + const result = extractContentBatchResults(asBatch(batch), [asContent({ contentID: 1 })], [500]); + expect(result.failedItems[0].error).toBe("Column 'promoTitle' does not allow nulls."); + }); +}); + +describe("logBatchErrors — names the right item (PROD-2542)", () => { + const payloads = [ + { properties: { referenceName: "first" } }, + { properties: { referenceName: "second" } }, + { properties: { referenceName: "third" } }, + ]; + + it("resolves failedItems via batchItemId → position in batch.items, not position in the failed list", () => { + // Item 1 succeeded, item 3 failed: failedItems[0] is the THIRD payload. The old fallback + // (index within failedItems) named "first". + const batch = { + items: [ + { batchItemID: 11, itemID: 101 }, + { batchItemID: 12, itemID: 102 }, + { batchItemID: 13, itemID: 0 }, + ], + failedItems: [{ batchItemId: 13, itemId: 0, itemType: "Content", errorType: "SqlException", errorMessage: "boom" }], + }; + logBatchErrors(asBatch(batch), payloads); + + const logged = (console.error as jest.Mock).mock.calls.map((c) => String(c[0])).join("\n"); + expect(logged).toContain("(third)"); + expect(logged).not.toContain("(first)"); + }); + + it("logs 'unknown' rather than guessing when the item can't be located", () => { + const batch = { + items: [{ batchItemID: 11, itemID: 0 }], + failedItems: [{ batchItemId: 99, itemId: 0, itemType: "Content", errorType: "Error", errorMessage: "boom" }], + }; + logBatchErrors(asBatch(batch), payloads.slice(0, 1)); + const logged = (console.error as jest.Mock).mock.calls.map((c) => String(c[0])).join("\n"); + expect(logged).toContain("(unknown)"); + }); + + it("works for a single-page batch (page pusher passes [payload])", () => { + const batch = { + items: [{ batchItemID: 5, itemID: 0 }], + failedItems: [{ batchItemId: 5, itemId: 0, itemType: "Page", errorType: "Error", errorMessage: "bad zone" }], + }; + logBatchErrors(asBatch(batch), [{ name: "home" }]); + const logged = (console.error as jest.Mock).mock.calls.map((c) => String(c[0])).join("\n"); + expect(logged).toContain("(home)"); + }); +}); + +describe("resolvePayloadForBatchItem", () => { + it("returns undefined when items and payloads differ in length", () => { + const batch = { items: [{ batchItemID: 1 }, { batchItemID: 2 }] }; + expect(resolvePayloadForBatchItem(asBatch(batch), 1, [{ a: 1 }])).toBeUndefined(); + }); + + it("matches batchItemId loosely across number/string", () => { + const batch = { items: [{ batchItemID: 1 }, { batchItemID: 2 }] }; + expect(resolvePayloadForBatchItem(asBatch(batch), "2", [{ a: 1 }, { a: 2 }])).toEqual({ a: 2 }); + }); +}); diff --git a/src/lib/pushers/tests/model-pusher.test.ts b/src/lib/pushers/tests/model-pusher.test.ts index 4a944b5a..76fe0933 100644 --- a/src/lib/pushers/tests/model-pusher.test.ts +++ b/src/lib/pushers/tests/model-pusher.test.ts @@ -928,3 +928,58 @@ describe("pushModels — alternating-direction ping-pong (PROD-2604)", () => { expect(record.sourceLastModifiedDate).toBe(JAN); }); }); + +// ─── PROD-2542: effective target model registry ─────────────────────────────── + +describe("pushModels — effective target model registry (PROD-2542)", () => { + const requiredTitle = [{ name: "Title", type: "Text", settings: { Required: "True" } }]; + + it("registers the saved model after a create + field update", async () => { + const saved = makeModel({ id: 88, referenceName: "PromoBanner", fields: requiredTitle }); + const saveModel = jest + .fn() + .mockResolvedValueOnce(makeModel({ id: 88, referenceName: "PromoBanner", fields: [] })) // stub + .mockResolvedValueOnce(saved); // field update + jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel)); + + const { pushModels } = await import("../model-pusher"); + await pushModels([makeModel({ referenceName: "PromoBanner", fields: requiredTitle })], []); + + expect(stateModule.getEffectiveTargetModel("promobanner")).toBe(saved); + }); + + it("registers the sent payload when the save response carries no fields", async () => { + const saveModel = jest.fn().mockResolvedValue({ id: 89, referenceName: "NoFields" }); + jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel)); + + const { pushModels } = await import("../model-pusher"); + await pushModels([makeModel({ referenceName: "NoFields", fields: requiredTitle })], []); + + const registered = stateModule.getEffectiveTargetModel("NoFields"); + expect(registered.fields).toEqual(requiredTitle); + expect(registered.id).toBe(89); + }); + + it("registers nothing when the save fails", async () => { + const saveModel = jest.fn().mockRejectedValue(new Error("API error")); + jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel)); + + const { pushModels } = await import("../model-pusher"); + await pushModels([makeModel({ referenceName: "Broken", fields: requiredTitle })], []); + + expect(stateModule.getEffectiveTargetModel("Broken")).toBeUndefined(); + }); + + it("registers the source model for a planned create under --preflight (no saveModel)", async () => { + setState({ preflight: true }); + const saveModel = jest.fn(); + jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel)); + + const { pushModels } = await import("../model-pusher"); + const source = makeModel({ referenceName: "Planned", fields: requiredTitle }); + await pushModels([source], []); + + expect(saveModel).not.toHaveBeenCalled(); + expect(stateModule.getEffectiveTargetModel("Planned")).toBe(source); + }); +}); diff --git a/src/tests/sync/scenarios/17-required-field-missing-in-batch/scenario.json b/src/tests/sync/scenarios/17-required-field-missing-in-batch/scenario.json new file mode 100644 index 00000000..a818ba66 --- /dev/null +++ b/src/tests/sync/scenarios/17-required-field-missing-in-batch/scenario.json @@ -0,0 +1,21 @@ +{ + "name": "17-required-field-missing-in-batch", + "description": "PROD-2542: four source items of a model whose Title is Required (settings.Required \"True\"); the second item has no title key at all. It must be rejected client-side by filterContentItemsForProcessing and never reach the batch, and the three survivors must map to the right target IDs (no index shift onto the wrong source item).", + "sourceGuid": "src-1", + "targetGuid": "tgt-1", + "locale": "en-us", + "expect": { + "apiCalls": { + "saveContentItems": 1 + }, + "mappings": { + "item": [ + { "sourceContentID": 1000, "targetContentID": 9001 }, + { "sourceContentID": 1200, "targetContentID": 9002 }, + { "sourceContentID": 1300, "targetContentID": 9003 } + ] + }, + "noDuplicateMappingsBySourceID": true + }, + "base": "_base" +} diff --git a/src/tests/sync/scenarios/17-required-field-missing-in-batch/state/src-1/en-us/item/1000.json b/src/tests/sync/scenarios/17-required-field-missing-in-batch/state/src-1/en-us/item/1000.json new file mode 100644 index 00000000..2ab4cf3c --- /dev/null +++ b/src/tests/sync/scenarios/17-required-field-missing-in-batch/state/src-1/en-us/item/1000.json @@ -0,0 +1,13 @@ +{ + "contentID": 1000, + "properties": { + "referenceName": "Posts", + "definitionName": "Post", + "versionID": 1, + "state": 2, + "itemOrder": 0 + }, + "fields": { "title": "Post 1000", "slug": "post-1000" }, + "seo": null, + "scripts": null +} diff --git a/src/tests/sync/scenarios/17-required-field-missing-in-batch/state/src-1/en-us/item/1100.json b/src/tests/sync/scenarios/17-required-field-missing-in-batch/state/src-1/en-us/item/1100.json new file mode 100644 index 00000000..c54a36ee --- /dev/null +++ b/src/tests/sync/scenarios/17-required-field-missing-in-batch/state/src-1/en-us/item/1100.json @@ -0,0 +1,13 @@ +{ + "contentID": 1100, + "properties": { + "referenceName": "Posts", + "definitionName": "Post", + "versionID": 1, + "state": 2, + "itemOrder": 0 + }, + "fields": { "slug": "no-title" }, + "seo": null, + "scripts": null +} diff --git a/src/tests/sync/scenarios/17-required-field-missing-in-batch/state/src-1/en-us/item/1200.json b/src/tests/sync/scenarios/17-required-field-missing-in-batch/state/src-1/en-us/item/1200.json new file mode 100644 index 00000000..bb21df77 --- /dev/null +++ b/src/tests/sync/scenarios/17-required-field-missing-in-batch/state/src-1/en-us/item/1200.json @@ -0,0 +1,13 @@ +{ + "contentID": 1200, + "properties": { + "referenceName": "Posts", + "definitionName": "Post", + "versionID": 1, + "state": 2, + "itemOrder": 0 + }, + "fields": { "title": "Post 1200", "slug": "post-1200" }, + "seo": null, + "scripts": null +} diff --git a/src/tests/sync/scenarios/17-required-field-missing-in-batch/state/src-1/en-us/item/1300.json b/src/tests/sync/scenarios/17-required-field-missing-in-batch/state/src-1/en-us/item/1300.json new file mode 100644 index 00000000..a61ed1c6 --- /dev/null +++ b/src/tests/sync/scenarios/17-required-field-missing-in-batch/state/src-1/en-us/item/1300.json @@ -0,0 +1,13 @@ +{ + "contentID": 1300, + "properties": { + "referenceName": "Posts", + "definitionName": "Post", + "versionID": 1, + "state": 2, + "itemOrder": 0 + }, + "fields": { "title": "Post 1300", "slug": "post-1300" }, + "seo": null, + "scripts": null +} diff --git a/src/tests/sync/scenarios/17-required-field-missing-in-batch/state/src-1/models/10.json b/src/tests/sync/scenarios/17-required-field-missing-in-batch/state/src-1/models/10.json new file mode 100644 index 00000000..67c04ef4 --- /dev/null +++ b/src/tests/sync/scenarios/17-required-field-missing-in-batch/state/src-1/models/10.json @@ -0,0 +1,13 @@ +{ + "id": 10, + "displayName": "Post", + "referenceName": "Post", + "description": "Defines a blog post.", + "lastModifiedDate": "2025-01-01T00:00:00.000", + "lastModifiedBy": "Test", + "contentDefinitionTypeName": "Content List", + "fields": [ + { "type": "Text", "name": "Title", "label": "Title", "settings": { "Required": "True", "Length": "" } }, + { "type": "Text", "name": "Slug", "label": "URL Slug", "settings": { "Required": "False" } } + ] +} diff --git a/src/types/sourceData.ts b/src/types/sourceData.ts index d1c6d3b5..c2e1be80 100644 --- a/src/types/sourceData.ts +++ b/src/types/sourceData.ts @@ -54,6 +54,8 @@ export interface FailureDetail { mediaID?: number; // Source media ID for asset failures guid?: string; // Source instance GUID locale?: string; // Locale code + model?: string; // PROD-2542: model of a content item that failed client-side validation + fields?: string[]; // PROD-2542: required fields it was missing, as "name (Type)" } /**