Skip to content
Permalink

Comparing changes

This is a direct comparison between two commits made in this repository or its related repositories. View the default comparison for this range or learn more about diff comparisons.

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also . Learn more about diff comparisons here.
base repository: nodejs/undici
Failed to load repositories. Confirm that selected base ref is valid, then try again.
Loading
base: c478460c56e1a0a137e3172a1276f17647a63bb7
Choose a base ref
..
head repository: nodejs/undici
Failed to load repositories. Confirm that selected head ref is valid, then try again.
Loading
compare: 85c7843f9c8696dd07f437223c7578d76a6cca0f
Choose a head ref
8 changes: 7 additions & 1 deletion lib/cache/memory-cache-store.js
Original file line number Diff line number Diff line change
@@ -79,7 +79,13 @@ class MemoryCacheStore {
const entry = this.#entries.get(topLevelKey)?.find((entry) => (
entry.deleteAt > now &&
entry.method === key.method &&
(entry.vary == null || Object.keys(entry.vary).every(headerName => entry.vary[headerName] === key.headers[headerName]))
(entry.vary == null || Object.keys(entry.vary).every(headerName => {
if (entry.vary[headerName] === null) {
return key.headers[headerName] === undefined
}

return entry.vary[headerName] === key.headers[headerName]
}))
))

return entry == null
21 changes: 10 additions & 11 deletions lib/cache/sqlite-cache-store.js
Original file line number Diff line number Diff line change
@@ -411,10 +411,6 @@ module.exports = class SqliteCacheStore {
let matches = true

if (value.vary) {
if (!headers) {
return undefined
}

const vary = JSON.parse(value.vary)

for (const header in vary) {
@@ -440,18 +436,21 @@ module.exports = class SqliteCacheStore {
* @returns {boolean}
*/
function headerValueEquals (lhs, rhs) {
if (lhs == null && rhs == null) {
return true
}

if ((lhs == null && rhs != null) ||
(lhs != null && rhs == null)) {
return false
}

if (Array.isArray(lhs) && Array.isArray(rhs)) {
if (lhs.length !== rhs.length) {
return false
}

for (let i = 0; i < lhs.length; i++) {
if (rhs.includes(lhs[i])) {
return false
}
}

return true
return lhs.every((x, i) => x === rhs[i])
}

return lhs === rhs
4 changes: 2 additions & 2 deletions lib/util/cache.js
Original file line number Diff line number Diff line change
@@ -264,7 +264,7 @@ function parseVaryHeader (varyHeader, headers) {
return headers
}

const output = /** @type {Record<string, string | string[] | undefined>} */ ({})
const output = /** @type {Record<string, string | string[] | null>} */ ({})

const varyingHeaders = typeof varyHeader === 'string'
? varyHeader.split(',')
@@ -273,7 +273,7 @@ function parseVaryHeader (varyHeader, headers) {
for (const header of varyingHeaders) {
const trimmedHeader = header.trim().toLowerCase()

output[trimmedHeader] = headers[trimmedHeader]
output[trimmedHeader] = headers[trimmedHeader] ?? null
}

return output
34 changes: 34 additions & 0 deletions test/cache-interceptor/utils.js
Original file line number Diff line number Diff line change
@@ -214,6 +214,40 @@ describe('parseVaryHeader', () => {
'another-one': '123'
})
})

test('handles missing headers with null', () => {
const result = parseVaryHeader('Accept-Encoding, Authorization', {})
deepStrictEqual(result, {
'accept-encoding': null,
authorization: null
})
})

test('handles mix of present and missing headers', () => {
const result = parseVaryHeader('Accept-Encoding, Authorization', {
authorization: 'example-value'
})
deepStrictEqual(result, {
'accept-encoding': null,
authorization: 'example-value'
})
})

test('handles array input', () => {
const result = parseVaryHeader(['Accept-Encoding', 'Authorization'], {
'accept-encoding': 'gzip'
})
deepStrictEqual(result, {
'accept-encoding': 'gzip',
authorization: null
})
})

test('preserves existing * behavior', () => {
const headers = { accept: 'text/html' }
const result = parseVaryHeader('*', headers)
deepStrictEqual(result, headers)
})
})

describe('isEtagUsable', () => {
16 changes: 13 additions & 3 deletions test/issue-3959.js
Original file line number Diff line number Diff line change
@@ -2,11 +2,12 @@ const { describe, test, after } = require('node:test')
const assert = require('node:assert')
const { createServer } = require('node:http')
const MemoryCacheStore = require('../lib/cache/memory-cache-store.js')
const SqliteCacheStore = require('../lib/cache/sqlite-cache-store.js')
const { request, Agent, setGlobalDispatcher } = require('..')
const { interceptors } = require('..')

describe('Cache with Vary headers', () => {
test('should cache response when Vary header exists but request header is missing', async () => {
async function runCacheTest (store) {
let requestCount = 0
const server = createServer((req, res) => {
requestCount++
@@ -19,7 +20,6 @@ describe('Cache with Vary headers', () => {
const port = server.address().port
const url = `http://localhost:${port}`

const store = new MemoryCacheStore()
const agent = new Agent()
setGlobalDispatcher(
agent.compose(
@@ -50,6 +50,16 @@ describe('Cache with Vary headers', () => {
assert.strictEqual(body3, 'Request count: 2')
assert.strictEqual(requestCount, 2)

after(() => server.close())
await new Promise(resolve => server.close(resolve))
}

test('should cache response with MemoryCacheStore when Vary header exists but request header is missing', async () => {
await runCacheTest(new MemoryCacheStore())
})

test('should cache response with SqliteCacheStore when Vary header exists but request header is missing', async () => {
const sqliteStore = new SqliteCacheStore()

Check failure on line 61 in test/issue-3959.js

GitHub Actions / test (20, ubuntu-latest) / Test with Node.js 20 on ubuntu-latest

should cache response with SqliteCacheStore when Vary header exists but request header is missing

[Error [ERR_TEST_FAILURE]: No such built-in module: node:sqlite] { code: 'ERR_TEST_FAILURE', failureType: 'testCodeFailure', cause: Error [ERR_UNKNOWN_BUILTIN_MODULE]: No such built-in module: node:sqlite at Module._load (node:internal/modules/cjs/loader:1044:13) at Module.require (node:internal/modules/cjs/loader:1311:19) at require (node:internal/modules/helpers:179:18) at new SqliteCacheStore (/home/runner/work/undici/undici/lib/cache/sqlite-cache-store.js:113:22) at TestContext.<anonymous> (/home/runner/work/undici/undici/test/issue-3959.js:61:25) at Test.runInAsyncScope (node:async_hooks:206:9) at Test.run (node:internal/test_runner/test:796:25) at Suite.processPendingSubtests (node:internal/test_runner/test:527:18) at Test.postRun (node:internal/test_runner/test:889:19) at Test.run (node:internal/test_runner/test:835:12) { code: 'ERR_UNKNOWN_BUILTIN_MODULE' } }

Check failure on line 61 in test/issue-3959.js

GitHub Actions / test (20, macos-latest) / Test with Node.js 20 on macos-latest

should cache response with SqliteCacheStore when Vary header exists but request header is missing

[Error [ERR_TEST_FAILURE]: No such built-in module: node:sqlite] { code: 'ERR_TEST_FAILURE', failureType: 'testCodeFailure', cause: Error [ERR_UNKNOWN_BUILTIN_MODULE]: No such built-in module: node:sqlite at Module._load (node:internal/modules/cjs/loader:1044:13) at Module.require (node:internal/modules/cjs/loader:1311:19) at require (node:internal/modules/helpers:179:18) at new SqliteCacheStore (/Users/runner/work/undici/undici/lib/cache/sqlite-cache-store.js:113:22) at TestContext.<anonymous> (/Users/runner/work/undici/undici/test/issue-3959.js:61:25) at Test.runInAsyncScope (node:async_hooks:206:9) at Test.run (node:internal/test_runner/test:796:25) at Suite.processPendingSubtests (node:internal/test_runner/test:527:18) at Test.postRun (node:internal/test_runner/test:889:19) at Test.run (node:internal/test_runner/test:835:12) { code: 'ERR_UNKNOWN_BUILTIN_MODULE' } }

Check failure on line 61 in test/issue-3959.js

GitHub Actions / Test with Node.js 20 compiled --without-intl

should cache response with SqliteCacheStore when Vary header exists but request header is missing

[Error [ERR_TEST_FAILURE]: No such built-in module: node:sqlite] { code: 'ERR_TEST_FAILURE', failureType: 'testCodeFailure', cause: Error [ERR_UNKNOWN_BUILTIN_MODULE]: No such built-in module: node:sqlite at Module._load (node:internal/modules/cjs/loader:1044:13) at Module.require (node:internal/modules/cjs/loader:1311:19) at require (node:internal/modules/helpers:179:18) at new SqliteCacheStore (/home/runner/work/undici/undici/lib/cache/sqlite-cache-store.js:113:22) at TestContext.<anonymous> (/home/runner/work/undici/undici/test/issue-3959.js:61:25) at Test.runInAsyncScope (node:async_hooks:206:9) at Test.run (node:internal/test_runner/test:796:25) at Suite.processPendingSubtests (node:internal/test_runner/test:527:18) at Test.postRun (node:internal/test_runner/test:889:19) at Test.run (node:internal/test_runner/test:835:12) { code: 'ERR_UNKNOWN_BUILTIN_MODULE' } }
await runCacheTest(sqliteStore)
after(() => sqliteStore.close())
})
})
8 changes: 4 additions & 4 deletions test/types/cache-interceptor.test-d.ts
Original file line number Diff line number Diff line change
@@ -83,7 +83,7 @@ expectAssignable<CacheInterceptor.CacheValue>({
statusMessage: 'OK',
headers: {},
vary: {
'accept-encoding': undefined,
'accept-encoding': null,
authorization: 'example-value'
},
cachedAt: 0,
@@ -96,8 +96,8 @@ expectAssignable<CacheInterceptor.CacheValue>({
statusMessage: 'OK',
headers: {},
vary: {
'accept-encoding': undefined,
authorization: undefined
'accept-encoding': null,
authorization: null
},
cachedAt: 0,
staleAt: 0,
@@ -109,7 +109,7 @@ expectNotAssignable<CacheInterceptor.CacheValue>({
statusMessage: 'OK',
headers: {},
vary: {
'accept-encoding': false,
'accept-encoding': undefined,
authorization: 'example-value'
},
cachedAt: 0,
41 changes: 0 additions & 41 deletions test/utils/cache.js

This file was deleted.

4 changes: 2 additions & 2 deletions types/cache-interceptor.d.ts
Original file line number Diff line number Diff line change
@@ -70,7 +70,7 @@ declare namespace CacheHandler {
statusCode: number
statusMessage: string
headers: Record<string, string | string[]>
vary?: Record<string, string | string[] | undefined>
vary?: Record<string, string | string[] | null>
etag?: string
cacheControlDirectives?: CacheControlDirectives
cachedAt: number
@@ -88,7 +88,7 @@ declare namespace CacheHandler {
statusCode: number
statusMessage: string
headers: Record<string, string | string[]>
vary?: Record<string, string | string[] | undefined>
vary?: Record<string, string | string[] | null>
etag?: string
body?: Readable | Iterable<Buffer> | AsyncIterable<Buffer> | Buffer | Iterable<string> | AsyncIterable<string> | string
cacheControlDirectives: CacheControlDirectives,