diff --git a/src/commands/destroy.ts b/src/commands/destroy.ts index 739049f..afc1374 100644 --- a/src/commands/destroy.ts +++ b/src/commands/destroy.ts @@ -1,6 +1,6 @@ import { Command } from "commander"; import { authedSession } from "../api/session.js"; -import { CtApiError } from "../api/ctClient.js"; +import { CtApiError, type CtClient } from "../api/ctClient.js"; import { resolveConfig } from "../config.js"; import { loadState, resolveStatePath, saveState, type State } from "../state/state.js"; import { loadConfig, resolveConfigPath } from "../config/load.js"; @@ -105,28 +105,57 @@ export function destroyCommand(): Command { return; } - for (const key of ordered) { - const managed = state.resources[key]!; - const spec = RESOURCES[managed.type]; - if (!spec) { - error(`No write spec for type "${managed.type}" — skipping ${key}.`); - continue; - } - const path = spec.itemPath(managed.id); - assertNotPeople(path); - try { - await client.request("DELETE", path); - } catch (err) { - const message = err instanceof Error ? err.message : String(err); - error( - `Stopped at ${key}: ${message}. Already-deleted targets are removed from state — re-run with the remaining targets to resume.`, - ); - process.exitCode = 1; - return; - } + await runDeleteLoop({ client, state, statePath, ordered }); + }); +} + +export interface DeleteLoopCtx { + client: Pick; + state: State; + statePath: string; + ordered: string[]; + /** Injection seam for tests; defaults to the real state writer. */ + save?: (path: string, state: State) => Promise; +} + +/** + * Delete each ordered target, removing it from state and saving after each success. + * + * A 404 means the target was already deleted in ChurchTools (e.g. by hand in the UI): + * treat it as success-with-note — drop the state entry, save, and continue to the next + * target. Any non-404 error stops the run with state saved up to that point, so a re-run + * can resume with the remaining targets. (Mirrors the backup loop's 404 tolerance.) + */ +export async function runDeleteLoop(ctx: DeleteLoopCtx): Promise { + const { client, state, statePath, ordered } = ctx; + const save = ctx.save ?? saveState; + for (const key of ordered) { + const managed = state.resources[key]!; + const spec = RESOURCES[managed.type]; + if (!spec) { + error(`No write spec for type "${managed.type}" — skipping ${key}.`); + continue; + } + const path = spec.itemPath(managed.id); + assertNotPeople(path); + try { + await client.request("DELETE", path); + } catch (err) { + if (err instanceof CtApiError && err.status === 404) { delete state.resources[key]; - await saveState(statePath, state); - success(`Destroyed ${managed.type}.${key} (#${managed.id})`); + await save(statePath, state); + success(`${managed.type}.${key} (#${managed.id}) already deleted in ChurchTools — removed from state`); + continue; } - }); + const message = err instanceof Error ? err.message : String(err); + error( + `Stopped at ${key}: ${message}. State saved up to this point — re-run with the remaining targets to resume.`, + ); + process.exitCode = 1; + return; + } + delete state.resources[key]; + await save(statePath, state); + success(`Destroyed ${managed.type}.${key} (#${managed.id})`); + } } diff --git a/src/config/context.ts b/src/config/context.ts index ad17338..f5d148d 100644 --- a/src/config/context.ts +++ b/src/config/context.ts @@ -50,6 +50,9 @@ export interface ConfigContext { ageGroup(input: ResourceInput): void; targetGroup(input: ResourceInput): void; relationshipType(input: ResourceInput): void; + /** Master-data group role (`/group/roles`). Named `roleDefinition` to avoid colliding + * with the `groupRole` permission function below. */ + roleDefinition(input: ResourceInput): void; groupRole(input: PermissionInput): void; groupTypeRole(input: PermissionInput): void; } @@ -185,6 +188,7 @@ export function createContext(): { ageGroup: define("age-group"), targetGroup: define("target-group"), relationshipType: define("relationship-type"), + roleDefinition: define("group-role"), groupRole: definePermission("group_role"), groupTypeRole: definePermission("group_type_role"), }; diff --git a/src/engine/dynamic.ts b/src/engine/dynamic.ts index 6d6c664..934fdcc 100644 --- a/src/engine/dynamic.ts +++ b/src/engine/dynamic.ts @@ -25,7 +25,16 @@ export function stripCosmeticLabels(node: unknown): unknown { return node; } -/** Coerce numeric-string leaves to numbers (CT is int/string-inconsistent for `var` values). */ +/** + * Coerce numeric-string leaves to numbers (CT is int/string-inconsistent for `var` values). + * + * Only *canonical* integer strings are coerced: no leading zeros (`/^(-?[1-9]\d*|0)$/`) and + * within `Number.MAX_SAFE_INTEGER`. This leaves semantic strings that merely look numeric — + * leading-zero zip codes like `'01067'`, and >2^53 digit strings that would lose precision — + * untouched, so they round-trip byte-identical through normalize + write-back instead of being + * silently retyped (which broke their JSONLogic string comparisons). A canonical `"5"` still + * coerces to `5`, so a `5` vs `"5"` int/string pair still diffs equal. + */ export function coerceScalars(node: unknown): unknown { if (Array.isArray(node)) return node.map(coerceScalars); if (node && typeof node === "object") { @@ -33,7 +42,10 @@ export function coerceScalars(node: unknown): unknown { for (const [k, v] of Object.entries(node as Record)) out[k] = coerceScalars(v); return out; } - if (typeof node === "string" && /^-?\d+$/.test(node)) return Number.parseInt(node, 10); + if (typeof node === "string" && /^(-?[1-9]\d*|0)$/.test(node)) { + const n = Number.parseInt(node, 10); + if (Number.isSafeInteger(n)) return n; + } return node; } diff --git a/src/engine/synthetic.ts b/src/engine/synthetic.ts index b7fe525..628f907 100644 --- a/src/engine/synthetic.ts +++ b/src/engine/synthetic.ts @@ -114,18 +114,31 @@ const dynamicField: SyntheticField = { errors.push(`dynamic ${managed.key} status (#${managed.id}): ${message}`); } } - const augmented = desired.map((d) => - d.type === "group" && d.dynamic !== undefined - ? { ...d, fields: { ...d.fields, dynamic: normalizeDynamic({ status: d.dynamic.status, ruleset: resolveRulesetRef(d.dynamic.ruleset, configDir, d.key) }) } } - : d, - ); + const augmented = desired.map((d) => { + if (d.type !== "group" || d.dynamic === undefined) return d; + // Demote-to-none: fold to the SAME sentinel the actual side uses for a non-dynamic group + // ({ status: "none", ruleset: {} }). The docs tell users to KEEP the dynamic block when + // demoting, so their authored ruleset is still present here — but folding it would diff + // forever against the sentinel actual. Collapsing both sides makes a demoted group converge. + const dynamic = + d.dynamic.status === "none" + ? { status: "none" as DynamicStatus, ruleset: {} } + : normalizeDynamic({ status: d.dynamic.status, ruleset: resolveRulesetRef(d.dynamic.ruleset, configDir, d.key) }); + return { ...d, fields: { ...d.fields, dynamic } }; + }); return { desired: augmented, errors }; }, async apply({ client, id, change }) { const to = change.to as { status: DynamicStatus; ruleset: Record } | undefined; if (!to || to.status === "none") { assertNotPeople(`/dynamicgroups/${id}/ruleset`); - await client.request("DELETE", `/dynamicgroups/${id}/ruleset`); + // A group that was never dynamic (or is already demoted) has no ruleset to delete — CT 404s. + // Tolerate that: the desired end-state (no ruleset) already holds, so treat it as done. + try { + await client.request("DELETE", `/dynamicgroups/${id}/ruleset`); + } catch (err) { + if (!(err instanceof CtApiError && err.status === 404)) throw err; + } assertNotPeople(`/dynamicgroups/${id}/status`); await client.request("PUT", `/dynamicgroups/${id}/status`, { dynamicGroupStatus: "none" }); return; diff --git a/src/resources/registry.ts b/src/resources/registry.ts index d18c65d..d16bbbf 100644 --- a/src/resources/registry.ts +++ b/src/resources/registry.ts @@ -18,6 +18,12 @@ export interface AdoptableResource { deriveKey: (resource: Record) => string; /** The subset of fields we manage — the desired-state baseline. */ managedFields: (resource: Record) => Record; + /** + * DSL function name `configSnippet` emits for this type. Defaults to the camelCase of + * the type name. Set it when the natural camelCase collides with another DSL surface + * (e.g. `group-role` → `roleDefinition`, because `groupRole` is the permission function). + */ + dslName?: string; } /** Build a full spec, deriving `itemPath` from the collection path so each entry names its path once. */ @@ -104,6 +110,9 @@ export const RESOURCES: Record = { updateMethod: "PUT", deriveKey: (r) => slug(str(r, "name")), managedFields: (r) => ({ name: r.name, nameTranslated: r.nameTranslated, groupTypeId: r.groupTypeId }), + // `groupRole` is taken by the permissions DSL (`ct.groupRole` = definePermission("group_role")), + // so the master-data role resource declares under a distinct name. + dslName: "roleDefinition", }), }; @@ -116,9 +125,18 @@ export function resourceType(type: string): AdoptableResource { return entry; } -/** Render a config entry as a TS-as-code call, e.g. `campus({ key: "mainz", name: "Mainz" })`. */ +/** Camel-case a hyphenated type name: `group-type` → `groupType`. */ +function camelCase(type: string): string { + return type.replace(/-([a-z])/g, (_, c: string) => c.toUpperCase()); +} + +/** + * Render a config entry as a TS-as-code call, e.g. `campus({ key: "mainz", name: "Mainz" })`. + * The function name comes from the registry entry's `dslName` (default: camelCase of the type), + * so the emitted snippet always names an actual `ConfigContext` function — never a colliding one. + */ export function configSnippet(type: string, key: string, fields: Record): string { - const fn = type.replace(/-([a-z])/g, (_, c: string) => c.toUpperCase()); + const fn = RESOURCES[type]?.dslName ?? camelCase(type); return `${fn}(${tsObject({ key, ...fields })});`; } diff --git a/tests/context.test.ts b/tests/context.test.ts index 3d0dcce..cc2aa71 100644 --- a/tests/context.test.ts +++ b/tests/context.test.ts @@ -107,11 +107,22 @@ describe("config context", () => { ct.ageGroup({ key: "d", name: "d" }); ct.targetGroup({ key: "e", name: "e" }); ct.relationshipType({ key: "f", name: "f" }); + ct.roleDefinition({ key: "g", name: "g", groupTypeId: 2 }); for (const r of resources) { expect(isKnownType(r.type), `type "${r.type}" is missing from TYPE_TIER`).toBe(true); } }); + it("declares the master-data group role as a plannable group-role resource (not the permission surface)", () => { + const { ct, resources, permissions } = createContext(); + ct.roleDefinition({ key: "leiter", name: "Leiter", groupTypeId: 2 }); + expect(permissions).toEqual([]); // roleDefinition is a resource, never a permission grant + expect(resources).toEqual([ + expect.objectContaining({ type: "group-role", key: "leiter", fields: { name: "Leiter", groupTypeId: 2 } }), + ]); + expect(isKnownType(resources[0]!.type)).toBe(true); // has an apply tier → plannable + }); + it("evaluateConfig runs a module against a fresh context (blueprints + loops)", async () => { const { resources } = await evaluateConfig((ct) => { for (const c of ["mainz", "berlin"]) { diff --git a/tests/destroy.test.ts b/tests/destroy.test.ts index 3b3070f..2cc4518 100644 --- a/tests/destroy.test.ts +++ b/tests/destroy.test.ts @@ -1,6 +1,15 @@ -import { describe, it, expect } from "vitest"; -import { parseTargets, orderDestroy } from "../src/commands/destroy.js"; -import { emptyState } from "../src/state/state.js"; +import { describe, it, expect, vi } from "vitest"; +import { parseTargets, orderDestroy, runDeleteLoop } from "../src/commands/destroy.js"; +import { emptyState, type State } from "../src/state/state.js"; +import { CtApiError, type CtClient } from "../src/api/ctClient.js"; + +function stateWith(...entries: Array<{ key: string; type: string; id: number }>): State { + const state = emptyState("h"); + for (const e of entries) { + state.resources[e.key] = { type: e.type, id: e.id, key: e.key, fields: {}, adoptedAt: "t", updatedAt: "t" }; + } + return state; +} describe("parseTargets", () => { it("splits commas, trims, and dedupes", () => { @@ -30,3 +39,42 @@ describe("orderDestroy", () => { expect(orderDestroy(state, ["mainz", "team"])).toEqual(["team", "mainz"]); }); }); + +describe("runDeleteLoop", () => { + const asClient = (request: unknown) => ({ request }) as unknown as Pick; + + it("treats a 404 (already deleted in CT) as success: removes from state and continues to the next target", async () => { + const state = stateWith({ key: "gone", type: "group", id: 1 }, { key: "other", type: "group", id: 2 }); + const request = vi.fn(async (_m: string, path: string) => { + if (path === "/groups/1") throw new CtApiError("Not Found", 404, null); + return {}; + }); + const save = vi.fn(async () => {}); + + await runDeleteLoop({ client: asClient(request), state, statePath: "s.json", ordered: ["gone", "other"], save }); + + expect(state.resources.gone).toBeUndefined(); // already-deleted target removed from state + expect(state.resources.other).toBeUndefined(); // loop continued and deleted the rest + expect(request).toHaveBeenCalledTimes(2); + expect(save).toHaveBeenCalledTimes(2); + }); + + it("stops on a non-404 error with state saved up to that point and leaves later targets untouched", async () => { + const state = stateWith({ key: "boom", type: "group", id: 1 }, { key: "later", type: "group", id: 2 }); + const request = vi.fn(async (_m: string, path: string) => { + if (path === "/groups/1") throw new CtApiError("Server Error", 500, null); + return {}; + }); + const save = vi.fn(async () => {}); + const prevExit = process.exitCode; + + await runDeleteLoop({ client: asClient(request), state, statePath: "s.json", ordered: ["boom", "later"], save }); + + expect(state.resources.boom).toBeDefined(); // not removed — the DELETE failed + expect(state.resources.later).toBeDefined(); // never reached + expect(request).toHaveBeenCalledTimes(1); // stopped at the first target + expect(save).not.toHaveBeenCalled(); + expect(process.exitCode).toBe(1); + process.exitCode = prevExit; + }); +}); diff --git a/tests/dynamic.test.ts b/tests/dynamic.test.ts index 5b39124..d141fa8 100644 --- a/tests/dynamic.test.ts +++ b/tests/dynamic.test.ts @@ -24,6 +24,20 @@ describe("coerceScalars", () => { expect(coerceScalars({ "==": [{ var: "groupmember.groupMemberStatus" }, "active"] })) .toEqual({ "==": [{ var: "groupmember.groupMemberStatus" }, "active"] }); }); + it("leaves leading-zero and >2^53 numeric strings as strings (no corruption, no precision loss)", () => { + // A leading-zero zip code is a semantic string; parseInt would drop the zero and break the compare. + expect(coerceScalars({ "==": [{ var: "person.zip" }, "01067"] })) + .toEqual({ "==": [{ var: "person.zip" }, "01067"] }); + // A digit string beyond MAX_SAFE_INTEGER can't be represented exactly — must stay a string. + const big = "90071992547409910"; // > 2^53 + expect(coerceScalars({ "==": [{ var: "x.id" }, big] })) + .toEqual({ "==": [{ var: "x.id" }, big] }); + // Canonical ints still coerce, so a 5 vs "5" int/string pair keeps diffing equal. + expect(coerceScalars({ "==": [{ var: "x.n" }, "5"] })) + .toEqual({ "==": [{ var: "x.n" }, 5] }); + expect(coerceScalars({ "==": [{ var: "x.n" }, "0"] })) + .toEqual({ "==": [{ var: "x.n" }, 0] }); + }); }); describe("normalizeRuleset", () => { @@ -49,6 +63,15 @@ describe("normalizeRuleset", () => { expect(out.query).toEqual({ "==": [{ var: "ctgroup.id" }, 112] }); // filter operand coerced }); + it("round-trips a leading-zero query leaf byte-identical (no retype on write-back)", () => { + const authored = { description: "Zip filter", query: { "==": [{ var: "person.zip" }, "01067"] }, process: {} }; + const once = normalizeRuleset(authored); + // The zip leaf survives normalization untouched — apply PUTs `to.ruleset`, so any retype here + // would be written back to CT and silently break the JSONLogic string comparison. + expect(once.query).toEqual({ "==": [{ var: "person.zip" }, "01067"] }); + expect(JSON.stringify(normalizeRuleset(once))).toBe(JSON.stringify(once)); // byte-identical + idempotent + }); + it("read-then-normalize of every live fixture is stable and label-free", () => { for (const name of ["ruleset-683", "ruleset-2022", "ruleset-1092"]) { const raw = JSON.parse(readFileSync(`tests/fixtures/dynamic/${name}.get.json`, "utf8")); // array shape diff --git a/tests/registry.test.ts b/tests/registry.test.ts index 2050278..af8a06f 100644 --- a/tests/registry.test.ts +++ b/tests/registry.test.ts @@ -1,5 +1,9 @@ import { describe, it, expect } from "vitest"; +import { mkdtempSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { slug, resourceType, configSnippet, RESOURCES } from "../src/resources/registry.js"; +import { loadConfig } from "../src/config/load.js"; describe("slug", () => { it("normalises names to underscore keys", () => { @@ -62,6 +66,29 @@ describe("configSnippet", () => { 'group({ key: "team", name: "Team" });', ); }); + + it("emits the master-data role under roleDefinition, not the colliding permission name groupRole", () => { + expect(configSnippet("group-role", "leiter", { name: "Leiter", groupTypeId: 2 })).toBe( + 'roleDefinition({ key: "leiter", name: "Leiter", groupTypeId: 2 });', + ); + }); +}); + +// Round-trip guarantee: whatever `adopt` prints for any adoptable type must be declarable. +// Wrapping every registry type's snippet in a config and loading it through the real loader +// pins the adopt→config contract so a DSL-name collision (issue #31) can't regress silently. +describe("configSnippet round-trips through the config loader for every adoptable type", () => { + it.each(Object.keys(RESOURCES))("%s", async (type) => { + const key = `adopted_${type.replace(/-/g, "_")}`; + const snippet = configSnippet(type, key, { name: "Round Trip" }); + const dir = mkdtempSync(join(tmpdir(), "ct-adopt-")); + writeFileSync(join(dir, "ct.config.ts"), `export default (ct) => { ct.${snippet} };`); + + const { resources } = await loadConfig(join(dir, "ct.config.ts")); + const loaded = resources.find((r) => r.key === key); + expect(loaded, `snippet for "${type}" did not load: ${snippet}`).toBeDefined(); + expect(loaded?.type).toBe(type); + }); }); describe("write specs", () => { diff --git a/tests/synthetic-dynamic.test.ts b/tests/synthetic-dynamic.test.ts index e2b9f19..e9021b0 100644 --- a/tests/synthetic-dynamic.test.ts +++ b/tests/synthetic-dynamic.test.ts @@ -4,6 +4,7 @@ import { tmpdir } from "node:os"; import { join } from "node:path"; import { SYNTHETIC_FIELDS, syntheticField, foldSynthetic } from "../src/engine/synthetic.js"; import { normalizeDynamic, resolveRulesetRef } from "../src/engine/dynamic.js"; +import { diffFields } from "../src/engine/plan.js"; import { loadConfig } from "../src/config/load.js"; import { CtApiError } from "../src/api/ctClient.js"; import type { State } from "../src/state/state.js"; @@ -71,6 +72,28 @@ describe("dynamic synthetic field — fold", () => { expect(actual.get("g")).not.toHaveProperty("dynamic"); // NOT clobbered with the sentinel }); + it("demote-to-none converges: a kept authored ruleset folds to the same sentinel as the actual side (no-op)", async () => { + // docs/dynamic-groups.md tells users to KEEP the dynamic block when demoting, so the authored + // ruleset is still present with status "none". The actual side is the { status:"none", ruleset:{} } + // sentinel — folding the full ruleset would diff forever. Both must collapse to the same sentinel. + const state: State = { version: 1, host: "h", + resources: { g: { type: "group", id: 5, key: "g", fields: { name: "G" }, adoptedAt: "t", updatedAt: "t" } } }; + const actual = new Map>([["g", { name: "G" }]]); + const desired: DesiredResource[] = [ + { type: "group", key: "g", fields: { name: "G" }, dependsOn: [], + dynamic: { status: "none", ruleset: { description: "kept", query: { "==": [{ var: "x" }, "1"] }, process: {} } } }, + ]; + // Group is already non-dynamic in CT → ruleset GET 404s → actual sentinel. + const client = { get: vi.fn(async () => { throw new CtApiError("Not Found", 404, null); }) }; + const out = await dynamicField().fold({ client: getClient(client), state, desired, actual }); + expect(out.errors).toEqual([]); + const sentinel = { status: "none", ruleset: {} }; + expect(actual.get("g")?.dynamic).toEqual(sentinel); // actual side sentinel + expect(out.desired[0]?.fields.dynamic).toEqual(sentinel); // desired folds to the SAME sentinel + // Second plan is a no-op: the two sides are deep-equal, so diffFields reports no dynamic change. + expect(diffFields(out.desired[0]!.fields, actual.get("g")!).find((c) => c.field === "dynamic")).toBeUndefined(); + }); + it("ignores groups that did not opt into dynamic", async () => { const state: State = { version: 1, host: "h", resources: { g: { type: "group", id: 5, key: "g", fields: { name: "G" }, adoptedAt: "t", updatedAt: "t" } } }; @@ -103,6 +126,32 @@ describe("dynamic synthetic field — apply", () => { expect(request).toHaveBeenNthCalledWith(1, "DELETE", "/dynamicgroups/5/ruleset"); expect(request).toHaveBeenNthCalledWith(2, "PUT", "/dynamicgroups/5/status", { dynamicGroupStatus: "none" }); }); + + it("tolerates a 404 on the demote DELETE (never-dynamic / already-demoted group) and still PUTs status none", async () => { + const request = vi.fn(async (method: string, path: string) => { + if (method === "DELETE" && path.endsWith("/ruleset")) throw new CtApiError("Not Found", 404, null); + return {}; + }); + const state: State = { version: 1, host: "h", resources: {} }; + await expect( + dynamicField().apply({ client: { request } as unknown as Pick, state, id: 5, + change: { field: "dynamic", from: undefined, to: { status: "none", ruleset: {} } } }), + ).resolves.toBeUndefined(); // 404 swallowed — apply does not abort + expect(request).toHaveBeenNthCalledWith(1, "DELETE", "/dynamicgroups/5/ruleset"); + expect(request).toHaveBeenNthCalledWith(2, "PUT", "/dynamicgroups/5/status", { dynamicGroupStatus: "none" }); + }); + + it("re-throws a non-404 error on the demote DELETE (real failures still abort)", async () => { + const request = vi.fn(async (method: string, path: string) => { + if (method === "DELETE" && path.endsWith("/ruleset")) throw new CtApiError("Server Error", 500, null); + return {}; + }); + const state: State = { version: 1, host: "h", resources: {} }; + await expect( + dynamicField().apply({ client: { request } as unknown as Pick, state, id: 5, + change: { field: "dynamic", from: undefined, to: { status: "none", ruleset: {} } } }), + ).rejects.toThrow(/Server Error/); + }); }); describe("resolveRulesetRef", () => {