fix: kv:key get (#360)

The api for fetching a kv value, unlike every other cloudflare api, returns just the raw value as a string (as opposed to the `FetchResult`-style json). However, our fetch utility tries to convert every api response to json before parsing it further. This leads to bugs like #359. The fix is to special case for `kv:key get`. Fixes #359.
cloudflare · Feb 2, 2022 · f590943 · f590943
1 parent 85ceb84
commit f590943
Show file tree

Hide file tree

Showing 8 changed files with 114 additions and 53 deletions.
diff --git a/.changeset/early-lies-sniff.md b/.changeset/early-lies-sniff.md
@@ -0,0 +1,9 @@
+---
+"wrangler": patch
+---
+
+fix: `kv:key get`
+
+The api for fetching a kv value, unlike every other cloudflare api, returns just the raw value as a string (as opposed to the `FetchResult`-style json). However, our fetch utility tries to convert every api response to json before parsing it further. This leads to bugs like https://github.com/cloudflare/wrangler2/issues/359. The fix is to special case for `kv:key get`.
+
+Fixes https://github.com/cloudflare/wrangler2/issues/359.
diff --git a/packages/wrangler/src/__tests__/helpers/mock-cfetch.ts b/packages/wrangler/src/__tests__/helpers/mock-cfetch.ts
@@ -140,23 +140,23 @@ export function setMockResponse<ResponseType>(
 /**
  * A helper to make it easier to create `FetchResult` objects in tests.
  */
-export function createFetchResult<ResponseType>(
-  result: ResponseType,
+export async function createFetchResult<ResponseType>(
+  result: ResponseType | Promise<ResponseType>,
   success = true,
   errors = [],
   messages = [],
   result_info?: unknown
-): FetchResult<ResponseType> {
+): Promise<FetchResult<ResponseType>> {
   return result_info
     ? {
-        result,
+        result: await result,
         success,
         errors,
         messages,
         result_info,
       }
     : {
-        result,
+        result: await result,
         success,
         errors,
         messages,
@@ -171,3 +171,37 @@ export function createFetchResult<ResponseType>(
 export function unsetAllMocks() {
   mocks.length = 0;
 }
+
+/**
+ * We special-case fetching the request for `kv:key get`, because it's
+ * the only cloudflare API endpoint that returns a plain string as the
+ * value, and not as the "standard" FetchResult-style json. Hence, we also
+ * special-case mocking it here.
+ */
+
+const kvGetMocks = new Map<string, string>();
+
+export function mockFetchKVGetValue(
+  accountId: string,
+  namespaceId: string,
+  key: string
+) {
+  const mapKey = `${accountId}/${namespaceId}/${key}`;
+  if (kvGetMocks.has(mapKey)) {
+    return kvGetMocks.get(mapKey);
+  }
+  throw new Error(`no mock value found for \`kv:key get\` - ${mapKey}`);
+}
+
+export function setMockFetchKVGetValue(
+  accountId: string,
+  namespaceId: string,
+  key: string,
+  value: string
+) {
+  kvGetMocks.set(`${accountId}/${namespaceId}/${key}`, value);
+}
+
+export function unsetMockFetchKVGetValues() {
+  kvGetMocks.clear();
+}
diff --git a/packages/wrangler/src/__tests__/jest.setup.ts b/packages/wrangler/src/__tests__/jest.setup.ts
@@ -1,7 +1,7 @@
 import fetchMock from "jest-fetch-mock";
-import { fetchInternal } from "../cfetch/internal";
+import { fetchInternal, fetchKVGetValue } from "../cfetch/internal";
 import { confirm, prompt } from "../dialogs";
-import { mockFetchInternal } from "./helpers/mock-cfetch";
+import { mockFetchInternal, mockFetchKVGetValue } from "./helpers/mock-cfetch";
 
 jest.mock("undici", () => {
   return {
@@ -16,6 +16,7 @@ fetchMock.doMock(() => {
 
 jest.mock("../cfetch/internal");
 (fetchInternal as jest.Mock).mockImplementation(mockFetchInternal);
+(fetchKVGetValue as jest.Mock).mockImplementation(mockFetchKVGetValue);
 
 jest.mock("../dialogs");
 

diff --git a/packages/wrangler/src/__tests__/kv.test.ts b/packages/wrangler/src/__tests__/kv.test.ts
@@ -2,8 +2,9 @@ import { writeFileSync } from "node:fs";
 import { mockAccountId, mockApiToken } from "./helpers/mock-account-id";
 import {
   setMockResponse,
-  setMockRawResponse,
   unsetAllMocks,
+  unsetMockFetchKVGetValues,
+  setMockFetchKVGetValue,
 } from "./helpers/mock-cfetch";
 import { mockConsoleMethods } from "./helpers/mock-console";
 import { mockKeyListRequest } from "./helpers/mock-kv";
@@ -862,64 +863,54 @@ describe("wrangler", () => {
     });
 
     describe("get", () => {
-      function mockKeyGetRequest(
-        expectedNamespaceId: string,
-        expectedKey: string,
-        expectedValue: string
-      ) {
-        const requests = { count: 0 };
-        setMockRawResponse(
-          "/accounts/:accountId/storage/kv/namespaces/:namespaceId/values/:key",
-          ([_url, accountId, namespaceId, key]) => {
-            requests.count++;
-            expect(accountId).toEqual("some-account-id");
-            expect(namespaceId).toEqual(expectedNamespaceId);
-            expect(key).toEqual(expectedKey);
-            return expectedValue;
-          }
-        );
-        return requests;
-      }
+      afterEach(() => {
+        unsetMockFetchKVGetValues();
+      });
 
       it("should get a key in a given namespace specified by namespace-id", async () => {
-        const requests = mockKeyGetRequest(
+        setMockFetchKVGetValue(
+          "some-account-id",
           "some-namespace-id",
           "my-key",
           "my-value"
         );
         await runWrangler("kv:key get my-key --namespace-id some-namespace-id");
         expect(std.out).toMatchInlineSnapshot(`"my-value"`);
         expect(std.err).toMatchInlineSnapshot(`""`);
-        expect(requests.count).toEqual(1);
       });
 
       it("should get a key in a given namespace specified by binding", async () => {
         writeWranglerConfig();
-        const requests = mockKeyGetRequest("bound-id", "my-key", "my-value");
+        setMockFetchKVGetValue(
+          "some-account-id",
+          "bound-id",
+          "my-key",
+          "my-value"
+        );
         await runWrangler(
           "kv:key get my-key --binding someBinding --preview false"
         );
         expect(std.out).toMatchInlineSnapshot(`"my-value"`);
         expect(std.err).toMatchInlineSnapshot(`""`);
-        expect(requests.count).toEqual(1);
       });
 
       it("should get a key in a given preview namespace specified by binding", async () => {
         writeWranglerConfig();
-        const requests = mockKeyGetRequest(
+        setMockFetchKVGetValue(
+          "some-account-id",
           "preview-bound-id",
           "my-key",
           "my-value"
         );
         await runWrangler("kv:key get my-key --binding someBinding --preview");
         expect(std.out).toMatchInlineSnapshot(`"my-value"`);
         expect(std.err).toMatchInlineSnapshot(`""`);
-        expect(requests.count).toEqual(1);
       });
 
       it("should get a key for the specified environment in a given namespace", async () => {
         writeWranglerConfig();
-        const requests = mockKeyGetRequest(
+        setMockFetchKVGetValue(
+          "some-account-id",
           "env-bound-id",
           "my-key",
           "my-value"
@@ -929,7 +920,6 @@ describe("wrangler", () => {
         );
         expect(std.out).toMatchInlineSnapshot(`"my-value"`);
         expect(std.err).toMatchInlineSnapshot(`""`);
-        expect(requests.count).toEqual(1);
       });
 
       it("should error if no key is provided", async () => {

diff --git a/packages/wrangler/src/cfetch/index.ts b/packages/wrangler/src/cfetch/index.ts
@@ -18,16 +18,7 @@ export interface FetchResult<ResponseType = unknown> {
   result_info?: unknown;
 }
 
-/**
- * Make a fetch request for a raw JSON value.
- */
-export async function fetchRaw<ResponseType>(
-  resource: string,
-  init: RequestInit = {},
-  queryParams?: URLSearchParams
-): Promise<ResponseType> {
-  return fetchInternal<ResponseType>(resource, init, queryParams);
-}
+export { fetchKVGetValue } from "./internal";
 
 /**
  * Make a fetch request, and extract the `result` from the JSON response.

diff --git a/packages/wrangler/src/cfetch/internal.ts b/packages/wrangler/src/cfetch/internal.ts
@@ -31,7 +31,6 @@ export async function fetchInternal<ResponseType>(
     ...init,
     headers,
   });
-
   const jsonText = await response.text();
   try {
     const json = JSON.parse(jsonText);
@@ -70,3 +69,33 @@ function addAuthorizationHeader(headers: HeadersInit, apiToken: string): void {
   }
   headers["Authorization"] = `Bearer ${apiToken}`;
 }
+
+/**
+ * The implementation for fetching a kv value from the cloudflare API.
+ * We special-case this one call, because it's the only API call that
+ * doesn't return json. We inline the implementation and try not to share
+ * any code with the other calls. We should push back on any new APIs that
+ * try to introduce non-"standard" response structures.
+ */
+
+export async function fetchKVGetValue(
+  accountId: string,
+  namespaceId: string,
+  key: string
+): Promise<string> {
+  await requireLoggedIn();
+  const apiToken = requireApiToken();
+  const headers = { Authorization: `Bearer ${apiToken}` };
+  const resource = `${CF_API_BASE_URL}/accounts/${accountId}/storage/kv/namespaces/${namespaceId}/values/${key}`;
+  const response = await fetch(resource, {
+    method: "GET",
+    headers,
+  });
+  if (response.ok) {
+    return await response.text();
+  } else {
+    throw new Error(
+      `Failed to fetch ${resource} - ${response.status}: ${response.statusText});`
+    );
+  }
+}
diff --git a/packages/wrangler/src/index.tsx b/packages/wrangler/src/index.tsx
@@ -10,7 +10,7 @@ import onExit from "signal-exit";
 import makeCLI from "yargs";
 import { version as wranglerVersion } from "../package.json";
 import { toFormData } from "./api/form_data";
-import { fetchResult, fetchRaw } from "./cfetch";
+import { fetchResult } from "./cfetch";
 import { normaliseAndValidateEnvironmentsConfig } from "./config";
 import Dev from "./dev";
 import { confirm, prompt } from "./dialogs";
@@ -23,6 +23,7 @@ import {
   deleteBulkKeyValue,
   createNamespace,
   isValidNamespaceBinding,
+  getKeyValue,
 } from "./kv";
 import { npm } from "./npm-installer";
 import { pages } from "./pages";
@@ -1755,13 +1756,11 @@ export async function main(argv: string[]): Promise<void> {
                 }
               }
               // -- snip, end --
-            }
 
-            console.log(
-              await fetchRaw(
-                `/accounts/${config.account_id}/storage/kv/namespaces/${namespaceId}/values/${key}`
-              )
-            );
+              console.log(
+                await getKeyValue(config.account_id, namespaceId, key)
+              );
+            }
           }
         )
         .command(

diff --git a/packages/wrangler/src/kv.tsx b/packages/wrangler/src/kv.tsx
@@ -1,5 +1,5 @@
 import { URLSearchParams } from "node:url";
-import { fetchListResult, fetchResult } from "./cfetch";
+import { fetchListResult, fetchResult, fetchKVGetValue } from "./cfetch";
 import type { Config } from "./config";
 
 type KvArgs = {
@@ -115,6 +115,14 @@ export async function putKeyValue(
   );
 }
 
+export async function getKeyValue(
+  accountId: string,
+  namespaceId: string,
+  key: string
+): Promise<string> {
+  return await fetchKVGetValue(accountId, namespaceId, key);
+}
+
 export async function putBulkKeyValue(
   accountId: string,
   namespaceId: string,