Check If-None-Match before fulfilling preservation cache (#7546)

* Refactor ETag handling

* Check if-none-match before fulfilling preservation cache

Author: GregBrimble

.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

packages/pages-shared/__tests__/asset-server/handler.test.ts

@@ -572,38 +572,52 @@ 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,
 				caches,
 				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",
 				"cf-cache-status": "HIT", // new header
 			});
 
 			// 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",

packages/pages-shared/asset-server/handler.ts

@@ -72,6 +72,25 @@ type FindAssetEntryForPath<AssetEntry> = (
 	path: string
 ) => Promise<null | AssetEntry>;
 
+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: AssetEntry,
 	options?: { preserve: boolean }
@@ -80,7 +99,10 @@ type ServeAsset<AssetEntry> = (
 type CacheStatus = "hit" | "miss";
 type CacheResult<A extends string> = `${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<string, string> = {
-				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) {