Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(fetch): use pages for pagination instead of cursor #4402

Merged
merged 4 commits into from
Nov 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .changeset/orange-lamps-happen.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
"wrangler": patch
---

This PR adds a fetch handler that uses `page`, assuming `result_info` provided by the endpoint contains `page`, `per_page`, and `total`

This is needed as the existing `fetchListResult` handler for fetching potentially paginated results doesn't work for endpoints that don't implement `cursor`.

Fixes #4349
38 changes: 38 additions & 0 deletions packages/wrangler/src/__tests__/cfetch-utils.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { hasMorePages } from "../cfetch";

/**
hasMorePages is a function that returns a boolean based on the result_info object returned from the cloudflare v4 API - if the current page is less than the total number of pages, it returns true, otherwise false.
*/

describe("hasMorePages", () => {
it("should handle result_info not having enough results to paginate", () => {
expect(
hasMorePages({
page: 1,
per_page: 10,
count: 5,
total_count: 5,
})
).toBe(false);
});
it("should return true if the current page is less than the total number of pages", () => {
expect(
hasMorePages({
page: 1,
per_page: 10,
count: 10,
total_count: 100,
})
).toBe(true);
});
it("should return false if we are on the last page of results", () => {
expect(
hasMorePages({
page: 10,
per_page: 10,
count: 10,
total_count: 100,
})
).toBe(false);
});
});
60 changes: 60 additions & 0 deletions packages/wrangler/src/cfetch/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,66 @@
return results;
}

/**
* Make a fetch request for a list of values using pages,
* extracting the `result` from the JSON response,
* and repeating the request if the results are paginated.
*
* This is similar to fetchListResult, but it uses the `page` query parameter instead of `cursor`.
*/
export async function fetchPagedListResult<ResponseType>(
resource: string,
init: RequestInit = {},
queryParams?: URLSearchParams
): Promise<ResponseType[]> {
const results: ResponseType[] = [];
let getMoreResults = true;
let page = 1;
while (getMoreResults) {
queryParams = new URLSearchParams(queryParams);
queryParams.set("page", String(page));

const json = await fetchInternal<FetchResult<ResponseType[]>>(
resource,
init,
queryParams
);
if (json.success) {
results.push(...json.result);
if (hasMorePages(json.result_info)) {
page = page + 1;

Check warning on line 130 in packages/wrangler/src/cfetch/index.ts

View check run for this annotation

Codecov / codecov/patch

packages/wrangler/src/cfetch/index.ts#L130

Added line #L130 was not covered by tests
} else {
getMoreResults = false;
}
} else {
throwFetchError(resource, json);
}
}
return results;
}

interface PageResultInfo {
page: number;
per_page: number;
count: number;
total_count: number;
}

export function hasMorePages(
result_info: unknown
): result_info is PageResultInfo {
const page = (result_info as PageResultInfo | undefined)?.page;
const per_page = (result_info as PageResultInfo | undefined)?.per_page;
const total = (result_info as PageResultInfo | undefined)?.total_count;

return (
page !== undefined &&
per_page !== undefined &&
total !== undefined &&
page * per_page < total
);
}

function throwFetchError(
resource: string,
response: FetchResult<unknown>
Expand Down
4 changes: 2 additions & 2 deletions packages/wrangler/src/user/choose-account.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { fetchListResult } from "../cfetch";
import { fetchPagedListResult } from "../cfetch";
import { getCloudflareAccountIdFromEnv } from "./auth-variables";

export type ChooseAccountItem = {
Expand All @@ -15,7 +15,7 @@ export async function getAccountChoices(): Promise<ChooseAccountItem[]> {
return [{ id: accountIdFromEnv, name: "" }];
} else {
try {
const response = await fetchListResult<{
const response = await fetchPagedListResult<{
account: ChooseAccountItem;
}>(`/memberships`);
const accounts = response.map((r) => r.account);
Expand Down
4 changes: 2 additions & 2 deletions packages/wrangler/src/whoami.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import chalk from "chalk";
import { fetchListResult, fetchResult } from "./cfetch";
import { fetchPagedListResult, fetchResult } from "./cfetch";
import { logger } from "./logger";
import { getAPIToken, getAuthFromEnv, getScopes } from "./user";

Expand Down Expand Up @@ -91,7 +91,7 @@ async function getEmail(): Promise<string | undefined> {
type AccountInfo = { name: string; id: string };

async function getAccounts(): Promise<AccountInfo[]> {
return await fetchListResult<AccountInfo>("/accounts");
return await fetchPagedListResult<AccountInfo>("/accounts");
}

async function getTokenPermissions(): Promise<string[] | undefined> {
Expand Down