diff --git a/.changeset/tricky-baboons-build.md b/.changeset/tricky-baboons-build.md new file mode 100644 index 000000000000..3c8846771672 --- /dev/null +++ b/.changeset/tricky-baboons-build.md @@ -0,0 +1,9 @@ +--- +"wrangler": patch +--- + +Improve validation message for `kv:namespace create` + +Previously, if the user passed multiple positional arguments (which is invalid) +the error message would suggest that these should be grouped in quotes. +But this is also wrong, since a namespace binding name must not contain spaces. diff --git a/.vscode/settings.json b/.vscode/settings.json index 7550fa01dd89..c8c4aea6168a 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -1,4 +1,4 @@ { "editor.defaultFormatter": "esbenp.prettier-vscode", - "cSpell.words": ["cfetch", "execa", "iarna", "Miniflare"] + "cSpell.words": ["cfetch", "execa", "iarna", "Miniflare", "Positionals"] } diff --git a/packages/wrangler/src/__tests__/kv.test.ts b/packages/wrangler/src/__tests__/kv.test.ts index 2cef808ce315..237a2f90c37a 100644 --- a/packages/wrangler/src/__tests__/kv.test.ts +++ b/packages/wrangler/src/__tests__/kv.test.ts @@ -1,76 +1,210 @@ import { setMock, unsetAllMocks } from "./mock-cfetch"; import { runWrangler } from "./run-wrangler"; +import { runInTempDir } from "./run-in-tmp"; +import { writeFileSync } from "fs"; describe("wrangler", () => { + runInTempDir(); + afterEach(() => { unsetAllMocks(); }); describe("kv:namespace", () => { - it("can create a namespace", async () => { - const KVNamespaces: { title: string; id: string }[] = []; - setMock("/accounts/:accountId/storage/kv/namespaces", (uri, init) => { - expect(init.method === "POST"); - expect(uri[0]).toEqual( - "/accounts/some-account-id/storage/kv/namespaces" + describe("create", () => { + function mockCreateRequest(expectedTitle: string) { + setMock( + "/accounts/:accountId/storage/kv/namespaces", + "POST", + ([_url, accountId], { body }) => { + expect(accountId).toEqual("some-account-id"); + const title = JSON.parse(body).title; + expect(title).toEqual(expectedTitle); + return { id: "some-namespace-id" }; + } + ); + } + + it("should error if no namespace is given", async () => { + const { stdout, stderr } = await runWrangler("kv:namespace create"); + expect(stdout).toMatchInlineSnapshot(`""`); + expect(stderr).toMatchInlineSnapshot(` + "wrangler kv:namespace create + + Create a new namespace + + Positionals: + namespace The name of the new namespace [string] [required] + + Flags: + -c, --config Path to .toml configuration file [string] + -h, --help Show help [boolean] + -v, --version Show version number [boolean] + + Options: + -l, --local Run on my machine [boolean] [default: false] + --env Perform on a specific environment [string] + --preview Interact with a preview namespace [boolean] + Not enough non-option arguments: got 0, need at least 1" + `); + }); + + it("should error if the namespace to create contains spaces", async () => { + const { stdout, stderr } = await runWrangler( + "kv:namespace create abc def ghi" ); - const { title } = JSON.parse(init.body); - expect(title).toEqual("worker-UnitTestNamespace"); - KVNamespaces.push({ title, id: "some-namespace-id" }); - return { id: "some-namespace-id" }; + expect(stdout).toMatchInlineSnapshot(`""`); + expect(stderr).toMatchInlineSnapshot(` + "wrangler kv:namespace create + + Create a new namespace + + Positionals: + namespace The name of the new namespace [string] [required] + + Flags: + -c, --config Path to .toml configuration file [string] + -h, --help Show help [boolean] + -v, --version Show version number [boolean] + + Options: + -l, --local Run on my machine [boolean] [default: false] + --env Perform on a specific environment [string] + --preview Interact with a preview namespace [boolean] + Unexpected additional positional arguments \\"def ghi\\"." + `); }); - await runWrangler("kv:namespace create UnitTestNamespace"); + it("should error if the namespace to create is not valid", async () => { + const { stdout, stderr } = await runWrangler( + "kv:namespace create abc-def" + ); + expect(stdout).toMatchInlineSnapshot(`""`); + expect(stderr).toMatchInlineSnapshot(` + "wrangler kv:namespace create - expect(KVNamespaces).toEqual([ - { - title: "worker-UnitTestNamespace", - id: "some-namespace-id", - }, - ]); + Create a new namespace + + Positionals: + namespace The name of the new namespace [string] [required] + + Flags: + -c, --config Path to .toml configuration file [string] + -h, --help Show help [boolean] + -v, --version Show version number [boolean] + + Options: + -l, --local Run on my machine [boolean] [default: false] + --env Perform on a specific environment [string] + --preview Interact with a preview namespace [boolean] + The namespace binding name \\"abc-def\\" is invalid. It can only have alphanumeric and _ characters, and cannot begin with a number." + `); + }); + + it("should create a namespace", async () => { + mockCreateRequest("worker-UnitTestNamespace"); + const { stdout } = await runWrangler( + "kv:namespace create UnitTestNamespace" + ); + expect(stdout).toMatchInlineSnapshot(` + "🌀 Creating namespace with title \\"worker-UnitTestNamespace\\" + ✨ Success! + Add the following to your configuration file in your kv_namespaces array: + { binding = \\"UnitTestNamespace\\", id = \\"some-namespace-id\\" }" + `); + }); + + it("should create a preview namespace if configured to do so", async () => { + mockCreateRequest("worker-UnitTestNamespace_preview"); + const { stdout } = await runWrangler( + "kv:namespace create UnitTestNamespace --preview" + ); + expect(stdout).toMatchInlineSnapshot(` + "🌀 Creating namespace with title \\"worker-UnitTestNamespace_preview\\" + ✨ Success! + Add the following to your configuration file in your kv_namespaces array: + { binding = \\"UnitTestNamespace\\", preview_id = \\"some-namespace-id\\" }" + `); + }); + + it("should create a namespace using configured worker name", async () => { + writeFileSync("./wrangler.toml", 'name = "otherWorker"', "utf-8"); + mockCreateRequest("otherWorker-UnitTestNamespace"); + const { stdout } = await runWrangler( + "kv:namespace create UnitTestNamespace" + ); + expect(stdout).toMatchInlineSnapshot(` + "🌀 Creating namespace with title \\"otherWorker-UnitTestNamespace\\" + ✨ Success! + Add the following to your configuration file in your kv_namespaces array: + { binding = \\"UnitTestNamespace\\", id = \\"some-namespace-id\\" }" + `); + }); + + it("should create a namespace in an environment if configured to do so", async () => { + mockCreateRequest("worker-customEnv-UnitTestNamespace"); + const { stdout } = await runWrangler( + "kv:namespace create UnitTestNamespace --env customEnv" + ); + expect(stdout).toMatchInlineSnapshot(` + "🌀 Creating namespace with title \\"worker-customEnv-UnitTestNamespace\\" + ✨ Success! + Add the following to your configuration file in your kv_namespaces array under [env.customEnv]: + { binding = \\"UnitTestNamespace\\", id = \\"some-namespace-id\\" }" + `); + }); }); - it("can list namespaces", async () => { - const KVNamespaces: { title: string; id: string }[] = [ - { title: "title-1", id: "id-1" }, - { title: "title-2", id: "id-2" }, - ]; - setMock( - "/accounts/:accountId/storage/kv/namespaces\\?:qs", - (uri, init) => { - expect(uri[0]).toContain( - "/accounts/some-account-id/storage/kv/namespaces" - ); - expect(uri[2]).toContain("per_page=100"); - expect(uri[2]).toContain("order=title"); - expect(uri[2]).toContain("direction=asc"); - expect(uri[2]).toContain("page=1"); - expect(init).toBe(undefined); - return KVNamespaces; - } - ); - const { stdout } = await runWrangler("kv:namespace list"); - const namespaces = JSON.parse(stdout) as { id: string; title: string }[]; - expect(namespaces).toEqual(KVNamespaces); + describe("list", () => { + it("should list namespaces", async () => { + const KVNamespaces: { title: string; id: string }[] = [ + { title: "title-1", id: "id-1" }, + { title: "title-2", id: "id-2" }, + ]; + setMock( + "/accounts/:accountId/storage/kv/namespaces\\?:qs", + (uri, init) => { + expect(uri[0]).toContain( + "/accounts/some-account-id/storage/kv/namespaces" + ); + expect(uri[2]).toContain("per_page=100"); + expect(uri[2]).toContain("order=title"); + expect(uri[2]).toContain("direction=asc"); + expect(uri[2]).toContain("page=1"); + expect(init).toBe(undefined); + return KVNamespaces; + } + ); + const { stdout } = await runWrangler("kv:namespace list"); + const namespaces = JSON.parse(stdout) as { + id: string; + title: string; + }[]; + expect(namespaces).toEqual(KVNamespaces); + }); }); - it("can delete a namespace", async () => { - let accountId = ""; - let namespaceId = ""; - setMock( - "/accounts/:accountId/storage/kv/namespaces/:namespaceId", - (uri, init) => { - accountId = uri[1]; - namespaceId = uri[2]; - expect(uri[0]).toEqual( - "/accounts/some-account-id/storage/kv/namespaces/some-namespace-id" - ); - expect(init.method).toBe("DELETE"); - } - ); - await runWrangler(`kv:namespace delete --namespace-id some-namespace-id`); - expect(accountId).toEqual("some-account-id"); - expect(namespaceId).toEqual("some-namespace-id"); + describe("delete", () => { + it("should delete a namespace", async () => { + let accountId = ""; + let namespaceId = ""; + setMock( + "/accounts/:accountId/storage/kv/namespaces/:namespaceId", + (uri, init) => { + accountId = uri[1]; + namespaceId = uri[2]; + expect(uri[0]).toEqual( + "/accounts/some-account-id/storage/kv/namespaces/some-namespace-id" + ); + expect(init.method).toBe("DELETE"); + } + ); + await runWrangler( + `kv:namespace delete --namespace-id some-namespace-id` + ); + expect(accountId).toEqual("some-account-id"); + expect(namespaceId).toEqual("some-namespace-id"); + }); }); }); }); diff --git a/packages/wrangler/src/__tests__/mock-cfetch.js b/packages/wrangler/src/__tests__/mock-cfetch.js index af8befbe8dc0..2c0d692adcf0 100644 --- a/packages/wrangler/src/__tests__/mock-cfetch.js +++ b/packages/wrangler/src/__tests__/mock-cfetch.js @@ -9,21 +9,24 @@ const { pathToRegexp } = require("path-to-regexp"); let mocks = []; export function mockCfetch(resource, init) { - for (const { regexp, handler } of mocks) { + for (const { regexp, method, handler } of mocks) { // The `resource` regular expression will extract the labelled groups from the URL. // Let's pass these through to the handler, to allow it to do additional checks or behaviour. const uri = regexp.exec(resource); - if (uri !== null) { + // Does the resource path match and (if specified) the HTTP method? + if (uri !== null && (!method || method === init.method)) { return handler(uri, init); // TODO: should we have some kind of fallthrough system? we'll see. } } throw new Error(`no mocks found for ${resource}`); } -export function setMock(resource, handler) { +export function setMock(resource, methodOrHandler, handler = methodOrHandler) { + const hasMethod = typeof methodOrHandler === "string"; const mock = { resource, - handler, + method: hasMethod ? methodOrHandler : undefined, + handler: hasMethod ? handler : methodOrHandler, regexp: pathToRegexp(resource), }; mocks.push(mock); diff --git a/packages/wrangler/src/index.tsx b/packages/wrangler/src/index.tsx index 22e873ebe788..aad1528266a9 100644 --- a/packages/wrangler/src/index.tsx +++ b/packages/wrangler/src/index.tsx @@ -25,6 +25,8 @@ import { putKeyValue, putBulkKeyValue, deleteBulkKeyValue, + createNamespace, + isValidNamespaceBinding, } from "./kv"; import { pages } from "./pages"; @@ -1140,13 +1142,17 @@ export async function main(argv: string[]): Promise { }); }, async (args) => { - if (args._.length !== 2) { - throw new Error( - `Did you forget to add quotes around "${ - args.namespace - } ${args._.slice(2).join(" ")}"?` + if (args._.length > 2) { + const extraArgs = args._.slice(2).join(" "); + throw `Unexpected additional positional arguments "${extraArgs}".`; + } + + if (!isValidNamespaceBinding(args.namespace)) { + throw new CommandLineArgsError( + `The namespace binding name "${args.namespace}" is invalid. It can only have alphanumeric and _ characters, and cannot begin with a number.` ); } + const config = args.config as Config; if (!config.name) { console.warn( @@ -1154,13 +1160,10 @@ export async function main(argv: string[]): Promise { ); } - const title = `${config.name || "worker"}${ - args.env ? `-${args.env}` : "" - }-${args.namespace}${args.preview ? "_preview" : ""}`; - - if (/[\W]+/.test(args.namespace)) { - throw new Error("invalid binding name, needs to be js friendly"); - } + const name = config.name || "worker"; + const environment = args.env ? `-${args.env}` : ""; + const preview = args.preview ? "_preview" : ""; + const title = `${name}${environment}-${args.namespace}${preview}`; if (args.local) { const { Miniflare } = await import("miniflare"); @@ -1197,30 +1200,16 @@ export async function main(argv: string[]): Promise { // TODO: generate a binding name stripping non alphanumeric chars console.log(`🌀 Creating namespace with title "${title}"`); - - const response = await cfetch<{ id: string }>( - `/accounts/${config.account_id}/storage/kv/namespaces`, - { - method: "POST", - headers: { - "Content-Type": "application/json", - }, - body: JSON.stringify({ - title, - }), - } - ); + const namespaceId = await createNamespace(config.account_id, title); console.log("✨ Success!"); + const envString = args.env ? ` under [env.${args.env}]` : ""; + const previewString = args.preview ? "preview_" : ""; console.log( - `Add the following to your configuration file in your kv_namespaces array${ - args.env ? ` under [env.${args.env}]` : "" - }:` + `Add the following to your configuration file in your kv_namespaces array${envString}:` ); console.log( - `{ binding = "${args.namespace}", ${ - args.preview ? "preview_" : "" - }id = "${response.id}" }` + `{ binding = "${args.namespace}", ${previewString}id = "${namespaceId}" }` ); } ) diff --git a/packages/wrangler/src/kv.tsx b/packages/wrangler/src/kv.tsx index 115c11d8257f..1956e92d0d38 100644 --- a/packages/wrangler/src/kv.tsx +++ b/packages/wrangler/src/kv.tsx @@ -10,6 +10,31 @@ type KvArgs = { config?: Config; }; +/** + * Create a new namespace under the given `accountId` with the given `title`. + * + * @returns the generated id of the created namespace. + */ +export async function createNamespace( + accountId: string, + title: string +): Promise { + const response = await cfetch<{ id: string }>( + `/accounts/${accountId}/storage/kv/namespaces`, + { + method: "POST", + headers: { + "Content-Type": "application/json", + }, + body: JSON.stringify({ + title, + }), + } + ); + + return response.id; +} + export async function listNamespaces(accountId: string) { let page = 1, done = false, @@ -213,3 +238,10 @@ export function getNamespaceId({ return namespaceId; } + +/** + * KV namespace binding names must be valid JS identifiers. + */ +export function isValidNamespaceBinding(binding: string): boolean { + return /^[a-zA-Z_][a-zA-Z0-9_]*$/.test(binding); +}