fix: kv:key binary puts/gets

.changeset/curly-lizards-rescue.md

@@ -0,0 +1,9 @@
+---
+"wrangler": patch
+---
+
+fix: kv:key put/get binary file
+
+As raised in https://github.com/cloudflare/wrangler2/issues/1254, it was discovered that binary uploads were being mangled by wrangler 2, whereas they worked in wrangler 1. This is because they were read into a string by providing an explicit encoding of `utf-8`. This fix reads provided files into a node `Buffer` that is then passed directly to the request.
+
+Subsequently https://github.com/cloudflare/wrangler2/issues/1273 was raised in relation to a similar issue with gets from wrangler 2. This was happening due to the downloaded file being converted to `utf-8` encoding as it was pushed through `console.log`. By leveraging `process.stdout.write` we can push the fetched `ArrayBuffer` to std out directly without inferring any specific encoding value.

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

@@ -179,7 +179,7 @@ export function unsetAllMocks() {
  * special-case mocking it here.
  */
 
-const kvGetMocks = new Map<string, string>();
+const kvGetMocks = new Map<string, string | Buffer>();
 
 /**
  * @mocked typeof fetchKVGetValue
@@ -201,7 +201,7 @@ export function setMockFetchKVGetValue(
   accountId: string,
   namespaceId: string,
   key: string,
-  value: string
+  value: string | Buffer
 ) {
   kvGetMocks.set(`${accountId}/${namespaceId}/${key}`, value);
 }

packages/wrangler/src/__tests__/helpers/mock-process.ts

@@ -0,0 +1,24 @@
+/**
+ * We use this module to mock process methods (write only for now),
+ * and optionally assert on the values they're called with in our tests.
+ */
+
+let writeSpy: jest.SpyInstance;
+
+function captureWriteCall(spy: jest.SpyInstance): Buffer {
+  return spy.mock.calls[0]?.[0] ?? Buffer.alloc(0);
+}
+
+export function mockProcess() {
+  beforeEach(() => {
+    writeSpy = jest.spyOn(process.stdout, "write").mockImplementation();
+  });
+  afterEach(() => {
+    writeSpy.mockRestore();
+  });
+  return {
+    get write() {
+      return captureWriteCall(writeSpy);
+    },
+  };
+}

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

@@ -10,6 +10,7 @@ import {
 import { mockConsoleMethods } from "./helpers/mock-console";
 import { clearConfirmMocks, mockConfirm } from "./helpers/mock-dialogs";
 import { mockKeyListRequest } from "./helpers/mock-kv";
+import { mockProcess } from "./helpers/mock-process";
 import { runInTempDir } from "./helpers/run-in-tmp";
 import { runWrangler } from "./helpers/run-wrangler";
 import type { KeyValue, KVNamespaceInfo, NamespaceKeyInfo } from "../kv";
@@ -19,6 +20,7 @@ describe("wrangler", () => {
   mockApiToken();
   runInTempDir();
   const std = mockConsoleMethods();
+  const proc = mockProcess();
 
   afterEach(() => {
     unsetAllMocks();
@@ -448,10 +450,11 @@ describe("wrangler", () => {
       });
 
       it("should put a key with a value loaded from a given path", async () => {
-        writeFileSync("foo.txt", "file-contents", "utf-8");
+        const buf = Buffer.from("file-contents", "utf-8");
+        writeFileSync("foo.txt", buf);
         const requests = mockKeyPutRequest("some-namespace-id", {
           key: "my-key",
-          value: "file-contents",
+          value: buf,
         });
         await runWrangler(
           "kv:key put my-key --namespace-id some-namespace-id --path foo.txt"
@@ -463,6 +466,26 @@ describe("wrangler", () => {
         expect(requests.count).toEqual(1);
       });
 
+      it("should put a key with a binary value loaded from a given path", async () => {
+        const buf = Buffer.from(
+          "iVBORw0KGgoAAAANSUhEUgAAAAoAAAAKCAYAAACNMs+9AAAACXBIWXMAAAsTAAALEwEAmpwYAAAAAXNSR0IArs4c6QAAAARnQU1BAACxjwv8YQUAAAAiSURBVHgB7coxEQAACMPAgH/PgAM6dGwu49fA/deIBXrgAj2cAhIFT4QxAAAAAElFTkSuQmCC",
+          "base64"
+        );
+        writeFileSync("test.png", buf);
+        const requests = mockKeyPutRequest("another-namespace-id", {
+          key: "my-key",
+          value: buf,
+        });
+        await runWrangler(
+          "kv:key put my-key --namespace-id another-namespace-id --path test.png"
+        );
+        expect(std.out).toMatchInlineSnapshot(
+          `"Writing the contents of test.png to the key \\"my-key\\" on namespace another-namespace-id."`
+        );
+        expect(std.err).toMatchInlineSnapshot(`""`);
+        expect(requests.count).toEqual(1);
+      });
+
       it("should error if no key is provided", async () => {
         await expect(
           runWrangler("kv:key put")
@@ -874,7 +897,23 @@ describe("wrangler", () => {
           "my-value"
         );
         await runWrangler("kv:key get my-key --namespace-id some-namespace-id");
-        expect(std.out).toMatchInlineSnapshot(`"my-value"`);
+        expect(proc.write).toEqual(Buffer.from("my-value"));
+        expect(std.err).toMatchInlineSnapshot(`""`);
+      });
+
+      it("should get a binary file by key in a given namespace specified by namespace-id", async () => {
+        const buf = Buffer.from(
+          "iVBORw0KGgoAAAANSUhEUgAAAAoAAAAKCAYAAACNMs+9AAAACXBIWXMAAAsTAAALEwEAmpwYAAAAAXNSR0IArs4c6QAAAARnQU1BAACxjwv8YQUAAAAiSURBVHgB7coxEQAACMPAgH/PgAM6dGwu49fA/deIBXrgAj2cAhIFT4QxAAAAAElFTkSuQmCC",
+          "base64"
+        );
+        setMockFetchKVGetValue(
+          "some-account-id",
+          "some-namespace-id",
+          "my-key",
+          buf
+        );
+        await runWrangler("kv:key get my-key --namespace-id some-namespace-id");
+        expect(proc.write).toEqual(buf);
         expect(std.err).toMatchInlineSnapshot(`""`);
       });
 
@@ -889,7 +928,7 @@ describe("wrangler", () => {
         await runWrangler(
           "kv:key get my-key --binding someBinding --preview false"
         );
-        expect(std.out).toMatchInlineSnapshot(`"my-value"`);
+        expect(proc.write).toEqual(Buffer.from("my-value"));
         expect(std.err).toMatchInlineSnapshot(`""`);
       });
 
@@ -902,7 +941,7 @@ describe("wrangler", () => {
           "my-value"
         );
         await runWrangler("kv:key get my-key --binding someBinding --preview");
-        expect(std.out).toMatchInlineSnapshot(`"my-value"`);
+        expect(proc.write).toEqual(Buffer.from("my-value"));
         expect(std.err).toMatchInlineSnapshot(`""`);
       });
 
@@ -917,7 +956,7 @@ describe("wrangler", () => {
         await runWrangler(
           "kv:key get my-key --binding someBinding --env some-environment --preview false"
         );
-        expect(std.out).toMatchInlineSnapshot(`"my-value"`);
+        expect(proc.write).toEqual(Buffer.from("my-value"));
         expect(std.err).toMatchInlineSnapshot(`""`);
       });
 
@@ -931,7 +970,7 @@ describe("wrangler", () => {
         await runWrangler(
           "kv:key get /my,key --namespace-id some-namespace-id"
         );
-        expect(std.out).toMatchInlineSnapshot(`"my-value"`);
+        expect(proc.write).toEqual(Buffer.from("my-value"));
         expect(std.err).toMatchInlineSnapshot(`""`);
       });
 

packages/wrangler/src/cfetch/internal.ts

@@ -123,7 +123,7 @@ export async function fetchKVGetValue(
   accountId: string,
   namespaceId: string,
   key: string
-): Promise<string> {
+): Promise<ArrayBuffer> {
   await requireLoggedIn();
   const apiToken = requireApiToken();
   const headers = { Authorization: `Bearer ${apiToken}` };
@@ -133,7 +133,7 @@ export async function fetchKVGetValue(
     headers,
   });
   if (response.ok) {
-    return await response.text();
+    return await response.arrayBuffer();
   } else {
     throw new Error(
       `Failed to fetch ${resource} - ${response.status}: ${response.statusText});`

packages/wrangler/src/index.tsx

@@ -48,6 +48,7 @@ import {
   parsePackageJSON,
   parseTOML,
   readFileSync,
+  readFileSyncToBuffer,
 } from "./parse";
 import publish from "./publish";
 import { createR2Bucket, deleteR2Bucket, listR2Buckets } from "./r2";
@@ -2272,7 +2273,7 @@ function createCLIParser(argv: string[]) {
             const namespaceId = getKVNamespaceId(args, config);
             // One of `args.path` and `args.value` must be defined
             const value = args.path
-              ? readFileSync(args.path)
+              ? readFileSyncToBuffer(args.path)
               : // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
                 args.value!;
 
@@ -2389,7 +2390,9 @@ function createCLIParser(argv: string[]) {
 
             const accountId = await requireAuth(config);
 
-            logger.log(await getKVKeyValue(accountId, namespaceId, key));
+            process.stdout.write(
+              Buffer.from(await getKVKeyValue(accountId, namespaceId, key))
+            );
           }
         )
         .command(

packages/wrangler/src/kv.ts

@@ -111,7 +111,7 @@ export async function deleteKVNamespace(
  */
 export interface KeyValue {
   key: string;
-  value: string;
+  value: string | Buffer;
   expiration?: number;
   expiration_ttl?: number;
   metadata?: object;
@@ -214,7 +214,7 @@ export async function getKVKeyValue(
   accountId: string,
   namespaceId: string,
   key: string
-): Promise<string> {
+): Promise<ArrayBuffer> {
   return await fetchKVGetValue(accountId, namespaceId, encodeURIComponent(key));
 }
 

packages/wrangler/src/parse.ts

@@ -139,6 +139,25 @@ export function parseJSON<T>(input: string, file?: string): T {
   }
 }
 
+/**
+ * Reads a file into a node Buffer.
+ */
+export function readFileSyncToBuffer(file: string): Buffer {
+  try {
+    return fs.readFileSync(file);
+  } catch (err) {
+    const { message } = err as Error;
+    throw new ParseError({
+      text: `Could not read file: ${file}`,
+      notes: [
+        {
+          text: message.replace(file, resolve(file)),
+        },
+      ],
+    });
+  }
+}
+
 /**
  * Reads a file and parses it based on its type.
  */