Skip to content

Commit

Permalink
Check If-None-Match before fulfilling preservation cache (#7546)
Browse files Browse the repository at this point in the history
* Refactor ETag handling

* Check if-none-match before fulfilling preservation cache
  • Loading branch information
GregBrimble authored Dec 19, 2024
1 parent 1488e11 commit 004fd33
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 18 deletions.
5 changes: 5 additions & 0 deletions .changeset/fuzzy-nails-invent.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@cloudflare/pages-shared": minor
---

feat: Return a 304 Not Modified response when matching an asset preservation cache request if appropriate
28 changes: 21 additions & 7 deletions packages/pages-shared/__tests__/asset-server/handler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
51 changes: 40 additions & 11 deletions packages/pages-shared/asset-server/handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand All @@ -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";
};

Expand Down Expand Up @@ -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";
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit 004fd33

Please sign in to comment.