diff --git a/.changeset/fetch-all-pages-cursor-forwarding.md b/.changeset/fetch-all-pages-cursor-forwarding.md new file mode 100644 index 000000000..961e59803 --- /dev/null +++ b/.changeset/fetch-all-pages-cursor-forwarding.md @@ -0,0 +1,9 @@ +--- +'@openchoreo/backstage-plugin-catalog-backend-module': patch +--- + +Fix three `fetchAllPages` call sites in the scheduled entity provider that +ignored the pagination cursor (workflowplanes, observabilityplanes and +deploymentpipelines). Each closure now forwards the cursor and requests +`limit: 100`, so namespaces with more than one page of these resources are +fully ingested instead of silently stopping after the first page. diff --git a/.changeset/fetch-all-pages-hardening.md b/.changeset/fetch-all-pages-hardening.md new file mode 100644 index 000000000..7d9d4d996 --- /dev/null +++ b/.changeset/fetch-all-pages-hardening.md @@ -0,0 +1,18 @@ +--- +'@openchoreo/openchoreo-client-node': minor +--- + +Harden `fetchAllPages` with an optional options bag: `maxPages` caps how +many pages are fetched (throwing instead of silently truncating, and kept +opt-in with no default so existing callers see no new failure modes), +`timeoutMs` gives the whole run a wall-clock budget (chosen as a finite +60s default so unbounded pagination cannot hang a backend, with `0` as +the escape hatch that disables it), and `signal` lets callers abort the +run at entry and between pages. The helper now also detects stuck +cursors (a page returning the same non-empty cursor it was fetched with) +and malformed page responses (a nullish page or a missing `items` +array), throwing descriptive errors that name the page index, cursor, +and collected item count. `PaginatedResponse` and the new +`FetchAllPagesOptions` type are now exported. Behavior is unchanged for +callers that pass only `fetchPage`, apart from the new default timeout +kicking in. diff --git a/packages/openchoreo-client-node/src/index.ts b/packages/openchoreo-client-node/src/index.ts index d893ad949..822976522 100644 --- a/packages/openchoreo-client-node/src/index.ts +++ b/packages/openchoreo-client-node/src/index.ts @@ -52,7 +52,11 @@ export { } from './resource-utils'; // Export pagination utilities (new API) -export { fetchAllPages } from './pagination-utils'; +export { + fetchAllPages, + type FetchAllPagesOptions, + type PaginatedResponse, +} from './pagination-utils'; // Export generated types as namespaces export * as OpenChoreoAPI from './generated/openchoreo'; diff --git a/packages/openchoreo-client-node/src/pagination-utils.test.ts b/packages/openchoreo-client-node/src/pagination-utils.test.ts index bbc89ca1a..66732a1e5 100644 --- a/packages/openchoreo-client-node/src/pagination-utils.test.ts +++ b/packages/openchoreo-client-node/src/pagination-utils.test.ts @@ -1,3 +1,4 @@ +import { getEventListeners } from 'events'; import { fetchAllPages } from './pagination-utils'; describe('fetchAllPages', () => { @@ -99,3 +100,255 @@ describe('fetchAllPages', () => { expect(fetchPage).toHaveBeenCalledTimes(2); }); }); + +async function captureError(run: () => Promise): Promise { + try { + await run(); + } catch (error) { + return error as Error; + } + throw new Error('Expected the operation to reject, but it resolved'); +} + +describe('fetchAllPages hardening', () => { + it('throws when fetchPage resolves undefined, naming the page index', async () => { + const fetchPage = jest.fn().mockResolvedValue(undefined); + + const error = await captureError(() => fetchAllPages(fetchPage)); + + expect(error).toBeInstanceOf(Error); + expect(error.message).toContain('page 0'); + expect(error.message).toContain('undefined'); + }); + + it('throws when a page has no items array', async () => { + const fetchPage = jest.fn().mockResolvedValue({}); + + const error = await captureError(() => fetchAllPages(fetchPage)); + + expect(error).toBeInstanceOf(Error); + expect(error.message).toContain('page 0'); + expect(error.message).toContain('items'); + }); + + it('throws when the next cursor repeats the cursor used to fetch the page', async () => { + const fetchPage = jest + .fn() + .mockResolvedValueOnce({ + items: [{ id: 1 }], + pagination: { nextCursor: 'cursor-1' }, + }) + .mockResolvedValueOnce({ + items: [{ id: 2 }], + pagination: { nextCursor: 'cursor-1' }, + }); + + const error = await captureError(() => fetchAllPages(fetchPage)); + + expect(error).toBeInstanceOf(Error); + expect(error.message).toContain('stuck'); + // The error must name the cursor value that is not advancing. + expect(error.message).toContain('"cursor-1"'); + expect(fetchPage).toHaveBeenCalledTimes(2); + }); + + it('stops when nextCursor is null', async () => { + const fetchPage = jest + .fn() + .mockResolvedValueOnce({ + items: [{ id: 1 }], + pagination: { nextCursor: 'cursor-1' }, + }) + .mockResolvedValueOnce({ + items: [{ id: 2 }], + pagination: { nextCursor: null }, + }); + + const result = await fetchAllPages(fetchPage); + + expect(result).toEqual([{ id: 1 }, { id: 2 }]); + expect(fetchPage).toHaveBeenCalledTimes(2); + }); + + it('stops when nextCursor is an empty string', async () => { + const fetchPage = jest + .fn() + .mockResolvedValueOnce({ + items: [{ id: 1 }], + pagination: { nextCursor: 'cursor-1' }, + }) + .mockResolvedValueOnce({ + items: [{ id: 2 }], + pagination: { nextCursor: '' }, + }); + + const result = await fetchAllPages(fetchPage); + + expect(result).toEqual([{ id: 1 }, { id: 2 }]); + expect(fetchPage).toHaveBeenCalledTimes(2); + }); + + it('throws when fetching more pages than maxPages allows', async () => { + const fetchPage = jest + .fn() + .mockResolvedValueOnce({ + items: [{ id: 1 }, { id: 2 }], + pagination: { nextCursor: 'cursor-1' }, + }) + .mockResolvedValueOnce({ + items: [{ id: 3 }, { id: 4 }], + pagination: { nextCursor: 'cursor-2' }, + }) + .mockResolvedValueOnce({ + items: [{ id: 5 }, { id: 6 }], + pagination: {}, + }); + + const error = await captureError(() => + fetchAllPages(fetchPage, { maxPages: 2 }), + ); + + expect(error).toBeInstanceOf(Error); + expect(error.message).toContain('maxPages of 2'); + expect(error.message).toContain('6 items'); + expect(fetchPage).toHaveBeenCalledTimes(3); + }); + + it('does not throw when the page count exactly reaches maxPages', async () => { + const fetchPage = jest + .fn() + .mockResolvedValueOnce({ + items: [{ id: 1 }], + pagination: { nextCursor: 'cursor-1' }, + }) + .mockResolvedValueOnce({ + items: [{ id: 2 }], + pagination: {}, + }); + + const result = await fetchAllPages(fetchPage, { maxPages: 2 }); + + expect(result).toEqual([{ id: 1 }, { id: 2 }]); + expect(fetchPage).toHaveBeenCalledTimes(2); + }); +}); + +describe('fetchAllPages timeout', () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + if (jest.getTimerCount() !== 0) { + throw new Error(`leaked ${jest.getTimerCount()} fake timer(s)`); + } + jest.useRealTimers(); + }); + + it('rejects when the default overall timeout expires', async () => { + const fetchPage = jest.fn().mockImplementation( + () => + new Promise(resolve => { + setTimeout( + () => resolve({ items: [{ id: 1 }], pagination: {} }), + 120_000, + ); + }), + ); + + const settled = fetchAllPages(fetchPage).catch(error => error); + + await jest.advanceTimersByTimeAsync(60_000); + const error = (await settled) as Error; + + expect(error).toBeInstanceOf(Error); + expect(error.message).toBe('Pagination timed out after 60000 ms'); + + // Let the abandoned fetch settle so no fake timers are left pending. + await jest.advanceTimersByTimeAsync(60_000); + }); + + it('does not time out when timeoutMs is 0', async () => { + const fetchPage = jest.fn().mockImplementation( + () => + new Promise(resolve => { + setTimeout( + () => resolve({ items: [{ id: 1 }], pagination: {} }), + 120_000, + ); + }), + ); + + const pending = fetchAllPages(fetchPage, { timeoutMs: 0 }); + + // Far beyond the 60s default budget; the run must still complete. + await jest.advanceTimersByTimeAsync(120_000); + + await expect(pending).resolves.toEqual([{ id: 1 }]); + }); +}); + +describe('fetchAllPages cancellation', () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + if (jest.getTimerCount() !== 0) { + throw new Error(`leaked ${jest.getTimerCount()} fake timer(s)`); + } + jest.useRealTimers(); + }); + + it('rejects without fetching when the signal is already aborted', async () => { + const controller = new AbortController(); + controller.abort(); + const fetchPage = jest.fn(); + + const error = await captureError(() => + fetchAllPages(fetchPage, { signal: controller.signal }), + ); + + expect(error.name).toBe('AbortError'); + expect(error.message).toContain('Pagination aborted'); + expect(fetchPage).not.toHaveBeenCalled(); + }); + + it('rejects when the signal is aborted after the first page', async () => { + const controller = new AbortController(); + const fetchPage = jest + .fn() + .mockResolvedValueOnce({ + items: [{ id: 1 }], + pagination: { nextCursor: 'cursor-1' }, + }) + .mockImplementationOnce(() => { + controller.abort(); + return new Promise(() => {}); + }); + + const error = await captureError(() => + fetchAllPages(fetchPage, { signal: controller.signal }), + ); + + expect(error.name).toBe('AbortError'); + expect(error.message).toContain('Pagination aborted'); + expect(fetchPage).toHaveBeenCalledTimes(2); + }); + + it('removes its timer and abort listener once it completes', async () => { + const controller = new AbortController(); + const fetchPage = jest.fn().mockResolvedValue({ + items: [{ id: 1 }], + pagination: {}, + }); + + const result = await fetchAllPages(fetchPage, { + signal: controller.signal, + }); + + expect(result).toEqual([{ id: 1 }]); + expect(jest.getTimerCount()).toBe(0); + expect(getEventListeners(controller.signal, 'abort')).toHaveLength(0); + }); +}); diff --git a/packages/openchoreo-client-node/src/pagination-utils.ts b/packages/openchoreo-client-node/src/pagination-utils.ts index f5b5bf82d..7c95e969b 100644 --- a/packages/openchoreo-client-node/src/pagination-utils.ts +++ b/packages/openchoreo-client-node/src/pagination-utils.ts @@ -4,17 +4,53 @@ * @packageDocumentation */ -interface PaginatedResponse { +/** + * A single page of items from a cursor-based paginated API endpoint. + */ +export interface PaginatedResponse { items: T[]; pagination?: { nextCursor?: string; }; } +/** + * Optional guards that protect {@link fetchAllPages} against runaway + * pagination. All of them throw rather than silently truncating results. + */ +export interface FetchAllPagesOptions { + /** Hard cap on pages fetched. Exceeding it throws; it never silently truncates. */ + maxPages?: number; + /** Wall-clock budget for the entire run. Defaults to 60_000; 0 disables the timeout. */ + timeoutMs?: number; + /** Caller cancellation, checked at entry and between pages. */ + signal?: AbortSignal; +} + +/** Default wall-clock budget for an entire pagination run, in milliseconds. */ +const DEFAULT_TIMEOUT_MS = 60_000; + +function describeCursor(cursor: string | undefined): string { + return cursor === undefined ? 'undefined' : `"${cursor}"`; +} + +function abortError(message: string): Error { + const error = new Error(message); + error.name = 'AbortError'; + return error; +} + /** * Fetches all pages from a cursor-based paginated API endpoint. * + * A page whose `nextCursor` is `undefined`, `null` or `''` terminates the + * loop. Broken or runaway pagination never silently truncates: a nullish + * page, a page without an `items` array, a cursor that stops advancing, + * more pages than `options.maxPages`, an expired `options.timeoutMs` + * budget, and an aborted `options.signal` all throw. + * * @param fetchPage - Function that fetches a single page given an optional cursor. + * @param options - Optional guards; see {@link FetchAllPagesOptions}. * @returns All items concatenated across every page. * * @example @@ -33,16 +69,94 @@ interface PaginatedResponse { * ``` */ export async function fetchAllPages( - fetchPage: (cursor?: string) => Promise>, + fetchPage: ( + cursor?: string, + ) => Promise | null | undefined>, + options?: FetchAllPagesOptions, ): Promise { + const maxPages = options?.maxPages; + const signal = options?.signal; + const timeoutMs = options?.timeoutMs ?? DEFAULT_TIMEOUT_MS; + + if (signal?.aborted) { + throw abortError('Pagination aborted before the first page was fetched'); + } + const allItems: T[] = []; let cursor: string | undefined; + let pageIndex = 0; + + let timeoutId: ReturnType | undefined; + let timeoutPromise: Promise | undefined; + if (timeoutMs > 0) { + timeoutPromise = new Promise((_, reject) => { + timeoutId = setTimeout( + () => reject(new Error(`Pagination timed out after ${timeoutMs} ms`)), + timeoutMs, + ); + }); + } + + let onAbort: (() => void) | undefined; + let abortPromise: Promise | undefined; + if (signal) { + abortPromise = new Promise((_, reject) => { + onAbort = () => reject(abortError('Pagination aborted')); + signal.addEventListener('abort', onAbort, { once: true }); + }); + } + + try { + do { + const page = await Promise.race([ + fetchPage(cursor), + ...(timeoutPromise ? [timeoutPromise] : []), + ...(abortPromise ? [abortPromise] : []), + ]); + + const cursorDesc = describeCursor(cursor); + + if (page === null || page === undefined) { + const returned = page === null ? 'null' : 'undefined'; + throw new Error( + `Pagination failed: fetchPage returned ${returned} for page ${pageIndex} (cursor: ${cursorDesc})`, + ); + } + + if (!Array.isArray(page.items)) { + throw new Error( + `Pagination failed: page ${pageIndex} did not return an items array (cursor: ${cursorDesc})`, + ); + } + + const nextCursor = page.pagination?.nextCursor; + if (cursor && nextCursor === cursor) { + // nextCursor equals cursor here, so cursorDesc describes both. + throw new Error( + `Pagination is stuck: page ${pageIndex} returned nextCursor ${cursorDesc}, which is the cursor that was already used to fetch it`, + ); + } + + allItems.push(...page.items); + pageIndex += 1; + + if (maxPages !== undefined && pageIndex > maxPages) { + throw new Error( + `Pagination exceeded maxPages of ${maxPages} after ${pageIndex} pages and ${allItems.length} items collected`, + ); + } - do { - const page = await fetchPage(cursor); - allItems.push(...page.items); - cursor = page.pagination?.nextCursor; - } while (cursor); + // A nextCursor of undefined, null or '' all terminate the loop. + cursor = nextCursor; + } while (cursor); - return allItems; + return allItems; + } finally { + if (timeoutId !== undefined) { + clearTimeout(timeoutId); + } + if (signal && onAbort) { + signal.removeEventListener('abort', onAbort); + } + } } diff --git a/plugins/catalog-backend-module-openchoreo/src/provider/OpenChoreoEntityProvider.ts b/plugins/catalog-backend-module-openchoreo/src/provider/OpenChoreoEntityProvider.ts index 456f284aa..a06672d93 100644 --- a/plugins/catalog-backend-module-openchoreo/src/provider/OpenChoreoEntityProvider.ts +++ b/plugins/catalog-backend-module-openchoreo/src/provider/OpenChoreoEntityProvider.ts @@ -547,11 +547,12 @@ export class OpenChoreoEntityProvider implements EntityProvider { for (const ns of namespaces) { const nsName = getName(ns)!; try { - const workflowplanes = await fetchAllPages(() => + const workflowplanes = await fetchAllPages(cursor => client .GET('/api/v1/namespaces/{namespaceName}/workflowplanes', { params: { path: { namespaceName: nsName }, + query: { limit: 100, cursor }, }, }) .then(res => { @@ -587,11 +588,12 @@ export class OpenChoreoEntityProvider implements EntityProvider { const nsName = getName(ns)!; try { const observabilityplanes = - await fetchAllPages(() => + await fetchAllPages(cursor => client .GET('/api/v1/namespaces/{namespaceName}/observabilityplanes', { params: { path: { namespaceName: nsName }, + query: { limit: 100, cursor }, }, }) .then(res => { @@ -663,20 +665,25 @@ export class OpenChoreoEntityProvider implements EntityProvider { >(); try { - const pipelines = await fetchAllPages(() => - client - .GET('/api/v1/namespaces/{namespaceName}/deploymentpipelines', { - params: { - path: { namespaceName: nsName }, - }, - }) - .then(res => { - if (res.error) - throw new Error( - `Failed to fetch deployment pipelines for ${nsName}`, - ); - return res.data; - }), + const pipelines = await fetchAllPages( + cursor => + client + .GET( + '/api/v1/namespaces/{namespaceName}/deploymentpipelines', + { + params: { + path: { namespaceName: nsName }, + query: { limit: 100, cursor }, + }, + }, + ) + .then(res => { + if (res.error) + throw new Error( + `Failed to fetch deployment pipelines for ${nsName}`, + ); + return res.data; + }), ); // The DP↔Project relation pair is emitted by