From 0e771f0607f55ddeed2321bc7ff8ac5a106bc2d9 Mon Sep 17 00:00:00 2001 From: Kevin Date: Wed, 14 Oct 2026 14:13:36 -0400 Subject: [PATCH] PROD-2542: validate content against the target model before sending it; harden batch result alignment PROD-2542: locale-scoped failed-content registry and run-scoped effective target models - failedContentRegistry is keyed by locale + contentID, so an es-us failure no longer labels the en-us module that references the same source contentID. getFailedContent now takes the locale; process-page passes it. - New effectiveTargetModels registry in state (cleared per push run and by resetState). model-pusher registers the saved model after a successful create/update (the payload when the response has no fields), and the source model for planned creates/updates under --preflight. The target models/.json cache isn't rewritten after saveModel, so it is stale for the rest of the run. - Preflight page entries note "N component(s) would be dropped" when modules point at content registered as failed in that locale. PROD-2542: validate content against the target model before sending it New pure validator (util/validate-required-fields.ts): a Required field whose key is absent (matched case-insensitively) or null fails the item for scalar types (Text, LongText, HTML, Integer, Decimal, Date, DropdownList, Boolean) - the DB rejects these ("Column 'promoTitle' does not allow nulls."). Attachments, galleries, linked content, Link and custom fields only warn. Empty values, Text over its Length and DropdownList values outside Choices ("Label|value" lines) only warn. filterContentItemsForProcessing validates every item it would create/update against the effective target model (run registry -> target cache -> source model -> none) and routes failures to a new itemsToFail list, so they never reach a batch. Warnings are logged and the item proceeds. - Real runs: content-pusher folds itemsToFail into totalFailed and the failure details (with model + missing fields), so they reach the console/log, the ERROR SUMMARY and --jsonSummary, for both the linked and normal passes; each is registered as failed content immediately. - Preflight: recorded under a new "invalid" action (counted separately, shown with its reason, never a create/update, no exit-code change); JSON summary preflight totals/phases gain an additive "invalid" count. PROD-2542: guarantee batch result alignment; name the skipped child in cascades - prepareContentPayloads returns one paired {source, payload} list; the payloads sent and the sources results are matched against both derive from it. An empty prepared list skips the API call. If the save/poll throws, only the prepared items are failed (prep failures were already counted, and results already recorded are not re-failed). - extractBatchResults: batch.items must have one entry per item sent, or every item fails with "unverifiable batch result alignment" instead of guessing. For updates of a target item known to exist, a returned itemID that differs from the contentID sent fails that item. - logBatchErrors resolves a failed item via batchItemId -> position in batch.items -> payload (the old fallback used the index within the failed list and could name the wrong item); unresolvable -> "unknown". - "Unresolved content reference(s)" now appends "(child '' skipped: )" when the referenced child failed earlier in the same locale. Co-Authored-By: Claude Opus 5.5 --- src/core/json-summary.ts | 10 +- src/core/push.ts | 3 + src/core/state.ts | 57 +++- src/core/tests/json-summary.test.ts | 39 ++- src/core/tests/state.test.ts | 41 ++- src/lib/preflight/preflight-report.ts | 41 ++- .../preflight/tests/preflight-report.test.ts | 56 +++- src/lib/pushers/batch-polling.ts | 99 +++++- .../content-pusher/content-batch-processor.ts | 99 ++++-- .../pushers/content-pusher/content-pusher.ts | 38 ++- .../tests/content-batch-processor.test.ts | 296 +++++++++++++++--- .../tests/content-pusher-validation.test.ts | 197 ++++++++++++ .../filter-content-items-for-processing.ts | 84 ++++- .../util/resolve-effective-target-model.ts | 55 ++++ ...ilter-content-items-for-processing.test.ts | 128 +++++++- .../resolve-effective-target-model.test.ts | 78 +++++ .../tests/validate-required-fields.test.ts | 224 +++++++++++++ .../util/validate-required-fields.ts | 134 ++++++++ src/lib/pushers/model-pusher.ts | 12 +- src/lib/pushers/page-pusher/process-page.ts | 22 +- .../page-pusher/tests/process-page.test.ts | 105 ++++++- src/lib/pushers/tests/batch-polling.test.ts | 128 ++++++++ src/lib/pushers/tests/model-pusher.test.ts | 55 ++++ .../scenario.json | 21 ++ .../state/src-1/en-us/item/1000.json | 13 + .../state/src-1/en-us/item/1100.json | 13 + .../state/src-1/en-us/item/1200.json | 13 + .../state/src-1/en-us/item/1300.json | 13 + .../state/src-1/models/10.json | 13 + src/types/sourceData.ts | 2 + 30 files changed, 1969 insertions(+), 120 deletions(-) create mode 100644 src/lib/pushers/content-pusher/tests/content-pusher-validation.test.ts create mode 100644 src/lib/pushers/content-pusher/util/resolve-effective-target-model.ts create mode 100644 src/lib/pushers/content-pusher/util/tests/resolve-effective-target-model.test.ts create mode 100644 src/lib/pushers/content-pusher/util/tests/validate-required-fields.test.ts create mode 100644 src/lib/pushers/content-pusher/util/validate-required-fields.ts create mode 100644 src/tests/sync/scenarios/17-required-field-missing-in-batch/scenario.json create mode 100644 src/tests/sync/scenarios/17-required-field-missing-in-batch/state/src-1/en-us/item/1000.json create mode 100644 src/tests/sync/scenarios/17-required-field-missing-in-batch/state/src-1/en-us/item/1100.json create mode 100644 src/tests/sync/scenarios/17-required-field-missing-in-batch/state/src-1/en-us/item/1200.json create mode 100644 src/tests/sync/scenarios/17-required-field-missing-in-batch/state/src-1/en-us/item/1300.json create mode 100644 src/tests/sync/scenarios/17-required-field-missing-in-batch/state/src-1/models/10.json 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)" } /**