diff --git a/src/lib/pushers/model-pusher.ts b/src/lib/pushers/model-pusher.ts index 75f8f4b..b71f29c 100644 --- a/src/lib/pushers/model-pusher.ts +++ b/src/lib/pushers/model-pusher.ts @@ -20,6 +20,26 @@ function modelTypeMatches(a: mgmtApi.Model, b: mgmtApi.Model): boolean { return aType === bType; } +/** Human-readable model kind for messages. 1 = content, 2 = component/module. */ +function modelKindName(model: mgmtApi.Model): string { + const t = (model as any)?.contentDefinitionTypeID; + return t === 1 ? "content" : t === 2 ? "component/module" : `type ${t}`; +} + +/** + * PROD-2315: message for a cross-kind reference-name collision — a same-named model exists on both + * sides but as different kinds (content vs component/module). Agility forbids content and component + * models from sharing a reference name, so this can never be created; the user must reconcile it. + */ +function crossKindCollisionMessage(source: mgmtApi.Model, target: mgmtApi.Model): string { + return ( + `Model "${source.referenceName}" is a ${modelKindName(source)} model on the source, but a ` + + `${modelKindName(target)} model with that reference name already exists on the target. ` + + `Agility does not allow content and component models to share a reference name — ` + + `rename one of them (or remove the target model), then re-sync.` + ); +} + /** * Re-query the target for a model matching (referenceName, contentDefinitionTypeID). * @@ -82,6 +102,8 @@ export async function pushModels(sourceData: mgmtApi.Model[], targetData: mgmtAp let shouldUpdateFields = []; let shouldSkip: { model: mgmtApi.Model; reason: string }[] = []; let stubCreated = []; + // PROD-2315: same reference name on both sides but a different kind (content vs component). + const crossKindConflicts: { model: mgmtApi.Model; target: mgmtApi.Model }[] = []; // PROD-2250: process source models that already have a mapping before brand-new // (unmapped) models. Reconciling existing target models first — using their known @@ -180,6 +202,22 @@ export async function pushModels(sourceData: mgmtApi.Model[], targetData: mgmtAp } } + // PROD-2315: a target model with the same referenceName exists, but as a different KIND + // (content vs component/module). modelTypeMatches() already excluded it from + // targetModelByReference, so it would otherwise fall through to "create" and hit a guaranteed + // server 409 (Agility forbids content + component models sharing a name). Because + // targetModelByReference used modelTypeMatches — which returns true when either side's type is + // unknown — crossKindTarget is non-null ONLY when both types are known and differ. Type-unknown + // pulls are unaffected and keep today's behavior. + const crossKindTarget = + !sourceMapping && !targetModelByReference + ? targetData.find((t) => t.referenceName === sourceModel.referenceName) || null + : null; + if (crossKindTarget) { + crossKindConflicts.push({ model: sourceModel, target: crossKindTarget }); + continue; + } + if (!sourceMapping && !targetModel) { shouldCreateStub.push(sourceModel); continue; @@ -239,6 +277,15 @@ export async function pushModels(sourceData: mgmtApi.Model[], targetData: mgmtAp detail: reason, }); } + // PROD-2315: preview cross-kind collisions as conflicts (a real run would fail them). + for (const { model, target } of crossKindConflicts) { + preflightReport.record({ + phase: "Models", + action: "conflict", + name: model.referenceName, + detail: crossKindCollisionMessage(model, target), + }); + } return { status: "success", successful: shouldCreateStub.length + shouldUpdateFields.length, @@ -249,14 +296,18 @@ export async function pushModels(sourceData: mgmtApi.Model[], targetData: mgmtAp } for (const model of shouldCreateStub) { - const result = await createNewModel(model, referenceMapper, apiClient, targetGuid[0], logger); + const { result, error } = await createNewModel(model, referenceMapper, apiClient, targetGuid[0], logger); if (result === "created") { stubCreated.push(model); } else { failed++; failureDetails.push({ name: model.referenceName, - error: `Failed to create model "${model.referenceName}" (ID: ${model.id})`, + // PROD-2315: surface the server's reason (e.g. the 409 detail) in the ERROR SUMMARY instead + // of the generic message; the detail was previously only in the push-log file. + error: error + ? `Failed to create model "${model.referenceName}" (ID: ${model.id}): ${error}` + : `Failed to create model "${model.referenceName}" (ID: ${model.id})`, guid: sourceGuid[0], }); } @@ -294,6 +345,16 @@ export async function pushModels(sourceData: mgmtApi.Model[], targetData: mgmtAp skipped++; } + // PROD-2315: cross-kind reference-name collisions can never be created (Agility forbids content and + // component models sharing a name). Fail them with an actionable message that reaches the ERROR + // SUMMARY, rather than attempting a create that is guaranteed to 409. + for (const { model, target } of crossKindConflicts) { + const message = crossKindCollisionMessage(model, target); + logger.model.error(model, new Error(message), targetGuid[0]); + failed++; + failureDetails.push({ name: model.referenceName, error: message, guid: sourceGuid[0] }); + } + return { status: "success", successful, @@ -312,7 +373,7 @@ const createNewModel = async ( apiClient: mgmtApi.ApiClient, targetGuid: string, logger: Logs -): Promise<"created" | "updated" | "skipped" | "failed"> => { +): Promise<{ result: "created" | "failed"; error?: string }> => { try { // process the model without fields const createPayload = { @@ -324,7 +385,7 @@ const createNewModel = async ( const newModel = await apiClient.modelMethods.saveModel(createPayload, targetGuid); logger.model.created(model, "created", targetGuid); referenceMapper.addMapping(model, newModel); - return "created"; + return { result: "created" }; } catch (error: any) { // PROD-2211: saveModel can reject even though the API created the model server-side. Before // declaring failure — which leaves the mapping file without a row and wedges future syncs — @@ -334,10 +395,11 @@ const createNewModel = async ( if (recovered) { logger.model.created(model, "created", targetGuid); referenceMapper.addMapping(model, recovered); - return "created"; + return { result: "created" }; } logger.model.error(model, error, targetGuid); - return "failed"; + // PROD-2315: return the server's message so the caller can surface it in the ERROR SUMMARY. + return { result: "failed", error: error?.message ? String(error.message) : undefined }; } }; diff --git a/src/lib/pushers/tests/model-pusher.test.ts b/src/lib/pushers/tests/model-pusher.test.ts index 67884ab..ad0b069 100644 --- a/src/lib/pushers/tests/model-pusher.test.ts +++ b/src/lib/pushers/tests/model-pusher.test.ts @@ -340,9 +340,8 @@ describe("pushModels — exists in target without mapping, non-default (PROD-221 expect(mapper.getModelMappingByID(501, "source")?.targetID).toBe(10); }); - it("does NOT adopt a same-name target model of a different type (type-blind lookup)", async () => { - const createdStub = makeModel({ id: 888, referenceName: "PromoBanner", contentDefinitionTypeID: 2 }); - const saveModel = jest.fn().mockResolvedValue(createdStub); + it("does NOT adopt a same-name target model of a different type; flags it as a cross-kind collision (type-blind lookup + PROD-2315)", async () => { + const saveModel = jest.fn(); jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel)); const { pushModels } = await import("../model-pusher"); @@ -352,7 +351,11 @@ describe("pushModels — exists in target without mapping, non-default (PROD-221 const result = await pushModels([sourceModel], [targetContentList]); - expect(saveModel).toHaveBeenCalled(); // goes through create, not adopted + // Not adopted (types differ), and — PROD-2315 — no longer blindly created: recognized as a + // forbidden cross-kind reference-name collision and failed with an actionable message rather + // than attempting a create that is guaranteed to 409. + expect(saveModel).not.toHaveBeenCalled(); + expect(result.failed).toBe(1); expect(result.skipped).toBe(0); }); }); @@ -479,3 +482,101 @@ describe("pushModels — mapped-before-unmapped ordering (PROD-2250)", () => { expect(result.successful).toBe(4); }); }); + +// ─── pushModels — cross-kind reference-name collision (PROD-2315) ────────────── + +describe("pushModels — cross-kind reference-name collision (PROD-2315)", () => { + it("fails a same-name model of a different kind with an actionable message instead of creating it", async () => { + const saveModel = jest.fn(); + jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel)); + + const { pushModels } = await import("../model-pusher"); + + // Source LinkCard is a component (type 2); target LinkCard is a content model (type 1). + const sourceModel = makeModel({ referenceName: "LinkCard", contentDefinitionTypeID: 2 }); + const targetModel = makeModel({ id: 500, referenceName: "LinkCard", contentDefinitionTypeID: 1 }); + + const result = await pushModels([sourceModel], [targetModel]); + + // Guaranteed-409 create is never attempted… + expect(saveModel).not.toHaveBeenCalled(); + // …and it is reported as a failure with an actionable message. + expect(result.failed).toBe(1); + expect(result.successful).toBe(0); + expect(result.failureDetails).toHaveLength(1); + expect(result.failureDetails![0].name).toBe("LinkCard"); + expect(result.failureDetails![0].error).toMatch(/component\/module model on the source/); + expect(result.failureDetails![0].error).toMatch(/content model with that reference name already exists/); + }); + + it("does NOT flag a same-name model when the kinds are unknown (falls back to adopt-by-reference)", async () => { + const saveModel = jest.fn(); + jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel)); + + const { pushModels } = await import("../model-pusher"); + + // Neither side has contentDefinitionTypeID → modelTypeMatches() is true → adopt, not a collision. + const now = new Date().toISOString(); + const sourceModel = makeModel({ referenceName: "AmbiguousKind", lastModifiedDate: now }); + const targetModel = makeModel({ id: 501, referenceName: "AmbiguousKind", lastModifiedDate: now }); + + const result = await pushModels([sourceModel], [targetModel]); + + expect(saveModel).not.toHaveBeenCalled(); // adopted, not created + expect(result.failed).toBe(0); + }); + + it("does NOT flag a same-name same-kind model (still adopts by reference, PROD-2211)", async () => { + const saveModel = jest.fn(); + jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel)); + + const { pushModels } = await import("../model-pusher"); + + const now = new Date().toISOString(); + const sourceModel = makeModel({ referenceName: "SameKind", contentDefinitionTypeID: 1, lastModifiedDate: now }); + const targetModel = makeModel({ id: 502, referenceName: "SameKind", contentDefinitionTypeID: 1, lastModifiedDate: now }); + + const result = await pushModels([sourceModel], [targetModel]); + + expect(saveModel).not.toHaveBeenCalled(); + expect(result.failed).toBe(0); + }); + + it("previews the collision as a conflict under --preflight without calling saveModel", async () => { + const saveModel = jest.fn(); + jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel)); + state.preflight = true; + + const { pushModels } = await import("../model-pusher"); + + const sourceModel = makeModel({ referenceName: "LinkCard", contentDefinitionTypeID: 2 }); + const targetModel = makeModel({ id: 503, referenceName: "LinkCard", contentDefinitionTypeID: 1 }); + + const result = await pushModels([sourceModel], [targetModel]); + + expect(saveModel).not.toHaveBeenCalled(); + expect(result.status).toBe("success"); // preflight is a dry run + }); +}); + +// ─── pushModels — create failure surfaces the server error (PROD-2315) ───────── + +describe("pushModels — create failure surfaces the server error (PROD-2315)", () => { + it("includes the server error message in failureDetails when a create fails", async () => { + const saveModel = jest + .fn() + .mockRejectedValue(new Error("A content model with the reference name 'Foo' already exists.")); + // getContentModules returns [] (default) so findTargetModelAfterSave finds no recovery. + jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel)); + + const { pushModels } = await import("../model-pusher"); + + const sourceModel = makeModel({ referenceName: "Foo" }); + + const result = await pushModels([sourceModel], []); + + expect(result.failed).toBe(1); + expect(result.failureDetails).toHaveLength(1); + expect(result.failureDetails![0].error).toMatch(/A content model with the reference name 'Foo' already exists\./); + }); +});