Repository: nodejs/undici

Problem statement:
Easier way to attach `ETag` to `If-None-Match` after #3562
## This would solve...

#3562 introduces HTTP Caching (RFC9110) to undici, which also implements HTTP 304 with `If-Modified-Since`. But it seems that `ETag` is missing.

cc @flakey5

## The implementation should look like...

IMHO #3562 should not be delayed anymore, this could be implemented after #3562 is merged.

The `undici` could check the stored `ETag` header from the cache store and use that to negotiate with the `If-None-Match` in future requests. If the server returns HTTP 304, undici would then return the cached (transparent) HTTP 200 response to the client (the same behavior of `fetch` as in browsers).

## Additional context

#3562 

https://github.com/JakeChampion/fetch/issues/241

`ETag` is described in RFC9110 here: https://httpwg.org/specs/rfc9110.html#field.etag
`If-None-Match` is described in RFC9110 here: https://httpwg.org/specs/rfc9110.html#field.if-none-match
HTTP 304 is described in RFC9110 here: https://httpwg.org/specs/rfc9110.html#status.304
The handling of HTTP 304 is described in RFC9110 here: https://httpwg.org/specs/rfc9111.html#freshening.responses


Hints:
I have a question. In the fetch spec, 304 (along with some other status codes like 101 and 204) are null body status (https://fetch.spec.whatwg.org/#concept-status).

undici could return a "transparent" cached `Response` object with 200 OKs when the server returns 304 (which is also what browsers do), which consists as I mentioned.

But in the context of Node.js, some people might also want to know whether the response is from cached or not.
We haven't really assessed if/how this would integrate with fetch.
> But in the context of Node.js, some people might also want to know whether the response is from cached or not.

We currently use make-fetch-happen and their `X-Local-Cache-Status` header to see if the response is stale is not (https://github.com/PREreview/prereview.org/blob/2fc7a676df9951494d67aa1fa3d275818fd6796d/src/fetch.ts#L27). While in this case it should be possible to use the `Age` header, it's generally far easier to have custom headers for this.
> > But in the context of Node.js, some people might also want to know whether the response is from cached or not.
> 
> We currently use make-fetch-happen and their `X-Local-Cache-Status` header to see if the response is stale is not (https://github.com/PREreview/prereview.org/blob/2fc7a676df9951494d67aa1fa3d275818fd6796d/src/fetch.ts#L27). While in this case it should be possible to use the `Age` header, it's generally far easier to have custom headers for this.

I am also using `make-fetch-happen` and `X-Local-Cache-Status`.

But with undici, it is possible to use `undici.request` over `undici.fetch`, and undici could add a `cacheStatus` property on the return value. We shall see!

Patch:
diff --git a/lib/cache/memory-cache-store.js b/lib/cache/memory-cache-store.js
--- 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
--- 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
--- 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
--- 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/types/cache-interceptor.d.ts b/types/cache-interceptor.d.ts
--- 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


Test patch:
diff --git a/test/cache-interceptor/cache-stores.js b/test/cache-interceptor/cache-stores.js
--- 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
--- 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
--- 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')