fix: improve the fetch handling (and testing) of requests to the Clou…

…dflare API

.changeset/modern-books-leave.md

@@ -0,0 +1,17 @@
+---
+"wrangler": patch
+---
+
+Refactor raw value extraction from Cloudflare APIs
+
+Most API responses are JSON of the form:
+
+```
+{ result, success, errors, messages, result_info }
+```
+
+where the `result` contains the actual response value.
+
+But some API responses only contain the result value.
+
+This change refactors the client-side fetch API to allow callers to specify what kind of response they expect.

.changeset/thick-beans-thank.md

@@ -0,0 +1,9 @@
+---
+"wrangler": patch
+---
+
+Fix pagination handling of list requests to the Cloudflare API
+
+When doing a list request to the API, the server may respond with only a single page of results.
+In this case, it will also provide a `cursor` value in the `result_info` part of the response, which can be used to request the next page.
+This change implements this on the client-side so that we get all the results by requesting further pages when there is a cursor.

packages/wrangler/src/__tests__/jest.setup.ts

@@ -1,6 +1,10 @@
+import { mockFetchInternal } from "./mock-cfetch";
 import { confirm, prompt } from "../dialogs";
+import { fetchInternal } from "../cfetch/internal";
+
+jest.mock("../cfetch/internal");
+(fetchInternal as jest.Mock).mockImplementation(mockFetchInternal);
 
-jest.mock("../cfetch", () => jest.requireActual("./mock-cfetch"));
 jest.mock("../dialogs");
 
 // By default (if not configured by mockConfirm()) calls to `confirm()` should throw.

packages/wrangler/src/__tests__/kv.test.ts

@@ -1,7 +1,11 @@
-import { setMock, unsetAllMocks } from "./mock-cfetch";
+import { writeFileSync } from "fs";
+import {
+  setMockResponse,
+  setMockRawResponse,
+  unsetAllMocks,
+} from "./mock-cfetch";
 import { runWrangler } from "./run-wrangler";
 import { runInTempDir } from "./run-in-tmp";
-import { writeFileSync } from "fs";
 
 describe("wrangler", () => {
   runInTempDir();
@@ -13,14 +17,19 @@ describe("wrangler", () => {
   describe("kv:namespace", () => {
     describe("create", () => {
       function mockCreateRequest(expectedTitle: string) {
-        setMock(
+        setMockResponse(
           "/accounts/:accountId/storage/kv/namespaces",
           "POST",
           ([_url, accountId], { body }) => {
             expect(accountId).toEqual("some-account-id");
-            const title = JSON.parse(body).title;
+            const title = JSON.parse(body as string).title;
             expect(title).toEqual(expectedTitle);
-            return { id: "some-namespace-id" };
+            return {
+              success: true,
+              result: { id: "some-namespace-id" },
+              errors: [],
+              messages: [],
+            };
           }
         );
       }
@@ -172,19 +181,24 @@ describe("wrangler", () => {
     describe("list", () => {
       function mockListRequest(namespaces: unknown[]) {
         const requests = { count: 0 };
-        setMock(
-          "/accounts/:accountId/storage/kv/namespaces\\?:qs",
-          ([_url, accountId, query], init) => {
+        setMockResponse(
+          "/accounts/:accountId/storage/kv/namespaces",
+          ([_url, accountId], init, query) => {
             requests.count++;
             expect(accountId).toEqual("some-account-id");
-            expect(query).toContain("per_page=100");
-            expect(query).toContain("order=title");
-            expect(query).toContain("direction=asc");
-            expect(query).toContain("page=");
-            expect(init).toBe(undefined);
-            const pageSize = Number(/\bper_page=(\d+)\b/.exec(query)[1]);
-            const page = Number(/\bpage=(\d+)/.exec(query)[1]);
-            return namespaces.slice((page - 1) * pageSize, page * pageSize);
+            expect(query.get("per_page")).toEqual("100");
+            expect(query.get("order")).toEqual("title");
+            expect(query.get("direction")).toEqual("asc");
+            expect(query.get("page")).toEqual(`${requests.count}`);
+            expect(init).toEqual({});
+            const pageSize = Number(query.get("per_page"));
+            const page = Number(query.get("page"));
+            return {
+              success: true,
+              result: namespaces.slice((page - 1) * pageSize, page * pageSize),
+              errors: [],
+              messages: [],
+            };
           }
         );
         return requests;
@@ -196,7 +210,11 @@ describe("wrangler", () => {
           { title: "title-2", id: "id-2" },
         ];
         mockListRequest(KVNamespaces);
-        const { stdout } = await runWrangler("kv:namespace list");
+        const { error, stdout, stderr } = await runWrangler(
+          "kv:namespace list"
+        );
+        expect(error).toMatchInlineSnapshot(`undefined`);
+        expect(stderr).toMatchInlineSnapshot(`""`);
         const namespaces = JSON.parse(stdout);
         expect(namespaces).toEqual(KVNamespaces);
       });
@@ -218,7 +236,7 @@ describe("wrangler", () => {
     describe("delete", () => {
       function mockDeleteRequest(expectedNamespaceId: string) {
         const requests = { count: 0 };
-        setMock(
+        setMockResponse(
           "/accounts/:accountId/storage/kv/namespaces/:namespaceId",
           "DELETE",
           ([_url, accountId, namespaceId]) => {
@@ -312,18 +330,23 @@ describe("wrangler", () => {
         expirationTtl?: number
       ) {
         const requests = { count: 0 };
-        setMock(
-          "/accounts/:accountId/storage/kv/namespaces/:namespaceId/values/:key\\?:query",
+        setMockResponse(
+          "/accounts/:accountId/storage/kv/namespaces/:namespaceId/values/:key",
           "PUT",
-          ([_url, accountId, namespaceId, key, query], { body }) => {
+          ([_url, accountId, namespaceId, key], { body }, query) => {
             requests.count++;
             expect(accountId).toEqual("some-account-id");
             expect(namespaceId).toEqual(expectedNamespaceId);
             expect(key).toEqual(expectedKey);
             expect(body).toEqual(expectedValue);
-            expect(query).toContain(`expiration=${expiration ?? ""}`);
-            expect(query).toContain(`expiration_ttl=${expirationTtl ?? ""}`);
-            return null;
+            expect(query.get("expiration")).toEqual(`${expiration}`);
+            expect(query.get("expiration_ttl")).toEqual(`${expirationTtl}`);
+            return {
+              success: true,
+              result: null,
+              errors: [],
+              messages: [],
+            };
           }
         );
         return requests;
@@ -660,7 +683,7 @@ describe("wrangler", () => {
         expectedValue: string
       ) {
         const requests = { count: 0 };
-        setMock(
+        setMockRawResponse(
           "/accounts/:accountId/storage/kv/namespaces/:namespaceId/values/:key",
           ([_url, accountId, namespaceId, key]) => {
             requests.count++;

packages/wrangler/src/__tests__/mock-cfetch.ts

@@ -1,47 +1,84 @@
-// This file mocks ../cfetch.ts
-// so we can insert whatever responses we want from it
+import type { RequestInit } from "node-fetch";
+import type { URLSearchParams } from "node:url";
 import { pathToRegexp } from "path-to-regexp";
+import { CF_API_BASE_URL } from "../cfetch";
+import type { FetchResult } from "../cfetch";
+
+/**
+ * The signature of the function that will handle a mock request.
+ */
+export type MockHandler<ResponseType> = (
+  uri: RegExpExecArray,
+  init?: RequestInit,
+  queryParams?: URLSearchParams
+) => ResponseType;
 
-// Sadly we cannot give use the correct `RequestInit` type from node-fetch.
-// Jest needs to transform the code as part of the module mocking, and it doesn't know how to cope with such types.
-type RequestInit = { method: string; body: string };
-type MockHandler = (uri: RegExpExecArray, init?: RequestInit) => unknown;
-type MockFetch = {
-  regexp: RegExp;
-  method: string | undefined;
-  handler: MockHandler;
-};
 type RemoveMockFn = () => void;
 
-let mocks: MockFetch[] = [];
+interface MockFetch<ResponseType> {
+  regexp: RegExp;
+  method: string | undefined;
+  handler: MockHandler<ResponseType>;
+}
+const mocks: MockFetch<unknown>[] = [];
 
-export default function mockCfetch(
+/**
+ * The mock implementation of `cfApi.fetch()`.
+ *
+ * This function will attempt to match the given request to one of the mock handlers configured by calls to `setMock()`.
+ *
+ * Once found the handler will be used to generate a mock response.
+ */
+export async function mockFetchInternal(
   resource: string,
-  init: RequestInit
-): unknown {
+  init: RequestInit = {},
+  queryParams?: URLSearchParams
+) {
   for (const { regexp, method, handler } of mocks) {
-    const uri = regexp.exec(resource);
+    const resourcePath = new URL(resource, CF_API_BASE_URL).pathname;
+    const uri = regexp.exec(resourcePath);
     // Do the resource path and (if specified) the HTTP method match?
     if (uri !== null && (!method || method === init.method)) {
       // The `resource` regular expression will extract the labelled groups from the URL.
       // These are passed through to the `handler` call, to allow it to do additional checks or behaviour.
-      return handler(uri, init); // TODO: should we have some kind of fallthrough system? we'll see.
+      return handler(uri, init, queryParams); // TODO: should we have some kind of fallthrough system? we'll see.
     }
   }
   throw new Error(`no mocks found for ${init.method}: ${resource}`);
 }
 
 /**
- * Specify an expected resource path that is to be handled.
+ * Specify an expected resource path that is to be handled, resulting in a raw JSON response.
+ *
+ * @param resource The path of the resource to be matched.
+ * This can include wildcards whose value will be passed to the `handler`.
+ * @param handler The function that will generate the mock response for this request.
  */
-export function setMock(
+export function setMockRawResponse<ResponseType>(
+  resource: string,
+  handler: MockHandler<ResponseType>
+): RemoveMockFn;
+/**
+ * Specify an expected resource path that is to be handled, resulting in a raw JSON response.
+ *
+ * @param resource The path of the resource to be matched.
+ * This can include wildcards whose value will be passed to the `handler`.
+ * @param method The HTTP method (e.g. GET, POST, etc) that the request must have to match this mock handler.
+ * @param handler The function that will generate the mock response for this request.
+ */
+export function setMockRawResponse<ResponseType>(
   resource: string,
   method: string,
-  handler: MockHandler
+  handler: MockHandler<ResponseType>
 ): RemoveMockFn;
-export function setMock(resource: string, handler: MockHandler): RemoveMockFn;
-export function setMock(resource: string, ...args: unknown[]): RemoveMockFn {
-  const handler = args.pop() as MockHandler;
+/**
+ * Specify an expected resource path that is to be handled, resulting in a raw JSON response.
+ */
+export function setMockRawResponse<ResponseType>(
+  resource: string,
+  ...args: [string, MockHandler<ResponseType>] | [MockHandler<ResponseType>]
+): RemoveMockFn {
+  const handler = args.pop() as MockHandler<ResponseType>;
   const method = args.pop() as string;
   const mock = {
     resource,
@@ -51,13 +88,82 @@ export function setMock(resource: string, ...args: unknown[]): RemoveMockFn {
   };
   mocks.push(mock);
   return () => {
-    mocks = mocks.filter((x) => x !== mock);
+    const mockIndex = mocks.indexOf(mock);
+    if (mockIndex !== -1) {
+      mocks.splice(mockIndex, 1);
+    }
   };
 }
 
-export function unsetAllMocks() {
-  mocks = [];
+/**
+ * Specify an expected resource path that is to be handled, resulting in a FetchRequest.
+ *
+ * @param resource The path of the resource to be matched.
+ * This can include wildcards whose value will be passed to the `handler`.
+ * @param handler The function that will generate the mock response for this request.
+ */
+export function setMockResponse<ResponseType>(
+  resource: string,
+  handler: MockHandler<FetchResult<ResponseType>>
+): RemoveMockFn;
+/**
+ * Specify an expected resource path that is to be handled, resulting in a FetchRequest..
+ *
+ * @param resource The path of the resource to be matched.
+ * This can include wildcards whose value will be passed to the `handler`.
+ * @param method The HTTP method (e.g. GET, POST, etc) that the request must have to match this mock handler.
+ * @param handler The function that will generate the mock response for this request.
+ */
+export function setMockResponse<ResponseType>(
+  resource: string,
+  method: string,
+  handler: MockHandler<FetchResult<ResponseType>>
+): RemoveMockFn;
+/**
+ * Specify an expected resource path that is to be handled, resulting in a FetchRequest.
+ */
+export function setMockResponse<ResponseType>(
+  resource: string,
+  ...args:
+    | [string, MockHandler<FetchResult<ResponseType>>]
+    | [MockHandler<FetchResult<ResponseType>>]
+): RemoveMockFn {
+  const handler = args.pop() as MockHandler<FetchResult<ResponseType>>;
+  const method = args.pop() as string;
+  return setMockRawResponse(resource, method, handler);
+}
+
+/**
+ * A helper to make it easier to create `FetchResult` objects in tests.
+ */
+export function createFetchResult<ResponseType>(
+  result: ResponseType,
+  success = true,
+  errors = [],
+  messages = [],
+  result_info?: unknown
+): FetchResult<ResponseType> {
+  return result_info
+    ? {
+        result,
+        success,
+        errors,
+        messages,
+        result_info,
+      }
+    : {
+        result,
+        success,
+        errors,
+        messages,
+      };
 }
 
-export const CF_API_BASE_URL =
-  process.env.CF_API_BASE_URL || "https://api.cloudflare.com/client/v4";
+/**
+ * Remove all the configured mock handlers.
+ *
+ * This should be called in an `afterEach()` block to ensure that mock handlers do not leak between tests.
+ */
+export function unsetAllMocks() {
+  mocks.length = 0;
+}

packages/wrangler/src/api/preview.ts

@@ -1,7 +1,7 @@
-import cfetch from "../cfetch";
+import fetch from "node-fetch";
+import { fetchResult } from "../cfetch";
 import { toFormData } from "./form_data";
 import type { CfAccount, CfWorkerInit } from "./worker";
-import fetch from "node-fetch";
 
 /**
  * A preview mode.
@@ -60,7 +60,7 @@ async function sessionToken(account: CfAccount): Promise<CfPreviewToken> {
     ? `/zones/${zoneId}/workers/edge-preview`
     : `/accounts/${accountId}/workers/subdomain/edge-preview`;
 
-  const { exchange_url } = await cfetch<{ exchange_url: string }>(initUrl);
+  const { exchange_url } = await fetchResult<{ exchange_url: string }>(initUrl);
   const { inspector_websocket, token } = (await (
     await fetch(exchange_url)
   ).json()) as { inspector_websocket: string; token: string };
@@ -106,7 +106,7 @@ export async function previewToken(
   const formData = toFormData(worker);
   formData.set("wrangler-session-config", JSON.stringify(mode));
 
-  const { preview_token } = await cfetch<{ preview_token: string }>(url, {
+  const { preview_token } = await fetchResult<{ preview_token: string }>(url, {
     method: "POST",
     // @ts-expect-error TODO: fix this
     body: formData,