diff --git a/docs/handbuch/dynamic-groups.md b/docs/handbuch/dynamic-groups.md index f8570ef..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: d67f76530e841833 +sources_hash: 13e4a54533b0e8a2 reviewed: 2026-08-17 --- diff --git a/src/commands/adopt-group.ts b/src/commands/adopt-group.ts index 1f18d72..e3be69e 100644 --- a/src/commands/adopt-group.ts +++ b/src/commands/adopt-group.ts @@ -101,28 +101,66 @@ async function resolveGroupId( return Number(candidates[0]!.id); } +/** + * 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. + */ +function childId(raw: unknown, parentId: number): number { + let candidate: unknown = raw; + if (raw !== null && typeof raw === "object") { + const child = raw as Record; + candidate = child.domainIdentifier ?? child.id; + if (candidate == null && 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/${parentId}/children returned a child without a usable id (${shape}); ` + + `expected a number or an object with id, domainIdentifier, or apiUrl.`, + ); + } + return id; +} + /** * 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 { +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 : []; - 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 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); await walk(cid); diff --git a/tests/adopt-group-command.test.ts b/tests/adopt-group-command.test.ts index 3f06300..45fd446 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" | "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,24 @@ 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}`, + })), + }; + } + return rows; + } if (path === "/group/grouptypes") return groupTypes; if (path === "/campuses") return campuses; if (path === "/group/roles") return roles; @@ -97,10 +116,13 @@ function makeClient() { 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 }; @@ -246,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 () => { @@ -255,6 +281,29 @@ 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("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]);