From 6868e5551fc61bc62eb2bd37a8971bb86b7125a0 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 23 Oct 2026 12:23:10 +0000 Subject: [PATCH 1/4] fix(cli): Route all limit-bounded pagination through paginate() Sentry's page-number paginators (OffsetPaginator, used by projects, teams, members, repositories, and dashboard revisions) put the page size in the cursor. Asking for "the remaining budget" on the last page reads the wrong window and silently skips rows, so paginate() now keeps one page size per walk, picks one that divides the limit, and stops a page early rather than trimming away a usable cursor. Hand-written limit-bounded cursor loops are migrated to paginate() and rejected by a new sentry-cli/no-manual-pagination lint rule. A property test ports Sentry's paginator cursor math so this class of bug fails CI. Co-Authored-By: BYK --- .oxlintrc.json | 1 + packages/cli/AGENTS.md | 30 +++-- packages/cli/lint-rules/cli-oxlint-plugin.js | 28 ++++- .../cli/src/commands/alert/issues/list.ts | 53 ++------ .../cli/src/commands/alert/metrics/list.ts | 46 ++----- packages/cli/src/commands/dashboard/list.ts | 3 + .../cli/src/commands/dashboard/revisions.ts | 64 +++------- packages/cli/src/lib/api/events.ts | 48 ++------ packages/cli/src/lib/api/explore.ts | 71 +++-------- packages/cli/src/lib/api/infrastructure.ts | 94 ++++++++++---- packages/cli/src/lib/api/issues.ts | 69 ++++------- .../test/commands/dashboard/revisions.test.ts | 76 +++++------- .../cli/test/lib/api-client.coverage.test.ts | 9 +- .../cli/test/lib/api/events-overshoot.test.ts | 19 ++- packages/cli/test/lib/api/explore.test.ts | 16 ++- .../cli/test/lib/api/infrastructure.test.ts | 46 +++++-- packages/cli/test/lib/api/logs.test.ts | 9 +- .../cli/test/lib/api/project-dsns.test.ts | 6 +- packages/cli/test/lib/api/replays.test.ts | 10 +- packages/cli/test/lib/api/traces.test.ts | 44 +++---- .../test/lib/auto-paginate.property.test.ts | 5 +- packages/cli/test/lib/org-list.test.ts | 7 +- .../cli/test/lib/paginate.property.test.ts | 115 ++++++++++++++++++ 23 files changed, 451 insertions(+), 418 deletions(-) create mode 100644 packages/cli/test/lib/paginate.property.test.ts diff --git a/.oxlintrc.json b/.oxlintrc.json index 233e8d2e0..f9096c72a 100644 --- a/.oxlintrc.json +++ b/.oxlintrc.json @@ -71,6 +71,7 @@ "sentry-cli/no-direct-target-resolution": "error", "sentry-cli/no-generic-is-record": "error", "sentry-cli/prefer-paginate-helper": "error", + "sentry-cli/no-manual-pagination": "error", "sentry-cli/no-namespace-import": "error", "no-restricted-imports": [ "error", diff --git a/packages/cli/AGENTS.md b/packages/cli/AGENTS.md index 2e8d0c163..cf64bc66e 100644 --- a/packages/cli/AGENTS.md +++ b/packages/cli/AGENTS.md @@ -411,24 +411,28 @@ that fits your use case): **Auto-pagination for large limits:** When `--limit` exceeds `API_MAX_PER_PAGE` (100), list commands MUST transparently -fetch multiple pages to fill the requested limit. Cap `perPage` at -`Math.min(flags.limit, API_MAX_PER_PAGE)` and loop until `results.length >= limit` -or pages are exhausted. This matches the `listIssuesAllPages` pattern. +fetch multiple pages to fill the requested limit. Always do this with +`paginate()` from `src/lib/api/infrastructure.ts`; never write the cursor loop +yourself. The `sentry-cli/no-manual-pagination` lint rule rejects loops that +follow cursors and count toward a limit. ```typescript -const perPage = Math.min(flags.limit, API_MAX_PER_PAGE); -for (let page = 0; page < MAX_PAGINATION_PAGES; page++) { - const { data, nextCursor } = await listPaginated(org, { perPage, cursor }); - results.push(...data); - if (results.length >= flags.limit || !nextCursor) break; - cursor = nextCursor; -} +const { data, nextCursor } = await paginate( + { limit: flags.limit, cursor }, + (perPage, pageCursor) => + listPaginated(org, { perPage, cursor: pageCursor }), +); ``` -Never pass a `per_page` value larger than `API_MAX_PER_PAGE` to the API — the -server silently caps it, causing the command to return fewer items than requested. +Pass `perPage` to the request unchanged. `paginate()` keeps one page size for +the whole walk because Sentry's page-number paginators (`OffsetPaginator`, +used by projects, teams, members, repositories, and dashboard revisions) +encode the page size in the cursor; asking for "the remaining budget" on the +last page makes them skip rows. It also only returns a cursor that resumes at +the first row the caller has not received. Test pagination changes against +the paginator models in `test/lib/paginate.property.test.ts`. -Reference template: `trace/list.ts`, `span/list.ts`, `dashboard/list.ts` +Reference template: `trace/list.ts`, `span/list.ts`, `dashboard/revisions.ts` ### ID Validation diff --git a/packages/cli/lint-rules/cli-oxlint-plugin.js b/packages/cli/lint-rules/cli-oxlint-plugin.js index d1035dd58..e9f68d55f 100644 --- a/packages/cli/lint-rules/cli-oxlint-plugin.js +++ b/packages/cli/lint-rules/cli-oxlint-plugin.js @@ -226,18 +226,42 @@ const rules = { "prefer-paginate-helper": rule( "Use paginate() from infrastructure.js instead of a raw limit.", (_context, file, report) => - file.includes("/src/lib/api/") && !file.endsWith("/infrastructure.ts") + file.includes("/src/") && !file.endsWith("/lib/api/infrastructure.ts") ? { CallExpression(node) { + const limit = node.arguments[1]; if ( node.callee.name === "autoPaginate" && - node.arguments[1]?.name === "limit" + (limit?.name === "limit" || member(limit, "limit")) ) report(node); }, } : {}, ), + "no-manual-pagination": rule( + "Use paginate() from infrastructure.js instead of a limit-bounded cursor loop.", + (_context, file, report) => { + if ( + !file.includes("/src/") || + file.endsWith("/lib/api/infrastructure.ts") + ) + return {}; + // A loop that follows cursors and also counts toward a limit is + // re-deriving per_page and cursor-after-trim rules that paginate() owns. + const check = (node) => { + const followsCursors = + containsIdentifier(node, "MAX_PAGINATION_PAGES") || + containsIdentifier(node, "nextCursor"); + if (followsCursors && containsIdentifier(node, "limit")) report(node); + }; + return { + ForStatement: check, + WhileStatement: check, + DoWhileStatement: check, + }; + }, + ), "no-namespace-import": rule( "Import only the names used by production code.", (_context, file, report) => diff --git a/packages/cli/src/commands/alert/issues/list.ts b/packages/cli/src/commands/alert/issues/list.ts index a7778109e..f154afa74 100644 --- a/packages/cli/src/commands/alert/issues/list.ts +++ b/packages/cli/src/commands/alert/issues/list.ts @@ -13,11 +13,8 @@ import type { SentryContext } from "../../../context.js"; import { buildProjectAliasMap } from "../../../lib/alias.js"; import type { IssueAlertRule } from "../../../lib/api/alerts.js"; -import { MAX_PAGINATION_PAGES } from "../../../lib/api/infrastructure.js"; -import { - API_MAX_PER_PAGE, - listIssueAlertsPaginated, -} from "../../../lib/api-client.js"; +import { paginate } from "../../../lib/api/infrastructure.js"; +import { listIssueAlertsPaginated } from "../../../lib/api-client.js"; import { parseOrgProjectArg } from "../../../lib/arg-parsing.js"; import { openInBrowser } from "../../../lib/browser.js"; import { @@ -137,43 +134,15 @@ async function fetchRulesForTarget( options: { limit: number; startCursor?: string }, ): Promise { const result = await withAuthGuard(async () => { - const rules: IssueAlertRule[] = []; - let serverCursor = options.startCursor; - - for (let page = 0; page < MAX_PAGINATION_PAGES; page++) { - const { data, nextCursor } = await listIssueAlertsPaginated( - target.org, - target.project, - { - perPage: Math.min(options.limit - rules.length, API_MAX_PER_PAGE), - cursor: serverCursor, - }, - ); - - for (const rule of data) { - rules.push(rule); - if (rules.length >= options.limit) { - return { - target, - rules, - hasMore: !!nextCursor, - nextCursor: nextCursor ?? undefined, - }; - } - } - - if (!nextCursor) { - return { target, rules, hasMore: false }; - } - serverCursor = nextCursor; - } - - return { - target, - rules, - hasMore: !!serverCursor, - nextCursor: serverCursor ?? undefined, - }; + const { data: rules, nextCursor } = await paginate( + { limit: options.limit, cursor: options.startCursor }, + (perPage, cursor) => + listIssueAlertsPaginated(target.org, target.project, { + perPage, + cursor, + }), + ); + return { target, rules, hasMore: !!nextCursor, nextCursor }; }); if (!result.ok) { diff --git a/packages/cli/src/commands/alert/metrics/list.ts b/packages/cli/src/commands/alert/metrics/list.ts index f5f01fe17..9394a2e2e 100644 --- a/packages/cli/src/commands/alert/metrics/list.ts +++ b/packages/cli/src/commands/alert/metrics/list.ts @@ -12,11 +12,8 @@ import type { SentryContext } from "../../../context.js"; import type { MetricAlertRule } from "../../../lib/api/alerts.js"; -import { MAX_PAGINATION_PAGES } from "../../../lib/api/infrastructure.js"; -import { - API_MAX_PER_PAGE, - listMetricAlertsPaginated, -} from "../../../lib/api-client.js"; +import { paginate } from "../../../lib/api/infrastructure.js"; +import { listMetricAlertsPaginated } from "../../../lib/api-client.js"; import { parseOrgProjectArg } from "../../../lib/arg-parsing.js"; import { openInBrowser } from "../../../lib/browser.js"; import { @@ -143,39 +140,12 @@ async function fetchRulesForOrg( options: { limit: number; startCursor?: string }, ): Promise { const result = await withAuthGuard(async () => { - const rules: MetricAlertRule[] = []; - let serverCursor = options.startCursor; - - for (let page = 0; page < MAX_PAGINATION_PAGES; page++) { - const { data, nextCursor } = await listMetricAlertsPaginated(orgSlug, { - perPage: Math.min(options.limit - rules.length, API_MAX_PER_PAGE), - cursor: serverCursor, - }); - - for (const rule of data) { - rules.push(rule); - if (rules.length >= options.limit) { - return { - orgSlug, - rules, - hasMore: !!nextCursor, - nextCursor: nextCursor ?? undefined, - }; - } - } - - if (!nextCursor) { - return { orgSlug, rules, hasMore: false }; - } - serverCursor = nextCursor; - } - - return { - orgSlug, - rules, - hasMore: !!serverCursor, - nextCursor: serverCursor, - }; + const { data: rules, nextCursor } = await paginate( + { limit: options.limit, cursor: options.startCursor }, + (perPage, cursor) => + listMetricAlertsPaginated(orgSlug, { perPage, cursor }), + ); + return { orgSlug, rules, hasMore: !!nextCursor, nextCursor }; }); if (!result.ok) { diff --git a/packages/cli/src/commands/dashboard/list.ts b/packages/cli/src/commands/dashboard/list.ts index 6dfd52213..476d0b1a1 100644 --- a/packages/cli/src/commands/dashboard/list.ts +++ b/packages/cli/src/commands/dashboard/list.ts @@ -293,6 +293,9 @@ async function fetchDashboards( const allTitles: string[] = []; let cursorToStore: string | undefined; + // Filters titles client-side, so it cannot count rows with paginate(). It + // keeps one page size per walk and resumes mid-page via afterId bookmarks. + // oxlint-disable-next-line sentry-cli/no-manual-pagination for (let page = 0; page < MAX_PAGINATION_PAGES; page++) { const { data, nextCursor } = await listDashboardsPaginated(orgSlug, { perPage: opts.perPage, diff --git a/packages/cli/src/commands/dashboard/revisions.ts b/packages/cli/src/commands/dashboard/revisions.ts index 7cc551bac..5f034e05f 100644 --- a/packages/cli/src/commands/dashboard/revisions.ts +++ b/packages/cli/src/commands/dashboard/revisions.ts @@ -5,11 +5,8 @@ */ import type { SentryContext } from "../../context.js"; -import { MAX_PAGINATION_PAGES } from "../../lib/api/infrastructure.js"; -import { - API_MAX_PER_PAGE, - listDashboardRevisionsPaginated, -} from "../../lib/api-client.js"; +import { paginate } from "../../lib/api/infrastructure.js"; +import { listDashboardRevisionsPaginated } from "../../lib/api-client.js"; import { parseOrgProjectArg } from "../../lib/arg-parsing.js"; import { buildCommand } from "../../lib/command.js"; import { parseOrgProjectTarget } from "../../lib/completion-target.js"; @@ -169,11 +166,7 @@ export const revisionsCommand = buildCommand({ contextKey, ); - const results: DashboardRevision[] = []; - let cursor = rawCursor; - let nextCursor: string | undefined; - - await withDashboardErrors( + const { data: revisions, nextCursor } = await withDashboardErrors( { orgSlug, dashboardId, @@ -182,50 +175,29 @@ export const revisionsCommand = buildCommand({ () => withProgress( { message: "Fetching revisions...", json: flags.json }, - async () => { - for (let page = 0; page < MAX_PAGINATION_PAGES; page++) { - const { data, nextCursor: nc } = - await listDashboardRevisionsPaginated(orgSlug, dashboardId, { - // Ask only for the remaining budget so the last page's - // cursor points at the first row we did not return. - perPage: Math.min( - flags.limit - results.length, - API_MAX_PER_PAGE, - ), - cursor, - }); - results.push(...data); - nextCursor = nc; - if (results.length >= flags.limit || !nc) { - break; - } - cursor = nc; - } - }, + () => + paginate( + { limit: flags.limit, cursor: rawCursor }, + (perPage, pageCursor) => + listDashboardRevisionsPaginated(orgSlug, dashboardId, { + perPage, + cursor: pageCursor, + }), + ), ), ); + const hasMore = nextCursor !== undefined; - const trimmed = results.slice(0, flags.limit); - // A page larger than requested would make the server cursor skip the - // trimmed rows, so drop it (same as autoPaginate). - const cursorToStore = results.length > flags.limit ? undefined : nextCursor; - const hasMore = cursorToStore !== undefined; - - advancePaginationState( - PAGINATION_KEY, - contextKey, - direction, - cursorToStore, - ); + advancePaginationState(PAGINATION_KEY, contextKey, direction, nextCursor); const hasPrev = hasPreviousPage(PAGINATION_KEY, contextKey); const outputData: RevisionsResult = { - revisions: trimmed, + revisions, orgSlug, dashboardId, hasMore, hasPrev: hasPrev || undefined, - nextCursor: cursorToStore, + nextCursor, }; yield new CommandOutput(outputData); @@ -238,13 +210,13 @@ export const revisionsCommand = buildCommand({ }); const navStr = nav ? ` ${nav}` : ""; - if (trimmed.length === 0) { + if (revisions.length === 0) { return { hint: nav ? `No revisions found.${navStr}` : undefined }; } return { hint: - `Showing ${trimmed.length} revision(s) for dashboard ${dashboardId}.${navStr}\n` + + `Showing ${revisions.length} revision(s) for dashboard ${dashboardId}.${navStr}\n` + `Restore: sentry dashboard restore ${orgSlug}/ ${dashboardId} --revision \n` + `Dashboard: ${url}`, }; diff --git a/packages/cli/src/lib/api/events.ts b/packages/cli/src/lib/api/events.ts index 5ea97360f..8b08dca80 100644 --- a/packages/cli/src/lib/api/events.ts +++ b/packages/cli/src/lib/api/events.ts @@ -25,6 +25,7 @@ import { MAX_PAGINATION_PAGES, ORG_FANOUT_CONCURRENCY, type PaginatedResponse, + paginate, unwrapPaginatedResult, unwrapResult, } from "./infrastructure.js"; @@ -242,21 +243,9 @@ export type ListIssueEventsOptions = { /** * List events for a specific issue. * - * Uses the SDK's `listOrganizationIssueEvents` endpoint with region-aware routing. - * When `limit` exceeds {@link API_MAX_PER_PAGE} (100), auto-paginates through - * multiple API calls to fill the requested limit, bounded by - * {@link MAX_PAGINATION_PAGES}. - * - * Page size is capped at `min(limit, API_MAX_PER_PAGE)` via `per_page`, which - * Sentry accepts on this route at runtime even though it is absent from the - * OpenAPI spec. Capping page size keeps the server-issued `nextCursor` aligned - * to a page boundary, preventing the skip bug where trim + keep-cursor would - * jump past items. - * - * When trimming to `limit`, `nextCursor` is PRESERVED: the events cursor is - * offset-based, so resuming from it re-includes any trimmed tail rather than - * skipping it. This prevents both the original skip bug and the stall - * (drop-cursor) regression. + * Uses the SDK's `listOrganizationIssueEvents` endpoint with region-aware routing + * and {@link paginate} for limits above one page. `per_page` is accepted on + * this route at runtime even though it is absent from the OpenAPI spec. * * @param orgSlug - Organization slug for region routing * @param issueId - Numeric issue ID @@ -271,13 +260,8 @@ export async function listIssueEvents( const { limit = 25, query, full, cursor, statsPeriod, start, end } = options; const config = await getOrgSdkConfig(orgSlug); - const perPage = Math.min(limit, API_MAX_PER_PAGE); - - const allEvents: IssueEvent[] = []; - let currentCursor = cursor; - let nextCursor: string | undefined; - for (let page = 0; page < MAX_PAGINATION_PAGES; page += 1) { + return await paginate({ limit, cursor }, async (perPage, pageCursor) => { const result = await listOrganizationIssueEvents({ ...config, path: { @@ -287,7 +271,7 @@ export async function listIssueEvents( query: { query: query || undefined, full, - cursor: currentCursor, + cursor: pageCursor, statsPeriod, start, end, @@ -296,25 +280,9 @@ export async function listIssueEvents( per_page: perPage, } as Parameters[0]["query"], }); - - const paginated = unwrapPaginatedResult( + return unwrapPaginatedResult( result, "Failed to list issue events", ); - - allEvents.push(...(paginated.data as IssueEvent[])); - nextCursor = paginated.nextCursor; - - if (allEvents.length >= limit || !nextCursor) { - break; - } - currentCursor = nextCursor; - } - - // Trim to limit but PRESERVE nextCursor. The events cursor is offset-based, - // so resuming from it re-includes any trimmed tail rather than skipping it. - // Preserving the cursor lets `-c next` advance; dropping it would strand all - // events past the first page. - const data = allEvents.length > limit ? allEvents.slice(0, limit) : allEvents; - return { data, nextCursor }; + }); } diff --git a/packages/cli/src/lib/api/explore.ts b/packages/cli/src/lib/api/explore.ts index fc5ce1e7d..aa13574b4 100644 --- a/packages/cli/src/lib/api/explore.ts +++ b/packages/cli/src/lib/api/explore.ts @@ -15,6 +15,7 @@ import { apiRequestToRegion, MAX_PAGINATION_PAGES, type PaginatedResponse, + paginate, parseLinkHeader, } from "./infrastructure.js"; @@ -163,9 +164,8 @@ export async function queryMetricsMeta( * query, sort, and time range. Supports all standard Sentry Explore fields * including aggregates like `count()`, `count_unique(user)`, `p50(span.duration)`. * - * When `limit` exceeds {@link API_MAX_PER_PAGE}, transparently fetches multiple - * pages using cursor-based pagination (bounded by {@link MAX_PAGINATION_PAGES}). - * Meta is taken from the first page — it is identical across pages. + * Limits above one page are fetched with {@link paginate}. Meta is taken from + * the first page — it is identical across pages. * * @param orgSlug - Organization slug * @param options - Query options @@ -176,52 +176,21 @@ export async function queryEvents( options: ExploreQueryOptions, ): Promise> { const regionUrl = await resolveOrgRegion(orgSlug); - const limit = options.limit ?? 25; - const perPage = Math.min(limit, API_MAX_PER_PAGE); - - // Fast path: single-page fetch when limit fits in one API page - if (limit <= API_MAX_PER_PAGE) { - return await fetchEventsPage(regionUrl, orgSlug, options, perPage); - } - - // Multi-page: accumulate rows across pages up to the requested limit - const allRows: Record[] = []; - let meta: EventsTableResponse["meta"] | undefined; - let cursor: string | undefined = options.cursor; - - for (let page = 0; page < MAX_PAGINATION_PAGES; page += 1) { - const result = await fetchEventsPage( - regionUrl, - orgSlug, - { ...options, cursor }, - perPage, - ); - - if (page === 0) { - meta = result.data.meta; - } - - allRows.push(...result.data.data); - - // Stop when we've reached the requested limit or there are no more pages - if (allRows.length >= limit || !result.nextCursor) { - // Overshot — trim and drop nextCursor (cursor would skip items) - if (allRows.length > limit) { - return { - data: { data: allRows.slice(0, limit), meta }, - }; - } - return { - data: { data: allRows, meta }, - nextCursor: result.nextCursor, - }; - } - - cursor = result.nextCursor; - } - - // Safety limit reached — return what we have, no nextCursor - return { - data: { data: allRows.slice(0, limit), meta }, - }; + let meta: EventsTableResponse["meta"]; + const { data, nextCursor } = await paginate( + options, + async (perPage, cursor) => { + const page = await fetchEventsPage( + regionUrl, + orgSlug, + { ...options, cursor }, + perPage, + ); + // Meta is identical across pages; keep the first page's. + meta ??= page.data.meta; + return { data: page.data.data, nextCursor: page.nextCursor }; + }, + 25, + ); + return { data: { data, meta }, nextCursor }; } diff --git a/packages/cli/src/lib/api/infrastructure.ts b/packages/cli/src/lib/api/infrastructure.ts index af00846bb..7570a544d 100644 --- a/packages/cli/src/lib/api/infrastructure.ts +++ b/packages/cli/src/lib/api/infrastructure.ts @@ -478,22 +478,58 @@ export async function autoPaginate( } /** - * Wire a single-page fetcher into a limit-bounded, auto-paginating list call. + * Choose the single `per_page` used for every request of a {@link paginate} walk. + * + * Sentry's page-number paginators (`OffsetPaginator`, `ChainPaginator`, + * `CallbackPaginator`, used by projects, teams, members, repositories, and + * dashboard revisions) put the page size in the cursor + * (`::`). A request that pairs that cursor with a + * different `per_page` reads the wrong window; `OffsetPaginator` returns the + * end of the old-sized page and silently skips rows. So the page size must + * not change during a walk, and asking for "the remaining budget" on the last + * page is unsafe. + * + * A page size that divides `limit` lets the walk end exactly on `limit`. + * Otherwise {@link paginate} stops one page early, returning fewer rows but a + * cursor that still resumes at the first unreturned row. + * + * @param limit - Total number of items the caller wants + * @returns Page size between 1 and {@link API_MAX_PER_PAGE} + */ +export function pageSizeForLimit(limit: number): number { + if (limit <= API_MAX_PER_PAGE) { + return Math.max(1, limit); + } + const minPages = Math.ceil(limit / API_MAX_PER_PAGE); + // Accept up to twice the minimum number of requests (pages of ~50+ rows). + for (let pages = minPages; pages <= minPages * 2; pages += 1) { + if (limit % pages === 0) { + return limit / pages; + } + } + return API_MAX_PER_PAGE; +} + +/** + * Fetch up to `limit` rows and a cursor that resumes after the last one. * - * Centralizes the two things every list endpoint kept re-deriving by hand and - * occasionally got wrong (see #1458): capping `per_page` at - * {@link API_MAX_PER_PAGE} and threading `limit` plus the initial cursor into - * {@link autoPaginate}. Each request asks only for the remaining item budget, - * so a partial final page can return a cursor without skipping trimmed rows. - * Callers still own region resolution and - * endpoint-specific query building inside `fetchPage`. + * This is the only supported way to build a limit-bounded list; the + * `sentry-cli/no-manual-pagination` lint rule rejects hand-written loops. It + * owns the two decisions those loops kept getting wrong (#1458, #1608): the + * `per_page` of every request ({@link pageSizeForLimit}) and when a server + * cursor may be returned. The returned `nextCursor` always points at the first + * row the caller has not received; it is dropped only if the server ignored + * `per_page` and the rows had to be trimmed. + * + * Callers own region resolution and query building inside `fetchPage`, and + * must send `perPage` as the request's `per_page` unchanged. * * @param options - Caller list options; `limit` bounds total rows, `cursor` is the start cursor - * @param fetchPage - Fetches one page given the capped `perPage` and a page cursor + * @param fetchPage - Fetches one page given the page size and a page cursor * @param defaultLimit - Applied when `options.limit` is undefined - * @returns Accumulated items with optional nextCursor + * @returns Up to `limit` items with optional nextCursor */ -export function paginate( +export async function paginate( options: { limit?: number; cursor?: string }, fetchPage: ( perPage: number, @@ -502,19 +538,31 @@ export function paginate( defaultLimit = 10, ): Promise> { const limit = options.limit ?? defaultLimit; - let remaining = limit; - return autoPaginate( - async (cursor) => { - const result = await fetchPage( - Math.min(remaining, API_MAX_PER_PAGE), - cursor, - ); - remaining -= result.data.length; - return result; - }, - limit, - options.cursor, + const perPage = pageSizeForLimit(limit); + const rows: T[] = []; + let cursor = options.cursor; + + for (let page = 0; page < MAX_PAGINATION_PAGES; page += 1) { + const { data, nextCursor } = await fetchPage(perPage, cursor); + rows.push(...data); + + if (rows.length > limit) { + // The server ignored `per_page`; its cursor would skip the trimmed rows. + return { data: rows.slice(0, limit) }; + } + // Stop before a page that cannot fit: trimming it would leave no cursor + // that resumes at the first row the caller has not received. + if (!nextCursor || rows.length + perPage > limit) { + return { data: rows, nextCursor }; + } + cursor = nextCursor; + } + + logger.warn( + `Pagination limit reached (${MAX_PAGINATION_PAGES} pages, ${rows.length} items). ` + + "Use the returned cursor to continue.", ); + return { data: rows, nextCursor: cursor }; } /** diff --git a/packages/cli/src/lib/api/issues.ts b/packages/cli/src/lib/api/issues.ts index 00b95afc5..40a4132c1 100644 --- a/packages/cli/src/lib/api/issues.ts +++ b/packages/cli/src/lib/api/issues.ts @@ -26,8 +26,8 @@ import { apiRequest, apiRequestToRegion, getOrgSdkConfig, - MAX_PAGINATION_PAGES, type PaginatedResponse, + paginate, unwrapPaginatedResult, } from "./infrastructure.js"; @@ -213,10 +213,8 @@ export type IssuesPage = { * Auto-paginate through issues up to the requested limit. * * The Sentry API caps `per_page` at {@link API_MAX_PER_PAGE} server-side. When the caller - * requests more than that, this function transparently fetches multiple - * pages using cursor-based pagination and returns the combined result. - * - * Safety-bounded by {@link MAX_PAGINATION_PAGES} to prevent runaway requests. + * requests more than that, {@link paginate} transparently fetches multiple + * pages and returns the combined result. * * @param orgSlug - Organization slug * @param projectSlug - Single-slug fallback used when `options.projects` is @@ -255,45 +253,28 @@ export async function listIssuesAllPages( ); } - const allResults: SentryIssue[] = []; - let cursor: string | undefined = options.startCursor; - - for (let page = 0; page < MAX_PAGINATION_PAGES; page++) { - const remaining = options.limit - allResults.length; - const perPage = Math.min(remaining, API_MAX_PER_PAGE); - const response = await listIssuesPaginated(orgSlug, projectSlug, { - query: options.query, - cursor, - perPage, - sort: options.sort, - statsPeriod: options.statsPeriod, - start: options.start, - end: options.end, - projects: options.projects, - groupStatsPeriod: options.groupStatsPeriod, - collapse: options.collapse, - }); - - allResults.push(...response.data); - options.onPage?.(Math.min(allResults.length, options.limit), options.limit); - - // Request only the remaining number of items on each page. A conforming - // API therefore reaches the limit exactly and its cursor remains usable. - // Keep the defensive overshoot branch for servers that ignore `limit`. - if (allResults.length >= options.limit || !response.nextCursor) { - if (allResults.length > options.limit) { - // The cursor resumes after the oversized page; exposing it would skip - // the trimmed items that the caller has not received. - return { issues: allResults.slice(0, options.limit) }; - } - return { issues: allResults, nextCursor: response.nextCursor }; - } - - cursor = response.nextCursor; - } - - // Safety limit reached — return what we have, no nextCursor - return { issues: allResults.slice(0, options.limit) }; + let fetched = 0; + const { data: issues, nextCursor } = await paginate( + { limit: options.limit, cursor: options.startCursor }, + async (perPage, cursor) => { + const response = await listIssuesPaginated(orgSlug, projectSlug, { + query: options.query, + cursor, + perPage, + sort: options.sort, + statsPeriod: options.statsPeriod, + start: options.start, + end: options.end, + projects: options.projects, + groupStatsPeriod: options.groupStatsPeriod, + collapse: options.collapse, + }); + fetched += response.data.length; + options.onPage?.(Math.min(fetched, options.limit), options.limit); + return response; + }, + ); + return { issues, nextCursor }; } /** diff --git a/packages/cli/test/commands/dashboard/revisions.test.ts b/packages/cli/test/commands/dashboard/revisions.test.ts index 1d6b3a442..33d5aaa68 100644 --- a/packages/cli/test/commands/dashboard/revisions.test.ts +++ b/packages/cli/test/commands/dashboard/revisions.test.ts @@ -268,70 +268,54 @@ describe("dashboard revisions command", () => { expect(output).toContain("-c prev"); }); - test("auto-pagination: --limit larger than page size fetches multiple pages", async () => { - // First page returns 2 items + nextCursor, second page returns 1 item + test("auto-pagination: --limit above one page fetches equal-sized pages", async () => { + const page = Array.from({ length: 75 }, () => REVISION_A); listDashboardRevisionsPaginatedSpy - .mockResolvedValueOnce({ - data: [REVISION_A, REVISION_B], - nextCursor: "cursor-page-2", - }) - .mockResolvedValueOnce({ - data: [REVISION_C], - nextCursor: undefined, - }); + .mockResolvedValueOnce({ data: page, nextCursor: "75:1:0" }) + .mockResolvedValueOnce({ data: page, nextCursor: "75:2:0" }); const { context, stdoutWrite } = createMockContext(); const func = await revisionsCommand.loader(); - // Request 3 items, which exceeds a single page of 2 - await func.call(context, defaultFlags({ json: true, limit: 3 }), "123"); - - // Should have called API twice - expect(listDashboardRevisionsPaginatedSpy).toHaveBeenCalledTimes(2); - - const output = stdoutWrite.mock.calls.map((c) => c[0]).join(""); - const parsed = JSON.parse(output); - expect(parsed.data).toHaveLength(3); - expect(parsed.hasMore).toBe(false); + await func.call(context, defaultFlags({ json: true, limit: 150 }), "123"); + + // The revisions endpoint uses OffsetPaginator, whose cursor encodes the + // page size, so every request must send the same per_page. + expect( + listDashboardRevisionsPaginatedSpy.mock.calls.map((call) => call[2]), + ).toEqual([ + { perPage: 75, cursor: undefined }, + { perPage: 75, cursor: "75:1:0" }, + ]); + const parsed = JSON.parse(stdoutWrite.mock.calls.map((c) => c[0]).join("")); + expect(parsed.data).toHaveLength(150); + expect(parsed.hasMore).toBe(true); + expect(parsed.nextCursor).toBe("75:2:0"); }); - test("auto-pagination: later pages request only the remaining budget", async () => { - listDashboardRevisionsPaginatedSpy - .mockResolvedValueOnce({ - data: [REVISION_A, REVISION_B], - nextCursor: "cursor-page-2", - }) - .mockResolvedValueOnce({ - data: [REVISION_C], - nextCursor: "cursor-page-3", - }); + test("auto-pagination: a short page keeps its cursor instead of changing per_page", async () => { + listDashboardRevisionsPaginatedSpy.mockResolvedValueOnce({ + data: [REVISION_A, REVISION_B], + nextCursor: "cursor-page-2", + }); const { context, stdoutWrite } = createMockContext(); const func = await revisionsCommand.loader(); await func.call(context, defaultFlags({ json: true, limit: 3 }), "123"); - expect(listDashboardRevisionsPaginatedSpy).toHaveBeenLastCalledWith( - "test-org", - "123", - { perPage: 1, cursor: "cursor-page-2" }, - ); + expect(listDashboardRevisionsPaginatedSpy).toHaveBeenCalledTimes(1); const parsed = JSON.parse(stdoutWrite.mock.calls.map((c) => c[0]).join("")); - expect(parsed.data).toHaveLength(3); + expect(parsed.data).toHaveLength(2); expect(parsed.hasMore).toBe(true); - expect(parsed.nextCursor).toBe("cursor-page-3"); + expect(parsed.nextCursor).toBe("cursor-page-2"); }); test("auto-pagination: drops the cursor when a page overshoots the limit", async () => { // An endpoint that ignores per_page returns more rows than asked for; its // cursor would skip the trimmed rows on `-c next`. - listDashboardRevisionsPaginatedSpy - .mockResolvedValueOnce({ - data: [REVISION_A, REVISION_B], - nextCursor: "cursor-page-2", - }) - .mockResolvedValueOnce({ - data: [REVISION_C, REVISION_A], - nextCursor: "cursor-page-3", - }); + listDashboardRevisionsPaginatedSpy.mockResolvedValueOnce({ + data: [REVISION_A, REVISION_B, REVISION_C, REVISION_A], + nextCursor: "cursor-page-2", + }); const { context, stdoutWrite } = createMockContext(); const func = await revisionsCommand.loader(); diff --git a/packages/cli/test/lib/api-client.coverage.test.ts b/packages/cli/test/lib/api-client.coverage.test.ts index 65affaed6..9fe530166 100644 --- a/packages/cli/test/lib/api-client.coverage.test.ts +++ b/packages/cli/test/lib/api-client.coverage.test.ts @@ -437,7 +437,7 @@ describe("issues.ts", () => { expect(result.nextCursor).toBe("has-more"); }); - test("uses the remaining limit on the final page and preserves its cursor", async () => { + test("keeps one page size that divides the limit and preserves the last cursor", async () => { const requestedLimits: string[] = []; let callCount = 0; @@ -446,8 +446,7 @@ describe("issues.ts", () => { const req = new Request(input!, init); const url = new URL(req.url); requestedLimits.push(url.searchParams.get("limit") ?? ""); - const count = callCount === 3 ? 50 : 100; - const issues = Array.from({ length: count }, (_, index) => + const issues = Array.from({ length: 50 }, (_, index) => mockIssue({ id: `${callCount}-${index}` }), ); return new Response(JSON.stringify(issues), { @@ -464,8 +463,8 @@ describe("issues.ts", () => { }); expect(result.issues).toHaveLength(250); - expect(requestedLimits).toEqual(["100", "100", "50"]); - expect(result.nextCursor).toBe("cursor-4"); + expect(requestedLimits).toEqual(["50", "50", "50", "50", "50"]); + expect(result.nextCursor).toBe("cursor-6"); }); test("accepts startCursor option", async () => { diff --git a/packages/cli/test/lib/api/events-overshoot.test.ts b/packages/cli/test/lib/api/events-overshoot.test.ts index 6fec75cdc..f7ea9eb54 100644 --- a/packages/cli/test/lib/api/events-overshoot.test.ts +++ b/packages/cli/test/lib/api/events-overshoot.test.ts @@ -4,13 +4,13 @@ * Two bugs framed this behavior: * 1. The original skip bug: trimming an overshooting page while returning the * server's next-page cursor skipped the trimmed tail on `-c next`. - * 2. The overcorrection: dropping the cursor on overshoot stranded all events - * past the first `limit`, so `sentry issue events` could never page forward. + * 2. The overcorrection: without `per_page`, every page overshot the limit and + * dropping the cursor stranded all events past the first `limit`. * - * The fix caps page size with `per_page = min(limit, API_MAX_PER_PAGE)` so the - * server cursor is page-aligned, and trims defensively while PRESERVING the - * cursor (the events cursor is offset-based, so resuming re-includes any trimmed - * tail). These tests pin both the `per_page` request and the cursor preservation. + * `per_page` now matches the walk's page size, so a conforming server never + * overshoots and its cursor stays usable. If a server still returns more rows + * than asked, its cursor was computed from its own page size and would skip + * the trimmed rows, so it is dropped (the shared `paginate()` contract). */ import { afterEach, beforeEach, describe, expect, test } from "vitest"; @@ -72,7 +72,7 @@ describe("listIssueEvents pagination", () => { expect(new URL(capturedUrl).searchParams.get("per_page")).toBe("100"); }); - test("trims to limit but PRESERVES nextCursor when a page overshoots", async () => { + test("trims to limit and drops nextCursor when a page overshoots", async () => { // Server ignores per_page and returns 5 with a next cursor; caller wants 2. globalThis.fetch = mockFetch( async () => @@ -90,9 +90,8 @@ describe("listIssueEvents pagination", () => { expect(result.data).toHaveLength(2); expect(result.data[0]?.id).toBe("evt-0"); expect(result.data[1]?.id).toBe("evt-1"); - // Cursor preserved so `-c next` can advance; the offset-based events cursor - // re-includes the trimmed tail rather than skipping it. - expect(result.nextCursor).toBe("page2"); + // The server cursor resumes after its 5-row page and would skip evt-2..4. + expect(result.nextCursor).toBeUndefined(); }); test("preserves nextCursor when the page exactly fills the limit", async () => { diff --git a/packages/cli/test/lib/api/explore.test.ts b/packages/cli/test/lib/api/explore.test.ts index e9cf7d1a8..d07152280 100644 --- a/packages/cli/test/lib/api/explore.test.ts +++ b/packages/cli/test/lib/api/explore.test.ts @@ -266,13 +266,13 @@ describe("queryEvents", () => { test("auto-paginates when limit exceeds API_MAX_PER_PAGE", async () => { const { getCapturedUrls } = mockSequential([ { - body: { data: makeRows(100), meta: PAGE_META }, + body: { data: makeRows(75), meta: PAGE_META }, headers: { - Link: `; rel="next"; results="true"; cursor="0:100:0"`, + Link: `; rel="next"; results="true"; cursor="0:75:0"`, }, }, { - body: { data: makeRows(50), meta: PAGE_META }, + body: { data: makeRows(75), meta: PAGE_META }, headers: { Link: `; rel="next"; results="false"; cursor=""`, }, @@ -288,20 +288,24 @@ describe("queryEvents", () => { expect(result.data.meta?.fields).toEqual(PAGE_META.fields); expect(result.nextCursor).toBeUndefined(); expect(getCapturedUrls()).toHaveLength(2); + for (const url of getCapturedUrls()) { + expect(url).toContain("per_page=75"); + } }); test("trims results and drops nextCursor when overshoot", async () => { mockSequential([ { - body: { data: makeRows(100), meta: PAGE_META }, + body: { data: makeRows(60), meta: PAGE_META }, headers: { - Link: `; rel="next"; results="true"; cursor="0:100:0"`, + Link: `; rel="next"; results="true"; cursor="0:60:0"`, }, }, { + // Server ignores per_page=60. body: { data: makeRows(100), meta: PAGE_META }, headers: { - Link: `; rel="next"; results="true"; cursor="0:200:0"`, + Link: `; rel="next"; results="true"; cursor="0:160:0"`, }, }, ]); diff --git a/packages/cli/test/lib/api/infrastructure.test.ts b/packages/cli/test/lib/api/infrastructure.test.ts index 95f212f09..688c87381 100644 --- a/packages/cli/test/lib/api/infrastructure.test.ts +++ b/packages/cli/test/lib/api/infrastructure.test.ts @@ -6,6 +6,7 @@ import { fetchAllPages, isTextualContentType, MAX_PAGINATION_PAGES, + pageSizeForLimit, paginate, rawApiRequest, throwApiError, @@ -888,7 +889,7 @@ describe("paginate", () => { expect(spy).toHaveBeenCalledWith(5, undefined); }); - test("accumulates across pages when limit exceeds cap", async () => { + test("keeps one page size that divides the limit", async () => { const spy = vi.fn((perPage: number, cursor: string | undefined) => { const offset = cursor ? Number(cursor) : 0; const data = Array.from({ length: perPage }, (_, i) => offset + i); @@ -900,32 +901,53 @@ describe("paginate", () => { const result = await paginate({ limit: 150 }, spy); expect(result.data).toEqual(Array.from({ length: 150 }, (_, i) => i)); - expect(spy.mock.calls.map(([perPage]) => perPage)).toEqual([ - API_MAX_PER_PAGE, - 50, - ]); + expect(spy.mock.calls.map(([perPage]) => perPage)).toEqual([75, 75]); expect(result.nextCursor).toBe("150"); }); - test("drops nextCursor when a page exceeds the remaining budget", async () => { + test("stops a page early instead of trimming when no page size divides the limit", async () => { + const spy = vi.fn().mockResolvedValue({ + data: Array.from({ length: 100 }, (_, i) => i), + nextCursor: "c1", + }); + + const result = await paginate({ limit: 101 }, spy); + expect(spy).toHaveBeenCalledTimes(1); + expect(spy).toHaveBeenCalledWith(API_MAX_PER_PAGE, undefined); + expect(result.data).toHaveLength(100); + expect(result.nextCursor).toBe("c1"); + }); + + test("drops nextCursor when the server ignores per_page", async () => { const spy = vi .fn() .mockResolvedValueOnce({ - data: Array.from({ length: 100 }, (_, i) => i), + data: Array.from({ length: 75 }, (_, i) => i), nextCursor: "c1", }) .mockResolvedValueOnce({ - data: Array.from({ length: 100 }, (_, i) => 100 + i), + data: Array.from({ length: 100 }, (_, i) => 75 + i), nextCursor: "c2", }); const result = await paginate({ limit: 150 }, spy); expect(result.data).toHaveLength(150); expect(result.nextCursor).toBeUndefined(); - expect(spy.mock.calls.map(([perPage]) => perPage)).toEqual([ - API_MAX_PER_PAGE, - 50, - ]); + }); +}); + +describe("pageSizeForLimit", () => { + test.each([ + [1, 1], + [25, 25], + [100, 100], + [150, 75], + [200, 100], + [250, 50], + [1000, 100], + [101, API_MAX_PER_PAGE], + ])("limit %i uses pages of %i", (limit, perPage) => { + expect(pageSizeForLimit(limit)).toBe(perPage); }); }); diff --git a/packages/cli/test/lib/api/logs.test.ts b/packages/cli/test/lib/api/logs.test.ts index 220c24399..ec3bb7b85 100644 --- a/packages/cli/test/lib/api/logs.test.ts +++ b/packages/cli/test/lib/api/logs.test.ts @@ -206,11 +206,11 @@ describe("listLogs", () => { const responses = [ { - body: { data: makeRows(100, 0), meta: { fields: {} } }, - link: `; rel="next"; results="true"; cursor="0:100:0"`, + body: { data: makeRows(75, 0), meta: { fields: {} } }, + link: `; rel="next"; results="true"; cursor="0:75:0"`, }, { - body: { data: makeRows(50, 100), meta: { fields: {} } }, + body: { data: makeRows(75, 75), meta: { fields: {} } }, link: `; rel="next"; results="false"; cursor=""`, }, ]; @@ -230,6 +230,9 @@ describe("listLogs", () => { expect(logs).toHaveLength(150); expect(urls).toHaveLength(2); + for (const url of urls) { + expect(url).toContain("per_page=75"); + } }); }); diff --git a/packages/cli/test/lib/api/project-dsns.test.ts b/packages/cli/test/lib/api/project-dsns.test.ts index 5ee0e6320..9a86afb66 100644 --- a/packages/cli/test/lib/api/project-dsns.test.ts +++ b/packages/cli/test/lib/api/project-dsns.test.ts @@ -144,7 +144,7 @@ describe.each([ expect(projectIds.sort()).toEqual(organization ? ["42", "7"] : []); }); - test("forwards page budgets and cursors, reusing project lookups across pages", async () => { + test("keeps one page size per walk and forwards cursors, reusing project lookups", async () => { const { requests, projectIds } = mockKeys((url) => { expect(url.pathname).toBe(path); expect(url.searchParams.get("project")).toBe(organization ? "-1" : null); @@ -172,8 +172,8 @@ describe.each([ return [query.get("per_page"), query.get("cursor")]; }), ).toEqual([ - ["100", "0:25:0"], - ["50", "0:125:0"], + ["75", "0:25:0"], + ["75", "0:100:0"], ["50", "0:175:0"], ]); expect(second.nextCursor).toBeUndefined(); diff --git a/packages/cli/test/lib/api/replays.test.ts b/packages/cli/test/lib/api/replays.test.ts index 381234e24..558977b23 100644 --- a/packages/cli/test/lib/api/replays.test.ts +++ b/packages/cli/test/lib/api/replays.test.ts @@ -138,19 +138,19 @@ describe("listReplays", () => { const body = callIndex === 0 ? { - data: Array.from({ length: 100 }, (_, index) => + data: Array.from({ length: 75 }, (_, index) => replayRow(index.toString(16).padStart(32, "a").slice(-32)), ), } : { - data: Array.from({ length: 50 }, (_, index) => + data: Array.from({ length: 75 }, (_, index) => replayRow(index.toString(16).padStart(32, "b").slice(-32)), ), }; const headers = callIndex === 0 ? { - Link: `; rel="next"; results="true"; cursor="0:100:0"`, + Link: `; rel="next"; results="true"; cursor="0:75:0"`, } : { Link: `; rel="next"; results="false"; cursor=""`, @@ -168,8 +168,8 @@ describe("listReplays", () => { expect(result.data).toHaveLength(150); expect(result.nextCursor).toBeUndefined(); expect(capturedUrls).toHaveLength(2); - expect(capturedUrls[0]).toContain("per_page=100"); - expect(capturedUrls[1]).toContain("per_page=50"); + expect(capturedUrls[0]).toContain("per_page=75"); + expect(capturedUrls[1]).toContain("per_page=75"); }); }); diff --git a/packages/cli/test/lib/api/traces.test.ts b/packages/cli/test/lib/api/traces.test.ts index 9b6cb5ac0..fda8260d2 100644 --- a/packages/cli/test/lib/api/traces.test.ts +++ b/packages/cli/test/lib/api/traces.test.ts @@ -115,16 +115,16 @@ describe("listTransactions", () => { expect(decodeURIComponent(capturedUrl)).toContain("is_transaction:true"); }); - test("uses the remaining item budget for the final page", async () => { + test("keeps one page size that divides the limit", async () => { const { getCapturedUrls } = mockSequential([ { - body: { data: makeTxnRows(100), meta: TX_META }, + body: { data: makeTxnRows(75), meta: TX_META }, headers: { - Link: `; rel="next"; results="true"; cursor="0:100:0"`, + Link: `; rel="next"; results="true"; cursor="0:75:0"`, }, }, { - body: { data: makeTxnRows(50), meta: TX_META }, + body: { data: makeTxnRows(75), meta: TX_META }, headers: { Link: `; rel="next"; results="false"; cursor=""`, }, @@ -133,8 +133,8 @@ describe("listTransactions", () => { await listTransactions("my-org", "my-project", { limit: 150 }); - expect(getCapturedUrls()[0]).toContain("per_page=100"); - expect(getCapturedUrls()[1]).toContain("per_page=50"); + expect(getCapturedUrls()[0]).toContain("per_page=75"); + expect(getCapturedUrls()[1]).toContain("per_page=75"); }); test("sends sort=-timestamp by default", async () => { @@ -206,7 +206,7 @@ describe("listTransactions", () => { }, }, { - body: { data: makeTxnRows(50), meta: TX_META }, + body: { data: makeTxnRows(100), meta: TX_META }, headers: { Link: `; rel="next"; results="false"; cursor=""`, }, @@ -214,10 +214,10 @@ describe("listTransactions", () => { ]); const result = await listTransactions("my-org", "my-project", { - limit: 150, + limit: 200, }); - expect(result.data).toHaveLength(150); + expect(result.data).toHaveLength(200); expect(result.nextCursor).toBeUndefined(); expect(getCapturedUrls()).toHaveLength(2); }); @@ -225,9 +225,9 @@ describe("listTransactions", () => { test("trims results and drops nextCursor when overshoot", async () => { mockSequential([ { - body: { data: makeTxnRows(100), meta: TX_META }, + body: { data: makeTxnRows(60), meta: TX_META }, headers: { - Link: `; rel="next"; results="true"; cursor="0:100:0"`, + Link: `; rel="next"; results="true"; cursor="0:60:0"`, }, }, { @@ -427,16 +427,16 @@ describe("listSpans", () => { expect(capturedUrl).toContain("dataset=spans"); }); - test("uses the remaining item budget for the final page", async () => { + test("keeps one page size that divides the limit", async () => { const { getCapturedUrls } = mockSequential([ { - body: { data: makeSpanRows(100), meta: SPAN_META }, + body: { data: makeSpanRows(75), meta: SPAN_META }, headers: { - Link: `; rel="next"; results="true"; cursor="0:100:0"`, + Link: `; rel="next"; results="true"; cursor="0:75:0"`, }, }, { - body: { data: makeSpanRows(50), meta: SPAN_META }, + body: { data: makeSpanRows(75), meta: SPAN_META }, headers: { Link: `; rel="next"; results="false"; cursor=""`, }, @@ -445,8 +445,8 @@ describe("listSpans", () => { await listSpans("my-org", "my-project", { limit: 150 }); - expect(getCapturedUrls()[0]).toContain("per_page=100"); - expect(getCapturedUrls()[1]).toContain("per_page=50"); + expect(getCapturedUrls()[0]).toContain("per_page=75"); + expect(getCapturedUrls()[1]).toContain("per_page=75"); }); test("sends sort=-timestamp by default", async () => { @@ -495,16 +495,16 @@ describe("listSpans", () => { }, }, { - body: { data: makeSpanRows(50), meta: SPAN_META }, + body: { data: makeSpanRows(100), meta: SPAN_META }, headers: { Link: `; rel="next"; results="false"; cursor=""`, }, }, ]); - const result = await listSpans("my-org", "my-project", { limit: 150 }); + const result = await listSpans("my-org", "my-project", { limit: 200 }); - expect(result.data).toHaveLength(150); + expect(result.data).toHaveLength(200); expect(result.nextCursor).toBeUndefined(); expect(getCapturedUrls()).toHaveLength(2); }); @@ -512,9 +512,9 @@ describe("listSpans", () => { test("trims results when overshoot", async () => { mockSequential([ { - body: { data: makeSpanRows(100), meta: SPAN_META }, + body: { data: makeSpanRows(60), meta: SPAN_META }, headers: { - Link: `; rel="next"; results="true"; cursor="0:100:0"`, + Link: `; rel="next"; results="true"; cursor="0:60:0"`, }, }, { diff --git a/packages/cli/test/lib/auto-paginate.property.test.ts b/packages/cli/test/lib/auto-paginate.property.test.ts index df22c9f3b..c5e4995f3 100644 --- a/packages/cli/test/lib/auto-paginate.property.test.ts +++ b/packages/cli/test/lib/auto-paginate.property.test.ts @@ -5,9 +5,8 @@ * * - Fast path (`limit <= API_MAX_PER_PAGE`): returns a single page verbatim, * WITHOUT trimming — so a page that overshoots `limit` is returned as-is with - * its nextCursor. Callers whose endpoint can overshoot a single page (e.g. - * `listIssueEvents`, which has no per-page param) must trim-and-drop-cursor - * themselves; that behavior is covered by `events-overshoot.test.ts`. + * its nextCursor. Limit-bounded lists use `paginate()` instead, which trims + * and drops the cursor (see `paginate.property.test.ts`). * - Multi-page path (`limit > API_MAX_PER_PAGE`): accumulates across pages up to * `limit`. On true overshoot (a page pushes the total strictly past `limit`), * it trims to `limit` AND drops nextCursor, so cursor navigation never skips diff --git a/packages/cli/test/lib/org-list.test.ts b/packages/cli/test/lib/org-list.test.ts index e8a16b636..327ac511a 100644 --- a/packages/cli/test/lib/org-list.test.ts +++ b/packages/cli/test/lib/org-list.test.ts @@ -511,11 +511,10 @@ describe("handleOrgAll", () => { }); expect(result.items).toHaveLength(250); - expect(listPaginated).toHaveBeenCalledTimes(3); - // Each request uses only the remaining item budget, capped at API_MAX_PER_PAGE, - // so the final page requests only the remaining 50 items (250 - 100 - 100). + // Every request uses the same page size (team and repo lists use + // page-number cursors), chosen to divide the limit: 250 = 5 x 50. expect(listPaginated.mock.calls.map((call) => call[1].perPage)).toEqual([ - 100, 100, 50, + 50, 50, 50, 50, 50, ]); // The mock always returns a nextCursor, so hasMore is true and // the cursor points to the first item beyond the fetched 250. diff --git a/packages/cli/test/lib/paginate.property.test.ts b/packages/cli/test/lib/paginate.property.test.ts new file mode 100644 index 000000000..29576ef0d --- /dev/null +++ b/packages/cli/test/lib/paginate.property.test.ts @@ -0,0 +1,115 @@ +/** + * Property-based tests for paginate() against models of Sentry's paginators. + * + * Earlier pagination tests modeled every cursor as an absolute row offset, + * which hid a skip bug: Sentry's page-number paginators put the page size in + * the cursor, so changing `per_page` mid-walk reads the wrong rows. These + * models port the cursor math from `src/sentry/api/paginator.py`, and the + * properties walk `-c next` the way the CLI does, asserting that every row is + * returned exactly once, in order. + */ + +import { asyncProperty, assert as fcAssert, integer, nat } from "fast-check"; +import { describe, expect, test } from "vitest"; +import { + API_MAX_PER_PAGE, + type PaginatedResponse, + paginate, +} from "../../src/lib/api/infrastructure.js"; +import { DEFAULT_NUM_RUNS } from "../model-based/helpers.js"; + +type FetchPage = ( + perPage: number, + cursor: string | undefined, +) => Promise>; + +function parseCursor(cursor: string | undefined): [number, number] { + const [value = 0, offset = 0] = (cursor ?? "0:0:0").split(":").map(Number); + return [value, offset]; +} + +/** `GenericOffsetPaginator`: cursor offset is an absolute row offset. */ +function genericOffsetPaginator(total: number): FetchPage { + return (perPage, cursor) => { + const [, offset] = parseCursor(cursor); + const end = Math.min(offset + perPage, total); + const data = Array.from({ length: end - offset }, (_, i) => offset + i); + return Promise.resolve({ + data, + nextCursor: end < total ? `0:${end}:0` : undefined, + }); + }; +} + +/** + * `OffsetPaginator`: cursor is `::`. + * + * Port of `OffsetPaginator.get_result`, including the window it reads when + * the cursor's page size differs from the request's `per_page`. + */ +function offsetPaginator(total: number): FetchPage { + const rows = Array.from({ length: total }, (_, i) => i); + return (perPage, cursor) => { + const [value, page] = parseCursor(cursor); + const limit = Math.min(perPage, API_MAX_PER_PAGE); + const offset = page * value; + const stop = offset + (value || limit) + 1; + let results = rows.slice(offset, stop); + if (value !== limit) { + results = results.slice(-(limit + 1)); + } + return Promise.resolve({ + data: results.slice(0, limit), + nextCursor: results.length > limit ? `${limit}:${page + 1}:0` : undefined, + }); + }; +} + +const paginators = [ + { name: "GenericOffsetPaginator", make: genericOffsetPaginator }, + { name: "OffsetPaginator", make: offsetPaginator }, +]; + +/** Follow `-c next` until the cursor runs out, like repeated CLI calls. */ +async function walk(fetchPage: FetchPage, total: number, limit: number) { + const seen: number[] = []; + let cursor: string | undefined; + // Each call returns at least one row while rows remain. + for (let call = 0; call <= total; call += 1) { + const result = await paginate({ limit, cursor }, fetchPage); + expect(result.data.length).toBeLessThanOrEqual(limit); + seen.push(...result.data); + if (!result.nextCursor) { + return seen; + } + cursor = result.nextCursor; + } + throw new Error("walk did not finish"); +} + +const totalArb = nat(API_MAX_PER_PAGE * 5); +const limitArb = integer({ min: 1, max: API_MAX_PER_PAGE * 4 }); + +describe.each(paginators)("property: paginate over $name", ({ make }) => { + test("returns every row exactly once, in order, across -c next", async () => { + await fcAssert( + asyncProperty(totalArb, limitArb, async (total, limit) => { + const seen = await walk(make(total), total, limit); + expect(seen).toEqual(Array.from({ length: total }, (_, i) => i)); + }), + { numRuns: DEFAULT_NUM_RUNS }, + ); + }); + + test("fills the limit whenever a page size divides it", async () => { + await fcAssert( + asyncProperty(totalArb, limitArb, async (total, limit) => { + const result = await paginate({ limit }, make(total)); + if (limit <= API_MAX_PER_PAGE || limit % API_MAX_PER_PAGE === 0) { + expect(result.data).toHaveLength(Math.min(total, limit)); + } + }), + { numRuns: DEFAULT_NUM_RUNS }, + ); + }); +}); From 2d2cb0f8477c67fa14fc0589058f3981356c2ef5 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 23 Oct 2026 14:23:42 +0000 Subject: [PATCH 2/4] fix(cli): Carry the page size in paginate() cursors Resuming a page-number cursor with a different --limit misaligned, because Sentry's OffsetPaginator encodes the page size in its cursor and a different per_page reads the wrong window. paginate() now returns opaque CLI cursors (cli1~~~) that record the walk's page size and how many rows of the current page were already returned. A later -c next reuses that page size whatever --limit is, and resumes mid-page by refetching the page and skipping returned rows, so it no longer stops a page early or drops the cursor when a server ignores per_page. Raw server cursors are still accepted as a starting point. issue list resumes through listIssuesAllPages so wrapped cursors are decoded, and dashboard list always requests full pages, since its afterId bookmarks already handle mid-page resumes. --- packages/cli/AGENTS.md | 18 ++- packages/cli/src/commands/dashboard/list.ts | 11 +- packages/cli/src/commands/issue/list.ts | 26 +--- packages/cli/src/lib/api/infrastructure.ts | 138 ++++++++++++++---- .../cli/test/commands/dashboard/list.test.ts | 14 +- .../test/commands/dashboard/revisions.test.ts | 58 ++++++-- packages/cli/test/commands/dsn/list.test.ts | 2 +- .../test/commands/integration/list.test.ts | 4 +- packages/cli/test/commands/issue/list.test.ts | 36 +++-- .../cli/test/commands/project/list.test.ts | 12 +- packages/cli/test/commands/repo/list.test.ts | 4 +- packages/cli/test/commands/team/list.test.ts | 4 +- .../cli/test/lib/api-client.coverage.test.ts | 20 ++- packages/cli/test/lib/api-client.test.ts | 6 +- .../cli/test/lib/api/conversations.test.ts | 2 +- .../cli/test/lib/api/events-overshoot.test.ts | 17 ++- packages/cli/test/lib/api/explore.test.ts | 8 +- .../cli/test/lib/api/infrastructure.test.ts | 98 ++++++++----- .../cli/test/lib/api/project-dsns.test.ts | 7 +- packages/cli/test/lib/api/replays.test.ts | 7 +- packages/cli/test/lib/api/traces.test.ts | 14 +- packages/cli/test/lib/org-list.test.ts | 25 +++- .../cli/test/lib/paginate.property.test.ts | 57 ++++++-- 23 files changed, 388 insertions(+), 200 deletions(-) diff --git a/packages/cli/AGENTS.md b/packages/cli/AGENTS.md index cf64bc66e..0d6925176 100644 --- a/packages/cli/AGENTS.md +++ b/packages/cli/AGENTS.md @@ -424,13 +424,17 @@ const { data, nextCursor } = await paginate( ); ``` -Pass `perPage` to the request unchanged. `paginate()` keeps one page size for -the whole walk because Sentry's page-number paginators (`OffsetPaginator`, -used by projects, teams, members, repositories, and dashboard revisions) -encode the page size in the cursor; asking for "the remaining budget" on the -last page makes them skip rows. It also only returns a cursor that resumes at -the first row the caller has not received. Test pagination changes against -the paginator models in `test/lib/paginate.property.test.ts`. +Pass `perPage` to the request unchanged. Sentry's page-number paginators +(`OffsetPaginator`, used by projects, teams, members, repositories, and +dashboard revisions) encode the page size in the cursor, so a cursor is only +valid with the `per_page` that produced it. `paginate()` therefore returns +opaque CLI cursors (`cli1~~~`) that record the +walk's page size and how many rows of the current page were already returned. +A later `-c next` reuses that page size even when `--limit` changes, and +resumes mid-page by refetching the page and skipping returned rows. Always +pass these cursors back through `paginate()`; never send them to an API +request directly. Test pagination changes against the paginator models in +`test/lib/paginate.property.test.ts`. Reference template: `trace/list.ts`, `span/list.ts`, `dashboard/revisions.ts` diff --git a/packages/cli/src/commands/dashboard/list.ts b/packages/cli/src/commands/dashboard/list.ts index 476d0b1a1..134267003 100644 --- a/packages/cli/src/commands/dashboard/list.ts +++ b/packages/cli/src/commands/dashboard/list.ts @@ -294,7 +294,7 @@ async function fetchDashboards( let cursorToStore: string | undefined; // Filters titles client-side, so it cannot count rows with paginate(). It - // keeps one page size per walk and resumes mid-page via afterId bookmarks. + // always uses one page size and resumes mid-page via afterId bookmarks. // oxlint-disable-next-line sentry-cli/no-manual-pagination for (let page = 0; page < MAX_PAGINATION_PAGES; page++) { const { data, nextCursor } = await listDashboardsPaginated(orgSlug, { @@ -442,11 +442,10 @@ export const listCommand = buildListCommand("dashboard", { ? picomatch(titleFilter.toLowerCase(), { dot: true }) : undefined; - // When filtering, fetch max-size pages to minimize round trips. - // When not filtering, cap at the smaller of limit and API max. - const perPage = glob - ? API_MAX_PER_PAGE - : Math.min(flags.limit, API_MAX_PER_PAGE); + // Dashboards use a page-number paginator whose cursor is only valid with + // the page size that produced it, so every call uses the same size, even + // when `-c next` changes --limit. afterId bookmarks resume mid-page. + const perPage = API_MAX_PER_PAGE; const { dashboards: results, diff --git a/packages/cli/src/commands/issue/list.ts b/packages/cli/src/commands/issue/list.ts index c5be1e075..0e0758256 100644 --- a/packages/cli/src/commands/issue/list.ts +++ b/packages/cli/src/commands/issue/list.ts @@ -8,12 +8,10 @@ import type { SentryContext } from "../../context.js"; import { buildProjectAliasMap } from "../../lib/alias.js"; import { - API_MAX_PER_PAGE, buildIssueListCollapse, type IssueCollapseField, type IssuesPage, listIssuesAllPages, - listIssuesPaginated, } from "../../lib/api-client.js"; import { extractRequiredScopes } from "../../lib/api-scope.js"; import { @@ -744,8 +742,7 @@ function pageHint( /** * Fetch issues for an organization, optionally restricted to a project set. * - * When `cursor` is provided (--cursor resume), fetches a single page to keep the - * cursor chain intact. Otherwise auto-paginates up to the requested limit. + * Auto-paginates up to the requested limit, resuming from `cursor` when given. */ async function fetchOrganizationIssues( org: string, @@ -762,29 +759,10 @@ async function fetchOrganizationIssues( const timeParams = timeRangeToApiParams(timeRange); const { cursor, onPage, projects } = options; - // When resuming with --cursor, fetch a single page so the cursor chain stays intact. - if (cursor) { - const perPage = Math.min(flags.limit, API_MAX_PER_PAGE); - const response = await listIssuesPaginated(org, "", { - query: flags.query, - cursor, - perPage, - sort: flags.sort, - ...timeParams, - projects, - groupStatsPeriod: apiOpts.groupStatsPeriod, - collapse: apiOpts.collapse, - }); - return { - issues: applyQueryScopedCounts(response.data), - nextCursor: response.nextCursor, - }; - } - - // No cursor — auto-paginate from the beginning via the shared helper. const { issues, nextCursor } = await listIssuesAllPages(org, "", { query: flags.query, limit: flags.limit, + startCursor: cursor, sort: flags.sort, ...timeParams, projects, diff --git a/packages/cli/src/lib/api/infrastructure.ts b/packages/cli/src/lib/api/infrastructure.ts index 7570a544d..d4d9bc5e8 100644 --- a/packages/cli/src/lib/api/infrastructure.ts +++ b/packages/cli/src/lib/api/infrastructure.ts @@ -25,7 +25,12 @@ import { isEnvTokenActive, } from "../db/auth.js"; import { getEnv } from "../env.js"; -import { ApiError, AuthError, stringifyUnknown } from "../errors.js"; +import { + ApiError, + AuthError, + stringifyUnknown, + ValidationError, +} from "../errors.js"; import { logger } from "../logger.js"; import { resolveOrgRegion } from "../region.js"; import { @@ -478,20 +483,19 @@ export async function autoPaginate( } /** - * Choose the single `per_page` used for every request of a {@link paginate} walk. + * Choose the `per_page` for a {@link paginate} walk that starts fresh. * * Sentry's page-number paginators (`OffsetPaginator`, `ChainPaginator`, * `CallbackPaginator`, used by projects, teams, members, repositories, and * dashboard revisions) put the page size in the cursor * (`::`). A request that pairs that cursor with a * different `per_page` reads the wrong window; `OffsetPaginator` returns the - * end of the old-sized page and silently skips rows. So the page size must - * not change during a walk, and asking for "the remaining budget" on the last - * page is unsafe. + * end of the old-sized page and silently skips rows. So a walk keeps one page + * size from its first request through every `-c next`, and {@link paginate} + * records it in the cursors it returns. * - * A page size that divides `limit` lets the walk end exactly on `limit`. - * Otherwise {@link paginate} stops one page early, returning fewer rows but a - * cursor that still resumes at the first unreturned row. + * A page size that divides `limit` lets each call end on a page boundary, so + * the next call does not have to refetch a partly returned page. * * @param limit - Total number of items the caller wants * @returns Page size between 1 and {@link API_MAX_PER_PAGE} @@ -510,22 +514,78 @@ export function pageSizeForLimit(limit: number): number { return API_MAX_PER_PAGE; } +/** Marks cursors produced by {@link paginate}; Sentry cursors never use it. */ +const PAGE_CURSOR_PREFIX = "cli1~"; + +/** + * Where a {@link paginate} walk resumes: the server cursor that fetches the + * page, the page size it must be fetched with, and how many of that page's + * rows were already returned. + */ +type PagePosition = { + cursor: string | undefined; + perPage: number; + skip: number; +}; + +function encodePageCursor({ cursor, perPage, skip }: PagePosition): string { + return `${PAGE_CURSOR_PREFIX}${perPage}~${skip}~${cursor ?? ""}`; +} + +/** + * Decode a cursor from {@link paginate}, or treat anything else as a raw + * server cursor (older stored cursors, or one pasted from an API response). + */ +function decodePageCursor( + raw: string | undefined, +): Partial & { cursor: string | undefined } { + if (!raw?.startsWith(PAGE_CURSOR_PREFIX)) { + return { cursor: raw }; + } + const [perPage, skip, ...server] = raw + .slice(PAGE_CURSOR_PREFIX.length) + .split("~"); + const position = { + perPage: Number(perPage), + skip: Number(skip), + cursor: server.join("~") || undefined, + }; + if ( + !Number.isInteger(position.perPage) || + position.perPage < 1 || + position.perPage > API_MAX_PER_PAGE || + !Number.isInteger(position.skip) || + position.skip < 0 + ) { + throw new ValidationError(`Invalid pagination cursor '${raw}'.`, "cursor"); + } + return position; +} + /** * Fetch up to `limit` rows and a cursor that resumes after the last one. * * This is the only supported way to build a limit-bounded list; the * `sentry-cli/no-manual-pagination` lint rule rejects hand-written loops. It - * owns the two decisions those loops kept getting wrong (#1458, #1608): the - * `per_page` of every request ({@link pageSizeForLimit}) and when a server - * cursor may be returned. The returned `nextCursor` always points at the first - * row the caller has not received; it is dropped only if the server ignored - * `per_page` and the rows had to be trimmed. + * owns the decisions those loops kept getting wrong (#1458, #1608): + * + * - Every request of a walk, including later `-c next` calls with a different + * `--limit`, uses the same `per_page`, because page-number cursors are only + * valid with the page size that produced them ({@link pageSizeForLimit}). + * - The returned cursor always resumes at the first row the caller has not + * received. When a call stops inside a page (a smaller `--limit`, or a + * server that ignored `per_page`), the cursor refetches that page and skips + * the rows already returned. + * + * Returned cursors are opaque CLI cursors; pass them back through this + * function, never directly to an API request. Raw server cursors are accepted + * as a starting point. * * Callers own region resolution and query building inside `fetchPage`, and * must send `perPage` as the request's `per_page` unchanged. * * @param options - Caller list options; `limit` bounds total rows, `cursor` is the start cursor - * @param fetchPage - Fetches one page given the page size and a page cursor + * @param fetchPage - Fetches one page given the page size and a server cursor * @param defaultLimit - Applied when `options.limit` is undefined * @returns Up to `limit` items with optional nextCursor */ @@ -538,31 +598,49 @@ export async function paginate( defaultLimit = 10, ): Promise> { const limit = options.limit ?? defaultLimit; - const perPage = pageSizeForLimit(limit); + const start = decodePageCursor(options.cursor); + const perPage = start.perPage ?? pageSizeForLimit(limit); + let cursor = start.cursor; + let skip = start.skip ?? 0; const rows: T[] = []; - let cursor = options.cursor; for (let page = 0; page < MAX_PAGINATION_PAGES; page += 1) { const { data, nextCursor } = await fetchPage(perPage, cursor); - rows.push(...data); - - if (rows.length > limit) { - // The server ignored `per_page`; its cursor would skip the trimmed rows. - return { data: rows.slice(0, limit) }; + const unseen = data.slice(skip); + const remaining = limit - rows.length; + + if (unseen.length > remaining) { + rows.push(...unseen.slice(0, remaining)); + return { + data: rows, + nextCursor: encodePageCursor({ + cursor, + perPage, + skip: skip + remaining, + }), + }; } - // Stop before a page that cannot fit: trimming it would leave no cursor - // that resumes at the first row the caller has not received. - if (!nextCursor || rows.length + perPage > limit) { - return { data: rows, nextCursor }; + rows.push(...unseen); + if (!nextCursor) { + return { data: rows }; } cursor = nextCursor; + skip = 0; + if (rows.length === limit) { + break; + } } - logger.warn( - `Pagination limit reached (${MAX_PAGINATION_PAGES} pages, ${rows.length} items). ` + - "Use the returned cursor to continue.", - ); - return { data: rows, nextCursor: cursor }; + if (rows.length < limit) { + logger.warn( + `Pagination limit reached (${MAX_PAGINATION_PAGES} pages, ${rows.length} items). ` + + "Use the returned cursor to continue.", + ); + } + return { + data: rows, + nextCursor: encodePageCursor({ cursor, perPage, skip: 0 }), + }; } /** diff --git a/packages/cli/test/commands/dashboard/list.test.ts b/packages/cli/test/commands/dashboard/list.test.ts index 1f03d72d6..1d6f322e7 100644 --- a/packages/cli/test/commands/dashboard/list.test.ts +++ b/packages/cli/test/commands/dashboard/list.test.ts @@ -294,7 +294,7 @@ describe("dashboard list command", () => { // --limit flag // ------------------------------------------------------------------------- - test("passes limit as perPage to API", async () => { + test("requests full pages regardless of limit", async () => { resolveOrgSpy.mockResolvedValue("test-org"); listDashboardsPaginatedSpy.mockResolvedValue({ data: [DASHBOARD_A], @@ -306,12 +306,12 @@ describe("dashboard list command", () => { await func.call(context, defaultFlags({ json: true, limit: 10 })); expect(withProgressSpy).toHaveBeenCalled(); - // perPage is Math.min(flags.limit, API_MAX_PER_PAGE). Verify org and cursor; - // perPage derivation tested via integration in the command's own tests. - expect(listDashboardsPaginatedSpy).toHaveBeenCalledWith( - "test-org", - expect.objectContaining({ cursor: undefined }), - ); + // The page size stays fixed so a cursor resumed with another --limit + // still matches the page size the server encoded in it. + expect(listDashboardsPaginatedSpy).toHaveBeenCalledWith("test-org", { + cursor: undefined, + perPage: apiClient.API_MAX_PER_PAGE, + }); }); // ------------------------------------------------------------------------- diff --git a/packages/cli/test/commands/dashboard/revisions.test.ts b/packages/cli/test/commands/dashboard/revisions.test.ts index 33d5aaa68..138b1f301 100644 --- a/packages/cli/test/commands/dashboard/revisions.test.ts +++ b/packages/cli/test/commands/dashboard/revisions.test.ts @@ -289,29 +289,35 @@ describe("dashboard revisions command", () => { const parsed = JSON.parse(stdoutWrite.mock.calls.map((c) => c[0]).join("")); expect(parsed.data).toHaveLength(150); expect(parsed.hasMore).toBe(true); - expect(parsed.nextCursor).toBe("75:2:0"); + expect(parsed.nextCursor).toContain("75:2:0"); }); - test("auto-pagination: a short page keeps its cursor instead of changing per_page", async () => { - listDashboardRevisionsPaginatedSpy.mockResolvedValueOnce({ - data: [REVISION_A, REVISION_B], - nextCursor: "cursor-page-2", - }); + test("auto-pagination: a short page is followed without changing per_page", async () => { + listDashboardRevisionsPaginatedSpy + .mockResolvedValueOnce({ + data: [REVISION_A, REVISION_B], + nextCursor: "cursor-page-2", + }) + .mockResolvedValueOnce({ data: [REVISION_C], nextCursor: undefined }); const { context, stdoutWrite } = createMockContext(); const func = await revisionsCommand.loader(); await func.call(context, defaultFlags({ json: true, limit: 3 }), "123"); - expect(listDashboardRevisionsPaginatedSpy).toHaveBeenCalledTimes(1); + expect( + listDashboardRevisionsPaginatedSpy.mock.calls.map((call) => call[2]), + ).toEqual([ + { perPage: 3, cursor: undefined }, + { perPage: 3, cursor: "cursor-page-2" }, + ]); const parsed = JSON.parse(stdoutWrite.mock.calls.map((c) => c[0]).join("")); - expect(parsed.data).toHaveLength(2); - expect(parsed.hasMore).toBe(true); - expect(parsed.nextCursor).toBe("cursor-page-2"); + expect(parsed.data).toHaveLength(3); + expect(parsed.hasMore).toBe(false); }); - test("auto-pagination: drops the cursor when a page overshoots the limit", async () => { + test("auto-pagination: resumes inside a page that overshoots the limit", async () => { // An endpoint that ignores per_page returns more rows than asked for; its - // cursor would skip the trimmed rows on `-c next`. + // cursor would skip the trimmed rows, so `-c next` refetches the page. listDashboardRevisionsPaginatedSpy.mockResolvedValueOnce({ data: [REVISION_A, REVISION_B, REVISION_C, REVISION_A], nextCursor: "cursor-page-2", @@ -323,8 +329,32 @@ describe("dashboard revisions command", () => { const parsed = JSON.parse(stdoutWrite.mock.calls.map((c) => c[0]).join("")); expect(parsed.data).toHaveLength(3); - expect(parsed.hasMore).toBe(false); - expect(parsed.nextCursor).toBeUndefined(); + expect(parsed.hasMore).toBe(true); + + resolveCursorSpy.mockReturnValue({ + cursor: parsed.nextCursor, + direction: "next", + }); + listDashboardRevisionsPaginatedSpy.mockResolvedValueOnce({ + data: [REVISION_A, REVISION_B, REVISION_C, REVISION_A], + nextCursor: "cursor-page-2", + }); + stdoutWrite.mockClear(); + await func.call( + context, + defaultFlags({ json: true, limit: 1, cursor: "next" }), + "123", + ); + expect(listDashboardRevisionsPaginatedSpy).toHaveBeenLastCalledWith( + expect.anything(), + "123", + { perPage: 3, cursor: undefined }, + ); + const resumed = JSON.parse( + stdoutWrite.mock.calls.map((c) => c[0]).join(""), + ); + expect(resumed.data).toEqual([REVISION_A]); + expect(resumed.nextCursor).toContain("cursor-page-2"); }); // ------------------------------------------------------------------------- diff --git a/packages/cli/test/commands/dsn/list.test.ts b/packages/cli/test/commands/dsn/list.test.ts index 1cc8c73f8..9ad1c0ad7 100644 --- a/packages/cli/test/commands/dsn/list.test.ts +++ b/packages/cli/test/commands/dsn/list.test.ts @@ -554,7 +554,7 @@ describe("dsn list", () => { expect(first).toMatchObject({ hasMore: true, hasPrev: false, - nextCursor: "0:1:0", + nextCursor: expect.stringContaining("0:1:0"), data: [{ org: "test-org", project: "test-project" }], }); const second = await invokeJson({ limit: 1, cursor: "next" }); diff --git a/packages/cli/test/commands/integration/list.test.ts b/packages/cli/test/commands/integration/list.test.ts index e383578e4..a337f3449 100644 --- a/packages/cli/test/commands/integration/list.test.ts +++ b/packages/cli/test/commands/integration/list.test.ts @@ -98,11 +98,11 @@ describe("integration list", () => { const { context, stdoutWrite } = createMockContext(); const func = await listCommand.loader(); - await func.call(context, { limit: 25, json: true }, "my-org/"); + await func.call(context, { limit: 2, json: true }, "my-org/"); const parsed = JSON.parse(stdoutWrite.mock.calls.map((c) => c[0]).join("")); expect(parsed).toHaveProperty("hasMore", true); - expect(parsed).toHaveProperty("nextCursor", "0:25:0"); + expect(parsed.nextCursor).toContain("0:25:0"); expect(parsed.data).toHaveLength(2); expect(parsed.data[0]).toMatchObject({ id: "101", diff --git a/packages/cli/test/commands/issue/list.test.ts b/packages/cli/test/commands/issue/list.test.ts index aaa674bac..66ed0aaf8 100644 --- a/packages/cli/test/commands/issue/list.test.ts +++ b/packages/cli/test/commands/issue/list.test.ts @@ -1097,8 +1097,8 @@ describe("issue list: org-all mode (cursor pagination)", () => { cursor: "cached:cursor:789", direction: "next", }); - listIssuesPaginatedMock.mockResolvedValue({ - data: [sampleIssue], + listIssuesAllPagesMock.mockResolvedValue({ + issues: [sampleIssue], nextCursor: undefined, }); @@ -1121,10 +1121,10 @@ describe("issue list: org-all mode (cursor pagination)", () => { "my-org/", ); - expect(listIssuesPaginatedMock).toHaveBeenCalledWith( + expect(listIssuesAllPagesMock).toHaveBeenCalledWith( "my-org", "", - expect.objectContaining({ cursor: "cached:cursor:789" }), + expect.objectContaining({ limit: 10, startCursor: "cached:cursor:789" }), ); }); @@ -1165,8 +1165,8 @@ describe("issue list: org-all mode (cursor pagination)", () => { cursor: "explicit:cursor:val", direction: "next", }); - listIssuesPaginatedMock.mockResolvedValue({ - data: [sampleIssue], + listIssuesAllPagesMock.mockResolvedValue({ + issues: [sampleIssue], nextCursor: undefined, }); @@ -1189,10 +1189,13 @@ describe("issue list: org-all mode (cursor pagination)", () => { "my-org/", ); - expect(listIssuesPaginatedMock).toHaveBeenCalledWith( + expect(listIssuesAllPagesMock).toHaveBeenCalledWith( "my-org", "", - expect.objectContaining({ cursor: "explicit:cursor:val" }), + expect.objectContaining({ + limit: 10, + startCursor: "explicit:cursor:val", + }), ); }); }); @@ -1235,6 +1238,12 @@ describe("issue list: Phase 2 budget redistribution", () => { // Surplus: 6 - 4 = 2, expandable = [org-one] // Phase 2: fetch 2 more from org-one via cursor resume. + beforeEach(() => { + // Exercise the real pagination helpers, not overrides left by earlier tests. + listIssuesPaginatedMock.mockReset(); + listIssuesAllPagesMock.mockReset(); + }); + test("redistributes surplus to expandable targets", async () => { setOrgRegion("org-one", DEFAULT_SENTRY_URL); setOrgRegion("org-two", DEFAULT_SENTRY_URL); @@ -2198,10 +2207,6 @@ describe("issue list: comma-separated project slugs", () => { ], nextCursor: "1735689600:0:1", }); - listIssuesPaginatedMock.mockResolvedValue({ - data: [], - nextCursor: undefined, - }); await func.call(createContext().context, baseFlags, "test-org/web,api"); await func.call( @@ -2210,10 +2215,13 @@ describe("issue list: comma-separated project slugs", () => { "test-org/api,web", ); - expect(listIssuesPaginatedMock).toHaveBeenCalledWith( + expect(listIssuesAllPagesMock).toHaveBeenLastCalledWith( "test-org", "", - expect.objectContaining({ cursor: "1735689600:0:1", projects: [2, 1] }), + expect.objectContaining({ + startCursor: "1735689600:0:1", + projects: [2, 1], + }), ); }); diff --git a/packages/cli/test/commands/project/list.test.ts b/packages/cli/test/commands/project/list.test.ts index 4fcb85e15..5eaf6f2ec 100644 --- a/packages/cli/test/commands/project/list.test.ts +++ b/packages/cli/test/commands/project/list.test.ts @@ -668,14 +668,14 @@ describe("handleOrgAll", () => { const result = await handleOrgAll({ org: "test-org", - flags: { limit: 30, json: true, fresh: false }, + flags: { limit: 2, json: true, fresh: false }, contextKey: "type:org:test-org", cursor: undefined, direction: "next", }); expect(result.hasMore).toBe(true); - expect(result.nextCursor).toBe("1735689600000:100:0"); + expect(result.nextCursor).toContain("1735689600000:100:0"); expect(result.items).toHaveLength(2); }); @@ -702,7 +702,7 @@ describe("handleOrgAll", () => { await handleOrgAll({ org: "test-org", - flags: { limit: 30, json: false, fresh: false }, + flags: { limit: 2, json: false, fresh: false }, contextKey: "type:org:test-org", cursor: undefined, direction: "next", @@ -710,7 +710,7 @@ describe("handleOrgAll", () => { const state = getPaginationState(PAGINATION_KEY, "type:org:test-org"); expect(state).toBeDefined(); - expect(state!.stack).toContain("1735689600000:100:0"); + expect(state!.stack.at(-1)).toContain("1735689600000:100:0"); }); test("no hasMore truncates pagination stack", async () => { @@ -1485,7 +1485,7 @@ describe("handleAutoDetect", () => { }); const result = await handleAutoDetect("/tmp/test-project", { - limit: 30, + limit: 2, json: false, fresh: false, }); @@ -1503,7 +1503,7 @@ describe("handleAutoDetect", () => { }); const result = await handleAutoDetect("/tmp/test-project", { - limit: 30, + limit: 2, json: true, fresh: false, }); diff --git a/packages/cli/test/commands/repo/list.test.ts b/packages/cli/test/commands/repo/list.test.ts index 2e05adcac..6836d8715 100644 --- a/packages/cli/test/commands/repo/list.test.ts +++ b/packages/cli/test/commands/repo/list.test.ts @@ -414,12 +414,12 @@ describe("listCommand.func — org-all mode (cursor pagination)", () => { const { context, stdoutWrite } = createMockContext(); const func = await listCommand.loader(); - await func.call(context, { limit: 25, json: true }, "my-org/"); + await func.call(context, { limit: 2, json: true }, "my-org/"); const output = stdoutWrite.mock.calls.map((c) => c[0]).join(""); const parsed = JSON.parse(output); expect(parsed).toHaveProperty("hasMore", true); - expect(parsed).toHaveProperty("nextCursor", "cursor:abc:123"); + expect(parsed.nextCursor).toContain("cursor:abc:123"); expect(advancePaginationStateSpy).toHaveBeenCalled(); }); diff --git a/packages/cli/test/commands/team/list.test.ts b/packages/cli/test/commands/team/list.test.ts index fd3575a86..ebe02e60c 100644 --- a/packages/cli/test/commands/team/list.test.ts +++ b/packages/cli/test/commands/team/list.test.ts @@ -400,12 +400,12 @@ describe("listCommand.func — org-all mode (cursor pagination)", () => { const { context, stdoutWrite } = createMockContext(); const func = await listCommand.loader(); - await func.call(context, { limit: 25, json: true }, "my-org/"); + await func.call(context, { limit: 2, json: true }, "my-org/"); const output = stdoutWrite.mock.calls.map((c) => c[0]).join(""); const parsed = JSON.parse(output); expect(parsed).toHaveProperty("hasMore", true); - expect(parsed).toHaveProperty("nextCursor", "cursor:abc:123"); + expect(parsed.nextCursor).toContain("cursor:abc:123"); expect(advancePaginationStateSpy).toHaveBeenCalled(); }); diff --git a/packages/cli/test/lib/api-client.coverage.test.ts b/packages/cli/test/lib/api-client.coverage.test.ts index 9fe530166..6b2377bb3 100644 --- a/packages/cli/test/lib/api-client.coverage.test.ts +++ b/packages/cli/test/lib/api-client.coverage.test.ts @@ -349,7 +349,7 @@ describe("issues.ts", () => { expect(callCount).toBe(2); }); - test("trims when overshooting limit (no nextCursor)", async () => { + test("resumes inside a page that overshoots the limit", async () => { // Return 5 items but we only want 3 globalThis.fetch = mockFetch( async () => @@ -375,8 +375,12 @@ describe("issues.ts", () => { limit: 3, }); expect(result.issues).toHaveLength(3); - // Trimmed — no nextCursor to prevent skipping - expect(result.nextCursor).toBeUndefined(); + // The cursor refetches the page and skips the 3 returned issues. + const next = await listIssuesAllPages("test-org", "test-project", { + limit: 2, + startCursor: result.nextCursor, + }); + expect(next.issues.map((issue) => issue.id)).toEqual(["4", "5"]); }); test("throws when limit < 1", async () => { @@ -434,7 +438,7 @@ describe("issues.ts", () => { }); expect(result.issues).toHaveLength(2); // Exactly at limit with more pages → keep nextCursor - expect(result.nextCursor).toBe("has-more"); + expect(result.nextCursor).toContain("has-more"); }); test("keeps one page size that divides the limit and preserves the last cursor", async () => { @@ -464,7 +468,7 @@ describe("issues.ts", () => { expect(result.issues).toHaveLength(250); expect(requestedLimits).toEqual(["50", "50", "50", "50", "50"]); - expect(result.nextCursor).toBe("cursor-6"); + expect(result.nextCursor).toContain("cursor-6"); }); test("accepts startCursor option", async () => { @@ -1831,10 +1835,12 @@ describe("traces.ts (transactions)", () => { }); }); - const result = await listTransactions("test-org", "test-project"); + const result = await listTransactions("test-org", "test-project", { + limit: 1, + }); expect(result.data).toHaveLength(1); expect(result.data[0]!.transaction).toBe("GET /api/users"); - expect(result.nextCursor).toBe("txn-next"); + expect(result.nextCursor).toContain("txn-next"); }); test("uses numeric project ID via project param", async () => { diff --git a/packages/cli/test/lib/api-client.test.ts b/packages/cli/test/lib/api-client.test.ts index 0fd11195a..2f103a1fe 100644 --- a/packages/cli/test/lib/api-client.test.ts +++ b/packages/cli/test/lib/api-client.test.ts @@ -1489,10 +1489,12 @@ describe("listTransactions", () => { return new Response(JSON.stringify({}), { status: 200 }); }; - const result = await listTransactions("my-org", "my-project"); + const result = await listTransactions("my-org", "my-project", { + limit: 1, + }); expect(Array.isArray(result.data)).toBe(true); expect(result.data).toHaveLength(1); - expect(result.nextCursor).toBe("1735689600:0:0"); + expect(result.nextCursor).toContain("1735689600:0:0"); }); test("returns no nextCursor when link header has results=false", async () => { diff --git a/packages/cli/test/lib/api/conversations.test.ts b/packages/cli/test/lib/api/conversations.test.ts index c284a96e2..0aadb3d07 100644 --- a/packages/cli/test/lib/api/conversations.test.ts +++ b/packages/cli/test/lib/api/conversations.test.ts @@ -208,7 +208,7 @@ describe("listConversations", () => { const result = await listConversations(ORG); - expect(result.nextCursor).toBe("1735689600000:0:0"); + expect(result.nextCursor).toContain("1735689600000:0:0"); }); test("returns undefined nextCursor when no more pages", async () => { diff --git a/packages/cli/test/lib/api/events-overshoot.test.ts b/packages/cli/test/lib/api/events-overshoot.test.ts index f7ea9eb54..c30e71373 100644 --- a/packages/cli/test/lib/api/events-overshoot.test.ts +++ b/packages/cli/test/lib/api/events-overshoot.test.ts @@ -72,7 +72,7 @@ describe("listIssueEvents pagination", () => { expect(new URL(capturedUrl).searchParams.get("per_page")).toBe("100"); }); - test("trims to limit and drops nextCursor when a page overshoots", async () => { + test("trims to limit and resumes inside a page that overshoots", async () => { // Server ignores per_page and returns 5 with a next cursor; caller wants 2. globalThis.fetch = mockFetch( async () => @@ -90,8 +90,17 @@ describe("listIssueEvents pagination", () => { expect(result.data).toHaveLength(2); expect(result.data[0]?.id).toBe("evt-0"); expect(result.data[1]?.id).toBe("evt-1"); - // The server cursor resumes after its 5-row page and would skip evt-2..4. - expect(result.nextCursor).toBeUndefined(); + // The server cursor would skip evt-2..4, so the CLI cursor refetches the + // page and skips the two returned events. + const next = await listIssueEvents("test-org", "123", { + limit: 3, + cursor: result.nextCursor, + }); + expect(next.data.map((event) => event.id)).toEqual([ + "evt-2", + "evt-3", + "evt-4", + ]); }); test("preserves nextCursor when the page exactly fills the limit", async () => { @@ -109,7 +118,7 @@ describe("listIssueEvents pagination", () => { const result = await listIssueEvents("test-org", "123", { limit: 2 }); expect(result.data).toHaveLength(2); - expect(result.nextCursor).toBe("page2"); + expect(result.nextCursor).toContain("page2"); }); test("returns all events with no cursor when under the limit", async () => { diff --git a/packages/cli/test/lib/api/explore.test.ts b/packages/cli/test/lib/api/explore.test.ts index d07152280..9e361c492 100644 --- a/packages/cli/test/lib/api/explore.test.ts +++ b/packages/cli/test/lib/api/explore.test.ts @@ -194,6 +194,7 @@ describe("queryEvents", () => { const result = await queryEvents("my-org", { fields: ["title", "count()"], + limit: 1, }); expect(result.data.data).toHaveLength(1); @@ -201,7 +202,7 @@ describe("queryEvents", () => { title: "string", "count()": "integer", }); - expect(result.nextCursor).toBe(cursor); + expect(result.nextCursor).toContain(cursor); }); test("returns undefined nextCursor when results=false in Link header", async () => { @@ -293,7 +294,7 @@ describe("queryEvents", () => { } }); - test("trims results and drops nextCursor when overshoot", async () => { + test("trims results and resumes inside the overshooting page", async () => { mockSequential([ { body: { data: makeRows(60), meta: PAGE_META }, @@ -316,7 +317,8 @@ describe("queryEvents", () => { }); expect(result.data.data).toHaveLength(120); - expect(result.nextCursor).toBeUndefined(); + // Refetches the second page (cursor 0:60:0) and skips its returned rows. + expect(result.nextCursor).toContain("0:60:0"); }); test("single-page fast path when limit <= API_MAX_PER_PAGE", async () => { diff --git a/packages/cli/test/lib/api/infrastructure.test.ts b/packages/cli/test/lib/api/infrastructure.test.ts index 688c87381..d80175101 100644 --- a/packages/cli/test/lib/api/infrastructure.test.ts +++ b/packages/cli/test/lib/api/infrastructure.test.ts @@ -14,7 +14,11 @@ import { unwrapResult, } from "../../../src/lib/api/infrastructure.js"; import { setAuthToken } from "../../../src/lib/db/auth.js"; -import { ApiError, HostScopeError } from "../../../src/lib/errors.js"; +import { + ApiError, + HostScopeError, + ValidationError, +} from "../../../src/lib/errors.js"; import { mockFetch, useEnvSandbox, useTestConfigDir } from "../../helpers.js"; describe("SDK error compatibility", () => { @@ -889,51 +893,73 @@ describe("paginate", () => { expect(spy).toHaveBeenCalledWith(5, undefined); }); - test("keeps one page size that divides the limit", async () => { - const spy = vi.fn((perPage: number, cursor: string | undefined) => { + /** Serves rows by offset; the cursor is the next offset. */ + function offsetServer() { + return vi.fn((perPage: number, cursor: string | undefined) => { const offset = cursor ? Number(cursor) : 0; const data = Array.from({ length: perPage }, (_, i) => offset + i); - return Promise.resolve({ - data, - nextCursor: String(offset + perPage), - }); + return Promise.resolve({ data, nextCursor: String(offset + perPage) }); }); + } + + test("keeps the walk's page size when -c next changes the limit", async () => { + const spy = offsetServer(); + + const first = await paginate({ limit: 150 }, spy); + expect(first.data).toEqual(Array.from({ length: 150 }, (_, i) => i)); + const second = await paginate({ limit: 10, cursor: first.nextCursor }, spy); + expect(second.data).toEqual(Array.from({ length: 10 }, (_, i) => 150 + i)); + + expect(spy.mock.calls).toEqual([ + [75, undefined], + [75, "75"], + [75, "150"], + ]); + }); + + test("resumes inside a page it could not return in full", async () => { + const spy = offsetServer(); - const result = await paginate({ limit: 150 }, spy); - expect(result.data).toEqual(Array.from({ length: 150 }, (_, i) => i)); - expect(spy.mock.calls.map(([perPage]) => perPage)).toEqual([75, 75]); - expect(result.nextCursor).toBe("150"); + const first = await paginate({ limit: 101 }, spy); + expect(first.data).toEqual(Array.from({ length: 101 }, (_, i) => i)); + const second = await paginate({ limit: 5, cursor: first.nextCursor }, spy); + expect(second.data).toEqual([101, 102, 103, 104, 105]); + + expect(spy.mock.calls).toEqual([ + [100, undefined], + [100, "100"], + [100, "100"], + ]); }); - test("stops a page early instead of trimming when no page size divides the limit", async () => { - const spy = vi.fn().mockResolvedValue({ - data: Array.from({ length: 100 }, (_, i) => i), - nextCursor: "c1", + test("resumes after the last returned row when the server ignores per_page", async () => { + const spy = vi.fn((_perPage: number, cursor: string | undefined) => { + const offset = cursor ? Number(cursor) : 0; + const data = Array.from({ length: 100 }, (_, i) => offset + i); + return Promise.resolve({ data, nextCursor: String(offset + 100) }); }); - const result = await paginate({ limit: 101 }, spy); - expect(spy).toHaveBeenCalledTimes(1); - expect(spy).toHaveBeenCalledWith(API_MAX_PER_PAGE, undefined); - expect(result.data).toHaveLength(100); - expect(result.nextCursor).toBe("c1"); - }); - - test("drops nextCursor when the server ignores per_page", async () => { - const spy = vi - .fn() - .mockResolvedValueOnce({ - data: Array.from({ length: 75 }, (_, i) => i), - nextCursor: "c1", - }) - .mockResolvedValueOnce({ - data: Array.from({ length: 100 }, (_, i) => 75 + i), - nextCursor: "c2", - }); + const first = await paginate({ limit: 150 }, spy); + expect(first.data).toHaveLength(150); + const second = await paginate({ limit: 3, cursor: first.nextCursor }, spy); + expect(second.data).toEqual([150, 151, 152]); + }); - const result = await paginate({ limit: 150 }, spy); - expect(result.data).toHaveLength(150); - expect(result.nextCursor).toBeUndefined(); + test("returns no cursor once the server is exhausted", async () => { + const spy = vi.fn().mockResolvedValue({ data: [1, 2] }); + expect(await paginate({ limit: 5 }, spy)).toEqual({ data: [1, 2] }); }); + + test.each(["cli1~0~0~abc", "cli1~101~0~abc", "cli1~10~-1~abc", "cli1~x~0~"])( + "rejects the malformed CLI cursor %s", + async (cursor) => { + const spy = vi.fn().mockResolvedValue({ data: [] }); + await expect(paginate({ limit: 5, cursor }, spy)).rejects.toThrow( + ValidationError, + ); + expect(spy).not.toHaveBeenCalled(); + }, + ); }); describe("pageSizeForLimit", () => { diff --git a/packages/cli/test/lib/api/project-dsns.test.ts b/packages/cli/test/lib/api/project-dsns.test.ts index 9a86afb66..6264cab82 100644 --- a/packages/cli/test/lib/api/project-dsns.test.ts +++ b/packages/cli/test/lib/api/project-dsns.test.ts @@ -144,7 +144,7 @@ describe.each([ expect(projectIds.sort()).toEqual(organization ? ["42", "7"] : []); }); - test("keeps one page size per walk and forwards cursors, reusing project lookups", async () => { + test("keeps the walk's page size across calls and reuses project lookups", async () => { const { requests, projectIds } = mockKeys((url) => { expect(url.pathname).toBe(path); expect(url.searchParams.get("project")).toBe(organization ? "-1" : null); @@ -163,7 +163,7 @@ describe.each([ const first = await list({ limit: 150, cursor: "0:25:0" }); expect(projectIds).toEqual(organization ? ["42"] : []); expect(first.data).toHaveLength(150); - expect(first.nextCursor).toBe("0:175:0"); + expect(first.nextCursor).toContain("0:175:0"); const second = await list({ limit: 50, cursor: first.nextCursor }); expect( @@ -174,7 +174,8 @@ describe.each([ ).toEqual([ ["75", "0:25:0"], ["75", "0:100:0"], - ["50", "0:175:0"], + // A smaller --limit on -c next keeps the walk's page size. + ["75", "0:175:0"], ]); expect(second.nextCursor).toBeUndefined(); expect([...first.data, ...second.data].map((key) => key.name)).toEqual( diff --git a/packages/cli/test/lib/api/replays.test.ts b/packages/cli/test/lib/api/replays.test.ts index 558977b23..a92926233 100644 --- a/packages/cli/test/lib/api/replays.test.ts +++ b/packages/cli/test/lib/api/replays.test.ts @@ -69,7 +69,8 @@ describe("listReplays", () => { globalThis.fetch = mockFetch(async (input, init) => { const req = new Request(input!, init); capturedUrl = req.url; - return new Response(JSON.stringify({ data: [replayRow()] }), { + const data = Array.from({ length: 25 }, () => replayRow()); + return new Response(JSON.stringify({ data }), { status: 200, headers: { "Content-Type": "application/json", @@ -95,8 +96,8 @@ describe("listReplays", () => { expect(url.searchParams.get("per_page")).toBe("25"); expect(url.searchParams.getAll("field")).toContain("id"); expect(url.searchParams.getAll("field")).toContain("user"); - expect(result.data).toHaveLength(1); - expect(result.nextCursor).toBe("0:25:0"); + expect(result.data).toHaveLength(25); + expect(result.nextCursor).toContain("0:25:0"); }); test("passes replay environment filters and custom field selection", async () => { diff --git a/packages/cli/test/lib/api/traces.test.ts b/packages/cli/test/lib/api/traces.test.ts index fda8260d2..3a29c7292 100644 --- a/packages/cli/test/lib/api/traces.test.ts +++ b/packages/cli/test/lib/api/traces.test.ts @@ -222,7 +222,7 @@ describe("listTransactions", () => { expect(getCapturedUrls()).toHaveLength(2); }); - test("trims results and drops nextCursor when overshoot", async () => { + test("trims results and resumes inside the overshooting page", async () => { mockSequential([ { body: { data: makeTxnRows(60), meta: TX_META }, @@ -243,7 +243,8 @@ describe("listTransactions", () => { }); expect(result.data).toHaveLength(120); - expect(result.nextCursor).toBeUndefined(); + // Refetches the second page (cursor 0:60:0) and skips its returned rows. + expect(result.nextCursor).toContain("0:60:0"); }); test("single-page fast path for limit <= 100", async () => { @@ -309,7 +310,7 @@ describe("listTransactions", () => { limit: 10, }); - expect(result.nextCursor).toBe(cursor); + expect(result.nextCursor).toContain(cursor); }); test("returns undefined nextCursor when results=false", async () => { @@ -509,7 +510,7 @@ describe("listSpans", () => { expect(getCapturedUrls()).toHaveLength(2); }); - test("trims results when overshoot", async () => { + test("trims results and resumes inside the overshooting page", async () => { mockSequential([ { body: { data: makeSpanRows(60), meta: SPAN_META }, @@ -528,7 +529,8 @@ describe("listSpans", () => { const result = await listSpans("my-org", "my-project", { limit: 120 }); expect(result.data).toHaveLength(120); - expect(result.nextCursor).toBeUndefined(); + // Refetches the second page (cursor 0:60:0) and skips its returned rows. + expect(result.nextCursor).toContain("0:60:0"); }); test("single-page fast path for limit <= 100", async () => { @@ -639,7 +641,7 @@ describe("listSpans", () => { const result = await listSpans("my-org", "my-project", { limit: 10 }); - expect(result.nextCursor).toBe(cursor); + expect(result.nextCursor).toContain(cursor); }); test("returns undefined nextCursor when results=false", async () => { diff --git a/packages/cli/test/lib/org-list.test.ts b/packages/cli/test/lib/org-list.test.ts index 327ac511a..b74b4f4d6 100644 --- a/packages/cli/test/lib/org-list.test.ts +++ b/packages/cli/test/lib/org-list.test.ts @@ -312,14 +312,15 @@ describe("handleOrgAll", () => { const result = await handleOrgAll({ config, org: "my-org", - flags: { limit: 10, json: true }, + flags: { limit: 1, json: true }, contextKey: "key", cursor: undefined, direction: "next", }); expect(result.hasMore).toBe(true); - expect(result.nextCursor).toBe("next:123"); + // paginate() wraps the server cursor with the walk's page size. + expect(result.nextCursor).toContain("next:123"); expect(result.items).toHaveLength(1); expect(result.items[0]!.orgSlug).toBe("my-org"); }); @@ -453,7 +454,7 @@ describe("handleOrgAll", () => { "test-list", "ctx", "next", - "cursor:abc", + expect.stringContaining("cursor:abc"), ); }); @@ -519,7 +520,23 @@ describe("handleOrgAll", () => { // The mock always returns a nextCursor, so hasMore is true and // the cursor points to the first item beyond the fetched 250. expect(result.hasMore).toBe(true); - expect(result.nextCursor).toBe("250"); + + // `-c next` with another --limit keeps the walk's page size. + listPaginated.mockClear(); + const next = await handleOrgAll({ + config, + org: "my-org", + flags: { limit: 30, json: true }, + contextKey: "key", + cursor: result.nextCursor, + direction: "next", + }); + expect(next.items.map((item) => item.id)).toEqual( + Array.from({ length: 30 }, (_, i) => String(250 + i)), + ); + expect(listPaginated.mock.calls.map((call) => call[1])).toEqual([ + { cursor: "250", perPage: 50 }, + ]); }); }); diff --git a/packages/cli/test/lib/paginate.property.test.ts b/packages/cli/test/lib/paginate.property.test.ts index 29576ef0d..d48253732 100644 --- a/packages/cli/test/lib/paginate.property.test.ts +++ b/packages/cli/test/lib/paginate.property.test.ts @@ -5,11 +5,18 @@ * which hid a skip bug: Sentry's page-number paginators put the page size in * the cursor, so changing `per_page` mid-walk reads the wrong rows. These * models port the cursor math from `src/sentry/api/paginator.py`, and the - * properties walk `-c next` the way the CLI does, asserting that every row is - * returned exactly once, in order. + * properties walk `-c next` the way the CLI does — with a different + * `--limit` on every call — asserting that every row is returned exactly + * once, in order. */ -import { asyncProperty, assert as fcAssert, integer, nat } from "fast-check"; +import { + array, + asyncProperty, + assert as fcAssert, + integer, + nat, +} from "fast-check"; import { describe, expect, test } from "vitest"; import { API_MAX_PER_PAGE, @@ -65,19 +72,34 @@ function offsetPaginator(total: number): FetchPage { }; } +/** A server that ignores `per_page` and always serves pages of 30 rows. */ +function fixedPageServer(total: number): FetchPage { + return (_perPage, cursor) => genericOffsetPaginator(total)(30, cursor); +} + const paginators = [ { name: "GenericOffsetPaginator", make: genericOffsetPaginator }, { name: "OffsetPaginator", make: offsetPaginator }, + { name: "a server that ignores per_page", make: fixedPageServer }, ]; -/** Follow `-c next` until the cursor runs out, like repeated CLI calls. */ -async function walk(fetchPage: FetchPage, total: number, limit: number) { +/** + * Follow `-c next` until the cursor runs out, like repeated CLI calls, using + * the next limit from `limits` (cycled) on each call. + */ +async function walk(fetchPage: FetchPage, total: number, limits: number[]) { const seen: number[] = []; let cursor: string | undefined; // Each call returns at least one row while rows remain. for (let call = 0; call <= total; call += 1) { + const limit = limits[call % limits.length] ?? 1; const result = await paginate({ limit, cursor }, fetchPage); + // A resumed walk keeps its first page size, so a much larger later limit + // can stop at MAX_PAGINATION_PAGES; it must still make progress. expect(result.data.length).toBeLessThanOrEqual(limit); + if (seen.length < total) { + expect(result.data.length).toBeGreaterThan(0); + } seen.push(...result.data); if (!result.nextCursor) { return seen; @@ -88,26 +110,29 @@ async function walk(fetchPage: FetchPage, total: number, limit: number) { } const totalArb = nat(API_MAX_PER_PAGE * 5); -const limitArb = integer({ min: 1, max: API_MAX_PER_PAGE * 4 }); +const limitsArb = array(integer({ min: 1, max: API_MAX_PER_PAGE * 4 }), { + minLength: 1, + maxLength: 4, +}); describe.each(paginators)("property: paginate over $name", ({ make }) => { - test("returns every row exactly once, in order, across -c next", async () => { + test("fills the limit on a fresh walk", async () => { await fcAssert( - asyncProperty(totalArb, limitArb, async (total, limit) => { - const seen = await walk(make(total), total, limit); - expect(seen).toEqual(Array.from({ length: total }, (_, i) => i)); + asyncProperty(totalArb, limitsArb, async (total, [limit = 1]) => { + const result = await paginate({ limit }, make(total)); + expect(result.data).toEqual( + Array.from({ length: Math.min(limit, total) }, (_, i) => i), + ); }), { numRuns: DEFAULT_NUM_RUNS }, ); }); - test("fills the limit whenever a page size divides it", async () => { + test("returns every row exactly once, in order, across -c next", async () => { await fcAssert( - asyncProperty(totalArb, limitArb, async (total, limit) => { - const result = await paginate({ limit }, make(total)); - if (limit <= API_MAX_PER_PAGE || limit % API_MAX_PER_PAGE === 0) { - expect(result.data).toHaveLength(Math.min(total, limit)); - } + asyncProperty(totalArb, limitsArb, async (total, limits) => { + const seen = await walk(make(total), total, limits); + expect(seen).toEqual(Array.from({ length: total }, (_, i) => i)); }), { numRuns: DEFAULT_NUM_RUNS }, ); From fde9768fa915d0e904689bfb1118c7e16bfa9556 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 23 Oct 2026 14:43:52 +0000 Subject: [PATCH 3/4] fix(cli): Use full pages in paginate() and drop redundant tests Now that paginate() cursors carry the page size and resume mid-page, a page size that divides the limit no longer buys correctness. Use min(limit, API_MAX_PER_PAGE) so large limits need fewer requests, and remove pageSizeForLimit(). Drop per-endpoint tests that re-checked paginate()'s trimming, overshoot, and page-size rules; the property test and paginate's unit tests cover them. Each caller keeps its per_page and cursor wiring tests. --- packages/cli/src/lib/api/infrastructure.ts | 42 ++------- .../test/commands/dashboard/revisions.test.ts | 83 ++--------------- .../cli/test/lib/api-client.coverage.test.ts | 88 ------------------ .../cli/test/lib/api/events-overshoot.test.ts | 82 +---------------- packages/cli/test/lib/api/explore.test.ts | 39 ++------ .../cli/test/lib/api/infrastructure.test.ts | 57 ++---------- packages/cli/test/lib/api/logs.test.ts | 12 +-- .../cli/test/lib/api/project-dsns.test.ts | 10 +- packages/cli/test/lib/api/replays.test.ts | 14 +-- packages/cli/test/lib/api/traces.test.ts | 92 ------------------- packages/cli/test/lib/org-list.test.ts | 11 ++- 11 files changed, 55 insertions(+), 475 deletions(-) diff --git a/packages/cli/src/lib/api/infrastructure.ts b/packages/cli/src/lib/api/infrastructure.ts index d4d9bc5e8..3d13761f2 100644 --- a/packages/cli/src/lib/api/infrastructure.ts +++ b/packages/cli/src/lib/api/infrastructure.ts @@ -482,38 +482,6 @@ export async function autoPaginate( return { data: allRows.slice(0, limit) }; } -/** - * Choose the `per_page` for a {@link paginate} walk that starts fresh. - * - * Sentry's page-number paginators (`OffsetPaginator`, `ChainPaginator`, - * `CallbackPaginator`, used by projects, teams, members, repositories, and - * dashboard revisions) put the page size in the cursor - * (`::`). A request that pairs that cursor with a - * different `per_page` reads the wrong window; `OffsetPaginator` returns the - * end of the old-sized page and silently skips rows. So a walk keeps one page - * size from its first request through every `-c next`, and {@link paginate} - * records it in the cursors it returns. - * - * A page size that divides `limit` lets each call end on a page boundary, so - * the next call does not have to refetch a partly returned page. - * - * @param limit - Total number of items the caller wants - * @returns Page size between 1 and {@link API_MAX_PER_PAGE} - */ -export function pageSizeForLimit(limit: number): number { - if (limit <= API_MAX_PER_PAGE) { - return Math.max(1, limit); - } - const minPages = Math.ceil(limit / API_MAX_PER_PAGE); - // Accept up to twice the minimum number of requests (pages of ~50+ rows). - for (let pages = minPages; pages <= minPages * 2; pages += 1) { - if (limit % pages === 0) { - return limit / pages; - } - } - return API_MAX_PER_PAGE; -} - /** Marks cursors produced by {@link paginate}; Sentry cursors never use it. */ const PAGE_CURSOR_PREFIX = "cli1~"; @@ -570,8 +538,11 @@ function decodePageCursor( * owns the decisions those loops kept getting wrong (#1458, #1608): * * - Every request of a walk, including later `-c next` calls with a different - * `--limit`, uses the same `per_page`, because page-number cursors are only - * valid with the page size that produced them ({@link pageSizeForLimit}). + * `--limit`, uses the `per_page` of its first request. Sentry's page-number + * paginators (`OffsetPaginator`, used by projects, teams, members, + * repositories, and dashboard revisions) put the page size in the cursor + * (`::`), and pairing it with another `per_page` + * reads the wrong window and silently skips rows. * - The returned cursor always resumes at the first row the caller has not * received. When a call stops inside a page (a smaller `--limit`, or a * server that ignored `per_page`), the cursor refetches that page and skips @@ -599,7 +570,8 @@ export async function paginate( ): Promise> { const limit = options.limit ?? defaultLimit; const start = decodePageCursor(options.cursor); - const perPage = start.perPage ?? pageSizeForLimit(limit); + const perPage = + start.perPage ?? Math.max(1, Math.min(limit, API_MAX_PER_PAGE)); let cursor = start.cursor; let skip = start.skip ?? 0; const rows: T[] = []; diff --git a/packages/cli/test/commands/dashboard/revisions.test.ts b/packages/cli/test/commands/dashboard/revisions.test.ts index 138b1f301..700392871 100644 --- a/packages/cli/test/commands/dashboard/revisions.test.ts +++ b/packages/cli/test/commands/dashboard/revisions.test.ts @@ -268,93 +268,28 @@ describe("dashboard revisions command", () => { expect(output).toContain("-c prev"); }); - test("auto-pagination: --limit above one page fetches equal-sized pages", async () => { - const page = Array.from({ length: 75 }, () => REVISION_A); + test("auto-pagination: --limit above one page keeps one per_page", async () => { + const page = Array.from({ length: 100 }, () => REVISION_A); listDashboardRevisionsPaginatedSpy - .mockResolvedValueOnce({ data: page, nextCursor: "75:1:0" }) - .mockResolvedValueOnce({ data: page, nextCursor: "75:2:0" }); + .mockResolvedValueOnce({ data: page, nextCursor: "100:1:0" }) + .mockResolvedValueOnce({ data: page, nextCursor: "100:2:0" }); const { context, stdoutWrite } = createMockContext(); const func = await revisionsCommand.loader(); - await func.call(context, defaultFlags({ json: true, limit: 150 }), "123"); + await func.call(context, defaultFlags({ json: true, limit: 200 }), "123"); // The revisions endpoint uses OffsetPaginator, whose cursor encodes the // page size, so every request must send the same per_page. expect( listDashboardRevisionsPaginatedSpy.mock.calls.map((call) => call[2]), ).toEqual([ - { perPage: 75, cursor: undefined }, - { perPage: 75, cursor: "75:1:0" }, + { perPage: 100, cursor: undefined }, + { perPage: 100, cursor: "100:1:0" }, ]); const parsed = JSON.parse(stdoutWrite.mock.calls.map((c) => c[0]).join("")); - expect(parsed.data).toHaveLength(150); + expect(parsed.data).toHaveLength(200); expect(parsed.hasMore).toBe(true); - expect(parsed.nextCursor).toContain("75:2:0"); - }); - - test("auto-pagination: a short page is followed without changing per_page", async () => { - listDashboardRevisionsPaginatedSpy - .mockResolvedValueOnce({ - data: [REVISION_A, REVISION_B], - nextCursor: "cursor-page-2", - }) - .mockResolvedValueOnce({ data: [REVISION_C], nextCursor: undefined }); - - const { context, stdoutWrite } = createMockContext(); - const func = await revisionsCommand.loader(); - await func.call(context, defaultFlags({ json: true, limit: 3 }), "123"); - - expect( - listDashboardRevisionsPaginatedSpy.mock.calls.map((call) => call[2]), - ).toEqual([ - { perPage: 3, cursor: undefined }, - { perPage: 3, cursor: "cursor-page-2" }, - ]); - const parsed = JSON.parse(stdoutWrite.mock.calls.map((c) => c[0]).join("")); - expect(parsed.data).toHaveLength(3); - expect(parsed.hasMore).toBe(false); - }); - - test("auto-pagination: resumes inside a page that overshoots the limit", async () => { - // An endpoint that ignores per_page returns more rows than asked for; its - // cursor would skip the trimmed rows, so `-c next` refetches the page. - listDashboardRevisionsPaginatedSpy.mockResolvedValueOnce({ - data: [REVISION_A, REVISION_B, REVISION_C, REVISION_A], - nextCursor: "cursor-page-2", - }); - - const { context, stdoutWrite } = createMockContext(); - const func = await revisionsCommand.loader(); - await func.call(context, defaultFlags({ json: true, limit: 3 }), "123"); - - const parsed = JSON.parse(stdoutWrite.mock.calls.map((c) => c[0]).join("")); - expect(parsed.data).toHaveLength(3); - expect(parsed.hasMore).toBe(true); - - resolveCursorSpy.mockReturnValue({ - cursor: parsed.nextCursor, - direction: "next", - }); - listDashboardRevisionsPaginatedSpy.mockResolvedValueOnce({ - data: [REVISION_A, REVISION_B, REVISION_C, REVISION_A], - nextCursor: "cursor-page-2", - }); - stdoutWrite.mockClear(); - await func.call( - context, - defaultFlags({ json: true, limit: 1, cursor: "next" }), - "123", - ); - expect(listDashboardRevisionsPaginatedSpy).toHaveBeenLastCalledWith( - expect.anything(), - "123", - { perPage: 3, cursor: undefined }, - ); - const resumed = JSON.parse( - stdoutWrite.mock.calls.map((c) => c[0]).join(""), - ); - expect(resumed.data).toEqual([REVISION_A]); - expect(resumed.nextCursor).toContain("cursor-page-2"); + expect(parsed.nextCursor).toContain("100:2:0"); }); // ------------------------------------------------------------------------- diff --git a/packages/cli/test/lib/api-client.coverage.test.ts b/packages/cli/test/lib/api-client.coverage.test.ts index 6b2377bb3..9c53edd89 100644 --- a/packages/cli/test/lib/api-client.coverage.test.ts +++ b/packages/cli/test/lib/api-client.coverage.test.ts @@ -349,40 +349,6 @@ describe("issues.ts", () => { expect(callCount).toBe(2); }); - test("resumes inside a page that overshoots the limit", async () => { - // Return 5 items but we only want 3 - globalThis.fetch = mockFetch( - async () => - new Response( - JSON.stringify([ - mockIssue({ id: "1" }), - mockIssue({ id: "2" }), - mockIssue({ id: "3" }), - mockIssue({ id: "4" }), - mockIssue({ id: "5" }), - ]), - { - status: 200, - headers: { - "Content-Type": "application/json", - Link: linkHeader("next", true), - }, - }, - ), - ); - - const result = await listIssuesAllPages("test-org", "test-project", { - limit: 3, - }); - expect(result.issues).toHaveLength(3); - // The cursor refetches the page and skips the 3 returned issues. - const next = await listIssuesAllPages("test-org", "test-project", { - limit: 2, - startCursor: result.nextCursor, - }); - expect(next.issues.map((issue) => issue.id)).toEqual(["4", "5"]); - }); - test("throws when limit < 1", async () => { await expect( listIssuesAllPages("test-org", "test-project", { limit: 0 }), @@ -417,60 +383,6 @@ describe("issues.ts", () => { expect(onPageCalls[0]).toEqual([2, 10]); }); - test("returns nextCursor when last page has more results but limit reached", async () => { - // Return exactly limit items with a next cursor - globalThis.fetch = mockFetch( - async () => - new Response( - JSON.stringify([mockIssue({ id: "1" }), mockIssue({ id: "2" })]), - { - status: 200, - headers: { - "Content-Type": "application/json", - Link: linkHeader("has-more", true), - }, - }, - ), - ); - - const result = await listIssuesAllPages("test-org", "test-project", { - limit: 2, - }); - expect(result.issues).toHaveLength(2); - // Exactly at limit with more pages → keep nextCursor - expect(result.nextCursor).toContain("has-more"); - }); - - test("keeps one page size that divides the limit and preserves the last cursor", async () => { - const requestedLimits: string[] = []; - let callCount = 0; - - globalThis.fetch = mockFetch(async (input, init) => { - callCount += 1; - const req = new Request(input!, init); - const url = new URL(req.url); - requestedLimits.push(url.searchParams.get("limit") ?? ""); - const issues = Array.from({ length: 50 }, (_, index) => - mockIssue({ id: `${callCount}-${index}` }), - ); - return new Response(JSON.stringify(issues), { - status: 200, - headers: { - "Content-Type": "application/json", - Link: linkHeader(`cursor-${callCount + 1}`, true), - }, - }); - }); - - const result = await listIssuesAllPages("test-org", "test-project", { - limit: 250, - }); - - expect(result.issues).toHaveLength(250); - expect(requestedLimits).toEqual(["50", "50", "50", "50", "50"]); - expect(result.nextCursor).toContain("cursor-6"); - }); - test("accepts startCursor option", async () => { let capturedUrl = ""; diff --git a/packages/cli/test/lib/api/events-overshoot.test.ts b/packages/cli/test/lib/api/events-overshoot.test.ts index c30e71373..a62afb4c6 100644 --- a/packages/cli/test/lib/api/events-overshoot.test.ts +++ b/packages/cli/test/lib/api/events-overshoot.test.ts @@ -1,16 +1,9 @@ /** * Regression tests for listIssueEvents pagination. * - * Two bugs framed this behavior: - * 1. The original skip bug: trimming an overshooting page while returning the - * server's next-page cursor skipped the trimmed tail on `-c next`. - * 2. The overcorrection: without `per_page`, every page overshot the limit and - * dropping the cursor stranded all events past the first `limit`. - * - * `per_page` now matches the walk's page size, so a conforming server never - * overshoots and its cursor stays usable. If a server still returns more rows - * than asked, its cursor was computed from its own page size and would skip - * the trimmed rows, so it is dropped (the shared `paginate()` contract). + * Without `per_page`, every page overshot the limit. listIssueEvents now sends + * the walk's page size; trimming and cursor continuity are the shared + * `paginate()` contract, covered by test/lib/paginate.property.test.ts. */ import { afterEach, beforeEach, describe, expect, test } from "vitest"; @@ -32,11 +25,6 @@ afterEach(() => { globalThis.fetch = originalFetch; }); -/** Build a Sentry pagination Link header advertising a next page. */ -function nextLinkHeader(cursor: string): string { - return `; rel="next"; results="true"; cursor="${cursor}"`; -} - function makeEvents(count: number): Array<{ id: string }> { return Array.from({ length: count }, (_unused, i) => ({ id: `evt-${i}` })); } @@ -71,68 +59,4 @@ describe("listIssueEvents pagination", () => { expect(new URL(capturedUrl).searchParams.get("per_page")).toBe("100"); }); - - test("trims to limit and resumes inside a page that overshoots", async () => { - // Server ignores per_page and returns 5 with a next cursor; caller wants 2. - globalThis.fetch = mockFetch( - async () => - new Response(JSON.stringify(makeEvents(5)), { - status: 200, - headers: { - "Content-Type": "application/json", - Link: nextLinkHeader("page2"), - }, - }), - ); - - const result = await listIssueEvents("test-org", "123", { limit: 2 }); - - expect(result.data).toHaveLength(2); - expect(result.data[0]?.id).toBe("evt-0"); - expect(result.data[1]?.id).toBe("evt-1"); - // The server cursor would skip evt-2..4, so the CLI cursor refetches the - // page and skips the two returned events. - const next = await listIssueEvents("test-org", "123", { - limit: 3, - cursor: result.nextCursor, - }); - expect(next.data.map((event) => event.id)).toEqual([ - "evt-2", - "evt-3", - "evt-4", - ]); - }); - - test("preserves nextCursor when the page exactly fills the limit", async () => { - globalThis.fetch = mockFetch( - async () => - new Response(JSON.stringify(makeEvents(2)), { - status: 200, - headers: { - "Content-Type": "application/json", - Link: nextLinkHeader("page2"), - }, - }), - ); - - const result = await listIssueEvents("test-org", "123", { limit: 2 }); - - expect(result.data).toHaveLength(2); - expect(result.nextCursor).toContain("page2"); - }); - - test("returns all events with no cursor when under the limit", async () => { - globalThis.fetch = mockFetch( - async () => - new Response(JSON.stringify(makeEvents(3)), { - status: 200, - headers: { "Content-Type": "application/json" }, - }), - ); - - const result = await listIssueEvents("test-org", "123", { limit: 25 }); - - expect(result.data).toHaveLength(3); - expect(result.nextCursor).toBeUndefined(); - }); }); diff --git a/packages/cli/test/lib/api/explore.test.ts b/packages/cli/test/lib/api/explore.test.ts index 9e361c492..9514c68ca 100644 --- a/packages/cli/test/lib/api/explore.test.ts +++ b/packages/cli/test/lib/api/explore.test.ts @@ -267,13 +267,13 @@ describe("queryEvents", () => { test("auto-paginates when limit exceeds API_MAX_PER_PAGE", async () => { const { getCapturedUrls } = mockSequential([ { - body: { data: makeRows(75), meta: PAGE_META }, + body: { data: makeRows(100), meta: PAGE_META }, headers: { - Link: `; rel="next"; results="true"; cursor="0:75:0"`, + Link: `; rel="next"; results="true"; cursor="0:100:0"`, }, }, { - body: { data: makeRows(75), meta: PAGE_META }, + body: { data: makeRows(100), meta: PAGE_META }, headers: { Link: `; rel="next"; results="false"; cursor=""`, }, @@ -282,45 +282,18 @@ describe("queryEvents", () => { const result = await queryEvents("my-org", { fields: ["title", "count()"], - limit: 150, + limit: 200, }); - expect(result.data.data).toHaveLength(150); + expect(result.data.data).toHaveLength(200); expect(result.data.meta?.fields).toEqual(PAGE_META.fields); expect(result.nextCursor).toBeUndefined(); expect(getCapturedUrls()).toHaveLength(2); for (const url of getCapturedUrls()) { - expect(url).toContain("per_page=75"); + expect(url).toContain("per_page=100"); } }); - test("trims results and resumes inside the overshooting page", async () => { - mockSequential([ - { - body: { data: makeRows(60), meta: PAGE_META }, - headers: { - Link: `; rel="next"; results="true"; cursor="0:60:0"`, - }, - }, - { - // Server ignores per_page=60. - body: { data: makeRows(100), meta: PAGE_META }, - headers: { - Link: `; rel="next"; results="true"; cursor="0:160:0"`, - }, - }, - ]); - - const result = await queryEvents("my-org", { - fields: ["title", "count()"], - limit: 120, - }); - - expect(result.data.data).toHaveLength(120); - // Refetches the second page (cursor 0:60:0) and skips its returned rows. - expect(result.nextCursor).toContain("0:60:0"); - }); - test("single-page fast path when limit <= API_MAX_PER_PAGE", async () => { const { getCapturedUrls } = mockSequential([ { diff --git a/packages/cli/test/lib/api/infrastructure.test.ts b/packages/cli/test/lib/api/infrastructure.test.ts index d80175101..6f84d1a3d 100644 --- a/packages/cli/test/lib/api/infrastructure.test.ts +++ b/packages/cli/test/lib/api/infrastructure.test.ts @@ -6,7 +6,6 @@ import { fetchAllPages, isTextualContentType, MAX_PAGINATION_PAGES, - pageSizeForLimit, paginate, rawApiRequest, throwApiError, @@ -893,38 +892,22 @@ describe("paginate", () => { expect(spy).toHaveBeenCalledWith(5, undefined); }); - /** Serves rows by offset; the cursor is the next offset. */ - function offsetServer() { - return vi.fn((perPage: number, cursor: string | undefined) => { + // Cursor continuity across servers and limits is covered by + // test/lib/paginate.property.test.ts; this pins the request sequence. + test("keeps the walk's page size and resumes mid-page across -c next", async () => { + const spy = vi.fn((perPage: number, cursor: string | undefined) => { const offset = cursor ? Number(cursor) : 0; const data = Array.from({ length: perPage }, (_, i) => offset + i); return Promise.resolve({ data, nextCursor: String(offset + perPage) }); }); - } - - test("keeps the walk's page size when -c next changes the limit", async () => { - const spy = offsetServer(); const first = await paginate({ limit: 150 }, spy); expect(first.data).toEqual(Array.from({ length: 150 }, (_, i) => i)); const second = await paginate({ limit: 10, cursor: first.nextCursor }, spy); expect(second.data).toEqual(Array.from({ length: 10 }, (_, i) => 150 + i)); - expect(spy.mock.calls).toEqual([ - [75, undefined], - [75, "75"], - [75, "150"], - ]); - }); - - test("resumes inside a page it could not return in full", async () => { - const spy = offsetServer(); - - const first = await paginate({ limit: 101 }, spy); - expect(first.data).toEqual(Array.from({ length: 101 }, (_, i) => i)); - const second = await paginate({ limit: 5, cursor: first.nextCursor }, spy); - expect(second.data).toEqual([101, 102, 103, 104, 105]); - + // The first call stops inside page 2, so the second refetches it with the + // walk's page size rather than one derived from its own --limit. expect(spy.mock.calls).toEqual([ [100, undefined], [100, "100"], @@ -932,19 +915,6 @@ describe("paginate", () => { ]); }); - test("resumes after the last returned row when the server ignores per_page", async () => { - const spy = vi.fn((_perPage: number, cursor: string | undefined) => { - const offset = cursor ? Number(cursor) : 0; - const data = Array.from({ length: 100 }, (_, i) => offset + i); - return Promise.resolve({ data, nextCursor: String(offset + 100) }); - }); - - const first = await paginate({ limit: 150 }, spy); - expect(first.data).toHaveLength(150); - const second = await paginate({ limit: 3, cursor: first.nextCursor }, spy); - expect(second.data).toEqual([150, 151, 152]); - }); - test("returns no cursor once the server is exhausted", async () => { const spy = vi.fn().mockResolvedValue({ data: [1, 2] }); expect(await paginate({ limit: 5 }, spy)).toEqual({ data: [1, 2] }); @@ -962,21 +932,6 @@ describe("paginate", () => { ); }); -describe("pageSizeForLimit", () => { - test.each([ - [1, 1], - [25, 25], - [100, 100], - [150, 75], - [200, 100], - [250, 50], - [1000, 100], - [101, API_MAX_PER_PAGE], - ])("limit %i uses pages of %i", (limit, perPage) => { - expect(pageSizeForLimit(limit)).toBe(perPage); - }); -}); - describe("fetchAllPages", () => { const numbers = array(number()); diff --git a/packages/cli/test/lib/api/logs.test.ts b/packages/cli/test/lib/api/logs.test.ts index ec3bb7b85..31e9e4031 100644 --- a/packages/cli/test/lib/api/logs.test.ts +++ b/packages/cli/test/lib/api/logs.test.ts @@ -206,11 +206,11 @@ describe("listLogs", () => { const responses = [ { - body: { data: makeRows(75, 0), meta: { fields: {} } }, - link: `; rel="next"; results="true"; cursor="0:75:0"`, + body: { data: makeRows(100, 0), meta: { fields: {} } }, + link: `; rel="next"; results="true"; cursor="0:100:0"`, }, { - body: { data: makeRows(75, 75), meta: { fields: {} } }, + body: { data: makeRows(100, 100), meta: { fields: {} } }, link: `; rel="next"; results="false"; cursor=""`, }, ]; @@ -226,12 +226,12 @@ describe("listLogs", () => { }); }); - const logs = await listLogs("test-org", "my-project", { limit: 150 }); + const logs = await listLogs("test-org", "my-project", { limit: 200 }); - expect(logs).toHaveLength(150); + expect(logs).toHaveLength(200); expect(urls).toHaveLength(2); for (const url of urls) { - expect(url).toContain("per_page=75"); + expect(url).toContain("per_page=100"); } }); }); diff --git a/packages/cli/test/lib/api/project-dsns.test.ts b/packages/cli/test/lib/api/project-dsns.test.ts index 6264cab82..6ec9e8e37 100644 --- a/packages/cli/test/lib/api/project-dsns.test.ts +++ b/packages/cli/test/lib/api/project-dsns.test.ts @@ -163,7 +163,7 @@ describe.each([ const first = await list({ limit: 150, cursor: "0:25:0" }); expect(projectIds).toEqual(organization ? ["42"] : []); expect(first.data).toHaveLength(150); - expect(first.nextCursor).toContain("0:175:0"); + expect(first.nextCursor).toContain("0:125:0"); const second = await list({ limit: 50, cursor: first.nextCursor }); expect( @@ -172,10 +172,10 @@ describe.each([ return [query.get("per_page"), query.get("cursor")]; }), ).toEqual([ - ["75", "0:25:0"], - ["75", "0:100:0"], - // A smaller --limit on -c next keeps the walk's page size. - ["75", "0:175:0"], + ["100", "0:25:0"], + ["100", "0:125:0"], + // -c next refetches the partly returned page with the walk's page size. + ["100", "0:125:0"], ]); expect(second.nextCursor).toBeUndefined(); expect([...first.data, ...second.data].map((key) => key.name)).toEqual( diff --git a/packages/cli/test/lib/api/replays.test.ts b/packages/cli/test/lib/api/replays.test.ts index a92926233..aed2a3d8b 100644 --- a/packages/cli/test/lib/api/replays.test.ts +++ b/packages/cli/test/lib/api/replays.test.ts @@ -139,19 +139,19 @@ describe("listReplays", () => { const body = callIndex === 0 ? { - data: Array.from({ length: 75 }, (_, index) => + data: Array.from({ length: 100 }, (_, index) => replayRow(index.toString(16).padStart(32, "a").slice(-32)), ), } : { - data: Array.from({ length: 75 }, (_, index) => + data: Array.from({ length: 100 }, (_, index) => replayRow(index.toString(16).padStart(32, "b").slice(-32)), ), }; const headers = callIndex === 0 ? { - Link: `; rel="next"; results="true"; cursor="0:75:0"`, + Link: `; rel="next"; results="true"; cursor="0:100:0"`, } : { Link: `; rel="next"; results="false"; cursor=""`, @@ -164,13 +164,13 @@ describe("listReplays", () => { }); }); - const result = await listReplays("test-org", { limit: 150 }); + const result = await listReplays("test-org", { limit: 200 }); - expect(result.data).toHaveLength(150); + expect(result.data).toHaveLength(200); expect(result.nextCursor).toBeUndefined(); expect(capturedUrls).toHaveLength(2); - expect(capturedUrls[0]).toContain("per_page=75"); - expect(capturedUrls[1]).toContain("per_page=75"); + expect(capturedUrls[0]).toContain("per_page=100"); + expect(capturedUrls[1]).toContain("per_page=100"); }); }); diff --git a/packages/cli/test/lib/api/traces.test.ts b/packages/cli/test/lib/api/traces.test.ts index 3a29c7292..0d0b8bb11 100644 --- a/packages/cli/test/lib/api/traces.test.ts +++ b/packages/cli/test/lib/api/traces.test.ts @@ -115,28 +115,6 @@ describe("listTransactions", () => { expect(decodeURIComponent(capturedUrl)).toContain("is_transaction:true"); }); - test("keeps one page size that divides the limit", async () => { - const { getCapturedUrls } = mockSequential([ - { - body: { data: makeTxnRows(75), meta: TX_META }, - headers: { - Link: `; rel="next"; results="true"; cursor="0:75:0"`, - }, - }, - { - body: { data: makeTxnRows(75), meta: TX_META }, - headers: { - Link: `; rel="next"; results="false"; cursor=""`, - }, - }, - ]); - - await listTransactions("my-org", "my-project", { limit: 150 }); - - expect(getCapturedUrls()[0]).toContain("per_page=75"); - expect(getCapturedUrls()[1]).toContain("per_page=75"); - }); - test("sends sort=-timestamp by default", async () => { mockOk({ data: [], meta: TX_META }); @@ -222,31 +200,6 @@ describe("listTransactions", () => { expect(getCapturedUrls()).toHaveLength(2); }); - test("trims results and resumes inside the overshooting page", async () => { - mockSequential([ - { - body: { data: makeTxnRows(60), meta: TX_META }, - headers: { - Link: `; rel="next"; results="true"; cursor="0:60:0"`, - }, - }, - { - body: { data: makeTxnRows(100), meta: TX_META }, - headers: { - Link: `; rel="next"; results="true"; cursor="0:200:0"`, - }, - }, - ]); - - const result = await listTransactions("my-org", "my-project", { - limit: 120, - }); - - expect(result.data).toHaveLength(120); - // Refetches the second page (cursor 0:60:0) and skips its returned rows. - expect(result.nextCursor).toContain("0:60:0"); - }); - test("single-page fast path for limit <= 100", async () => { const { getCapturedUrls } = mockSequential([ { @@ -428,28 +381,6 @@ describe("listSpans", () => { expect(capturedUrl).toContain("dataset=spans"); }); - test("keeps one page size that divides the limit", async () => { - const { getCapturedUrls } = mockSequential([ - { - body: { data: makeSpanRows(75), meta: SPAN_META }, - headers: { - Link: `; rel="next"; results="true"; cursor="0:75:0"`, - }, - }, - { - body: { data: makeSpanRows(75), meta: SPAN_META }, - headers: { - Link: `; rel="next"; results="false"; cursor=""`, - }, - }, - ]); - - await listSpans("my-org", "my-project", { limit: 150 }); - - expect(getCapturedUrls()[0]).toContain("per_page=75"); - expect(getCapturedUrls()[1]).toContain("per_page=75"); - }); - test("sends sort=-timestamp by default", async () => { mockOk({ data: [], meta: SPAN_META }); @@ -510,29 +441,6 @@ describe("listSpans", () => { expect(getCapturedUrls()).toHaveLength(2); }); - test("trims results and resumes inside the overshooting page", async () => { - mockSequential([ - { - body: { data: makeSpanRows(60), meta: SPAN_META }, - headers: { - Link: `; rel="next"; results="true"; cursor="0:60:0"`, - }, - }, - { - body: { data: makeSpanRows(100), meta: SPAN_META }, - headers: { - Link: `; rel="next"; results="true"; cursor="0:200:0"`, - }, - }, - ]); - - const result = await listSpans("my-org", "my-project", { limit: 120 }); - - expect(result.data).toHaveLength(120); - // Refetches the second page (cursor 0:60:0) and skips its returned rows. - expect(result.nextCursor).toContain("0:60:0"); - }); - test("single-page fast path for limit <= 100", async () => { const { getCapturedUrls } = mockSequential([ { diff --git a/packages/cli/test/lib/org-list.test.ts b/packages/cli/test/lib/org-list.test.ts index b74b4f4d6..d74029db1 100644 --- a/packages/cli/test/lib/org-list.test.ts +++ b/packages/cli/test/lib/org-list.test.ts @@ -512,16 +512,17 @@ describe("handleOrgAll", () => { }); expect(result.items).toHaveLength(250); - // Every request uses the same page size (team and repo lists use - // page-number cursors), chosen to divide the limit: 250 = 5 x 50. + // Full pages keep requests to a minimum; the cursor carries the page size + // because team and repo lists use page-number cursors. expect(listPaginated.mock.calls.map((call) => call[1].perPage)).toEqual([ - 50, 50, 50, 50, 50, + 100, 100, 100, ]); // The mock always returns a nextCursor, so hasMore is true and // the cursor points to the first item beyond the fetched 250. expect(result.hasMore).toBe(true); - // `-c next` with another --limit keeps the walk's page size. + // `-c next` with another --limit keeps the walk's page size and refetches + // the third page, skipping the 50 rows already returned. listPaginated.mockClear(); const next = await handleOrgAll({ config, @@ -535,7 +536,7 @@ describe("handleOrgAll", () => { Array.from({ length: 30 }, (_, i) => String(250 + i)), ); expect(listPaginated.mock.calls.map((call) => call[1])).toEqual([ - { cursor: "250", perPage: 50 }, + { cursor: "200", perPage: 100 }, ]); }); }); From 113cef9b0e2565e6ef658cf9ccf2da387f0618cc Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Fri, 23 Oct 2026 14:54:07 +0000 Subject: [PATCH 4/4] test(cli): Expect CLI cursors in the feedback limit e2e test paginate() now wraps server cursors with the walk's page size, so the e2e test checks that the server cursor is carried instead of matching it exactly. --- packages/cli/test/e2e/feedback.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/cli/test/e2e/feedback.test.ts b/packages/cli/test/e2e/feedback.test.ts index bca4e7d81..4c08b5c73 100644 --- a/packages/cli/test/e2e/feedback.test.ts +++ b/packages/cli/test/e2e/feedback.test.ts @@ -285,7 +285,8 @@ describe("sentry feedback list", () => { "--json", ]); expect(firstPage.exitCode, firstPage.stderr + firstPage.stdout).toBe(0); - expect(JSON.parse(firstPage.stdout).nextCursor).toBe( + // paginate() wraps the server cursor with the walk's page size. + expect(JSON.parse(firstPage.stdout).nextCursor).toContain( `feedback-limit-${limit}-next`, ); }