From bab04cbabee2e95fb1004fcea9184b440a9aa6c1 Mon Sep 17 00:00:00 2001 From: Jules Exel Date: Wed, 22 Jul 2026 16:23:43 -0400 Subject: [PATCH] PROD-1492: halt sync on model/template mapping-validation errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related fixes so a mapping-inconsistency guard actually stops the sync instead of being swallowed and silently continuing. 1. Template validation was swallowed. The guid-level orchestrator only re-threw errors containing "Model validation failed", but the template guard throws "Page template validation failed" — so it fell through to a yellow warning and the sync kept going. Broaden the re-throw match to any "validation failed" so template (and future) guards hard-stop. 2. Duplicate model reference-name was undetected. When a source model is deleted and recreated (new ID, same referenceName), the mapping keeps two records sharing that name — one pointing at the dead source model. The content-side lookup is first-match-wins, so it can latch onto the dead record and SILENTLY skip that model's content, after which pages referencing it fail with "No content mapping". The ID-based rename guard misses this (different source IDs = duplicate name, not duplicate ID). Add ModelMapper.getDuplicateSourceReferenceNames() and a pre-push integrity gate in pushModels that throws "Model validation failed: duplicate model mapping ..." before any writes. Tests: new orchestrate-pushers test (template failure aborts before content/pages) and new model-pusher test (duplicate-name mapping throws and writes nothing). Added per-test mapping-file cleanup so seeded mappings no longer leak between model-pusher tests. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/lib/mappers/model-mapper.ts | 40 ++++++++++++++++ src/lib/pushers/model-pusher.ts | 23 +++++++++ src/lib/pushers/orchestrate-pushers.ts | 6 ++- src/lib/pushers/tests/model-pusher.test.ts | 48 +++++++++++++++++++ .../pushers/tests/orchestrate-pushers.test.ts | 27 +++++++++++ 5 files changed, 142 insertions(+), 2 deletions(-) diff --git a/src/lib/mappers/model-mapper.ts b/src/lib/mappers/model-mapper.ts index 9fff61f1..636d5fa3 100644 --- a/src/lib/mappers/model-mapper.ts +++ b/src/lib/mappers/model-mapper.ts @@ -57,6 +57,46 @@ export class ModelMapper { return mapping; } + /** + * PROD-1492: detect stale duplicate model mappings — two or more records that share a source + * `referenceName` but point at different source IDs. This happens when a source model is deleted + * and recreated (new ID, same reference name): the dead record lingers in the mapping alongside + * the live one. Because the content-side lookup (`getModelMappingByReferenceName`) is first-match- + * wins, it can latch onto the dead record, fail to read its (deleted) source model, and silently + * skip that model's content — after which every page referencing it fails. An Agility instance + * cannot have two models with the same reference name, so any such duplicate in the mapping is + * corruption, never a legitimate state. + * + * Returns one entry per offending reference name with its conflicting source IDs; empty when clean. + */ + getDuplicateSourceReferenceNames(): { referenceName: string; sourceIDs: number[] }[] { + // Group by lowercased reference name using a plain object + arrays (no Map/Set iteration, + // which would need tsconfig `downlevelIteration` this project doesn't enable). + const groups: Record = {}; + for (const m of this.mappings) { + if (!m.sourceReferenceName) continue; + const key = m.sourceReferenceName.toLowerCase(); + if (!groups[key]) { + groups[key] = { referenceName: m.sourceReferenceName, sourceIDs: [] }; + } + if (groups[key].sourceIDs.indexOf(m.sourceID) === -1) { + groups[key].sourceIDs.push(m.sourceID); + } + } + + const duplicates: { referenceName: string; sourceIDs: number[] }[] = []; + for (const key of Object.keys(groups)) { + const entry = groups[key]; + if (entry.sourceIDs.length > 1) { + duplicates.push({ + referenceName: entry.referenceName, + sourceIDs: entry.sourceIDs.slice().sort((a, b) => a - b), + }); + } + } + return duplicates; + } + getMappedEntity(mapping: ModelMapping, type: "source" | "target"): mgmtApi.Model | null { if (!mapping) return null; //fetch the model from the file system based on source or target GUID diff --git a/src/lib/pushers/model-pusher.ts b/src/lib/pushers/model-pusher.ts index b71f29c8..149cddf3 100644 --- a/src/lib/pushers/model-pusher.ts +++ b/src/lib/pushers/model-pusher.ts @@ -91,6 +91,29 @@ export async function pushModels(sourceData: mgmtApi.Model[], targetData: mgmtAp const referenceMapper = new ModelMapper(sourceGuid[0], targetGuid[0]); + // PROD-1492: fail fast on stale duplicate model mappings. When a source model is deleted and + // recreated (new ID, same reference name), the mapping ends up with two records sharing that name + // — one pointing at the dead source model, one at the live one. The content-side lookup is + // first-match-wins, so it can latch onto the dead record and SILENTLY skip that model's content + // (no per-item log), after which every page referencing it fails with "No content mapping". + // The ID-based rename guard below misses this because the two records have DIFFERENT source IDs + // (a duplicate reference name, not a duplicate ID). Throw a "Model validation failed" error so the + // orchestrator halts the sync before any partial push, rather than dropping content unnoticed. + const duplicateMappings = referenceMapper.getDuplicateSourceReferenceNames(); + if (duplicateMappings.length > 0) { + const detail = duplicateMappings + .map((d) => `"${d.referenceName}" (source IDs: ${d.sourceIDs.join(", ")})`) + .join("; "); + throw new Error( + `Model validation failed: duplicate model mapping detected for ${detail}. ` + + `Two mapping records share a reference name but point at different source model IDs — ` + + `this indicates a model that was deleted and recreated on the source, leaving a stale mapping. ` + + `Continuing would silently skip that model's content and fail the pages that reference it. ` + + `Stopping sync to avoid a partial push; remove the stale target model and re-sync. ` + + `Please contact AgilityCMS Support to resolve this issue` + ); + } + const apiClient = getApiClient(); let successful = 0; diff --git a/src/lib/pushers/orchestrate-pushers.ts b/src/lib/pushers/orchestrate-pushers.ts index 3bb6400b..6475172b 100644 --- a/src/lib/pushers/orchestrate-pushers.ts +++ b/src/lib/pushers/orchestrate-pushers.ts @@ -247,8 +247,10 @@ export class Pushers { } } } catch (error: any) { - // Re-throw validation errors immediately to stop sync - if (error?.message?.includes("Model validation failed")) { + // Re-throw validation errors immediately to stop sync. + // Matches any " validation failed" halt (e.g. "Model validation failed", + // "Page template validation failed") so every guard stops a partial push. + if (error?.message?.includes("validation failed")) { throw error; } // For other errors, log but don't stop (legacy behavior for guid-level ops) diff --git a/src/lib/pushers/tests/model-pusher.test.ts b/src/lib/pushers/tests/model-pusher.test.ts index ad0b069a..51e8de56 100644 --- a/src/lib/pushers/tests/model-pusher.test.ts +++ b/src/lib/pushers/tests/model-pusher.test.ts @@ -17,6 +17,11 @@ afterAll(() => { beforeEach(() => { resetState(); setState({ rootPath: tmpDir, sourceGuid: "src-model-u", targetGuid: "tgt-model-u", token: "test-token" }); + // Mapping files are stored centrally at /mappings and would otherwise ACCUMULATE across + // tests (shared tmpDir + fixed guids), leaking one test's seeded mappings into the next. Clear them + // so each test starts from an empty mapping — required now that pushModels validates the whole + // mapping for duplicate reference names (PROD-1492). + fs.rmSync(path.join(tmpDir, "mappings"), { recursive: true, force: true }); initializeGuidLogger("src-model-u", "push"); jest.spyOn(console, "log").mockImplementation(() => {}); jest.spyOn(console, "warn").mockImplementation(() => {}); @@ -204,6 +209,49 @@ describe("pushModels — source-side rename orphans a mapping and halts the sync }); }); +// ─── pushModels — duplicate reference-name mapping halts the sync (PROD-1492) ────── + +describe("pushModels — deleted-and-recreated model leaves a duplicate mapping and halts the sync (PROD-1492)", () => { + it('throws "Model validation failed" (and writes nothing) when two mapping records share a reference name with different source IDs', async () => { + const { ModelMapper } = await import("lib/mappers/model-mapper"); + + // Seed the mapping exactly as it looks after a deleted-and-recreated source model: + // dead source 46 -> target 138 and live source 48 -> target 139, both named "PromoBanner". + const seeder = new ModelMapper(state.sourceGuid[0], state.targetGuid[0]); + seeder.addMapping( + { id: 46, referenceName: "PromoBanner", lastModifiedDate: new Date(2025, 0, 1).toISOString() } as any, + { id: 138, referenceName: "PromoBanner", lastModifiedDate: new Date(2025, 0, 1).toISOString() } as any + ); + seeder.addMapping( + { id: 48, referenceName: "PromoBanner", lastModifiedDate: new Date(2025, 5, 1).toISOString() } as any, + { id: 139, referenceName: "PromoBanner", lastModifiedDate: new Date(2025, 5, 1).toISOString() } as any + ); + + const saveModel = jest.fn().mockResolvedValue(makeModel({ id: 999 })); + jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel)); + + const { pushModels } = await import("../model-pusher"); + + // Only the live model (48) still exists on the source; the dead record (46) is the stale duplicate. + const liveModel = makeModel({ + id: 48, + referenceName: "PromoBanner", + lastModifiedDate: new Date(2025, 5, 1).toISOString(), + }); + const targetModel = makeModel({ + id: 139, + referenceName: "PromoBanner", + lastModifiedDate: new Date(2025, 5, 1).toISOString(), + }); + + // The integrity gate must detect the duplicate reference name and stop the whole sync with a + // "Model validation failed" error — before any model is written. + await expect(pushModels([liveModel], [targetModel])).rejects.toThrow(/Model validation failed/); + + expect(saveModel).not.toHaveBeenCalled(); + }); +}); + // ─── PROD-2211: honest failure reporting ────────────────────────────────────── describe("pushModels — failed update is reported as failed, not success (PROD-2211)", () => { diff --git a/src/lib/pushers/tests/orchestrate-pushers.test.ts b/src/lib/pushers/tests/orchestrate-pushers.test.ts index f8bfe00a..8ef03235 100644 --- a/src/lib/pushers/tests/orchestrate-pushers.test.ts +++ b/src/lib/pushers/tests/orchestrate-pushers.test.ts @@ -360,4 +360,31 @@ describe("Pushers.instanceOrchestrator — models-first ordering (PROD-2202)", ( }), ]); }); + + it("a template-validation failure aborts the sync before content or pages are pushed (PROD-1492)", async () => { + setState({ sourceGuid: "src-u", targetGuid: "tgt-u", locales: "en-us" }); + await stubDataLoader(); + const spies = await stubAllHandlers(); + + // Templates run as a guid-level op; a mapping inconsistency must halt the sync just like models. + spies["pushTemplates"].mockRejectedValue( + new Error('Page template validation failed: mapping inconsistency for template "LeftSideBarTemplate" (ID: 2).') + ); + + const pushers = new Pushers(); + const results = await pushers.instanceOrchestrator(); + + // The templates handler ran and threw; content/pages in the locale loop were never reached. + expect(spies["pushTemplates"]).toHaveBeenCalledTimes(1); + expect(spies["pushContent"]).not.toHaveBeenCalled(); + expect(spies["pushPages"]).not.toHaveBeenCalled(); + + // The failure is recorded on the guid orchestration result and carries the validation message. + expect(results[0].failed).toEqual([ + expect.objectContaining({ + operation: "guid-orchestration", + error: expect.stringContaining("Page template validation failed"), + }), + ]); + }); });