From c6c7124af09934ee3ec67c761f66ceae1adadabc Mon Sep 17 00:00:00 2001 From: Felix Ezama-Vaughan Date: Sun, 25 May 2025 21:17:21 -0600 Subject: [PATCH 1/3] init commit --- lib/util/cache.js | 21 +- .../cache-store-test-utils.js | 208 ++++++++++++++++++ 2 files changed, 228 insertions(+), 1 deletion(-) diff --git a/lib/util/cache.js b/lib/util/cache.js index 6f347936941..7d260b3aac3 100644 --- a/lib/util/cache.js +++ b/lib/util/cache.js @@ -12,10 +12,29 @@ function makeCacheKey (opts) { throw new Error('opts.origin is undefined') } + let fullPath = opts.path || '/' + + if (opts.query && typeof opts.query === 'object') { + const params = new URLSearchParams() + for (const [key, value] of Object.entries(opts.query)) { + if (Array.isArray(value)) { + for (const v of value) { + params.append(key, v) + } + } else if (value !== undefined && value !== null) { + params.append(key, String(value)) + } + } + const queryString = params.toString() + if (queryString) { + fullPath += (fullPath.includes('?') ? '&' : '?') + queryString + } + } + return { origin: opts.origin.toString(), method: opts.method, - path: opts.path, + path: fullPath, headers: opts.headers } } diff --git a/test/cache-interceptor/cache-store-test-utils.js b/test/cache-interceptor/cache-store-test-utils.js index b97fe01f8d5..45167a1807b 100644 --- a/test/cache-interceptor/cache-store-test-utils.js +++ b/test/cache-interceptor/cache-store-test-utils.js @@ -5,6 +5,9 @@ const { describe, test, after } = require('node:test') const { Readable } = require('node:stream') const { once } = require('node:events') const FakeTimers = require('@sinonjs/fake-timers') +const { interceptors } = require('../../') +const { request, Agent, setGlobalDispatcher } = require('../../') +const http = require('node:http') /** * @typedef {import('../../types/cache-interceptor.d.ts').default.CacheStore} CacheStore @@ -345,6 +348,211 @@ function cacheStoreTests (CacheStore) { await compareGetResults(result, anotherValue, anotherBody) } }) + + // test('different query parameters create separate cache entries', async () => { + // /** + // * @type {import('../../types/cache-interceptor.d.ts').default.CacheKey} + // */ + // const baseKey = { + // origin: 'localhost', + // path: '/api/users', + // method: 'GET', + // headers: {} + // } + + // /** + // * @type {import('../../types/cache-interceptor.d.ts').default.CacheValue} + // */ + // const value1 = { + // statusCode: 200, + // statusMessage: '', + // headers: { 'content-type': 'application/json' }, + // cacheControlDirectives: {}, + // cachedAt: Date.now(), + // staleAt: Date.now() + 10000, + // deleteAt: Date.now() + 20000 + // } + + // /** + // * @type {import('../../types/cache-interceptor.d.ts').default.CacheValue} + // */ + // const value2 = { + // statusCode: 200, + // statusMessage: '', + // headers: { 'content-type': 'application/json' }, + // cacheControlDirectives: {}, + // cachedAt: Date.now(), + // staleAt: Date.now() + 10000, + // deleteAt: Date.now() + 20000 + // } + + // const body1 = [Buffer.from('page1data')] + // const body2 = [Buffer.from('page2data')] + + // const store = new CacheStore() + + // // Cache response for page=1 + // const key1 = { ...baseKey, query: { page: 1 } } + // { + // const writable = store.createWriteStream(key1, value1) + // notEqual(writable, undefined) + // writeBody(writable, body1) + // } + + // // Cache response for page=2 + // const key2 = { ...baseKey, query: { page: 2 } } + // { + // const writable = store.createWriteStream(key2, value2) + // notEqual(writable, undefined) + // writeBody(writable, body2) + // } + + // // Verify we get different responses for different query parameters + // { + // const result1 = await store.get(structuredClone(key1)) + // notEqual(result1, undefined) + // await compareGetResults(result1, value1, body1) + // } + + // { + // const result2 = await store.get(structuredClone(key2)) + // notEqual(result2, undefined) + // await compareGetResults(result2, value2, body2) + // } + + // // Verify the responses are actually different + // const result1Body = await readBody(await store.get(key1)) + // const result2Body = await readBody(await store.get(key2)) + + // notEqual( + // joinBufferArray(result1Body).toString(), + // joinBufferArray(result2Body).toString(), + // 'Different query parameters should return different cached responses' + // ) + // }) + + // test('complex query parameters are handled correctly', async () => { + // /** + // * @type {import('../../types/cache-interceptor.d.ts').default.CacheKey} + // */ + // const baseKey = { + // origin: 'localhost', + // path: '/api/search', + // method: 'GET', + // headers: {} + // } + + // /** + // * @type {import('../../types/cache-interceptor.d.ts').default.CacheValue} + // */ + // const value = { + // statusCode: 200, + // statusMessage: '', + // headers: { 'content-type': 'application/json' }, + // cacheControlDirectives: {}, + // cachedAt: Date.now(), + // staleAt: Date.now() + 10000, + // deleteAt: Date.now() + 20000 + // } + + // const body = [Buffer.from('search results')] + // const store = new CacheStore() + + // // Test with complex query parameters including arrays and special characters + // const complexKey = { + // ...baseKey, + // query: { + // q: 'hello world', + // tags: ['javascript', 'nodejs'], + // limit: 10, + // include_meta: true, + // 'special-chars': 'test@example.com' + // } + // } + + // // Cache the response + // { + // const writable = store.createWriteStream(complexKey, value) + // notEqual(writable, undefined) + // writeBody(writable, body) + // } + + // // Verify we can retrieve it with the same complex query + // { + // const result = await store.get(structuredClone(complexKey)) + // notEqual(result, undefined) + // await compareGetResults(result, value, body) + // } + + // // Verify that a slightly different query doesn't match + // const differentKey = { + // ...baseKey, + // query: { + // q: 'hello world', + // tags: ['javascript', 'nodejs'], + // limit: 20, // Different limit + // include_meta: true, + // 'special-chars': 'test@example.com' + // } + // } + + // equal(await store.get(differentKey), undefined, 'Different query parameters should not match existing cache entry') + // }) + + test('playground', async () => { + // Interceptors to add response caching, DNS caching and retrying to the dispatcher + const { cache, dns, retry } = interceptors + + const defaultDispatcher = new Agent({ + connections: 100, // Limit concurrent kept-alive connections to not run out of resources + headersTimeout: 10_000, // 10 seconds; set as appropriate for the remote servers you plan to connect to + bodyTimeout: 10_000 + }).compose(cache(), dns(), retry()) + + setGlobalDispatcher(defaultDispatcher) // Add these interceptors to all `fetch` and Undici `request` calls + + const server = new http.Server((req, res) => { + sleep(100).then(() => { + res + .writeHead(200, { + 'Content-Type': 'application/json', + 'Cache-Control': '"public, max-age=100, stale-while-revalidate=100"' + }) + .end(new Date().toISOString()) + }) + }) + server.listen('8080') + + const sleep = (t) => new Promise((resolve) => setTimeout(resolve, t)) + + const responses = new Set() + for (let i = 0; i <= 10; i++) { + const startedAt = performance.now() + const query = { i } + const res = await request({ + origin: 'http://localhost:8080', + + // path works fine + // path: `?i=${i}`, + + // while same via `query` fails? + query, + headers: { + 'Content-Tupe': 'application/json', + UserAgent: 'UndiciExample/1.0.0' + } + }) + const text = await res.body.text() + responses.add(text) + console.log({ + url: `/?i=${i}`, + duration: (performance.now() - startedAt) / 1000, + response: text + }) + } + + console.log('Unique responses', responses.size, responses) + }) }) } From a1f646d62c36bfc536fa70bac46624366a3e7e5d Mon Sep 17 00:00:00 2001 From: Felix Ezama-Vaughan Date: Sun, 25 May 2025 22:14:12 -0600 Subject: [PATCH 2/3] tested implementation --- .../cache-store-test-utils.js | 208 ---------------- test/interceptors/cache-query-params.js | 229 ++++++++++++++++++ 2 files changed, 229 insertions(+), 208 deletions(-) create mode 100644 test/interceptors/cache-query-params.js diff --git a/test/cache-interceptor/cache-store-test-utils.js b/test/cache-interceptor/cache-store-test-utils.js index 45167a1807b..b97fe01f8d5 100644 --- a/test/cache-interceptor/cache-store-test-utils.js +++ b/test/cache-interceptor/cache-store-test-utils.js @@ -5,9 +5,6 @@ const { describe, test, after } = require('node:test') const { Readable } = require('node:stream') const { once } = require('node:events') const FakeTimers = require('@sinonjs/fake-timers') -const { interceptors } = require('../../') -const { request, Agent, setGlobalDispatcher } = require('../../') -const http = require('node:http') /** * @typedef {import('../../types/cache-interceptor.d.ts').default.CacheStore} CacheStore @@ -348,211 +345,6 @@ function cacheStoreTests (CacheStore) { await compareGetResults(result, anotherValue, anotherBody) } }) - - // test('different query parameters create separate cache entries', async () => { - // /** - // * @type {import('../../types/cache-interceptor.d.ts').default.CacheKey} - // */ - // const baseKey = { - // origin: 'localhost', - // path: '/api/users', - // method: 'GET', - // headers: {} - // } - - // /** - // * @type {import('../../types/cache-interceptor.d.ts').default.CacheValue} - // */ - // const value1 = { - // statusCode: 200, - // statusMessage: '', - // headers: { 'content-type': 'application/json' }, - // cacheControlDirectives: {}, - // cachedAt: Date.now(), - // staleAt: Date.now() + 10000, - // deleteAt: Date.now() + 20000 - // } - - // /** - // * @type {import('../../types/cache-interceptor.d.ts').default.CacheValue} - // */ - // const value2 = { - // statusCode: 200, - // statusMessage: '', - // headers: { 'content-type': 'application/json' }, - // cacheControlDirectives: {}, - // cachedAt: Date.now(), - // staleAt: Date.now() + 10000, - // deleteAt: Date.now() + 20000 - // } - - // const body1 = [Buffer.from('page1data')] - // const body2 = [Buffer.from('page2data')] - - // const store = new CacheStore() - - // // Cache response for page=1 - // const key1 = { ...baseKey, query: { page: 1 } } - // { - // const writable = store.createWriteStream(key1, value1) - // notEqual(writable, undefined) - // writeBody(writable, body1) - // } - - // // Cache response for page=2 - // const key2 = { ...baseKey, query: { page: 2 } } - // { - // const writable = store.createWriteStream(key2, value2) - // notEqual(writable, undefined) - // writeBody(writable, body2) - // } - - // // Verify we get different responses for different query parameters - // { - // const result1 = await store.get(structuredClone(key1)) - // notEqual(result1, undefined) - // await compareGetResults(result1, value1, body1) - // } - - // { - // const result2 = await store.get(structuredClone(key2)) - // notEqual(result2, undefined) - // await compareGetResults(result2, value2, body2) - // } - - // // Verify the responses are actually different - // const result1Body = await readBody(await store.get(key1)) - // const result2Body = await readBody(await store.get(key2)) - - // notEqual( - // joinBufferArray(result1Body).toString(), - // joinBufferArray(result2Body).toString(), - // 'Different query parameters should return different cached responses' - // ) - // }) - - // test('complex query parameters are handled correctly', async () => { - // /** - // * @type {import('../../types/cache-interceptor.d.ts').default.CacheKey} - // */ - // const baseKey = { - // origin: 'localhost', - // path: '/api/search', - // method: 'GET', - // headers: {} - // } - - // /** - // * @type {import('../../types/cache-interceptor.d.ts').default.CacheValue} - // */ - // const value = { - // statusCode: 200, - // statusMessage: '', - // headers: { 'content-type': 'application/json' }, - // cacheControlDirectives: {}, - // cachedAt: Date.now(), - // staleAt: Date.now() + 10000, - // deleteAt: Date.now() + 20000 - // } - - // const body = [Buffer.from('search results')] - // const store = new CacheStore() - - // // Test with complex query parameters including arrays and special characters - // const complexKey = { - // ...baseKey, - // query: { - // q: 'hello world', - // tags: ['javascript', 'nodejs'], - // limit: 10, - // include_meta: true, - // 'special-chars': 'test@example.com' - // } - // } - - // // Cache the response - // { - // const writable = store.createWriteStream(complexKey, value) - // notEqual(writable, undefined) - // writeBody(writable, body) - // } - - // // Verify we can retrieve it with the same complex query - // { - // const result = await store.get(structuredClone(complexKey)) - // notEqual(result, undefined) - // await compareGetResults(result, value, body) - // } - - // // Verify that a slightly different query doesn't match - // const differentKey = { - // ...baseKey, - // query: { - // q: 'hello world', - // tags: ['javascript', 'nodejs'], - // limit: 20, // Different limit - // include_meta: true, - // 'special-chars': 'test@example.com' - // } - // } - - // equal(await store.get(differentKey), undefined, 'Different query parameters should not match existing cache entry') - // }) - - test('playground', async () => { - // Interceptors to add response caching, DNS caching and retrying to the dispatcher - const { cache, dns, retry } = interceptors - - const defaultDispatcher = new Agent({ - connections: 100, // Limit concurrent kept-alive connections to not run out of resources - headersTimeout: 10_000, // 10 seconds; set as appropriate for the remote servers you plan to connect to - bodyTimeout: 10_000 - }).compose(cache(), dns(), retry()) - - setGlobalDispatcher(defaultDispatcher) // Add these interceptors to all `fetch` and Undici `request` calls - - const server = new http.Server((req, res) => { - sleep(100).then(() => { - res - .writeHead(200, { - 'Content-Type': 'application/json', - 'Cache-Control': '"public, max-age=100, stale-while-revalidate=100"' - }) - .end(new Date().toISOString()) - }) - }) - server.listen('8080') - - const sleep = (t) => new Promise((resolve) => setTimeout(resolve, t)) - - const responses = new Set() - for (let i = 0; i <= 10; i++) { - const startedAt = performance.now() - const query = { i } - const res = await request({ - origin: 'http://localhost:8080', - - // path works fine - // path: `?i=${i}`, - - // while same via `query` fails? - query, - headers: { - 'Content-Tupe': 'application/json', - UserAgent: 'UndiciExample/1.0.0' - } - }) - const text = await res.body.text() - responses.add(text) - console.log({ - url: `/?i=${i}`, - duration: (performance.now() - startedAt) / 1000, - response: text - }) - } - - console.log('Unique responses', responses.size, responses) - }) }) } diff --git a/test/interceptors/cache-query-params.js b/test/interceptors/cache-query-params.js new file mode 100644 index 00000000000..8ce7742b196 --- /dev/null +++ b/test/interceptors/cache-query-params.js @@ -0,0 +1,229 @@ +'use strict' + +const { test, after } = require('node:test') +const { equal, notEqual } = require('node:assert') +const { createServer } = require('node:http') +const { once } = require('node:events') +const { Client, request, interceptors } = require('../../') + +test('query parameters create separate cache entries', async () => { + let requestCount = 0 + const server = createServer((req, res) => { + requestCount++ + const url = new URL(req.url, 'http://localhost') + const param = url.searchParams.get('param') || 'default' + + res.writeHead(200, { + 'Content-Type': 'application/json', + 'Cache-Control': 'public, max-age=100' + }) + res.end(JSON.stringify({ + message: `Response for param=${param}`, + requestNumber: requestCount + })) + }) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + after(async () => { + server.close() + await client.close() + }) + + const origin = `http://localhost:${server.address().port}` + + // First request with param=value1 + const response1 = await request(origin, { + dispatcher: client, + query: { param: 'value1' } + }) + const body1 = await response1.body.text() + equal(requestCount, 1, 'First request should hit the server') + + // Second request with same param - should be cached + const response2 = await request(origin, { + dispatcher: client, + query: { param: 'value1' } + }) + const body2 = await response2.body.text() + equal(requestCount, 1, 'Second request with same query should be cached') + equal(body1, body2, 'Cached response should match original') + + // Third request with different param - should NOT be cached + const response3 = await request(origin, { + dispatcher: client, + query: { param: 'value2' } + }) + const body3 = await response3.body.text() + equal(requestCount, 2, 'Request with different query should hit the server') + notEqual(body1, body3, 'Different query parameters should create separate cache entries') +}) + +test('complex query parameters are handled correctly', async () => { + let requestCount = 0 + const server = createServer((req, res) => { + requestCount++ + + res.writeHead(200, { + 'Content-Type': 'application/json', + 'Cache-Control': 'public, max-age=100' + }) + res.end(JSON.stringify({ + url: req.url, + requestNumber: requestCount + })) + }) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + after(async () => { + server.close() + await client.close() + }) + + const origin = `http://localhost:${server.address().port}` + + // Complex query with arrays and multiple parameters + const complexQuery = { + search: 'hello world', + tags: ['javascript', 'nodejs'], + limit: 10, + active: true + } + + // First request + const response1 = await request(origin, { + dispatcher: client, + query: complexQuery + }) + const body1 = await response1.body.text() + equal(requestCount, 1, 'First complex query should hit the server') + + // Same complex query - should be cached + const response2 = await request(origin, { + dispatcher: client, + query: complexQuery + }) + const body2 = await response2.body.text() + equal(requestCount, 1, 'Same complex query should be cached') + equal(body1, body2, 'Complex query parameters should be cached correctly') + + // Slightly different query - should NOT be cached + const response3 = await request(origin, { + dispatcher: client, + query: { + search: 'hello world', + tags: ['javascript', 'nodejs'], + limit: 20, // Different limit + active: true + } + }) + const body3 = await response3.body.text() + equal(requestCount, 2, 'Different complex query should hit the server') + notEqual(body1, body3, 'Different query parameters should create separate cache entries') +}) + +test('query parameters vs path equivalence', async () => { + let requestCount = 0 + const server = createServer((req, res) => { + requestCount++ + + res.writeHead(200, { + 'Content-Type': 'application/json', + 'Cache-Control': 'public, max-age=100' + }) + res.end(JSON.stringify({ + url: req.url, + requestNumber: requestCount + })) + }) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + after(async () => { + server.close() + await client.close() + }) + + const origin = `http://localhost:${server.address().port}` + + // Request using query object + const response1 = await request(origin, { + dispatcher: client, + query: { foo: 'bar', baz: 'qux' } + }) + const body1 = await response1.body.text() + equal(requestCount, 1, 'Query object request should hit the server') + + // Request using path with query string - should be cached if URLs match + const response2 = await request(`${origin}/?foo=bar&baz=qux`, { + dispatcher: client + }) + const body2 = await response2.body.text() + equal(requestCount, 1, 'Equivalent path query should be cached') + equal(body1, body2, 'Query object and path query should be equivalent') +}) + +test('empty and undefined query parameters', async () => { + let requestCount = 0 + const server = createServer((req, res) => { + requestCount++ + + res.writeHead(200, { + 'Content-Type': 'application/json', + 'Cache-Control': 'public, max-age=100' + }) + res.end(JSON.stringify({ + url: req.url, + requestNumber: requestCount + })) + }) + + server.listen(0) + await once(server, 'listening') + + const client = new Client(`http://localhost:${server.address().port}`) + .compose(interceptors.cache()) + + after(async () => { + server.close() + await client.close() + }) + + const origin = `http://localhost:${server.address().port}` + + // Request with no query + const response1 = await request(origin, { dispatcher: client }) + const body1 = await response1.body.text() + equal(requestCount, 1, 'No query request should hit the server') + + // Request with empty query object - should be cached + const response2 = await request(origin, { + dispatcher: client, + query: {} + }) + const body2 = await response2.body.text() + equal(requestCount, 1, 'Empty query object should be cached') + equal(body1, body2, 'No query and empty query should be equivalent') + + // Request with undefined query - should be cached + const response3 = await request(origin, { + dispatcher: client, + query: undefined + }) + const body3 = await response3.body.text() + equal(requestCount, 1, 'Undefined query should be cached') + equal(body1, body3, 'No query and undefined query should be equivalent') +}) From 4b308cf7109772ee6581d49a141b3c06e259ca3e Mon Sep 17 00:00:00 2001 From: Felix Ezama-Vaughan Date: Tue, 27 May 2025 22:19:59 -0600 Subject: [PATCH 3/3] reused code for query param serialization --- lib/util/cache.js | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/lib/util/cache.js b/lib/util/cache.js index 7d260b3aac3..10b0d8afcbc 100644 --- a/lib/util/cache.js +++ b/lib/util/cache.js @@ -4,6 +4,8 @@ const { safeHTTPMethods } = require('../core/util') +const { serializePathWithQuery } = require('../core/util') + /** * @param {import('../../types/dispatcher.d.ts').default.DispatchOptions} opts */ @@ -12,23 +14,12 @@ function makeCacheKey (opts) { throw new Error('opts.origin is undefined') } - let fullPath = opts.path || '/' - - if (opts.query && typeof opts.query === 'object') { - const params = new URLSearchParams() - for (const [key, value] of Object.entries(opts.query)) { - if (Array.isArray(value)) { - for (const v of value) { - params.append(key, v) - } - } else if (value !== undefined && value !== null) { - params.append(key, String(value)) - } - } - const queryString = params.toString() - if (queryString) { - fullPath += (fullPath.includes('?') ? '&' : '?') + queryString - } + let fullPath + try { + fullPath = serializePathWithQuery(opts.path || '/', opts.query) + } catch (error) { + // If fails (path already has query params), use as-is + fullPath = opts.path || '/' } return {