diff --git a/src/api/ctClient.ts b/src/api/ctClient.ts index 09a2f97..7ffa976 100644 --- a/src/api/ctClient.ts +++ b/src/api/ctClient.ts @@ -15,6 +15,7 @@ */ import { type CtConfig } from "../config.js"; import { fetchWithRetry } from "./http.js"; +import { meetsMinVersion, MIN_CT_VERSION, type CtInfo } from "./version.js"; export interface WhoAmI { id: number; @@ -39,6 +40,7 @@ type Json = Record; export class CtClient { private cookie: string | null = null; private csrfToken: string | null = null; + private ctVersion: string | null = null; constructor(private readonly config: CtConfig) {} @@ -46,6 +48,34 @@ export class CtClient { return this.config.host; } + /** + * Hard-fail if the ChurchTools instance is below the minimum version the CLI + * requires (group hierarchy / metadata CRUD need v3.96+). One `/info` GET, + * cached so repeated calls in a session cost nothing. plan/apply/destroy call + * this via {@link authedSession} — a half-applied structure from a stale + * instance is exactly what the gate exists to prevent. + */ + async assertMinVersion(min: string = MIN_CT_VERSION): Promise { + if (this.ctVersion === null) { + const info = await this.get("/info"); + this.ctVersion = info?.version ?? ""; + } + const version = this.ctVersion; + if (!version) { + throw new CtApiError( + `ChurchTools did not report a version (GET /info) — cannot verify the required minimum ${min}.`, + 0, + null, + ); + } + if (!meetsMinVersion(version, min)) { + throw new Error( + `ChurchTools ${version} is below the required minimum ${min}. ` + + `Upgrade ChurchTools before running plan/apply/destroy.`, + ); + } + } + /** Run the login-token handshake and cache the session cookie + CSRF token. */ async authenticate(loginToken: string): Promise { const url = `${this.config.host}/api/whoami?login_token=${encodeURIComponent(loginToken)}`; @@ -103,7 +133,19 @@ export class CtClient { if (res.status === 204) { return undefined as T; } - const parsed = (await res.json()) as { data?: T }; + // Any 2xx may carry an empty or non-JSON body (DELETEs commonly do). A bare + // res.json() there throws a raw SyntaxError naming no request. Read the text + // first: empty → undefined; unparseable → a CtApiError that names method+path. + const text = await res.text(); + if (text.trim() === "") { + return undefined as T; + } + let parsed: { data?: T }; + try { + parsed = JSON.parse(text) as { data?: T }; + } catch { + throw new CtApiError(`${method} ${path} returned a non-JSON body`, res.status, text); + } return (parsed.data ?? parsed) as T; } diff --git a/src/api/session.ts b/src/api/session.ts index 074d01c..e9a046f 100644 --- a/src/api/session.ts +++ b/src/api/session.ts @@ -19,5 +19,8 @@ export async function authedSession(): Promise { } const client = new CtClient(config); const me = await client.authenticate(token); + // Hard-fail below the minimum CT version before any command reads or writes — + // a stale instance half-applies (tier-0 writes succeed, hierarchy endpoints 404). + await client.assertMinVersion(); return { client, me }; } diff --git a/src/engine/build.ts b/src/engine/build.ts index bd08c35..ee9a4b5 100644 --- a/src/engine/build.ts +++ b/src/engine/build.ts @@ -38,6 +38,8 @@ export async function buildPlan( // Keyed by logical key (globally unique), not CT id (unique only within a type — the Mainz campus is id 0). const actual = new Map>(); const unresolved = new Set(); + // Keys whose fetch errored (non-404), mapped to a short status descriptor for the plan render. + const fetchFailed = new Map(); const fetchErrors: string[] = []; await mapConcurrent(Object.values(state.resources), FETCH_CONCURRENCY, async (managed) => { @@ -57,7 +59,10 @@ export async function buildPlan( return; // vanished in CT — the plan will propose recreating (or pruning) it } // A read-only plan should not abort on one bad fetch: record it, keep going, flag the plan as partial. + // Track the key separately from a real 404 so computePlan renders it as a fetch failure, not a recreate. const message = err instanceof Error ? err.message : String(err); + const status = err instanceof CtApiError ? String(err.status) : "error"; + fetchFailed.set(managed.key, status); fetchErrors.push(`${managed.type}.${managed.key} (#${managed.id}): ${message}`); warn(`Failed to fetch ${managed.type}.${managed.key} (#${managed.id}): ${message}`); } @@ -66,6 +71,6 @@ export async function buildPlan( // Synthetic sub-resource fields (parents, dynamic, …) fold into the diff on both sides. const folded = await foldSynthetic({ client, state, desired, actual, configDir: opts.configDir }); fetchErrors.push(...folded.errors); - const plan = computePlan(folded.desired, state, actual, { unresolved }); + const plan = computePlan(folded.desired, state, actual, { unresolved, fetchFailed }); return { plan, actual, fetchErrors }; } diff --git a/src/engine/execute.ts b/src/engine/execute.ts index ffbeb6d..5b07c2b 100644 --- a/src/engine/execute.ts +++ b/src/engine/execute.ts @@ -96,13 +96,20 @@ export async function executePlan(plan: Plan, deps: ExecuteDeps): Promise !isSyntheticField(c.field)); if (hasFieldChange) { + // PATCH resources take only the changed fields (unchanged/drifted siblings are left alone); + // PUT resources replace the whole object, so send actual ∪ changes to preserve those siblings. + const body = spec.updateMethod === "PATCH" ? snapshotFromChanges({}, item.changes) : snapshot; const path = spec.itemPath(id); assertNotPeople(path); - await client.request(spec.updateMethod, path, snapshot); + await client.request(spec.updateMethod, path, body); } upsert(state, { type: item.type, id, key: item.key, fields: snapshot }, now()); await save(statePath, state); diff --git a/src/engine/plan.ts b/src/engine/plan.ts index afd70b7..9c4b091 100644 --- a/src/engine/plan.ts +++ b/src/engine/plan.ts @@ -71,6 +71,13 @@ export function driftFields( export interface ComputePlanOptions { /** Logical keys whose managed type has no registry entry — cannot be fetched, so left untouched (not recreated/deleted). */ unresolved?: ReadonlySet; + /** + * Logical keys whose actual value could not be fetched (a non-404 error). Mapped + * to a short status descriptor (e.g. "500"). These are NOT vanished resources, so + * they must be excluded from create/recreate/stale classification and surfaced as + * a fetch failure — otherwise a transient 500 reads as "recreate — missing in CT". + */ + fetchFailed?: ReadonlyMap; } export function computePlan( @@ -80,6 +87,7 @@ export function computePlan( opts: ComputePlanOptions = {}, ): Plan { const unresolved = opts.unresolved ?? new Set(); + const fetchFailed = opts.fetchFailed ?? new Map(); for (const d of desired) { if (!isKnownType(d.type)) { @@ -124,6 +132,19 @@ export function computePlan( }); continue; } + if (fetchFailed.has(d.key)) { + // Fetch errored (non-404). We can't diff it, and it is NOT gone — do not propose a recreate. + updates.push({ + type: d.type, + key: d.key, + id: managed.id, + action: "no-op", + changes: [], + note: "fetch-failed", + detail: fetchFailed.get(d.key), + }); + continue; + } const a = actual.get(d.key); if (!a) { creates.push({ @@ -144,6 +165,8 @@ export function computePlan( id: managed.id, action: changes.length > 0 ? "update" : "no-op", changes, + // The fetched actual — the write body is built from this, not the stale state snapshot (#27). + actual: a, drift: drift.length > 0 ? drift : undefined, }); } @@ -163,6 +186,19 @@ export function computePlan( }); continue; } + if (fetchFailed.has(managed.key)) { + // Fetch errored — we can't tell if it is gone, so do not propose a stale-prune. + deletes.push({ + type: managed.type, + key: managed.key, + id: managed.id, + action: "no-op", + changes: [], + note: "fetch-failed", + detail: fetchFailed.get(managed.key), + }); + continue; + } const a = actual.get(managed.key); if (!a) { // Already gone from ChurchTools but still in the state file — surface it so the user prunes state (not a silent no-op). diff --git a/src/engine/render.ts b/src/engine/render.ts index 3628e44..9d5cfcf 100644 --- a/src/engine/render.ts +++ b/src/engine/render.ts @@ -27,9 +27,16 @@ export function renderPlan(plan: Plan): string { const drifted = plan.items.filter((i) => i.drift && i.drift.length > 0); const stale = plan.items.filter((i) => i.note === "stale"); const unresolved = plan.items.filter((i) => i.note === "unresolved-type"); + const fetchFailed = plan.items.filter((i) => i.note === "fetch-failed"); const lines: string[] = []; - if (changed.length === 0 && drifted.length === 0 && stale.length === 0 && unresolved.length === 0) { + if ( + changed.length === 0 && + drifted.length === 0 && + stale.length === 0 && + unresolved.length === 0 && + fetchFailed.length === 0 + ) { return pc.green("No changes. Desired state matches ChurchTools."); } @@ -74,6 +81,14 @@ export function renderPlan(plan: Plan): string { } } + if (fetchFailed.length > 0) { + lines.push(""); + lines.push(pc.yellow("Fetch failed (could not read from ChurchTools — diff unavailable, left untouched):")); + for (const item of fetchFailed) { + lines.push(` ? ${item.type}.${item.key} (#${item.id}) — fetch failed (${item.detail ?? "error"})`); + } + } + const s = summarize(plan); lines.push(""); lines.push(pc.bold(`Plan: ${s.create} to create, ${s.update} to update, ${s.delete} to delete.`)); diff --git a/src/engine/synthetic.ts b/src/engine/synthetic.ts index b7fe525..b4059ca 100644 --- a/src/engine/synthetic.ts +++ b/src/engine/synthetic.ts @@ -45,8 +45,13 @@ function resolveId(state: State, key: string): number { const parentsField: SyntheticField = { field: "parents", async fold({ client, state, desired, actual }) { - const hasManagedGroups = Object.values(state.resources).some((m) => m.type === "group"); - if (!hasManagedGroups) return { desired, errors: [] }; + // Gate on the DESIRED side, not the pre-apply state: on a fresh state the + // groups don't exist yet, so a state-side gate returns early and the first + // apply drops every declared hierarchy edge (flat groups, exit 0). Gating on + // "some desired group opted into parents" makes the first apply create edges, + // and still skips the /groups/hierarchies fetch when nobody opts in. + const optedIn = desired.some((d) => d.type === "group" && d.parents !== undefined); + if (!optedIn) return { desired, errors: [] }; try { const raw = await client.get("/groups/hierarchies"); const parentIds = parentIdsByGroupId(Array.isArray(raw) ? raw : []); diff --git a/src/engine/types.ts b/src/engine/types.ts index 010bc35..19b7e62 100644 --- a/src/engine/types.ts +++ b/src/engine/types.ts @@ -35,8 +35,10 @@ export type PlanAction = "create" | "update" | "delete" | "no-op"; * - `recreate` — desired + managed, but vanished from ChurchTools (a create that replaces a dead id). * - `stale` — managed + dropped from config + already gone from ChurchTools (nothing to delete; prune from state). * - `unresolved-type` — managed but its type has no registry entry, so it cannot be fetched/diffed (left untouched). + * - `fetch-failed` — managed but its actual value could not be fetched (non-404 error); NOT a vanish, so it is + * excluded from create/recreate/stale classification and rendered as a fetch failure. */ -export type PlanNote = "recreate" | "stale" | "unresolved-type"; +export type PlanNote = "recreate" | "stale" | "unresolved-type" | "fetch-failed"; export interface FieldChange { field: string; @@ -52,10 +54,18 @@ export interface PlanItem { action: PlanAction; /** For create: every desired field; update: only the differences; delete/no-op: empty. */ changes: FieldChange[]; + /** + * The fetched actual managed fields (updates only). The write body is built from + * THIS, not the stale state snapshot — so a field that drifted in the CT UI but + * isn't in `changes` passes through untouched instead of being reverted (#27). + */ + actual?: Record; /** Manual changes in ChurchTools since adoption (last-known snapshot vs actual). */ drift?: FieldChange[]; /** A non-standard state the plan must surface (see {@link PlanNote}). */ note?: PlanNote; + /** Extra human-readable context for a note (e.g. the HTTP status behind `fetch-failed`). */ + detail?: string; } export interface Plan { diff --git a/tests/build.test.ts b/tests/build.test.ts index 8e60533..5ee81a8 100644 --- a/tests/build.test.ts +++ b/tests/build.test.ts @@ -37,5 +37,35 @@ describe("buildPlan", () => { const item = plan.items.find((i) => i.key === "mainz")!; expect(item.action).toBe("update"); expect(item.changes).toEqual([{ field: "name", from: "Mainz", to: "Mainz City" }]); + // The fetched actual is threaded onto the update item so execute builds the body from it (#27). + expect(item.actual).toEqual({ name: "Mainz", shorty: "MZ" }); + }); + + it("renders a transient (non-404) fetch error as fetch-failed, not a recreate (#33)", async () => { + const state = emptyState("https://x.church.tools"); + state.resources.mainz = { + type: "campus", + id: 0, + key: "mainz", + fields: { name: "Mainz", shorty: "MZ" }, + adoptedAt: "t", + updatedAt: "t", + }; + const desired: DesiredResource[] = [ + { type: "campus", key: "mainz", fields: { name: "Mainz", shorty: "MZ" }, dependsOn: [] }, + ]; + const client = { + get: async (): Promise => { + throw new CtApiError("boom", 500, null); + }, + }; + const { plan, fetchErrors } = await buildPlan(client, state, desired); + expect(fetchErrors.length).toBe(1); + const item = plan.items.find((i) => i.key === "mainz")!; + // NOT classified as a create/recreate — a healthy resource must never be advised for recreation. + expect(item.action).toBe("no-op"); + expect(item.note).toBe("fetch-failed"); + expect(item.detail).toBe("500"); + expect(plan.items.some((i) => i.action === "create")).toBe(false); }); }); diff --git a/tests/ctClient.test.ts b/tests/ctClient.test.ts index 997a44b..bf2821a 100644 --- a/tests/ctClient.test.ts +++ b/tests/ctClient.test.ts @@ -54,4 +54,49 @@ describe("CtClient", () => { expect(headers.get("CSRF-Token")).toBe("csrf-xyz"); expect(headers.get("Cookie")).toContain("s=1"); }); + + async function authedClient(): Promise<{ client: CtClient; fetchMock: ReturnType> }> { + const fetchMock = vi + .fn() + .mockResolvedValueOnce(jsonResponse({ data: { id: 1 } }, { setCookie: "s=1; Path=/" })) + .mockResolvedValueOnce(jsonResponse({ data: "csrf" })); + vi.stubGlobal("fetch", fetchMock); + const client = new CtClient({ host: "https://eqrm.church.tools" }); + await client.authenticate("tok"); + return { client, fetchMock }; + } + + it("assertMinVersion passes on a supported instance and refuses an old one", async () => { + const { client, fetchMock } = await authedClient(); + fetchMock.mockResolvedValueOnce(jsonResponse({ data: { version: "3.123.0" } })); + await expect(client.assertMinVersion()).resolves.toBeUndefined(); + + const { client: old, fetchMock: oldFetch } = await authedClient(); + oldFetch.mockResolvedValueOnce(jsonResponse({ data: { version: "3.95.0" } })); + await expect(old.assertMinVersion()).rejects.toThrow(/3\.95\.0 is below the required minimum/); + }); + + it("assertMinVersion fetches /info only once (cached)", async () => { + const { client, fetchMock } = await authedClient(); + fetchMock.mockResolvedValueOnce(jsonResponse({ data: { version: "3.123.0" } })); + await client.assertMinVersion(); + await client.assertMinVersion(); + const infoCalls = fetchMock.mock.calls.filter((c) => String(c[0]).includes("/api/info")); + expect(infoCalls.length).toBe(1); + }); + + it("returns undefined for an empty 2xx body instead of throwing a raw SyntaxError", async () => { + const { client, fetchMock } = await authedClient(); + fetchMock.mockResolvedValueOnce(new Response("", { status: 200 })); + await expect(client.request("DELETE", "/groups/1/parents/2")).resolves.toBeUndefined(); + }); + + it("wraps a non-JSON 2xx body in a CtApiError naming method + path", async () => { + const { client, fetchMock } = await authedClient(); + fetchMock.mockResolvedValueOnce(new Response("oops", { status: 200 })); + await expect(client.request("PUT", "/campuses/0")).rejects.toMatchObject({ + name: "CtApiError", + message: expect.stringContaining("PUT /campuses/0"), + }); + }); }); diff --git a/tests/execute.test.ts b/tests/execute.test.ts index 471599f..122cb5e 100644 --- a/tests/execute.test.ts +++ b/tests/execute.test.ts @@ -102,7 +102,7 @@ describe("executePlan", () => { expect(state.resources.zurich).toMatchObject({ id: 8, key: "zurich" }); }); - it("updates a group via PATCH with the full managed snapshot", async () => { + it("updates a group via PATCH with only the changed field (siblings left alone)", async () => { const state = emptyState("h"); state.resources.team = { type: "group", @@ -121,6 +121,7 @@ describe("executePlan", () => { id: 9, action: "update", changes: [{ field: "name", from: "Team", to: "Team A" }], + actual: { name: "Team", groupTypeId: 2, groupStatusId: 1 }, }, ], }; @@ -132,14 +133,52 @@ describe("executePlan", () => { now: fixedNow, }); expect(result.updated).toEqual(["team"]); + // PATCH carries ONLY the planned change — CT keeps the untouched siblings. expect(calls[0]).toEqual({ method: "PATCH", path: "/groups/9", - body: { name: "Team A", groupTypeId: 2, groupStatusId: 1 }, + body: { name: "Team A" }, }); + // Post-apply state reflects what CT now holds: actual ∪ changes. expect(state.resources.team!.fields).toEqual({ name: "Team A", groupTypeId: 2, groupStatusId: 1 }); }); + it("does NOT revert a field that drifted in CT when a sibling field is updated (#27)", async () => { + // Campus adopted with { name, shorty }; an admin edited `shorty` in the CT UI after adoption, + // so state carries the adopt-time "MZ" while the fetched actual is "MZX". The user changed `name`. + const state = emptyState("h"); + state.resources.mainz = { + type: "campus", + id: 0, + key: "mainz", + fields: { name: "Mainz", shorty: "MZ" }, // stale adopt-time snapshot + adoptedAt: "t", + updatedAt: "t", + }; + const { client, calls } = recorder(); + const plan: Plan = { + items: [ + { + type: "campus", + key: "mainz", + id: 0, + action: "update", + changes: [{ field: "name", from: "Mainz", to: "Mainz HQ" }], + actual: { name: "Mainz", shorty: "MZX" }, // shorty drifted in CT + }, + ], + }; + await executePlan(plan, { client, state, statePath: "s.json", save: noSave, now: fixedNow }); + // PUT replaces the whole object, so it must carry the drifted actual `shorty`, never the stale "MZ". + expect(calls[0]).toEqual({ + method: "PUT", + path: "/campuses/0", + body: { name: "Mainz HQ", shorty: "MZX" }, + }); + // Post-apply state reflects the written body, not the stale snapshot. + expect(state.resources.mainz!.fields).toEqual({ name: "Mainz HQ", shorty: "MZX" }); + }); + it("reconciles hierarchy edges via PUT/DELETE and never stores parents in state", async () => { const state = emptyState("h"); state.resources.parent = { @@ -175,6 +214,46 @@ describe("executePlan", () => { expect(state.resources.child!.fields.parents).toBeUndefined(); }); + it("writes hierarchy edges on a first apply (creates carry a parents change) (#28)", async () => { + // Fresh state: parent + child both created in dependency order, and the child's create carries + // a `parents` change so the edge is written on the very first apply. + const state = emptyState("h"); + const { client, calls } = recorder({ "POST /groups": { id: 0 } }); + // Return distinct ids for the two POSTs so resolveId can find the parent. + let nextId = 1; + client.request = async (method: string, path: string, body?: unknown): Promise => { + calls.push({ method, path, body }); + if (method === "POST" && path === "/groups") return { id: nextId++ } as T; + return {} as T; + }; + const plan: Plan = { + items: [ + { + type: "group", + key: "parent", + id: null, + action: "create", + changes: [{ field: "name", from: undefined, to: "parent" }], + }, + { + type: "group", + key: "child", + id: null, + action: "create", + changes: [ + { field: "name", from: undefined, to: "child" }, + { field: "parents", from: undefined, to: ["parent"] }, + ], + }, + ], + }; + const result = await executePlan(plan, { client, state, statePath: "s.json", save: noSave, now: fixedNow }); + expect(result.failed).toBeUndefined(); + expect(result.created).toEqual(["parent", "child"]); + // The edge PUT resolves the parent's freshly-assigned id (1) against the child's (2). + expect(calls).toContainEqual({ method: "PUT", path: "/groups/2/parents/1", body: undefined }); + }); + it("skips deletes (apply never deletes)", async () => { const state = emptyState("h"); state.resources.old = { diff --git a/tests/synthetic.test.ts b/tests/synthetic.test.ts index ec3842d..7dde4b3 100644 --- a/tests/synthetic.test.ts +++ b/tests/synthetic.test.ts @@ -53,4 +53,43 @@ describe("synthetic-field registry", () => { expect(actual.get("child")?.parents).toEqual(["parent"]); expect(out.desired[0]?.fields.parents).toEqual(["parent"]); }); + + it("augments desired parents on a FRESH (empty) state — first apply keeps hierarchy edges (#28)", async () => { + // Fresh state: no groups exist yet. The old gate keyed on pre-apply state and returned early, + // so create items carried no parents change and the first apply produced a flat hierarchy. + const state: State = { version: 1, host: "h", resources: {} }; + const actual = new Map>(); + const desired: DesiredResource[] = [ + { type: "group", key: "child", fields: { name: "child" }, parents: ["parent"], dependsOn: ["parent"] }, + { type: "group", key: "parent", fields: { name: "parent" }, dependsOn: [] }, + ]; + let fetched = false; + const client = { + get: async (): Promise => { + fetched = true; + return [] as unknown as T; // no hierarchies exist yet on a fresh instance + }, + }; + const out = await foldSynthetic({ client, state, desired, actual }); + expect(fetched).toBe(true); + expect(out.desired.find((d) => d.key === "child")?.fields.parents).toEqual(["parent"]); + }); + + it("does NOT fetch /groups/hierarchies when no desired group opts into parents (#28 / #17.5)", async () => { + const state: State = { version: 1, host: "h", resources: {} }; + const actual = new Map>(); + const desired: DesiredResource[] = [ + { type: "group", key: "team", fields: { name: "team" }, dependsOn: [] }, + ]; + let fetched = false; + const client = { + get: async (): Promise => { + fetched = true; + return [] as unknown as T; + }, + }; + const out = await foldSynthetic({ client, state, desired, actual }); + expect(fetched).toBe(false); + expect(out.errors).toEqual([]); + }); });