diff --git a/src/core/logs.ts b/src/core/logs.ts index 13c81eb..e6704e1 100644 --- a/src/core/logs.ts +++ b/src/core/logs.ts @@ -754,8 +754,12 @@ export class Logs { error: (payload: any, apiError: any, targetGuid?: string) => { const itemName = payload?.referenceName || payload?.displayName || `Model ${payload?.id || "Unknown"}`; const baseMessage = apiError?.message || "Unknown error"; - const responseData = apiError?.response?.data ?? apiError?.response?.body ?? apiError?.data; - const responseStatus = apiError?.response?.status ?? apiError?.status; + // The management SDK wraps the underlying axios error as `innerError`, so the real status/body + // live there — unwrap it (falling back to the top-level error) so the log captures them. + const inner = apiError?.innerError ?? apiError; + const responseData = + inner?.response?.data ?? inner?.response?.body ?? inner?.data ?? apiError?.response?.data ?? apiError?.data; + const responseStatus = inner?.response?.status ?? inner?.status ?? apiError?.response?.status ?? apiError?.status; const serverDetail = responseData ? ` | Server response (${responseStatus ?? "?"}): ${typeof responseData === "string" ? responseData : safeStringify(responseData)}` : ""; diff --git a/src/lib/pushers/model-pusher.ts b/src/lib/pushers/model-pusher.ts index 47a8345..d0ee97a 100644 --- a/src/lib/pushers/model-pusher.ts +++ b/src/lib/pushers/model-pusher.ts @@ -4,6 +4,48 @@ import { PusherResult, FailureDetail } from "../../types/sourceData"; import { ModelMapper } from "lib/mappers/model-mapper"; import { Logs } from "core/logs"; +/** + * Two Agility models can share a `referenceName` while differing in `contentDefinitionTypeID` + * (e.g. a Content List vs. a Module/Component named "PromoBanner"). A name-only lookup conflates + * them (the PROD-1505 / PROD-2211 type-blind-lookup family). This compares the model TYPE in + * addition to the name. `contentDefinitionTypeID` is present in the pulled model JSON but not on the + * SDK `Model` type, so we read it defensively; when either side lacks it (older pulls / fixtures) we + * fall back to name-only matching. + */ +function modelTypeMatches(a: mgmtApi.Model, b: mgmtApi.Model): boolean { + const aType = (a as any)?.contentDefinitionTypeID; + const bType = (b as any)?.contentDefinitionTypeID; + if (aType === undefined || aType === null || bType === undefined || bType === null) return true; + return aType === bType; +} + +/** + * Re-query the target for a model matching (referenceName, contentDefinitionTypeID). + * + * Used after a `saveModel` rejection to detect a false-negative — the SDK rethrows any failure as + * "Unable to save the model." even when the API actually persisted the model (timeout-with-server- + * completion, response-parse race, etc.). Returns the matching target model, or null if the re-query + * fails or finds nothing. Matching by type avoids mistaking a same-name different-type collision for + * a recovered save. + */ +async function findTargetModelAfterSave( + sourceModel: mgmtApi.Model, + apiClient: mgmtApi.ApiClient, + targetGuid: string +): Promise { + try { + // includeDefaults + includeModules so both content models and module/component models are returned. + const targetModels = await apiClient.modelMethods.getContentModules(true, targetGuid, true); + return ( + (targetModels || []).find( + (t) => t.referenceName === sourceModel.referenceName && modelTypeMatches(sourceModel, t) + ) || null + ); + } catch { + return null; + } +} + /** * Simple change detection for models */ @@ -73,7 +115,8 @@ export async function pushModels(sourceData: mgmtApi.Model[], targetData: mgmtAp // Handle models that exist in target but have no mapping // This ensures downstream containers can find their model mappings const targetModelByReference = targetData.find( - (targetModel) => targetModel.referenceName === sourceModel.referenceName + (targetModel) => + targetModel.referenceName === sourceModel.referenceName && modelTypeMatches(sourceModel, targetModel) ); const existsInTargetWithoutMapping = !sourceMapping && targetModelByReference; if (existsInTargetWithoutMapping) { @@ -102,6 +145,20 @@ export async function pushModels(sourceData: mgmtApi.Model[], targetData: mgmtAp `a rename or reassignment on the source. Stopping sync to avoid a partial push; review the model mappings and re-run. Please contact AgilityCMS Support to resolve this issue` ); } + + // PROD-2211: the model exists on the target by (referenceName, type) but has no mapping row + // (e.g. a prior create succeeded server-side yet was logged as a false-negative failure, or a + // model adopted by referenceName without a mapping ever being written). Previously this branch + // set `targetModel` but fell through every downstream condition (all gated on `sourceMapping`), + // so the model was silently dropped — not created, skipped, or failed — and the wedge never + // self-healed. Write the mapping now so downstream containers/content can translate and so + // re-syncs converge. + referenceMapper.addMapping(sourceModel, targetModel); + shouldSkip.push({ + model: sourceModel, + reason: "Model already exists on target without a mapping; mapping row created.", + }); + continue; } } @@ -170,7 +227,11 @@ export async function pushModels(sourceData: mgmtApi.Model[], targetData: mgmtAp targetGuid[0], logger ); - if (result) { + // PROD-2211: `updateExistingModel` returns the string "updated" | "failed". A bare `if (result)` + // treats "failed" as truthy, so failed field-updates were counted as successes — the run reported + // "N successful, 0 failed" with a green banner while models had genuinely failed. Compare + // explicitly so the summary, ERROR SUMMARY, and exit status reflect reality. + if (result === "updated") { successful++; } else { failed++; @@ -219,6 +280,16 @@ const createNewModel = async ( referenceMapper.addMapping(model, newModel); return "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 — + // re-query the target. If a model with the matching (referenceName, type) now exists, the stub + // create really succeeded: write the mapping and report created. + const recovered = await findTargetModelAfterSave(model, apiClient, targetGuid); + if (recovered) { + logger.model.created(model, "created", targetGuid); + referenceMapper.addMapping(model, recovered); + return "created"; + } logger.model.error(model, error, targetGuid); return "failed"; } @@ -246,11 +317,16 @@ async function updateExistingModel( referenceMapper.addMapping(sourceModel, updatedModel); return "updated"; } catch (error: any) { - const axiosErr = error?.innerError; - console.error(`[model-pusher] SAVE FAILED for ${sourceModel?.referenceName}:`); - console.error(` message: ${error?.message}`); - console.error(` status: ${axiosErr?.response?.status ?? axiosErr?.status ?? "n/a"}`); - console.error(` responseData: ${JSON.stringify(axiosErr?.response?.data ?? axiosErr?.data ?? null, null, 2)}`); + // PROD-2211: saveModel can reject even though the field update was persisted server-side. Re-query + // the target; treat it as a recovered update ONLY when the saved field set matches the source + // (field count). A genuine reject — e.g. a 404 "Definition for setting X not found" — leaves the + // fields unapplied, so the count won't match and it correctly stays a failure. + const recovered = await findTargetModelAfterSave(sourceModel, apiClient, targetGuid); + if (recovered && (recovered.fields?.length ?? 0) === (sourceModel.fields?.length ?? 0)) { + logger.model.updated(sourceModel, "updated", targetGuid); + referenceMapper.addMapping(sourceModel, recovered); + return "updated"; + } logger.model.error(sourceModel, error, targetGuid); return "failed"; } diff --git a/src/lib/pushers/tests/model-pusher.test.ts b/src/lib/pushers/tests/model-pusher.test.ts index e45339f..8dbe55d 100644 --- a/src/lib/pushers/tests/model-pusher.test.ts +++ b/src/lib/pushers/tests/model-pusher.test.ts @@ -43,10 +43,11 @@ function makeModel(overrides: Record = {}): any { }; } -function makeApiClient(saveModelImpl?: jest.Mock): any { +function makeApiClient(saveModelImpl?: jest.Mock, getContentModulesImpl?: jest.Mock): any { return { modelMethods: { saveModel: saveModelImpl ?? jest.fn().mockResolvedValue(makeModel({ id: 999 })), + getContentModules: getContentModulesImpl ?? jest.fn().mockResolvedValue([]), }, }; } @@ -202,3 +203,156 @@ describe("pushModels — source-side rename orphans a mapping and halts the sync expect(saveModel).not.toHaveBeenCalled(); }); }); + +// ─── PROD-2211: honest failure reporting ────────────────────────────────────── + +describe("pushModels — failed update is reported as failed, not success (PROD-2211)", () => { + it("counts a model whose field-update genuinely fails as failed", async () => { + // Stub create succeeds; the follow-up field update is rejected (e.g. 404). Must be a failure. + const saveModel = jest + .fn() + .mockResolvedValueOnce(makeModel({ id: 55, referenceName: "FooterLinks" })) + .mockRejectedValue(new Error("Unable to save the model.")); + // Re-query returns the stub (0 fields) — does NOT match the source field count, so no recovery. + const getContentModules = jest + .fn() + .mockResolvedValue([makeModel({ id: 55, referenceName: "FooterLinks", fields: [] })]); + jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel, getContentModules)); + + const { pushModels } = await import("../model-pusher"); + const sourceModel = makeModel({ + id: 410, + referenceName: "FooterLinks", + contentDefinitionTypeID: 1, + fields: [{ name: "a" }, { name: "b" }], + }); + + const result = await pushModels([sourceModel], []); + + expect(result.failed).toBe(1); + expect(result.successful).toBe(0); + expect(result.failureDetails?.some((f) => f.name === "FooterLinks")).toBe(true); + }); +}); + +// ─── PROD-2211: false-negative create recovery ──────────────────────────────── + +describe("pushModels — false-negative create recovery (PROD-2211)", () => { + it("recovers as success + writes a mapping when the create throws but the model exists on target", async () => { + const sourceModel = makeModel({ id: 700, referenceName: "RetailerLocatorSearchPanel", contentDefinitionTypeID: 1 }); + // Stub create rejects (false-negative); the follow-up update then succeeds normally. + const saveModel = jest + .fn() + .mockRejectedValueOnce(new Error("socket hang up")) + .mockResolvedValue(makeModel({ id: 8, referenceName: "RetailerLocatorSearchPanel" })); + const getContentModules = jest + .fn() + .mockResolvedValue([ + makeModel({ id: 8, referenceName: "RetailerLocatorSearchPanel", contentDefinitionTypeID: 1 }), + ]); + jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel, getContentModules)); + + const { ModelMapper } = await import("lib/mappers/model-mapper"); + const { pushModels } = await import("../model-pusher"); + + const result = await pushModels([sourceModel], []); + + expect(getContentModules).toHaveBeenCalled(); + expect(result.failed).toBe(0); + expect(result.successful).toBe(1); + const mapper = new ModelMapper(state.sourceGuid[0], state.targetGuid[0]); + expect(mapper.getModelMappingByID(700, "source")?.targetID).toBe(8); + }); + + it("stays failed when the create throws and only a different-typed same-name model exists", async () => { + const sourceModule = makeModel({ id: 800, referenceName: "PromoBanner", contentDefinitionTypeID: 2 }); + const saveModel = jest.fn().mockRejectedValue(new Error("Unable to save the model.")); + // Only a same-name Content List (type 1) exists — not a match for the Module (type 2). + const getContentModules = jest + .fn() + .mockResolvedValue([makeModel({ id: 9, referenceName: "PromoBanner", contentDefinitionTypeID: 1 })]); + jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel, getContentModules)); + + const { pushModels } = await import("../model-pusher"); + const result = await pushModels([sourceModule], []); + + expect(result.failed).toBe(1); + expect(result.successful).toBe(0); + }); + + it("recovers a failed UPDATE when the saved field set matches the source", async () => { + // Mapping exists (model already on target); the update throws but the fields were persisted. + const { ModelMapper } = await import("lib/mappers/model-mapper"); + const seeder = new ModelMapper(state.sourceGuid[0], state.targetGuid[0]); + seeder.addMapping( + { id: 300, referenceName: "Header", lastModifiedDate: new Date(2024, 0, 1).toISOString() } as any, + { id: 30, referenceName: "Header", lastModifiedDate: new Date(2024, 0, 1).toISOString() } as any + ); + + const saveModel = jest.fn().mockRejectedValue(new Error("Unable to save the model.")); + // Re-query shows the target now has the same field count as the source → recovered. + const getContentModules = jest + .fn() + .mockResolvedValue([makeModel({ id: 30, referenceName: "Header", fields: [{ name: "x" }, { name: "y" }] })]); + jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel, getContentModules)); + + const { pushModels } = await import("../model-pusher"); + const sourceModel = makeModel({ + id: 300, + referenceName: "Header", + lastModifiedDate: new Date(2025, 0, 1).toISOString(), // newer than mapping → triggers update + fields: [{ name: "x" }, { name: "y" }], + }); + + const targetModel = makeModel({ + id: 30, + referenceName: "Header", + lastModifiedDate: new Date(2024, 0, 1).toISOString(), + }); + + const result = await pushModels([sourceModel], [targetModel]); + + expect(result.successful).toBe(1); + expect(result.failed).toBe(0); + }); +}); + +// ─── PROD-2211: adopt existing target model without a mapping ───────────────── + +describe("pushModels — exists in target without mapping, non-default (PROD-2211)", () => { + it("writes a mapping row and skips instead of silently dropping the model", async () => { + const saveModel = jest.fn().mockResolvedValue(makeModel({ id: 999 })); + jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel)); + + const { ModelMapper } = await import("lib/mappers/model-mapper"); + const { pushModels } = await import("../model-pusher"); + + const sourceModel = makeModel({ id: 501, referenceName: "Header", contentDefinitionTypeID: 1 }); + const targetModel = makeModel({ id: 10, referenceName: "Header", contentDefinitionTypeID: 1 }); + + const result = await pushModels([sourceModel], [targetModel]); + + expect(saveModel).not.toHaveBeenCalled(); + expect(result.skipped).toBe(1); + expect(result.failed).toBe(0); + + const mapper = new ModelMapper(state.sourceGuid[0], state.targetGuid[0]); + 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); + jest.spyOn(stateModule, "getApiClient").mockReturnValue(makeApiClient(saveModel)); + + const { pushModels } = await import("../model-pusher"); + + const sourceModel = makeModel({ id: 600, referenceName: "PromoBanner", contentDefinitionTypeID: 2 }); + const targetContentList = makeModel({ id: 9, referenceName: "PromoBanner", contentDefinitionTypeID: 1 }); + + const result = await pushModels([sourceModel], [targetContentList]); + + expect(saveModel).toHaveBeenCalled(); // goes through create, not adopted + expect(result.skipped).toBe(0); + }); +});