From c14f3aeb32f3e78c8454e9b89f85a0650b7683c4 Mon Sep 17 00:00:00 2001 From: Alejo Gschwind <31289074+alejogschwind@users.noreply.github.com> Date: Mon, 30 Jan 2023 19:17:20 -0300 Subject: [PATCH] fix: cache mechanism for request with different headers (#8754) * Fix cache mechanism for request with different headers. * add headers to build_selector hash; * update serialize_data to include req headers; * remove data-hash if req data is an empty obj; * fix failing tests and update harcoded hash; * add new test; * update hash function to support multiple params; * fmt * simplify * remove runtime assertion, use for-of loop instead of forEach * simplify * simplify test * revert hash changes * Create silent-planes-smell.md --------- Co-authored-by: Rich Harris --- .changeset/silent-planes-smell.md | 5 ++++ packages/kit/src/runtime/client/fetcher.js | 15 ++++++++++-- packages/kit/src/runtime/hash.js | 24 ++++++++++--------- .../kit/src/runtime/server/page/load_data.js | 1 + .../src/runtime/server/page/serialize_data.js | 15 ++++++++++-- .../kit/src/runtime/server/page/types.d.ts | 1 + .../load/fetch-cache-control/+page.svelte | 3 +++ .../fetch-cache-control/headers-diff/+page.js | 18 ++++++++++++++ .../headers-diff/+page.svelte | 7 ++++++ .../headers-diff/+server.js | 12 ++++++++++ .../kit/test/apps/basics/test/client.test.js | 19 +++++++++++++++ packages/kit/test/apps/basics/test/test.js | 4 ++-- 12 files changed, 107 insertions(+), 17 deletions(-) create mode 100644 .changeset/silent-planes-smell.md create mode 100644 packages/kit/test/apps/basics/src/routes/load/fetch-cache-control/headers-diff/+page.js create mode 100644 packages/kit/test/apps/basics/src/routes/load/fetch-cache-control/headers-diff/+page.svelte create mode 100644 packages/kit/test/apps/basics/src/routes/load/fetch-cache-control/headers-diff/+server.js diff --git a/.changeset/silent-planes-smell.md b/.changeset/silent-planes-smell.md new file mode 100644 index 000000000000..df9173436bb6 --- /dev/null +++ b/.changeset/silent-planes-smell.md @@ -0,0 +1,5 @@ +--- +"@sveltejs/kit": patch +--- + +fix: consider headers when constructing request hash diff --git a/packages/kit/src/runtime/client/fetcher.js b/packages/kit/src/runtime/client/fetcher.js index 90db3fe4456c..e73da8f2ca51 100644 --- a/packages/kit/src/runtime/client/fetcher.js +++ b/packages/kit/src/runtime/client/fetcher.js @@ -127,8 +127,19 @@ function build_selector(resource, opts) { let selector = `script[data-sveltekit-fetched][data-url=${url}]`; - if (opts?.body && (typeof opts.body === 'string' || ArrayBuffer.isView(opts.body))) { - selector += `[data-hash="${hash(opts.body)}"]`; + if (opts?.headers || opts?.body) { + /** @type {import('types').StrictBody[]} */ + const values = []; + + if (opts.headers) { + values.push([...new Headers(opts.headers)].join(',')); + } + + if (opts.body && (typeof opts.body === 'string' || ArrayBuffer.isView(opts.body))) { + values.push(opts.body); + } + + selector += `[data-hash="${hash(...values)}"]`; } return selector; diff --git a/packages/kit/src/runtime/hash.js b/packages/kit/src/runtime/hash.js index 0056251d3462..cfebacb382fa 100644 --- a/packages/kit/src/runtime/hash.js +++ b/packages/kit/src/runtime/hash.js @@ -1,19 +1,21 @@ /** * Hash using djb2 - * @param {import('types').StrictBody} value + * @param {import('types').StrictBody[]} values */ -export function hash(value) { +export function hash(...values) { let hash = 5381; - if (typeof value === 'string') { - let i = value.length; - while (i) hash = (hash * 33) ^ value.charCodeAt(--i); - } else if (ArrayBuffer.isView(value)) { - const buffer = new Uint8Array(value.buffer, value.byteOffset, value.byteLength); - let i = buffer.length; - while (i) hash = (hash * 33) ^ buffer[--i]; - } else { - throw new TypeError('value must be a string or TypedArray'); + for (const value of values) { + if (typeof value === 'string') { + let i = value.length; + while (i) hash = (hash * 33) ^ value.charCodeAt(--i); + } else if (ArrayBuffer.isView(value)) { + const buffer = new Uint8Array(value.buffer, value.byteOffset, value.byteLength); + let i = buffer.length; + while (i) hash = (hash * 33) ^ buffer[--i]; + } else { + throw new TypeError('value must be a string or TypedArray'); + } } return (hash >>> 0).toString(36); diff --git a/packages/kit/src/runtime/server/page/load_data.js b/packages/kit/src/runtime/server/page/load_data.js index 31af151aee3c..0c212fa9e604 100644 --- a/packages/kit/src/runtime/server/page/load_data.js +++ b/packages/kit/src/runtime/server/page/load_data.js @@ -194,6 +194,7 @@ export function create_universal_fetch(event, state, fetched, csr, resolve_opts) ? await stream_to_string(cloned_body) : init?.body ), + request_headers: init?.headers, response_body: body, response: response }); diff --git a/packages/kit/src/runtime/server/page/serialize_data.js b/packages/kit/src/runtime/server/page/serialize_data.js index 5b275b0c6bea..f176fdfb41a0 100644 --- a/packages/kit/src/runtime/server/page/serialize_data.js +++ b/packages/kit/src/runtime/server/page/serialize_data.js @@ -73,8 +73,19 @@ export function serialize_data(fetched, filter, prerendering = false) { `data-url=${escape_html_attr(fetched.url)}` ]; - if (fetched.request_body) { - attrs.push(`data-hash=${escape_html_attr(hash(fetched.request_body))}`); + if (fetched.request_headers || fetched.request_body) { + /** @type {import('types').StrictBody[]} */ + const values = []; + + if (fetched.request_headers) { + values.push([...new Headers(fetched.request_headers)].join(',')); + } + + if (fetched.request_body) { + values.push(fetched.request_body); + } + + attrs.push(`data-hash="${hash(...values)}"`); } // Compute the time the response should be cached, taking into account max-age and age. diff --git a/packages/kit/src/runtime/server/page/types.d.ts b/packages/kit/src/runtime/server/page/types.d.ts index ccd13373a6c3..2bb6c89808a7 100644 --- a/packages/kit/src/runtime/server/page/types.d.ts +++ b/packages/kit/src/runtime/server/page/types.d.ts @@ -5,6 +5,7 @@ export interface Fetched { url: string; method: string; request_body?: string | ArrayBufferView | null; + request_headers?: HeadersInit | undefined; response_body: string; response: Response; } diff --git a/packages/kit/test/apps/basics/src/routes/load/fetch-cache-control/+page.svelte b/packages/kit/test/apps/basics/src/routes/load/fetch-cache-control/+page.svelte index 1148676b0b33..059f911b8567 100644 --- a/packages/kit/test/apps/basics/src/routes/load/fetch-cache-control/+page.svelte +++ b/packages/kit/test/apps/basics/src/routes/load/fetch-cache-control/+page.svelte @@ -1 +1,4 @@ load-data + +headers-diff + diff --git a/packages/kit/test/apps/basics/src/routes/load/fetch-cache-control/headers-diff/+page.js b/packages/kit/test/apps/basics/src/routes/load/fetch-cache-control/headers-diff/+page.js new file mode 100644 index 000000000000..b64c9e0a9a68 --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/load/fetch-cache-control/headers-diff/+page.js @@ -0,0 +1,18 @@ +export async function load({ fetch, url }) { + const r1 = await fetch(url.pathname, { + headers: { + 'x-foo': 'a' + } + }); + + const r2 = await fetch(url.pathname, { + headers: { + 'x-foo': 'b' + } + }); + + return { + a: r1.json(), + b: r2.json() + }; +} diff --git a/packages/kit/test/apps/basics/src/routes/load/fetch-cache-control/headers-diff/+page.svelte b/packages/kit/test/apps/basics/src/routes/load/fetch-cache-control/headers-diff/+page.svelte new file mode 100644 index 000000000000..8aa12f6b0843 --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/load/fetch-cache-control/headers-diff/+page.svelte @@ -0,0 +1,7 @@ + + +fetch-cache-control + +

{data.a.foo} / {data.b.foo}

diff --git a/packages/kit/test/apps/basics/src/routes/load/fetch-cache-control/headers-diff/+server.js b/packages/kit/test/apps/basics/src/routes/load/fetch-cache-control/headers-diff/+server.js new file mode 100644 index 000000000000..5b181f409286 --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/load/fetch-cache-control/headers-diff/+server.js @@ -0,0 +1,12 @@ +import { json } from '@sveltejs/kit'; + +/** @type {import('./$types').RequestHandler} */ +export async function GET({ request, setHeaders }) { + setHeaders({ + 'cache-control': 'public, max-age=7' + }); + + return json({ + foo: request.headers.get('x-foo') + }); +} diff --git a/packages/kit/test/apps/basics/test/client.test.js b/packages/kit/test/apps/basics/test/client.test.js index 3a806e066826..a2403e6110a7 100644 --- a/packages/kit/test/apps/basics/test/client.test.js +++ b/packages/kit/test/apps/basics/test/client.test.js @@ -193,6 +193,25 @@ test.describe('Load', () => { expect(did_request_data).toBe(false); }); + test('do not use cache if headers are different', async ({ page, clicknav }) => { + await page.goto('/load/fetch-cache-control/headers-diff'); + + // 1. We expect the right data + expect(await page.textContent('h2')).toBe('a / b'); + + // 2. Change to another route (client side) + await clicknav('[href="/load/fetch-cache-control"]'); + + // 3. Come back to the original page (client side) + const requests = []; + page.on('request', (request) => requests.push(request)); + await clicknav('[href="/load/fetch-cache-control/headers-diff"]'); + + // 4. We expect the same data and no new request because it was cached. + expect(await page.textContent('h2')).toBe('a / b'); + expect(requests).toEqual([]); + }); + if (process.env.DEV) { test('using window.fetch causes a warning', async ({ page, baseURL }) => { await Promise.all([ diff --git a/packages/kit/test/apps/basics/test/test.js b/packages/kit/test/apps/basics/test/test.js index 8c477321d9b4..e67b827bc14a 100644 --- a/packages/kit/test/apps/basics/test/test.js +++ b/packages/kit/test/apps/basics/test/test.js @@ -238,10 +238,10 @@ test.describe('Load', () => { const payload_b = '{"status":200,"statusText":"","headers":{},"body":"Y"}'; // by the time JS has run, hydration will have nuked these scripts const script_contents_a = await page.innerHTML( - 'script[data-sveltekit-fetched][data-url="/load/serialization-post.json"][data-hash="3t25"]' + 'script[data-sveltekit-fetched][data-url="/load/serialization-post.json"][data-hash="1vn6nlx"]' ); const script_contents_b = await page.innerHTML( - 'script[data-sveltekit-fetched][data-url="/load/serialization-post.json"][data-hash="3t24"]' + 'script[data-sveltekit-fetched][data-url="/load/serialization-post.json"][data-hash="1vn6nlw"]' ); expect(script_contents_a).toBe(payload_a);