From 7ed13ecf8eca2425d15902141ac6b47d47973790 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Sun, 17 Nov 2024 10:59:22 -0800 Subject: [PATCH 1/5] feat: cache etag support Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- docs/docs/api/CacheStore.md | 6 +-- lib/handler/cache-handler.js | 16 ++++++-- lib/interceptor/cache.js | 3 +- lib/util/cache.js | 43 +++++++++++++++++++++ test/cache-interceptor/utils.js | 29 ++++++++++++++- test/interceptors/cache.js | 66 +++++++++++++++++++++++++++++++++ types/cache-interceptor.d.ts | 1 + 7 files changed, 155 insertions(+), 9 deletions(-) diff --git a/docs/docs/api/CacheStore.md b/docs/docs/api/CacheStore.md index f8a6962d65a..de69e5647c9 100644 --- a/docs/docs/api/CacheStore.md +++ b/docs/docs/api/CacheStore.md @@ -36,7 +36,7 @@ If the request isn't cached, `undefined` is returned. Response properties: -* **response** `CachedResponse` - The cached response data. +* **response** `CacheValue` - The cached response data. * **body** `Readable | undefined` - The response's body. ### Function: `createWriteStream` @@ -44,11 +44,11 @@ Response properties: Parameters: * **req** `Dispatcher.RequestOptions` - Incoming request -* **value** `CachedResponse` - Response to store +* **value** `CacheValue` - Response to store Returns: `Writable | undefined` - If the store is full, return `undefined`. Otherwise, return a writable so that the cache interceptor can stream the body and trailers to the store. -## `CachedResponse` +## `CacheValue` This is an interface containing the majority of a response's data (minus the body). diff --git a/lib/handler/cache-handler.js b/lib/handler/cache-handler.js index e39c5479ac7..1afc1b98347 100644 --- a/lib/handler/cache-handler.js +++ b/lib/handler/cache-handler.js @@ -4,7 +4,8 @@ const util = require('../core/util') const DecoratorHandler = require('../handler/decorator-handler') const { parseCacheControlHeader, - parseVaryHeader + parseVaryHeader, + isEtagUsable } = require('../util/cache') const { nowAbsolute } = require('../util/timers.js') @@ -136,7 +137,10 @@ class CacheHandler extends DecoratorHandler { cacheControlDirectives ) - this.#writeStream = this.#store.createWriteStream(this.#cacheKey, { + /** + * @type {import('../../types/cache-interceptor.d.ts').default.CacheValue} + */ + const value = { statusCode, statusMessage, rawHeaders: strippedHeaders, @@ -144,7 +148,13 @@ class CacheHandler extends DecoratorHandler { cachedAt: now, staleAt, deleteAt - }) + } + + if (typeof headers.etag === 'string' && isEtagUsable(headers.etag)) { + value.etag = headers.etag + } + + this.#writeStream = this.#store.createWriteStream(this.#cacheKey, value) if (this.#writeStream) { const handler = this diff --git a/lib/interceptor/cache.js b/lib/interceptor/cache.js index 9895fcb8a86..75b639ae145 100644 --- a/lib/interceptor/cache.js +++ b/lib/interceptor/cache.js @@ -152,7 +152,8 @@ module.exports = (opts = {}) => { ...opts, headers: { ...opts.headers, - 'if-modified-since': new Date(result.cachedAt).toUTCString() + 'if-modified-since': new Date(result.cachedAt).toUTCString(), + etag: result.etag } }, new CacheRevalidationHandler( diff --git a/lib/util/cache.js b/lib/util/cache.js index cb5e84051b8..18acc73b71c 100644 --- a/lib/util/cache.js +++ b/lib/util/cache.js @@ -201,6 +201,48 @@ function parseVaryHeader (varyHeader, headers) { return output } +/** + * Note: this deviates from the spec a little. Empty etags ("", W/"") are valid, + * however, including them in cached resposnes serves little to no purpose. + * + * @see https://www.rfc-editor.org/rfc/rfc9110.html#name-etag + * + * @param {string} etag + * @returns {boolean} + */ +function isEtagUsable (etag) { + if (etag.length <= 2) { + // Shortest an etag can be is two chars (just ""). This is where we deviate + // from the spec requiring a min of 3 chars however + return false + } + + if (etag[0] === '"' && etag[etag.length - 1] === '"') { + if (etag[1] === '"' || etag.startsWith('"W/')) { + // ETag: ""asd123"" or ETag: "W/"asd123"", kinda undefined behavior in the + // spec. Some servers will accept these while others don't. + return false + } + + // ETag: "asd123" + return true + } + + if (etag.startsWith('W/"') && etag[etag.length - 1] === '"') { + if (etag.length === 4) { + // ETag: W/"", also where we deviate from the spec & require a min of 3 + // chars + return false + } + + // ETag: for W/"", W/"asd123" + return true + } + + // Anything else + return false +} + /** * @param {unknown} store * @returns {asserts store is import('../../types/cache-interceptor.d.ts').default.CacheStore} @@ -244,6 +286,7 @@ module.exports = { makeCacheKey, parseCacheControlHeader, parseVaryHeader, + isEtagUsable, assertCacheMethods, assertCacheStore } diff --git a/test/cache-interceptor/utils.js b/test/cache-interceptor/utils.js index f7e0a948ef3..2fc8bc17dea 100644 --- a/test/cache-interceptor/utils.js +++ b/test/cache-interceptor/utils.js @@ -1,8 +1,8 @@ 'use strict' const { describe, test } = require('node:test') -const { deepStrictEqual } = require('node:assert') -const { parseCacheControlHeader, parseVaryHeader } = require('../../lib/util/cache') +const { deepStrictEqual, equal } = require('node:assert') +const { parseCacheControlHeader, parseVaryHeader, isEtagUsable } = require('../../lib/util/cache') describe('parseCacheControlHeader', () => { test('all directives are parsed properly when in their correct format', () => { @@ -215,3 +215,28 @@ describe('parseVaryHeader', () => { }) }) }) + +describe('isEtagUsable', () => { + const valuesToTest = { + // Invalid etags + '': false, + asd: false, + '"W/"asd""': false, + '""asd""': false, + + // Valid etags + '"asd"': true, + 'W/"ads"': true, + + // Spec deviations + '""': false, + 'W/""': false + } + + for (const key in valuesToTest) { + const expectedValue = valuesToTest[key] + test(`\`${key}\` = ${expectedValue}`, () => { + equal(isEtagUsable(key), expectedValue) + }) + } +}) diff --git a/test/interceptors/cache.js b/test/interceptors/cache.js index 954a009c870..e59831e99b3 100644 --- a/test/interceptors/cache.js +++ b/test/interceptors/cache.js @@ -223,6 +223,72 @@ describe('Cache Interceptor', () => { strictEqual(await response.body.text(), 'asd123') }) + test('revalidates request w/ etag when provided', async (t) => { + let requestsToOrigin = 0 + + const clock = FakeTimers.install({ + shouldClearNativeTimers: true + }) + + const server = createServer((req, res) => { + res.setHeader('cache-control', 'public, s-maxage=1, stale-while-revalidate=10') + requestsToOrigin++ + + if (requestsToOrigin > 1) { + equal(req.headers['etag'], '"asd123"') + + if (requestsToOrigin === 3) { + res.end('asd123') + } else { + res.statusCode = 304 + res.end() + } + } else { + res.setHeader('etag', '"asd123"') + res.end('asd') + } + }).listen(0) + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + after(async () => { + server.close() + await client.close() + clock.uninstall() + }) + + await once(server, 'listening') + + strictEqual(requestsToOrigin, 0) + + const request = { + origin: 'localhost', + method: 'GET', + path: '/' + } + + // Send initial request. This should reach the origin + let response = await client.request(request) + strictEqual(requestsToOrigin, 1) + strictEqual(await response.body.text(), 'asd') + + clock.tick(1500) + + // Now we send two more requests. Both of these should reach the origin, + // but now with a conditional header asking if the resource has been + // updated. These need to be ran after the response is stale. + // No update for the second request + response = await client.request(request) + strictEqual(requestsToOrigin, 2) + strictEqual(await response.body.text(), 'asd') + + // This should be updated, even though the value isn't expired. + response = await client.request(request) + strictEqual(requestsToOrigin, 3) + strictEqual(await response.body.text(), 'asd123') + }) + test('respects cache store\'s isFull property', async () => { const server = createServer((_, res) => { res.end('asd') diff --git a/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts index fd71e30f586..59372a59d30 100644 --- a/types/cache-interceptor.d.ts +++ b/types/cache-interceptor.d.ts @@ -30,6 +30,7 @@ declare namespace CacheHandler { statusMessage: string rawHeaders: Buffer[] vary?: Record + etag?: string cachedAt: number staleAt: number deleteAt: number From 03e2235f5781b8d7cee8946c345a778590d0562b Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 18 Nov 2024 11:34:05 -0800 Subject: [PATCH 2/5] Apply suggestions from code review Co-authored-by: Carlos Fuentes --- lib/util/cache.js | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/lib/util/cache.js b/lib/util/cache.js index 18acc73b71c..a8b4cc87266 100644 --- a/lib/util/cache.js +++ b/lib/util/cache.js @@ -218,14 +218,10 @@ function isEtagUsable (etag) { } if (etag[0] === '"' && etag[etag.length - 1] === '"') { - if (etag[1] === '"' || etag.startsWith('"W/')) { - // ETag: ""asd123"" or ETag: "W/"asd123"", kinda undefined behavior in the - // spec. Some servers will accept these while others don't. - return false - } - + // ETag: ""asd123"" or ETag: "W/"asd123"", kinda undefined behavior in the + // spec. Some servers will accept these while others don't. // ETag: "asd123" - return true + return !(etag[1] === '"' || etag.startsWith('"W/')) } if (etag.startsWith('W/"') && etag[etag.length - 1] === '"') { From d87f0824f085798003efb26b416078293f73b465 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 18 Nov 2024 11:35:56 -0800 Subject: [PATCH 3/5] code review Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/util/cache.js | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/lib/util/cache.js b/lib/util/cache.js index a8b4cc87266..66ff1750015 100644 --- a/lib/util/cache.js +++ b/lib/util/cache.js @@ -225,14 +225,10 @@ function isEtagUsable (etag) { } if (etag.startsWith('W/"') && etag[etag.length - 1] === '"') { - if (etag.length === 4) { - // ETag: W/"", also where we deviate from the spec & require a min of 3 - // chars - return false - } - + // ETag: W/"", also where we deviate from the spec & require a min of 3 + // chars // ETag: for W/"", W/"asd123" - return true + return etag.length !== 4 } // Anything else From abc7f1d22170b80d75ae686a2a70d83ee27a9baf Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 18 Nov 2024 11:53:23 -0800 Subject: [PATCH 4/5] fixup Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- test/interceptors/cache.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/interceptors/cache.js b/test/interceptors/cache.js index e59831e99b3..70afb6fd7dd 100644 --- a/test/interceptors/cache.js +++ b/test/interceptors/cache.js @@ -229,6 +229,7 @@ describe('Cache Interceptor', () => { const clock = FakeTimers.install({ shouldClearNativeTimers: true }) + tick(0) const server = createServer((req, res) => { res.setHeader('cache-control', 'public, s-maxage=1, stale-while-revalidate=10') @@ -274,6 +275,7 @@ describe('Cache Interceptor', () => { strictEqual(await response.body.text(), 'asd') clock.tick(1500) + tick(1500) // Now we send two more requests. Both of these should reach the origin, // but now with a conditional header asking if the resource has been From 70c1e1c0975f859e84ad6b0ee491d2fba880bcc6 Mon Sep 17 00:00:00 2001 From: flakey5 <73616808+flakey5@users.noreply.github.com> Date: Mon, 18 Nov 2024 11:55:45 -0800 Subject: [PATCH 5/5] fixup Signed-off-by: flakey5 <73616808+flakey5@users.noreply.github.com> --- lib/cache/memory-cache-store.js | 1 + test/cache-interceptor/cache-stores.js | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js index 4c96af8860f..4579c41cc15 100644 --- a/lib/cache/memory-cache-store.js +++ b/lib/cache/memory-cache-store.js @@ -91,6 +91,7 @@ class MemoryCacheStore { statusCode: entry.statusCode, rawHeaders: entry.rawHeaders, body: entry.body, + etag: entry.etag, cachedAt: entry.cachedAt, staleAt: entry.staleAt, deleteAt: entry.deleteAt diff --git a/test/cache-interceptor/cache-stores.js b/test/cache-interceptor/cache-stores.js index ef48e11c713..0ac97fa72e0 100644 --- a/test/cache-interceptor/cache-stores.js +++ b/test/cache-interceptor/cache-stores.js @@ -58,6 +58,7 @@ function cacheStoreTests (CacheStore) { deepStrictEqual(await readResponse(readResult), { ...requestValue, + etag: undefined, body: requestBody }) @@ -94,6 +95,7 @@ function cacheStoreTests (CacheStore) { notEqual(readResult, undefined) deepStrictEqual(await readResponse(readResult), { ...anotherValue, + etag: undefined, body: anotherBody }) }) @@ -127,6 +129,7 @@ function cacheStoreTests (CacheStore) { const readResult = store.get(request) deepStrictEqual(await readResponse(readResult), { ...requestValue, + etag: undefined, body: requestBody }) }) @@ -198,6 +201,7 @@ function cacheStoreTests (CacheStore) { const { vary, ...responseValue } = requestValue deepStrictEqual(await readResponse(readStream), { ...responseValue, + etag: undefined, body: requestBody })