From fa386499cefd5a072a464903c4a15b132ad34d47 Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Thu, 9 Jul 2026 13:17:55 +0200 Subject: [PATCH] fix(get): auto-paginate list endpoints; surface meta + HTTP status/body on errors (#50) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ct get groups` and friends only returned ChurchTools' default first page (10 items), silently hiding the rest of a 300+-group instance. And a failing `ct get raw` printed only "✗ GET ... failed" with no HTTP status or response body, making the actual failure (e.g. a limit exceeding CT's max) invisible. - CtClient.getAll(): pages a list endpoint via ?page=N&limit=M (default limit 100, capped at 1000 pages as a loop guard) until meta.pagination.current >= lastPage, concatenating every page's data. Refactored via a shared, private requestEnvelope() so request()/get()'s existing `.data ?? envelope` unwrap semantics are unchanged for plan/apply/adopt/destroy callers. - get.ts: list resources (groups, campuses, group-hierarchies, etc.) now call getAll() and print a totals line to stderr; single-object resources (whoami, info, permissions/global) keep using the plain get(). - ui.ts: new formatError() renders a CtApiError's HTTP status and response body (truncated past 2000 chars), used by index.ts's top-level catch so `ct get raw` and every other command surface the real failure instead of a bare "... failed" message. --- src/api/ctClient.ts | 96 +++++++++++++++++++++++++++++++++++++-- src/commands/get.ts | 59 +++++++++++++++++------- src/index.ts | 4 +- src/ui.ts | 29 ++++++++++++ tests/ctClient.test.ts | 74 ++++++++++++++++++++++++++++++ tests/get-command.test.ts | 74 ++++++++++++++++++++++++++++++ tests/ui.test.ts | 42 +++++++++++++++++ 7 files changed, 354 insertions(+), 24 deletions(-) create mode 100644 tests/get-command.test.ts create mode 100644 tests/ui.test.ts diff --git a/src/api/ctClient.ts b/src/api/ctClient.ts index cb01262..7100944 100644 --- a/src/api/ctClient.ts +++ b/src/api/ctClient.ts @@ -37,6 +37,33 @@ export class CtApiError extends Error { type Json = Record; +/** + * ChurchTools' list-endpoint pagination envelope, carried as `meta.pagination` + * alongside `data`. Field names confirmed against the live API (#50): a page + * is exhausted once `current >= lastPage`. + */ +export interface CtPagination { + total?: number; + current?: number; + lastPage?: number; + limit?: number; + count?: number; +} + +export interface CtMeta { + pagination?: CtPagination; + [key: string]: unknown; +} + +export interface CtPage { + data: T[]; + meta?: CtMeta; +} + +/** Hard stop so a malformed/adversarial pagination response can't loop forever. */ +const MAX_PAGES = 1000; +const DEFAULT_PAGE_LIMIT = 100; + export class CtClient { private cookie: string | null = null; private csrfToken: string | null = null; @@ -108,6 +135,61 @@ export class CtClient { } async request(method: string, path: string, body?: Json): Promise { + const parsed = await this.requestEnvelope(method, path, body); + if (parsed === undefined) { + return undefined as T; + } + const envelope = parsed as { data?: T }; + return (envelope.data ?? envelope) as T; + } + + /** + * Fetch every page of a ChurchTools list endpoint and concatenate them, so + * callers see the whole collection instead of just CT's default first page + * (#50). CT caps `limit` at a per-endpoint maximum below 500 on real + * instances, so this defaults to a conservative page size and pages via + * `?page=N&limit=M` until `meta.pagination.current >= lastPage`. Endpoints + * that don't return pagination meta (or return everything on page 1) fall + * out after a single request. + */ + async getAll(path: string, options: { limit?: number } = {}): Promise> { + const limit = options.limit ?? DEFAULT_PAGE_LIMIT; + const items: T[] = []; + let meta: CtMeta | undefined; + let page = 1; + for (let i = 0; i < MAX_PAGES; i++) { + const parsed = await this.requestEnvelope("GET", withPageParams(path, page, limit)); + if (parsed === undefined) { + break; + } + const isArrayEnvelope = Array.isArray(parsed); + const envelope = isArrayEnvelope ? undefined : (parsed as { data?: unknown; meta?: CtMeta }); + const pageData = isArrayEnvelope ? parsed : (envelope?.data ?? parsed); + const pageItems = Array.isArray(pageData) ? (pageData as T[]) : []; + items.push(...pageItems); + const pageMeta = envelope?.meta; + meta = pageMeta ?? meta; + const pagination = pageMeta?.pagination; + if (pageItems.length === 0 || !pagination || pagination.current === undefined || pagination.lastPage === undefined) { + break; + } + if (pagination.current >= pagination.lastPage) { + break; + } + page += 1; + } + return { data: items, meta }; + } + + /** + * Shared fetch + parse for {@link request} and {@link getAll}: performs the + * HTTP call, throws a status/body-carrying {@link CtApiError} on failure, + * and returns the raw parsed JSON envelope (still carrying `data`/`meta`) — + * or `undefined` for an empty 2xx body. Kept private so `request()`'s + * `.data ?? envelope` unwrap stays the single source of truth for existing + * callers (plan/apply/adopt) while `getAll()` gets at `meta` too. + */ + private async requestEnvelope(method: string, path: string, body?: Json): Promise { if (!this.cookie) { throw new CtApiError("Not authenticated — run `ct auth login` first", 401, null); } @@ -138,22 +220,20 @@ export class CtClient { throw new CtApiError(`${method} ${path} failed`, res.status, await safeBody(res)); } if (res.status === 204) { - return undefined as T; + return undefined; } // 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; + return undefined; } - let parsed: { data?: T }; try { - parsed = JSON.parse(text) as { data?: T }; + return JSON.parse(text); } catch { throw new CtApiError(`${method} ${path} returned a non-JSON body`, res.status, text); } - return (parsed.data ?? parsed) as T; } private async refreshCsrfToken(): Promise { @@ -198,3 +278,9 @@ async function safeBody(res: Response): Promise { return null; } } + +/** Append `page`/`limit` query params, respecting any query string the caller already has. */ +function withPageParams(path: string, page: number, limit: number): string { + const separator = path.includes("?") ? "&" : "?"; + return `${path}${separator}page=${page}&limit=${limit}`; +} diff --git a/src/commands/get.ts b/src/commands/get.ts index 627ee4e..f277a16 100644 --- a/src/commands/get.ts +++ b/src/commands/get.ts @@ -1,38 +1,63 @@ import { Command } from "commander"; import { authedSession } from "../api/session.js"; import { CATALOG } from "../permissions/catalog.js"; -import { out } from "../ui.js"; +import { info, out } from "../ui.js"; + +interface ResourceSpec { + path: string; + /** + * Whether this endpoint returns a paged list (auto-paginate through every + * page) vs a single object (`whoami`, `info`, the global permissions blob) + * where paging params don't apply. Defaults to true. + */ + paginated?: boolean; +} /** * Read-only imperative queries — immediately useful before any declarative * engine exists. Resource → API path map. Paths confirmed against the live * spec by the Phase 0 spike (#2, CT 3.123.0); see docs/api-coverage.md. + * + * List endpoints are auto-paginated (#50): ChurchTools returns only its + * default page (10 items) per request, so `ct get groups` on an instance with + * 300+ groups silently returned just the first 10 before this fix. */ -const RESOURCE_PATHS: Record = { - whoami: "/whoami", - info: "/info", - campuses: "/campuses", - groups: "/groups", - "group-hierarchies": "/groups/hierarchies", - "group-types": "/group/grouptypes", - "group-roles": "/group/roles", - "age-groups": "/group/agegroups", - "target-groups": "/group/targetgroups", - "dynamic-groups": "/dynamicgroups", - "relationship-types": "/person/relationshiptypes", - permissions: "/permissions/global", +const RESOURCE_PATHS: Record = { + whoami: { path: "/whoami", paginated: false }, + info: { path: "/info", paginated: false }, + campuses: { path: "/campuses" }, + groups: { path: "/groups" }, + "group-hierarchies": { path: "/groups/hierarchies" }, + "group-types": { path: "/group/grouptypes" }, + "group-roles": { path: "/group/roles" }, + "age-groups": { path: "/group/agegroups" }, + "target-groups": { path: "/group/targetgroups" }, + "dynamic-groups": { path: "/dynamicgroups" }, + "relationship-types": { path: "/person/relationshiptypes" }, + permissions: { path: "/permissions/global", paginated: false }, }; export function getCommand(): Command { const cmd = new Command("get").description("Read structure resources from ChurchTools (JSON to stdout)"); - for (const [name, path] of Object.entries(RESOURCE_PATHS)) { + for (const [name, spec] of Object.entries(RESOURCE_PATHS)) { cmd .command(name) - .description(`GET ${path}`) + .description(`GET ${spec.path}`) .action(async () => { const { client } = await authedSession(); - out(await client.get(path)); + if (spec.paginated === false) { + out(await client.get(spec.path)); + return; + } + const { data, meta } = await client.getAll(spec.path); + out(data); + const total = meta?.pagination?.total; + if (total !== undefined && total !== data.length) { + info(`${data.length} of ${total} total`); + } else { + info(`${data.length} total`); + } }); } diff --git a/src/index.ts b/src/index.ts index 130b189..e816e5a 100644 --- a/src/index.ts +++ b/src/index.ts @@ -9,7 +9,7 @@ import { applyCommand } from "./commands/apply.js"; import { destroyCommand } from "./commands/destroy.js"; import { plannedCommands } from "./commands/placeholders.js"; import { isMainModule } from "./isMain.js"; -import { error } from "./ui.js"; +import { error, formatError } from "./ui.js"; export function buildProgram(): Command { const program = new Command(); @@ -40,7 +40,7 @@ async function main(): Promise { try { await program.parseAsync(process.argv); } catch (err) { - error(err instanceof Error ? err.message : String(err)); + error(formatError(err)); process.exitCode = 1; } } diff --git a/src/ui.ts b/src/ui.ts index a490887..fddf6bf 100644 --- a/src/ui.ts +++ b/src/ui.ts @@ -2,6 +2,35 @@ * Tiny terminal output helpers. Kept dependency-light on purpose. */ import pc from "picocolors"; +import { CtApiError } from "./api/ctClient.js"; + +/** Response bodies beyond this are truncated so a huge HTML/JSON dump doesn't flood the terminal. */ +const MAX_BODY_CHARS = 2000; + +function formatBody(body: unknown): string { + if (body === null || body === undefined) { + return ""; + } + const text = typeof body === "string" ? body : JSON.stringify(body, null, 2); + if (text.length > MAX_BODY_CHARS) { + return `${text.slice(0, MAX_BODY_CHARS)}\n… (truncated, ${text.length} chars total)`; + } + return text; +} + +/** + * Render a caught error for the terminal. For {@link CtApiError} this surfaces + * the HTTP status + response body — without it, a failing `ct get raw` (or any + * API call) prints only "✗ GET ... failed" with no way to see what ChurchTools + * actually said (#50). + */ +export function formatError(err: unknown): string { + if (err instanceof CtApiError) { + const body = formatBody(err.body); + return `${err.message} (HTTP ${err.status})${body ? `\n${body}` : ""}`; + } + return err instanceof Error ? err.message : String(err); +} export function info(message: string): void { process.stderr.write(`${message}\n`); diff --git a/tests/ctClient.test.ts b/tests/ctClient.test.ts index bf2821a..26e6f5b 100644 --- a/tests/ctClient.test.ts +++ b/tests/ctClient.test.ts @@ -99,4 +99,78 @@ describe("CtClient", () => { message: expect.stringContaining("PUT /campuses/0"), }); }); + + describe("getAll (#50)", () => { + it("auto-paginates until lastPage is reached, concatenating every page", async () => { + const { client, fetchMock } = await authedClient(); + fetchMock + .mockResolvedValueOnce( + jsonResponse({ + data: [{ id: 1 }, { id: 2 }], + meta: { pagination: { total: 3, current: 1, lastPage: 2, limit: 2 } }, + }), + ) + .mockResolvedValueOnce( + jsonResponse({ + data: [{ id: 3 }], + meta: { pagination: { total: 3, current: 2, lastPage: 2, limit: 2 } }, + }), + ); + + const result = await client.getAll("/groups", { limit: 2 }); + + expect(result.data).toEqual([{ id: 1 }, { id: 2 }, { id: 3 }]); + expect(result.meta?.pagination).toMatchObject({ total: 3, current: 2, lastPage: 2 }); + // calls 0-1 are the auth handshake (whoami + csrftoken); 2-3 are the two pages. + expect(String(fetchMock.mock.calls[2]?.[0])).toContain("page=1"); + expect(String(fetchMock.mock.calls[2]?.[0])).toContain("limit=2"); + expect(String(fetchMock.mock.calls[3]?.[0])).toContain("page=2"); + }); + + it("stops after a single page when the response carries no pagination meta", async () => { + const { client, fetchMock } = await authedClient(); + fetchMock.mockResolvedValueOnce(jsonResponse({ data: [{ id: 1 }] })); + + const result = await client.getAll("/campuses"); + + expect(result.data).toEqual([{ id: 1 }]); + expect(fetchMock).toHaveBeenCalledTimes(3); // 2 auth + 1 page, no second page requested + }); + + it("defaults the per-page limit to 100", async () => { + const { client, fetchMock } = await authedClient(); + fetchMock.mockResolvedValueOnce( + jsonResponse({ data: [], meta: { pagination: { total: 0, current: 1, lastPage: 1, limit: 100 } } }), + ); + + await client.getAll("/groups"); + + expect(String(fetchMock.mock.calls[2]?.[0])).toContain("limit=100"); + }); + + it("stops when a page comes back empty even if lastPage claims more", async () => { + const { client, fetchMock } = await authedClient(); + fetchMock.mockResolvedValueOnce( + jsonResponse({ data: [], meta: { pagination: { total: 3, current: 1, lastPage: 2, limit: 2 } } }), + ); + + const result = await client.getAll("/groups", { limit: 2 }); + + expect(result.data).toEqual([]); + expect(fetchMock).toHaveBeenCalledTimes(3); // does not spin forever on a degenerate response + }); + + it("propagates a CtApiError with status + body from a failing page", async () => { + const { client, fetchMock } = await authedClient(); + fetchMock.mockResolvedValueOnce( + new Response(JSON.stringify({ errors: ["limit exceeds max of 100"] }), { status: 400 }), + ); + + await expect(client.getAll("/groups", { limit: 500 })).rejects.toMatchObject({ + name: "CtApiError", + status: 400, + body: { errors: ["limit exceeds max of 100"] }, + }); + }); + }); }); diff --git a/tests/get-command.test.ts b/tests/get-command.test.ts new file mode 100644 index 0000000..b702750 --- /dev/null +++ b/tests/get-command.test.ts @@ -0,0 +1,74 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +const getAllMock = vi.fn(); +const getMock = vi.fn(); + +vi.mock("../src/api/session.js", () => ({ + authedSession: vi.fn(async () => ({ client: { get: getMock, getAll: getAllMock }, me: { id: 1 } })), +})); + +const { getCommand } = await import("../src/commands/get.js"); +const { CtApiError } = await import("../src/api/ctClient.js"); + +async function runGet(args: string[]): Promise { + await getCommand().parseAsync(args, { from: "user" }); +} + +describe("ct get (#50)", () => { + beforeEach(() => { + getAllMock.mockReset(); + getMock.mockReset(); + }); + + it("auto-paginates a list resource and prints every item, not just the first page", async () => { + const allGroups = Array.from({ length: 250 }, (_, i) => ({ id: i })); + getAllMock.mockResolvedValue({ + data: allGroups, + meta: { pagination: { total: 250, current: 3, lastPage: 3, limit: 100 } }, + }); + const writeSpy = vi.spyOn(process.stdout, "write").mockImplementation(() => true); + + await runGet(["groups"]); + + expect(getAllMock.mock.calls[0]?.[0]).toBe("/groups"); + const printed = JSON.parse(writeSpy.mock.calls[0]?.[0] as string) as unknown[]; + expect(printed).toHaveLength(250); + writeSpy.mockRestore(); + }); + + it("prints the total from meta.pagination to stderr for list output", async () => { + getAllMock.mockResolvedValue({ + data: [{ id: 1 }], + meta: { pagination: { total: 1, current: 1, lastPage: 1, limit: 100 } }, + }); + const errSpy = vi.spyOn(process.stderr, "write").mockImplementation(() => true); + + await runGet(["groups"]); + + const combined = errSpy.mock.calls.map((c) => String(c[0])).join("\n"); + expect(combined).toContain("1"); + errSpy.mockRestore(); + }); + + it("uses the plain (unpaginated) get for whoami, a single-object resource", async () => { + getMock.mockResolvedValue({ id: 1 }); + const writeSpy = vi.spyOn(process.stdout, "write").mockImplementation(() => true); + + await runGet(["whoami"]); + + expect(getMock).toHaveBeenCalledWith("/whoami"); + expect(getAllMock).not.toHaveBeenCalled(); + writeSpy.mockRestore(); + }); + + it("propagates a raw call's CtApiError (status + body) instead of swallowing it", async () => { + getMock.mockRejectedValue( + new CtApiError("GET /groups?limit=500 failed", 400, { errors: ["limit exceeds max of 100"] }), + ); + + await expect(runGet(["raw", "/groups?limit=500"])).rejects.toMatchObject({ + name: "CtApiError", + status: 400, + }); + }); +}); diff --git a/tests/ui.test.ts b/tests/ui.test.ts new file mode 100644 index 0000000..97671f5 --- /dev/null +++ b/tests/ui.test.ts @@ -0,0 +1,42 @@ +import { describe, it, expect } from "vitest"; +import { formatError } from "../src/ui.js"; +import { CtApiError } from "../src/api/ctClient.js"; + +describe("formatError (#50)", () => { + it("renders a plain Error's message unchanged", () => { + expect(formatError(new Error("boom"))).toBe("boom"); + }); + + it("renders a non-Error thrown value via String()", () => { + expect(formatError("boom")).toBe("boom"); + }); + + it("includes the HTTP status and a JSON response body for CtApiError", () => { + const err = new CtApiError("GET /groups?limit=500 failed", 400, { errors: ["limit exceeds max of 100"] }); + const rendered = formatError(err); + expect(rendered).toContain("400"); + expect(rendered).toContain("limit exceeds max of 100"); + }); + + it("includes a plain-string response body verbatim", () => { + const err = new CtApiError("GET /groups failed", 500, "internal server error"); + const rendered = formatError(err); + expect(rendered).toContain("500"); + expect(rendered).toContain("internal server error"); + }); + + it("truncates very large response bodies instead of dumping them whole", () => { + const big = "x".repeat(5000); + const err = new CtApiError("GET /groups failed", 500, big); + const rendered = formatError(err); + expect(rendered.length).toBeLessThan(big.length); + expect(rendered).toContain("truncated"); + }); + + it("omits a body section when there is no body", () => { + const err = new CtApiError("Not authenticated — run `ct auth login` first", 401, null); + const rendered = formatError(err); + expect(rendered).toContain("401"); + expect(rendered).not.toContain("null"); + }); +});