From 476f70730b5041300b13aa02f398456cc724936e Mon Sep 17 00:00:00 2001 From: Bernhard Weichel Date: Sat, 22 Aug 2026 19:03:34 +0200 Subject: [PATCH 1/3] fix: handle ChurchTools group children responses --- src/commands/adopt-group.ts | 60 +++++++++++++++++++++++++------ tests/adopt-group-command.test.ts | 54 ++++++++++++++++++++++++++-- 2 files changed, 102 insertions(+), 12 deletions(-) diff --git a/src/commands/adopt-group.ts b/src/commands/adopt-group.ts index 1f18d72..18e9942 100644 --- a/src/commands/adopt-group.ts +++ b/src/commands/adopt-group.ts @@ -107,22 +107,62 @@ async function resolveGroupId( * hierarchy (a live-API bug, not a valid DAG state) with a `visited` set — never re-descends into * an id already seen, so a back-reference to an ancestor cannot loop forever. */ +export function extractChildren(raw: unknown): unknown[] { + if (Array.isArray(raw)) return raw; + if (raw !== null && typeof raw === "object" && Array.isArray((raw as Record).data)) { + return (raw as { data: unknown[] }).data; + } + + const shape = + raw === null + ? "null" + : typeof raw === "object" + ? `{ ${Object.keys(raw as Record).join(", ")} }` + : typeof raw; + throw new Error( + `GET /groups/{id}/children returned an unsupported response (${shape}); expected an array or { data: [...] }.`, + ); +} + +function childId(raw: unknown): number { + let candidate: unknown = raw; + if (raw !== null && typeof raw === "object") { + const child = raw as Record; + candidate = child.id ?? child.domainIdentifier; + if (candidate === undefined && typeof child.apiUrl === "string") { + candidate = /\/groups\/(\d+)(?:[/?#]|$)/.exec(child.apiUrl)?.[1]; + } + } + + const id = + typeof candidate === "number" + ? candidate + : typeof candidate === "string" && /^\d+$/.test(candidate.trim()) + ? Number.parseInt(candidate, 10) + : Number.NaN; + if (!Number.isSafeInteger(id) || id < 0) { + const shape = + raw !== null && typeof raw === "object" + ? `{ ${Object.keys(raw as Record).join(", ")} }` + : JSON.stringify(raw); + throw new Error( + `GET /groups/{id}/children returned a child without a usable id (${shape}); ` + + `expected a number or an object with id, domainIdentifier, or apiUrl.`, + ); + } + return id; +} + async function collectSubtreeIds(rootId: number, client: Pick): Promise { const visited = new Set([rootId]); const order: number[] = []; async function walk(id: number): Promise { - let raw: unknown; - try { - raw = await client.get(`/groups/${id}/children`); - } catch (err) { - if (err instanceof CtApiError && err.status === 404) return; // no children (leaf, or unknown group) - throw err; - } - const children = Array.isArray(raw) ? raw : []; + const raw = await client.get(`/groups/${id}/children`); + const children = extractChildren(raw); for (const c of children) { - const cid = typeof c === "number" ? c : Number((c as Record | null)?.id); - if (!Number.isFinite(cid) || visited.has(cid)) continue; + const cid = childId(c); + if (visited.has(cid)) continue; visited.add(cid); order.push(cid); await walk(cid); diff --git a/tests/adopt-group-command.test.ts b/tests/adopt-group-command.test.ts index 3f06300..649cf71 100644 --- a/tests/adopt-group-command.test.ts +++ b/tests/adopt-group-command.test.ts @@ -15,7 +15,9 @@ import type { CtClient } from "../src/api/ctClient.js"; * - `/group/grouptypes` (for --type's logical-key resolution) * - `/dynamicgroups/{id}/ruleset` + `/status` (for --with-dynamic; #31 is deliberately NOT dynamic) */ -function makeClient() { +type ChildrenResponse = "array" | "envelope" | "domain-envelope" | "unknown" | "root-404"; + +function makeClient(childrenResponse: ChildrenResponse = "array") { const groups: Record> = { 10: { id: 10, name: "Area A", information: { groupTypeId: 5, groupStatusId: 1 } }, 11: { id: 11, name: "Area B", information: { groupTypeId: 5, groupStatusId: 1 } }, @@ -79,7 +81,25 @@ function makeClient() { return g; } m = /^\/groups\/(\d+)\/children$/.exec(path); - if (m) return (children[Number(m[1])] ?? []).map((id) => ({ id })); + if (m) { + const parentId = Number(m[1]); + if (childrenResponse === "root-404" && parentId === 40) { + throw new CtApiError("not found", 404, null); + } + const rows = (children[parentId] ?? []).map((id) => ({ id })); + if (childrenResponse === "envelope") return { data: rows }; + if (childrenResponse === "domain-envelope") { + return { + data: rows.map(({ id }) => ({ + domainIdentifier: String(id), + domainType: "group", + apiUrl: `/groups/${id}`, + })), + }; + } + if (childrenResponse === "unknown" && parentId === 40) return { items: rows }; + return rows; + } if (path === "/group/grouptypes") return groupTypes; if (path === "/campuses") return campuses; if (path === "/group/roles") return roles; @@ -255,6 +275,36 @@ describe("ct adopt group --children-of", () => { expect(ids).toEqual([51]); // 50 -> 51 -> 50: only 51 is a new descendant }); + it("accepts the raw { data: [...] } collection envelope", async () => { + client = makeClient("envelope"); + await run(["group", "--children-of", "40", "--state", statePath]); + + const state = await loadState(statePath, HOST); + expect(Object.values(state.resources).map((r) => r.id)).toEqual([41, 43, 42]); + }); + + it("reads ids from the domain resources documented by the live ChurchTools OpenAPI", async () => { + client = makeClient("domain-envelope"); + await run(["group", "--children-of", "40", "--state", statePath]); + + const state = await loadState(statePath, HOST); + expect(Object.values(state.resources).map((r) => r.id)).toEqual([41, 43, 42]); + }); + + it("rejects an unknown object response instead of silently treating it as an empty subtree", async () => { + client = makeClient("unknown"); + await expect(run(["group", "--children-of", "40", "--state", statePath])).rejects.toThrow( + /unsupported response.*items/, + ); + }); + + it("propagates a children endpoint 404 instead of treating the group as a leaf", async () => { + client = makeClient("root-404"); + await expect(run(["group", "--children-of", "40", "--state", statePath])).rejects.toMatchObject({ + status: 404, + }); + }); + it("resolves --children-of by an already-adopted state key", async () => { await run(["group", "40", "--state", statePath]); // adopt the root first, under its derived key "root" await run(["group", "--children-of", "root", "--state", statePath]); From aa081007de19c467448535e9ec8fb72dce5693ea Mon Sep 17 00:00:00 2001 From: Bernhard Weichel Date: Sun, 23 Aug 2026 11:28:39 +0200 Subject: [PATCH 2/3] docs: refresh dynamic group source signature --- docs/handbuch/dynamic-groups.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/handbuch/dynamic-groups.md b/docs/handbuch/dynamic-groups.md index f8570ef..d1c903f 100644 --- a/docs/handbuch/dynamic-groups.md +++ b/docs/handbuch/dynamic-groups.md @@ -6,7 +6,7 @@ sources: - src/engine/dynamic.ts - src/engine/synthetic.ts - src/commands/adopt-group.ts -sources_hash: d67f76530e841833 +sources_hash: b28f934e076cdbe6 reviewed: 2026-08-17 --- From fb2a0da97d13330c99621c1adf7bb1599048e354 Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Mon, 24 Aug 2026 10:23:57 +0200 Subject: [PATCH 3/3] fix(adopt): page /groups/{id}/children and prefer domainIdentifier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #137 found the new children handling reads a paginated list endpoint with a plain `get`, so `--children-of` saw only CT's default first page (10 rows) and never descended into the rest of a wide Bereich — silently producing an incomplete config that `ct plan` then reports as unmanaged groups. Same bug class as #101. - read the endpoint with `getAll`, never a plain `get` (#101). That also subsumes the hand-rolled envelope normalization (`extractChildren`, now removed): `getAll` already absorbs both the bare-array and the `{ data: [...] }` page shapes, and an empty 204 body — which for a leaf group means "no children" rather than the hard failure it had become. Detecting a wholly unknown object shape goes with it; that shape has never been observed from CT, and shape normalization for list reads belongs in `CtClient`, which has its own coverage. - prefer `domainIdentifier` over `id` when resolving a child row. For a domain resource (`domainType: "group"`) `domainIdentifier` is the authoritative group id; a sibling `id` would be the hierarchy edge's own id, so preferring it walks into the wrong groups with no error. - gate the `apiUrl` fallback on `== null`, not `=== undefined`: a row of all-null ids with a usable `apiUrl` used to throw. - name the failing group in both error messages instead of printing the literal `/groups/{id}/children` placeholder — `--children-of` walks whole subtrees, so "some group somewhere" is not actionable. - restore the `collectSubtreeIds` JSDoc, which had drifted onto the function inserted beneath it. Claude-Session: https://claude.ai/code/session_013f7Wx1b9m8w2AVp4HCsnot --- docs/handbuch/dynamic-groups.md | 2 +- src/commands/adopt-group.ts | 58 +++++++++++++++---------------- tests/adopt-group-command.test.ts | 23 ++++++------ 3 files changed, 40 insertions(+), 43 deletions(-) diff --git a/docs/handbuch/dynamic-groups.md b/docs/handbuch/dynamic-groups.md index d1c903f..7919861 100644 --- a/docs/handbuch/dynamic-groups.md +++ b/docs/handbuch/dynamic-groups.md @@ -6,7 +6,7 @@ sources: - src/engine/dynamic.ts - src/engine/synthetic.ts - src/commands/adopt-group.ts -sources_hash: b28f934e076cdbe6 +sources_hash: 13e4a54533b0e8a2 reviewed: 2026-08-17 --- diff --git a/src/commands/adopt-group.ts b/src/commands/adopt-group.ts index 18e9942..e3be69e 100644 --- a/src/commands/adopt-group.ts +++ b/src/commands/adopt-group.ts @@ -102,34 +102,21 @@ async function resolveGroupId( } /** - * Recursively collect a group's full hierarchy subtree via `/groups/{id}/children`, in - * parent-before-child (pre-order) sequence, excluding the root itself. Guards against a cyclic - * hierarchy (a live-API bug, not a valid DAG state) with a `visited` set — never re-descends into - * an id already seen, so a back-reference to an ancestor cannot loop forever. + * Resolve one `/groups/{id}/children` row to the group id it points at. + * + * CT answers this endpoint with either plain group rows (`{ id }`) or domain resources + * (`{ domainType: "group", domainIdentifier, apiUrl }`). For a domain resource the authoritative + * group id is `domainIdentifier` — a sibling `id`, if CT ever emits one, is the hierarchy edge's + * own id — so `domainIdentifier` is read first and `id` only backs it up. `apiUrl` is the last + * resort. `parentId` is threaded in purely so the error names the group that actually failed: + * `--children-of` walks whole subtrees, and "some group somewhere" is not actionable. */ -export function extractChildren(raw: unknown): unknown[] { - if (Array.isArray(raw)) return raw; - if (raw !== null && typeof raw === "object" && Array.isArray((raw as Record).data)) { - return (raw as { data: unknown[] }).data; - } - - const shape = - raw === null - ? "null" - : typeof raw === "object" - ? `{ ${Object.keys(raw as Record).join(", ")} }` - : typeof raw; - throw new Error( - `GET /groups/{id}/children returned an unsupported response (${shape}); expected an array or { data: [...] }.`, - ); -} - -function childId(raw: unknown): number { +function childId(raw: unknown, parentId: number): number { let candidate: unknown = raw; if (raw !== null && typeof raw === "object") { const child = raw as Record; - candidate = child.id ?? child.domainIdentifier; - if (candidate === undefined && typeof child.apiUrl === "string") { + candidate = child.domainIdentifier ?? child.id; + if (candidate == null && typeof child.apiUrl === "string") { candidate = /\/groups\/(\d+)(?:[/?#]|$)/.exec(child.apiUrl)?.[1]; } } @@ -146,22 +133,33 @@ function childId(raw: unknown): number { ? `{ ${Object.keys(raw as Record).join(", ")} }` : JSON.stringify(raw); throw new Error( - `GET /groups/{id}/children returned a child without a usable id (${shape}); ` + + `GET /groups/${parentId}/children returned a child without a usable id (${shape}); ` + `expected a number or an object with id, domainIdentifier, or apiUrl.`, ); } return id; } -async function collectSubtreeIds(rootId: number, client: Pick): Promise { +/** + * Recursively collect a group's full hierarchy subtree via `/groups/{id}/children`, in + * parent-before-child (pre-order) sequence, excluding the root itself. Guards against a cyclic + * hierarchy (a live-API bug, not a valid DAG state) with a `visited` set — never re-descends into + * an id already seen, so a back-reference to an ancestor cannot loop forever. + * + * `/groups/{id}/children` is a paginated list endpoint, so it is read with `getAll`, never a plain + * `get` (#101): a plain GET returns only CT's default first page, which would silently drop the + * tail of a wide Bereich and every subtree hanging off it. `getAll` also absorbs CT's inconsistent + * page shapes (bare array vs. `{ data: [...] }`) and an empty 204 body, which for a leaf group is + * simply "no children". + */ +async function collectSubtreeIds(rootId: number, client: Pick): Promise { const visited = new Set([rootId]); const order: number[] = []; async function walk(id: number): Promise { - const raw = await client.get(`/groups/${id}/children`); - const children = extractChildren(raw); - for (const c of children) { - const cid = childId(c); + const page = await client.getAll(`/groups/${id}/children`); + for (const c of page.data) { + const cid = childId(c, id); if (visited.has(cid)) continue; visited.add(cid); order.push(cid); diff --git a/tests/adopt-group-command.test.ts b/tests/adopt-group-command.test.ts index 649cf71..45fd446 100644 --- a/tests/adopt-group-command.test.ts +++ b/tests/adopt-group-command.test.ts @@ -15,7 +15,7 @@ import type { CtClient } from "../src/api/ctClient.js"; * - `/group/grouptypes` (for --type's logical-key resolution) * - `/dynamicgroups/{id}/ruleset` + `/status` (for --with-dynamic; #31 is deliberately NOT dynamic) */ -type ChildrenResponse = "array" | "envelope" | "domain-envelope" | "unknown" | "root-404"; +type ChildrenResponse = "array" | "envelope" | "domain-envelope" | "root-404"; function makeClient(childrenResponse: ChildrenResponse = "array") { const groups: Record> = { @@ -97,7 +97,6 @@ function makeClient(childrenResponse: ChildrenResponse = "array") { })), }; } - if (childrenResponse === "unknown" && parentId === 40) return { items: rows }; return rows; } if (path === "/group/grouptypes") return groupTypes; @@ -117,10 +116,13 @@ function makeClient(childrenResponse: ChildrenResponse = "array") { const getAll = vi.fn(async (path: string) => { if (path === "/groups") return { data: Object.values(groups) }; - // The Resolver reads master-data catalogs paginated (#99 review), so serve them here too — - // same rows as `get`, wrapped in the page envelope. + // The Resolver reads master-data catalogs paginated (#99 review), and `--children-of` reads + // `/groups/{id}/children` paginated (#101), so serve them here too — same rows as `get`, with + // the real client's envelope normalization (bare array or `{ data: [...] }` -> page items). const single = await get(path); - return { data: Array.isArray(single) ? single : [single] }; + if (Array.isArray(single)) return { data: single }; + const inner = (single as { data?: unknown }).data; + return { data: Array.isArray(inner) ? inner : [single] }; }); return { get, getAll }; @@ -266,6 +268,10 @@ describe("ct adopt group --children-of", () => { // 41 (child of root) must be adopted before 43 (child of 41). const order = Object.values(state.resources).map((r) => r.id); expect(order.indexOf(41)).toBeLessThan(order.indexOf(43)); + // `/groups/{id}/children` is a paginated list endpoint: read via `getAll`, never a plain `get` + // (#101), or a wide Bereich silently loses everything past CT's default first page. + expect(client.getAll).toHaveBeenCalledWith("/groups/40/children"); + expect(client.getAll).toHaveBeenCalledWith("/groups/41/children"); }); it("terminates on a cyclic hierarchy instead of looping forever (cycle guard)", async () => { @@ -291,13 +297,6 @@ describe("ct adopt group --children-of", () => { expect(Object.values(state.resources).map((r) => r.id)).toEqual([41, 43, 42]); }); - it("rejects an unknown object response instead of silently treating it as an empty subtree", async () => { - client = makeClient("unknown"); - await expect(run(["group", "--children-of", "40", "--state", statePath])).rejects.toThrow( - /unsupported response.*items/, - ); - }); - it("propagates a children endpoint 404 instead of treating the group as a leaf", async () => { client = makeClient("root-404"); await expect(run(["group", "--children-of", "40", "--state", statePath])).rejects.toMatchObject({