diff --git a/.changeset/common-worms-worry.md b/.changeset/common-worms-worry.md new file mode 100644 index 000000000000..16e3f069e565 --- /dev/null +++ b/.changeset/common-worms-worry.md @@ -0,0 +1,5 @@ +--- +"miniflare": patch +--- + +KV: improve error messages for bulk gets diff --git a/packages/miniflare/src/workers/kv/constants.ts b/packages/miniflare/src/workers/kv/constants.ts index 8529b1ff8808..4f72d9af2005 100644 --- a/packages/miniflare/src/workers/kv/constants.ts +++ b/packages/miniflare/src/workers/kv/constants.ts @@ -7,6 +7,7 @@ export const KVLimits = { MAX_VALUE_SIZE: 25 * 1024 * 1024 /* 25MiB */, MAX_VALUE_SIZE_TEST: 1024 /* 1KiB */, MAX_METADATA_SIZE: 1024 /* 1KiB */, + MAX_BULK_SIZE: 25 * 1024 * 1024 /* 25MiB */, } as const; export const KVParams = { diff --git a/packages/miniflare/src/workers/kv/namespace.worker.ts b/packages/miniflare/src/workers/kv/namespace.worker.ts index f29362a6b26f..ac6397143551 100644 --- a/packages/miniflare/src/workers/kv/namespace.worker.ts +++ b/packages/miniflare/src/workers/kv/namespace.worker.ts @@ -90,6 +90,7 @@ async function processKeyValue( } let val = null; + const size = decodedValue.length; try { val = !obj?.value ? null @@ -99,16 +100,19 @@ async function processKeyValue( } catch (err: any) { throw new HttpError( 400, - "At least one of the requested keys corresponds to a non-JSON value" + `At least one of the requested keys corresponds to a non-${type} value` ); } if (val && withMetadata) { - return { - value: val, - metadata: obj?.metadata ?? null, - }; + return [ + { + value: val, + metadata: obj?.metadata ?? null, + }, + size, + ]; } - return val; + return [val, size]; } export class KVNamespaceObject extends MiniflareDurableObject { @@ -132,24 +136,35 @@ export class KVNamespaceObject extends MiniflareDurableObject { const keys: string[] = parsedBody.keys; const type = parsedBody?.type; if (type && type !== "text" && type !== "json") { - return new Response(`Type ${type} is invalid`, { status: 400 }); + return new Response("Bad Request", { status: 400 }); } const obj: { [key: string]: any } = {}; if (keys.length > MAX_BULK_GET_KEYS) { - return new Response(`Accepting a max of 100 keys, got ${keys.length}`, { + return new Response("Bad Request", { status: 400, }); } + let totalBytes = 0; for (const key of keys) { validateGetOptions(key, { cacheTtl: parsedBody?.cacheTtl }); const entry = await this.storage.get(key); - const value = await processKeyValue( + const [value, size] = await processKeyValue( entry, parsedBody?.type, parsedBody?.withMetadata ); + totalBytes += size; obj[key] = value; } + const maxValueSize = this.beingTested + ? KVLimits.MAX_VALUE_SIZE_TEST + : KVLimits.MAX_BULK_SIZE; + if (totalBytes > maxValueSize) { + throw new HttpError( + 413, + `Total size of request exceeds the limit of ${maxValueSize / 1024 / 1024}MB` + ); + } return new Response(JSON.stringify(obj)); } diff --git a/packages/miniflare/test/plugins/kv/index.spec.ts b/packages/miniflare/test/plugins/kv/index.spec.ts index 3560192c5a09..de7ef8219c15 100644 --- a/packages/miniflare/test/plugins/kv/index.spec.ts +++ b/packages/miniflare/test/plugins/kv/index.spec.ts @@ -168,7 +168,7 @@ test("bulk get: request json type", async (t) => { } catch (error: any) { t.is( error.message, - "KV GET_BULK failed: 400 At least one of the requested keys corresponds to a non-JSON value" + "KV GET_BULK failed: 400 At least one of the requested keys corresponds to a non-json value" ); } }); @@ -224,6 +224,21 @@ test("bulk get: get with metadata for 404", async (t) => { t.deepEqual(result, expectedResult); }); +test("bulk get: get over size limit", async (t) => { + const { kv } = t.context; + const bigValue = new Array(1024).fill("x").join(""); + await kv.put("key1", bigValue); + await kv.put("key2", bigValue); + try { + await kv.getWithMetadata(["key1", "key2"]); + } catch (error: any) { + t.deepEqual( + error.message, + "KV GET_BULK failed: 413 Total size of request exceeds the limit of 0.0009765625MB" // 1024 Bytes for testing + ); + } +}); + test("get: returns null for non-existent keys", async (t) => { const { kv } = t.context; t.is(await kv.get("key"), null);