feat: cache etag support (#3758)

* feat: cache etag support Signed-off-by: flakey5 <[email protected]> * Apply suggestions from code review Co-authored-by: Carlos Fuentes <[email protected]> * code review Signed-off-by: flakey5 <[email protected]> * fixup Signed-off-by: flakey5 <[email protected]> * fixup Signed-off-by: flakey5 <[email protected]> --------- Signed-off-by: flakey5 <[email protected]> Co-authored-by: Carlos Fuentes <[email protected]>
nodejs · Nov 18, 2024 · a73fd09 · a73fd09
1 parent 24df4a5
commit a73fd09
Show file tree

Hide file tree

Showing 9 changed files with 154 additions and 9 deletions.
diff --git a/docs/docs/api/CacheStore.md b/docs/docs/api/CacheStore.md
@@ -36,19 +36,19 @@ 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`
 
 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/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/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,15 +137,24 @@ 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,
         vary: varyDirectives,
         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
@@ -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
@@ -201,6 +201,40 @@ 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] === '"') {
+    // ETag: ""asd123"" or ETag: "W/"asd123"", kinda undefined behavior in the
+    //  spec. Some servers will accept these while others don't.
+    // ETag: "asd123"
+    return !(etag[1] === '"' || etag.startsWith('"W/'))
+  }
+
+  if (etag.startsWith('W/"') && etag[etag.length - 1] === '"') {
+    // ETag: W/"", also where we deviate from the spec & require a min of 3
+    //  chars
+    // ETag: for W/"", W/"asd123"
+    return etag.length !== 4
+  }
+
+  // Anything else
+  return false
+}
+
 /**
  * @param {unknown} store
  * @returns {asserts store is import('../../types/cache-interceptor.d.ts').default.CacheStore}
@@ -244,6 +278,7 @@ module.exports = {
   makeCacheKey,
   parseCacheControlHeader,
   parseVaryHeader,
+  isEtagUsable,
   assertCacheMethods,
   assertCacheStore
 }
diff --git 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
       })
 

diff --git 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
@@ -223,6 +223,74 @@ 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
+    })
+    tick(0)
+
+    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)
+    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
@@ -30,6 +30,7 @@ declare namespace CacheHandler {
     statusMessage: string
     rawHeaders: Buffer[]
     vary?: Record<string, string | string[]>
+    etag?: string
     cachedAt: number
     staleAt: number
     deleteAt: number