diff --git a/.changeset/fuzzy-nails-invent.md b/.changeset/fuzzy-nails-invent.md new file mode 100644 index 000000000000..e21d23648c10 --- /dev/null +++ b/.changeset/fuzzy-nails-invent.md @@ -0,0 +1,5 @@ +--- +"@cloudflare/pages-shared": minor +--- + +feat: Return a 304 Not Modified response when matching an asset preservation cache request if appropriate diff --git a/packages/pages-shared/__tests__/asset-server/handler.test.ts b/packages/pages-shared/__tests__/asset-server/handler.test.ts index b1ba50cb35ea..b97c1a8e91f0 100644 --- a/packages/pages-shared/__tests__/asset-server/handler.test.ts +++ b/packages/pages-shared/__tests__/asset-server/handler.test.ts @@ -572,11 +572,25 @@ describe("asset-server handler", () => { '"asset-key-foo.html"' ); - // Delete the asset from the manifest and ensure it's served from preservation cache + // Delete the asset from the manifest and ensure it's served from preservation cache with a 304 when if-none-match is present findAssetEntryForPath = async (_path: string) => { return null; }; const { response: response2 } = await getTestResponse({ + request: new Request("https://example.com/foo", { + headers: { "if-none-match": expectedHeaders.etag }, + }), + metadata, + findAssetEntryForPath, + caches, + fetchAsset: () => + Promise.resolve(Object.assign(new Response("hello world!"))), + }); + expect(response2.status).toBe(304); + expect(await response2.text()).toMatchInlineSnapshot('""'); + + // Ensure the asset is served from preservation cache with a 200 if if-none-match is not present + const { response: response3 } = await getTestResponse({ request: new Request("https://example.com/foo"), metadata, findAssetEntryForPath, @@ -584,10 +598,10 @@ describe("asset-server handler", () => { fetchAsset: () => Promise.resolve(Object.assign(new Response("hello world!"))), }); - expect(response2.status).toBe(200); - expect(await response2.text()).toMatchInlineSnapshot('"hello world!"'); + expect(response3.status).toBe(200); + expect(await response3.text()).toMatchInlineSnapshot('"hello world!"'); // Cached responses have the same headers with a few changes/additions: - expect(Object.fromEntries(response2.headers)).toStrictEqual({ + expect(Object.fromEntries(response3.headers)).toStrictEqual({ ...expectedHeaders, "cache-control": "public, s-maxage=604800", "x-robots-tag": "noindex", @@ -595,15 +609,15 @@ describe("asset-server handler", () => { }); // Serve with a fresh cache and ensure we don't get a response - const { response: response3 } = await getTestResponse({ + const { response: response4 } = await getTestResponse({ request: new Request("https://example.com/foo"), metadata, findAssetEntryForPath, fetchAsset: () => Promise.resolve(Object.assign(new Response("hello world!"))), }); - expect(response3.status).toBe(404); - expect(Object.fromEntries(response3.headers)).toMatchInlineSnapshot(` + expect(response4.status).toBe(404); + expect(Object.fromEntries(response4.headers)).toMatchInlineSnapshot(` { "access-control-allow-origin": "*", "cache-control": "no-store", diff --git a/packages/pages-shared/asset-server/handler.ts b/packages/pages-shared/asset-server/handler.ts index be8b5ae84e32..d1f68e33e7c9 100644 --- a/packages/pages-shared/asset-server/handler.ts +++ b/packages/pages-shared/asset-server/handler.ts @@ -72,6 +72,25 @@ type FindAssetEntryForPath = ( path: string ) => Promise; +function generateETagHeader(assetKey: string) { + // https://support.cloudflare.com/hc/en-us/articles/218505467-Using-ETag-Headers-with-Cloudflare + // We sometimes remove etags unless they are wrapped in quotes + const strongETag = `"${assetKey}"`; + const weakETag = `W/"${assetKey}"`; + return { strongETag, weakETag }; +} + +function checkIfNoneMatch( + request: Request, + strongETag: string, + weakETag: string +) { + const ifNoneMatch = request.headers.get("if-none-match"); + + // We sometimes downgrade strong etags to a weak ones, so we need to check for both + return ifNoneMatch === weakETag || ifNoneMatch === strongETag; +} + type ServeAsset = ( assetEntry: AssetEntry, options?: { preserve: boolean } @@ -80,7 +99,10 @@ type ServeAsset = ( type CacheStatus = "hit" | "miss"; type CacheResult = `${A}-${CacheStatus}`; export type HandlerMetrics = { - preservationCacheResult?: CacheResult<"checked"> | "disabled"; + preservationCacheResult?: + | CacheResult<"checked"> + | "not-modified" + | "disabled"; earlyHintsResult?: CacheResult<"used" | "notused"> | "disabled"; }; @@ -518,22 +540,16 @@ export async function generateHandler< const assetKey = getAssetKey(servingAssetEntry, content); - // https://support.cloudflare.com/hc/en-us/articles/218505467-Using-ETag-Headers-with-Cloudflare - // We sometimes remove etags unless they are wrapped in quotes - const etag = `"${assetKey}"`; - const weakEtag = `W/${etag}`; - - const ifNoneMatch = request.headers.get("if-none-match"); - - // We sometimes downgrade strong etags to a weak ones, so we need to check for both - if (ifNoneMatch === weakEtag || ifNoneMatch === etag) { + const { strongETag, weakETag } = generateETagHeader(assetKey); + const isIfNoneMatch = checkIfNoneMatch(request, strongETag, weakETag); + if (isIfNoneMatch) { return new NotModifiedResponse(); } try { const asset = await fetchAsset(assetKey); const headers: Record = { - etag, + etag: strongETag, "content-type": asset.contentType, }; let encodeBody: BodyEncoding = "automatic"; @@ -654,6 +670,19 @@ export async function generateHandler< return new Response(null, preservedResponse); } if (assetKey) { + const { strongETag, weakETag } = generateETagHeader(assetKey); + const isIfNoneMatch = checkIfNoneMatch( + request, + strongETag, + weakETag + ); + if (isIfNoneMatch) { + if (setMetrics) { + setMetrics({ preservationCacheResult: "not-modified" }); + } + return new NotModifiedResponse(); + } + const asset = await fetchAsset(assetKey); if (asset) {