From d561fad7351b6e4fec2ddfac3bd136821ee4ebcc Mon Sep 17 00:00:00 2001 From: Aidan Daly Date: Fri, 4 Sep 2026 20:36:12 +0000 Subject: [PATCH] fix(tui): fill the identity provider pickers past the service page cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The API key and OAuth2 credential provider pickers clamped their page size to 20 rows because ListOauth2CredentialProviders caps maxResults at 20, so a tall terminal showed 20 rows, "more →", and blank space below. Assemble identity list pages in Core the way the gateway connector list does: FilteredPaginator gains an exact-fill mode (no predicate) that asks each scan only for what the page still needs and advances past a page that lands exactly on maxResults, so seams never repeat a row. The documented caps (20 for OAuth2, 100 for API key) move into IdentityClient and the TUI no longer carries a maxPageSize. --- src/components/PaginatedTablePicker.tsx | 4 +- src/components/usePagedList.tsx | 8 +- src/core/filteredPaginator.test.ts | 108 ++++++++++++++- src/core/filteredPaginator.ts | 19 ++- src/core/identity.test.ts | 127 ++++++++++++++++++ src/core/identity.tsx | 60 ++++++++- .../apikey.screen.test.tsx | 22 +-- .../list/screen.tsx | 4 - .../list/screen.tsx | 4 - .../oauth2.screen.test.tsx | 22 +-- 10 files changed, 330 insertions(+), 48 deletions(-) create mode 100644 src/core/identity.test.ts diff --git a/src/components/PaginatedTablePicker.tsx b/src/components/PaginatedTablePicker.tsx index 151a30ce3..2ff769195 100644 --- a/src/components/PaginatedTablePicker.tsx +++ b/src/components/PaginatedTablePicker.tsx @@ -26,7 +26,6 @@ export interface PaginatedTablePickerProps string; emptyMessage: string; emptyPageMessage: string; - maxPageSize?: number; } export function PaginatedTablePicker>({ @@ -44,9 +43,8 @@ export function PaginatedTablePicker errorMessage, emptyMessage, emptyPageMessage, - maxPageSize, }: PaginatedTablePickerProps) { - const paging = usePagedList(maxPageSize); + const paging = usePagedList(); const list = useQuery({ queryKey: [...queryKey, paging.pageSize, paging.token], queryFn: () => loadPage(paging.token, paging.pageSize), diff --git a/src/components/usePagedList.tsx b/src/components/usePagedList.tsx index c50ee32ef..377c30b87 100644 --- a/src/components/usePagedList.tsx +++ b/src/components/usePagedList.tsx @@ -37,13 +37,9 @@ function initialPagination(pageSize: number): PaginationState { // usePagedList holds the server-side pagination state shared by the picker // tables: a terminal-height-derived page size and the trail of nextTokens // leading to the current page, so ←/h can walk back through cached pages. -// -// maxPageSize caps the terminal-derived page size for APIs that constrain -// maxResults (e.g. identity list operation capped at 20) -export function usePagedList(maxPageSize?: number): PagedList { +export function usePagedList(): PagedList { const { rows } = useWindowSize(); - const fitsTerminal = Math.max(3, rows - CHROME_ROWS); - const pageSize = maxPageSize ? Math.min(fitsTerminal, maxPageSize) : fitsTerminal; + const pageSize = Math.max(3, rows - CHROME_ROWS); // A resize changes maxResults, which invalidates the token trail (tokens // encode positions relative to the old page size) — so every read derives diff --git a/src/core/filteredPaginator.test.ts b/src/core/filteredPaginator.test.ts index 0c07086f5..d8dde447f 100644 --- a/src/core/filteredPaginator.test.ts +++ b/src/core/filteredPaginator.test.ts @@ -106,6 +106,31 @@ describe("FilteredPaginator", () => { expect(second.nextToken).toBeUndefined(); }); + test("exact landing: a page completed without surplus advances, so nothing repeats", async () => { + const src = rows("AB.C..DE"); + const first = await FilteredPaginator.paginate({ + fetchPage: makeSource(src, 3).fetchPage, + predicate: keep, + nextToken: undefined, + maxResults: 3, + defaultPageSize: 10, + resourceLabel: "Test", + }); + expect(first.items.map((r) => r.id)).toEqual(["A", "B", "C"]); + expect(first.nextToken).toBe("6"); + + const second = await FilteredPaginator.paginate({ + fetchPage: makeSource(src, 3).fetchPage, + predicate: keep, + nextToken: first.nextToken, + maxResults: 3, + defaultPageSize: 10, + resourceLabel: "Test", + }); + expect(second.items.map((r) => r.id)).toEqual(["D", "E"]); + expect(second.nextToken).toBeUndefined(); + }); + test("guard: a single page holding a full page of matches over-returns and advances (no loop)", async () => { const src = rows("ABCDE"); const first = await FilteredPaginator.paginate({ @@ -194,6 +219,87 @@ describe("FilteredPaginator", () => { resourceLabel: "Test", }); expect(page.items.map((r) => r.id)).toEqual(["A", "B"]); - expect(page.nextToken).toBe("1"); + expect(page.nextToken).toBe("2"); + }); +}); + +describe("FilteredPaginator without a predicate", () => { + test("fills a page above the service cap, asking each scan only for what is still needed", async () => { + const src = rows("ABCDEFGHIJKLMNO"); + const first = makeSource(src, 100); + const page = await FilteredPaginator.paginate({ + fetchPage: first.fetchPage, + nextToken: undefined, + maxResults: 7, + defaultPageSize: 10, + scanPageSize: 3, + resourceLabel: "Test", + }); + expect(first.calls.map((c) => c.size)).toEqual([3, 3, 1]); + expect(page.items.map((r) => r.id)).toEqual([..."ABCDEFG"]); + expect(page.nextToken).toBe("7"); + + const second = makeSource(src, 100); + const next = await FilteredPaginator.paginate({ + fetchPage: second.fetchPage, + nextToken: page.nextToken, + maxResults: 7, + defaultPageSize: 10, + scanPageSize: 3, + resourceLabel: "Test", + }); + expect(next.items.map((r) => r.id)).toEqual([..."HIJKLMN"]); + expect(next.nextToken).toBe("14"); + }); + + test("a page within the cap is one request for exactly maxResults", async () => { + const { fetchPage, calls } = makeSource(rows("ABCDE"), 100); + const page = await FilteredPaginator.paginate({ + fetchPage, + nextToken: undefined, + maxResults: 2, + defaultPageSize: 10, + scanPageSize: 20, + resourceLabel: "Test", + }); + expect(calls).toEqual([{ token: undefined, size: 2 }]); + expect(page.items.map((r) => r.id)).toEqual(["A", "B"]); + expect(page.nextToken).toBe("2"); + }); + + test("without a scan size it asks for the whole page at once", async () => { + const { fetchPage, calls } = makeSource(rows("ABCDE"), 100); + await FilteredPaginator.paginate({ + fetchPage, + nextToken: undefined, + maxResults: 4, + defaultPageSize: 10, + resourceLabel: "Test", + }); + expect(calls).toEqual([{ token: undefined, size: 4 }]); + }); + + test("tops up a service page that came back short", async () => { + const src = rows("ABCDE"); + const sizes: (number | undefined)[] = []; + // Serves at most two items per call regardless of what was asked for. + const fetchPage = async (token: string | undefined, size: number | undefined) => { + sizes.push(size); + const start = token === undefined ? 0 : Number(token); + const items = src.slice(start, start + Math.min(size ?? 2, 2)); + const end = start + items.length; + return { items, nextToken: end < src.length ? String(end) : undefined }; + }; + const page = await FilteredPaginator.paginate({ + fetchPage, + nextToken: undefined, + maxResults: 5, + defaultPageSize: 10, + scanPageSize: 20, + resourceLabel: "Test", + }); + expect(sizes).toEqual([5, 3, 1]); + expect(page.items.map((r) => r.id)).toEqual([..."ABCDE"]); + expect(page.nextToken).toBeUndefined(); }); }); diff --git a/src/core/filteredPaginator.ts b/src/core/filteredPaginator.ts index 815bc6e98..5993a7db6 100644 --- a/src/core/filteredPaginator.ts +++ b/src/core/filteredPaginator.ts @@ -7,7 +7,11 @@ export type PaginateFilteredOptions = { token: string | undefined, maxResults: number | undefined, ) => Promise<{ items: T[]; nextToken: string | undefined }>; - predicate: (item: T) => boolean; + // predicate keeps the items that belong to the narrower listing. Omit it when + // every item counts and the service merely caps maxResults below the page + // being assembled: each scan then asks only for what the page still needs, so + // the page lands exactly on maxResults with no duplicated seam. + predicate?: (item: T) => boolean; nextToken: string | undefined; maxResults: number | undefined; defaultPageSize: number; @@ -37,11 +41,18 @@ export class FilteredPaginator { for (let scan = 0; scan < MAX_SCAN_REQUESTS; scan++) { const requestToken = token; - const page = await fetchPage(requestToken, scanPageSize); - const matches = page.items.filter(predicate); + const remaining = pageSize - results.length; + const requestSize = predicate ? scanPageSize : Math.min(scanPageSize ?? remaining, remaining); + const page = await fetchPage(requestToken, requestSize); + const matches = predicate ? page.items.filter(predicate) : page.items; results.push(...matches); - if (results.length >= pageSize) { + // Landed exactly: nothing from this page is left behind, so advance past it. + if (results.length === pageSize) { + return { items: results, nextToken: page.nextToken }; + } + + if (results.length > pageSize) { // Page holds >= pageSize matches by itself. Return every match found (the // page may exceed maxResults) and advance past it: replaying its token would // loop, and skipping the surplus would drop matches — so we over-return. diff --git a/src/core/identity.test.ts b/src/core/identity.test.ts new file mode 100644 index 000000000..0694acc2a --- /dev/null +++ b/src/core/identity.test.ts @@ -0,0 +1,127 @@ +import { describe, expect, mock, test } from "bun:test"; +import { + ListApiKeyCredentialProvidersCommand, + ListOauth2CredentialProvidersCommand, + type ApiKeyCredentialProviderItem, + type Oauth2CredentialProviderItem, +} from "@aws-sdk/client-bedrock-agentcore-control"; +import { InputValidationError } from "../errors"; +import type { AwsClients } from "./types"; +import { IdentityClient } from "./identity"; + +const options = { region: "us-west-2", endpointUrl: "https://agentcore.example.test" }; + +type ListCommand = ListApiKeyCredentialProvidersCommand | ListOauth2CredentialProvidersCommand; + +function identityClient(send: (command: ListCommand) => Promise): IdentityClient { + return new IdentityClient({ + control: () => ({ send: mock(send) }) as never, + } as unknown as Pick); +} + +const names = (from: number, to: number) => + Array.from({ length: to - from + 1 }, (_, index) => `provider-${from + index}`); + +// providerSource serves `total` providers in service pages of at most `cap`, +// like the Identity list APIs do, and records every request it sees. +function providerSource(total: number, cap: number, make: (name: string) => T) { + const all = names(1, total).map(make); + const requests: Array<{ op: string; input: ListCommand["input"] }> = []; + const send = async (command: ListCommand) => { + requests.push({ op: command.constructor.name, input: command.input }); + const { nextToken, maxResults } = command.input; + if (maxResults !== undefined && maxResults > cap) { + throw new Error(`maxResults ${maxResults} exceeds the service cap of ${cap}`); + } + const start = nextToken === undefined ? 0 : Number(nextToken); + const items = all.slice(start, start + (maxResults ?? all.length)); + const end = start + items.length; + return { credentialProviders: items, nextToken: end < all.length ? String(end) : undefined }; + }; + return { send, requests }; +} + +const oauth2 = (name: string) => ({ name }) as Oauth2CredentialProviderItem; +const apiKey = (name: string) => ({ name }) as ApiKeyCredentialProviderItem; + +describe("IdentityClient list pagination", () => { + test("OAuth2: a page above the service cap of 20 is assembled from consecutive calls", async () => { + const source = providerSource(50, 20, oauth2); + const client = identityClient(source.send); + + const page = await client.listOauth2CredentialProviders(undefined, 45, options); + + expect(source.requests).toEqual([ + { + op: "ListOauth2CredentialProvidersCommand", + input: { nextToken: undefined, maxResults: 20 }, + }, + { op: "ListOauth2CredentialProvidersCommand", input: { nextToken: "20", maxResults: 20 } }, + { op: "ListOauth2CredentialProvidersCommand", input: { nextToken: "40", maxResults: 5 } }, + ]); + expect(page.credentialProviders?.map((p) => p.name)).toEqual(names(1, 45)); + expect(page.nextToken).toBe("45"); + }); + + test("OAuth2: the returned token continues exactly where the page ended", async () => { + const source = providerSource(50, 20, oauth2); + const client = identityClient(source.send); + + const page = await client.listOauth2CredentialProviders("45", 45, options); + + expect(source.requests.map((r) => r.input)).toEqual([{ nextToken: "45", maxResults: 20 }]); + expect(page.credentialProviders?.map((p) => p.name)).toEqual(names(46, 50)); + expect(page.nextToken).toBeUndefined(); + }); + + test("a page within the cap is one call with maxResults passed as given", async () => { + const source = providerSource(50, 20, oauth2); + const client = identityClient(source.send); + + const page = await client.listOauth2CredentialProviders(undefined, 5, options); + + expect(source.requests.map((r) => r.input)).toEqual([{ nextToken: undefined, maxResults: 5 }]); + expect(page.credentialProviders?.map((p) => p.name)).toEqual(names(1, 5)); + expect(page.nextToken).toBe("5"); + }); + + test("no maxResults passes straight through and returns the service response", async () => { + const source = providerSource(50, 20, apiKey); + const client = identityClient(source.send); + + const page = await client.listApiKeyCredentialProviders(undefined, undefined, options); + + expect(source.requests).toEqual([ + { + op: "ListApiKeyCredentialProvidersCommand", + input: { nextToken: undefined, maxResults: undefined }, + }, + ]); + expect(page.credentialProviders?.map((p) => p.name)).toEqual(names(1, 50)); + expect(page.nextToken).toBeUndefined(); + }); + + test("API key: the service cap is 100", async () => { + const source = providerSource(150, 100, apiKey); + const client = identityClient(source.send); + + const page = await client.listApiKeyCredentialProviders(undefined, 120, options); + + expect(source.requests.map((r) => r.input)).toEqual([ + { nextToken: undefined, maxResults: 100 }, + { nextToken: "100", maxResults: 20 }, + ]); + expect(page.credentialProviders?.map((p) => p.name)).toEqual(names(1, 120)); + expect(page.nextToken).toBe("120"); + }); + + test("rejects a non-positive maxResults before calling the service", async () => { + const source = providerSource(5, 20, oauth2); + const client = identityClient(source.send); + + await expect( + client.listOauth2CredentialProviders(undefined, 0, options), + ).rejects.toBeInstanceOf(InputValidationError); + expect(source.requests).toEqual([]); + }); +}); diff --git a/src/core/identity.tsx b/src/core/identity.tsx index 9d8070451..35b9d84c8 100644 --- a/src/core/identity.tsx +++ b/src/core/identity.tsx @@ -37,14 +37,52 @@ import type { UpdateOauth2CredentialProviderInput, UpdatePaymentCredentialProviderInput, } from "../handlers/identity/types"; +import { FilteredPaginator } from "./filteredPaginator"; import type { AwsClients, CoreOptions } from "./types"; import { toClientConfig } from "./utils"; +// Documented maxResults ceilings of the Identity list APIs. A TUI page can be +// taller than either, so a larger page is assembled from several service calls. +const API_KEY_LIST_MAX_RESULTS = 100; +const OAUTH2_LIST_MAX_RESULTS = 20; + +type ProviderPage = { + credentialProviders: TItem[] | undefined; + nextToken?: string | undefined; +}; + +type ListProvidersInput = { nextToken?: string | undefined; maxResults?: number | undefined }; + export class IdentityClient implements CoreIdentityClient { // Narrowed to `control` so any holder of a cached control client satisfies it; // CoreClient passes itself. constructor(private readonly clients: Pick) {} + // Without maxResults the call passes straight through and the service picks + // the page size. With one, a page larger than the service cap is filled from + // consecutive service calls; the caps are the only Identity-specific input. + private static async listProviders( + send: (input: ListProvidersInput) => Promise>, + nextToken: string | undefined, + maxResults: number | undefined, + maxResultsCap: number, + resourceLabel: string, + ): Promise> { + if (maxResults === undefined) return send({ nextToken, maxResults }); + const page = await FilteredPaginator.paginate({ + fetchPage: async (token, size) => { + const response = await send({ nextToken: token, maxResults: size }); + return { items: response.credentialProviders ?? [], nextToken: response.nextToken }; + }, + nextToken, + maxResults, + defaultPageSize: maxResultsCap, + scanPageSize: maxResultsCap, + resourceLabel, + }); + return { credentialProviders: page.items, nextToken: page.nextToken }; + } + async createApiKeyCredentialProvider( input: CreateApiKeyCredentialProviderInput, options: CoreOptions, @@ -68,9 +106,14 @@ export class IdentityClient implements CoreIdentityClient { maxResults: number | undefined, options: CoreOptions, ): Promise { - return this.clients - .control(toClientConfig(options)) - .send(new ListApiKeyCredentialProvidersCommand({ nextToken, maxResults })); + const control = this.clients.control(toClientConfig(options)); + return IdentityClient.listProviders( + (input) => control.send(new ListApiKeyCredentialProvidersCommand(input)), + nextToken, + maxResults, + API_KEY_LIST_MAX_RESULTS, + "API key credential provider", + ); } async updateApiKeyCredentialProvider( @@ -114,9 +157,14 @@ export class IdentityClient implements CoreIdentityClient { maxResults: number | undefined, options: CoreOptions, ): Promise { - return this.clients - .control(toClientConfig(options)) - .send(new ListOauth2CredentialProvidersCommand({ nextToken, maxResults })); + const control = this.clients.control(toClientConfig(options)); + return IdentityClient.listProviders( + (input) => control.send(new ListOauth2CredentialProvidersCommand(input)), + nextToken, + maxResults, + OAUTH2_LIST_MAX_RESULTS, + "OAuth2 credential provider", + ); } async updateOauth2CredentialProvider( diff --git a/src/handlers/identity/api-key-credential-provider/apikey.screen.test.tsx b/src/handlers/identity/api-key-credential-provider/apikey.screen.test.tsx index 1ba1b463c..d2d9bf134 100644 --- a/src/handlers/identity/api-key-credential-provider/apikey.screen.test.tsx +++ b/src/handlers/identity/api-key-credential-provider/apikey.screen.test.tsx @@ -103,19 +103,21 @@ describe("API key credential provider picker", () => { ]); }); - test("caps maxResults at the service limit of 20 on a tall terminal", async () => { - const core = coreWithProviders([providerItem()]); + test("fills a tall terminal past the service's 20-item page", async () => { + const core = coreWithProviders( + Array.from({ length: 25 }, (_, index) => providerItem({ name: `provider-${index + 1}` })), + ); const screen = renderScreen("/agentcore/identity/api-key-credential-provider/list", { core }); - // Terminal taller than the 20-row service cap: page size must still clamp. + // Core assembles a page taller than one service page, so the picker asks for + // as many rows as the terminal holds and shows them all. await screen.resize(120, 60); - await waitFor(() => - core.identity.calls.some((call) => call.method === "listApiKeyCredentialProviders"), - ); - for (const call of core.identity.calls) { - if (call.method !== "listApiKeyCredentialProviders") continue; - expect(call.args[1] as number).toBeLessThanOrEqual(20); - } + await waitForText(screen.lastFrame, "provider-25"); + const pageSizes = core.identity.calls + .filter((call) => call.method === "listApiKeyCredentialProviders") + .map((call) => call.args[1] as number); + expect(pageSizes.at(-1)).toBeGreaterThan(20); + expect(screen.lastFrame()).not.toContain("more →"); }); test("shows first-page and later-page empty states", async () => { diff --git a/src/handlers/identity/api-key-credential-provider/list/screen.tsx b/src/handlers/identity/api-key-credential-provider/list/screen.tsx index e87abbe04..3afe558be 100644 --- a/src/handlers/identity/api-key-credential-provider/list/screen.tsx +++ b/src/handlers/identity/api-key-credential-provider/list/screen.tsx @@ -6,9 +6,6 @@ import type { DataTableColumn } from "../../../../components/ui/data-table"; import type { ScreenProps } from "../../../types"; import { coreOptsFromCtx } from "../../../utils"; -// identity list APIs cap maxResults at 20 -const MAX_PAGE_SIZE = 20; - interface ApiKeyProviderRow extends Record { name: string; createdAt: string; @@ -52,7 +49,6 @@ export function ApiKeyCredentialProviderListScreen({ ctx, core }: ScreenProps) { navigate(`/agentcore/identity/api-key-credential-provider/get/${encodeURIComponent(name)}`) } onBack={() => navigate("/agentcore/identity/api-key-credential-provider")} - maxPageSize={MAX_PAGE_SIZE} loadingMessage="loading API key credential providers…" errorMessage={(error) => `Error: ${error.message}`} emptyMessage="No API key credential providers found in this Region." diff --git a/src/handlers/identity/oauth2-credential-provider/list/screen.tsx b/src/handlers/identity/oauth2-credential-provider/list/screen.tsx index 2ab1f1b50..c825d0a87 100644 --- a/src/handlers/identity/oauth2-credential-provider/list/screen.tsx +++ b/src/handlers/identity/oauth2-credential-provider/list/screen.tsx @@ -6,9 +6,6 @@ import type { DataTableColumn } from "../../../../components/ui/data-table"; import type { ScreenProps } from "../../../types"; import { coreOptsFromCtx } from "../../../utils"; -// The Identity list APIs cap maxResults at 20 -const MAX_PAGE_SIZE = 20; - interface Oauth2ProviderRow extends Record { name: string; vendor: string; @@ -55,7 +52,6 @@ export function Oauth2CredentialProviderListScreen({ ctx, core }: ScreenProps) { navigate(`/agentcore/identity/oauth2-credential-provider/get/${encodeURIComponent(name)}`) } onBack={() => navigate("/agentcore/identity/oauth2-credential-provider")} - maxPageSize={MAX_PAGE_SIZE} loadingMessage="loading OAuth2 credential providers…" errorMessage={(error) => `Error: ${error.message}`} emptyMessage="No OAuth2 credential providers found in this Region." diff --git a/src/handlers/identity/oauth2-credential-provider/oauth2.screen.test.tsx b/src/handlers/identity/oauth2-credential-provider/oauth2.screen.test.tsx index 4429deb13..b0145d30a 100644 --- a/src/handlers/identity/oauth2-credential-provider/oauth2.screen.test.tsx +++ b/src/handlers/identity/oauth2-credential-provider/oauth2.screen.test.tsx @@ -106,19 +106,21 @@ describe("OAuth2 credential provider picker", () => { ]); }); - test("caps maxResults at the service limit of 20 on a tall terminal", async () => { - const core = coreWithProviders([providerItem()]); + test("fills a tall terminal past the service's 20-item page", async () => { + const core = coreWithProviders( + Array.from({ length: 25 }, (_, index) => providerItem({ name: `provider-${index + 1}` })), + ); const screen = renderScreen("/agentcore/identity/oauth2-credential-provider/list", { core }); - // Terminal taller than the 20-row service cap: page size must still clamp. + // Core assembles a page taller than one service page, so the picker asks for + // as many rows as the terminal holds and shows them all. await screen.resize(120, 60); - await waitFor(() => - core.identity.calls.some((call) => call.method === "listOauth2CredentialProviders"), - ); - for (const call of core.identity.calls) { - if (call.method !== "listOauth2CredentialProviders") continue; - expect(call.args[1] as number).toBeLessThanOrEqual(20); - } + await waitForText(screen.lastFrame, "provider-25"); + const pageSizes = core.identity.calls + .filter((call) => call.method === "listOauth2CredentialProviders") + .map((call) => call.args[1] as number); + expect(pageSizes.at(-1)).toBeGreaterThan(20); + expect(screen.lastFrame()).not.toContain("more →"); }); test("shows first-page and later-page empty states", async () => {