diff --git a/.changeset/early-lies-sniff.md b/.changeset/early-lies-sniff.md new file mode 100644 index 000000000000..c225bfddaa89 --- /dev/null +++ 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 index 0d60a827ab65..8c0700e721ee 100644 --- 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( /** * A helper to make it easier to create `FetchResult` objects in tests. */ -export function createFetchResult( - result: ResponseType, +export async function createFetchResult( + result: ResponseType | Promise, success = true, errors = [], messages = [], result_info?: unknown -): FetchResult { +): Promise> { 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( 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(); + +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 index ba3ed8eb51ca..62249ecb5268 100644 --- 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 index 9478853ebd3c..69e59a9b9772 100644 --- 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,27 +863,13 @@ 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" @@ -890,23 +877,27 @@ describe("wrangler", () => { 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" @@ -914,12 +905,12 @@ describe("wrangler", () => { 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 index a203d5dfb8a1..f4f0f4b6d33b 100644 --- a/packages/wrangler/src/cfetch/index.ts +++ b/packages/wrangler/src/cfetch/index.ts @@ -18,16 +18,7 @@ export interface FetchResult { result_info?: unknown; } -/** - * Make a fetch request for a raw JSON value. - */ -export async function fetchRaw( - resource: string, - init: RequestInit = {}, - queryParams?: URLSearchParams -): Promise { - return fetchInternal(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 index f2928404a356..1ab772cc57c8 100644 --- a/packages/wrangler/src/cfetch/internal.ts +++ b/packages/wrangler/src/cfetch/internal.ts @@ -31,7 +31,6 @@ export async function fetchInternal( ...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 { + 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 index bad62b17e65f..2f07c4c9e36c 100644 --- 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 { } } // -- 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 index 01e206e253eb..c222192e1261 100644 --- 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 { + return await fetchKVGetValue(accountId, namespaceId, key); +} + export async function putBulkKeyValue( accountId: string, namespaceId: string,