From 7d104ddf482d4f5f870faec33cf8d535c483c0d9 Mon Sep 17 00:00:00 2001 From: Harrison Lopes Date: Thu, 13 Jan 2022 18:06:43 -0300 Subject: [PATCH 01/10] refactor: add getBaseUrlOf util method --- test/http-data-source.test.ts | 99 +++++++++++++++++++---------------- 1 file changed, 55 insertions(+), 44 deletions(-) diff --git a/test/http-data-source.test.ts b/test/http-data-source.test.ts index de3cc03..3a1234e 100644 --- a/test/http-data-source.test.ts +++ b/test/http-data-source.test.ts @@ -38,7 +38,7 @@ test('Should be able to make a simple GET call', async (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -78,7 +78,7 @@ test('Should be able to make a simple POST call', async (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -125,7 +125,7 @@ test('Should be able to make a simple POST with JSON body', async (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -166,7 +166,7 @@ test('Should be able to make a simple DELETE call', async (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -203,7 +203,7 @@ test('Should be able to make a simple PUT call', async (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -240,7 +240,7 @@ test('Should be able to make a simple PATCH call', async (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -278,7 +278,7 @@ test('Should be able to pass query params', async (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -317,7 +317,7 @@ test('Should error on HTTP errors > 299 and != 304', async (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -372,7 +372,7 @@ test('Should not parse content as JSON when content-type header is missing', asy server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -409,7 +409,7 @@ test('Should memoize subsequent GET calls to the same endpoint', async (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -467,7 +467,7 @@ test('Should not memoize subsequent GET calls for unsuccessful responses', async server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -529,7 +529,7 @@ test('Should be able to define a custom cache key for request memoization', asyn server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -574,7 +574,7 @@ test('Should correctly calculate and sort query parameters', async (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -611,7 +611,7 @@ test('Should call onError on request error', async (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -668,7 +668,7 @@ test('Should be possible to pass a request context', async (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -713,7 +713,7 @@ test.cb('Should abort request when abortController signal is called', (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const abortController = new AbortController() @@ -769,7 +769,7 @@ test.cb('Should timeout because server does not respond fast enough', (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -826,7 +826,7 @@ test('Should be able to modify request in willSendRequest', async (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -869,7 +869,7 @@ test('Should be able to define base headers for every request', async (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -917,7 +917,7 @@ test('Initialize data source with cache and context', async (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -976,7 +976,7 @@ test('Should cache a GET response and respond with the result on subsequent call server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) let dataSource = new (class extends HTTPDataSource { constructor() { @@ -1093,7 +1093,7 @@ test('Should respond with stale-if-error cache on origin error', async (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) let dataSource = new (class extends HTTPDataSource { constructor() { @@ -1187,7 +1187,7 @@ test('Should not cache POST requests', async (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -1254,7 +1254,7 @@ test('Should only cache GET successful responses with the correct status code', server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -1354,11 +1354,13 @@ test.serial('Global maxAge should be used when no maxAge was set or similar.', a server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) - let testResponse: Response | { - memoized: boolean - } = { + let testResponse: + | Response + | { + memoized: boolean + } = { memoized: false, } const maxAge = 10000 @@ -1366,8 +1368,8 @@ test.serial('Global maxAge should be used when no maxAge was set or similar.', a constructor() { super(baseURL, { lru: { - maxAge: maxAge - } + maxAge: maxAge, + }, }) } getFoo() { @@ -1402,7 +1404,7 @@ test('Response is not cached due to origin error', async (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const dataSource = new (class extends HTTPDataSource { constructor() { @@ -1472,7 +1474,7 @@ test('Should be able to pass custom Undici Pool', async (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const pool = new Pool(baseURL) const dataSource = new (class extends HTTPDataSource { @@ -1498,17 +1500,19 @@ test('Should be merge headers', async (t) => { const mockHeaders = { 'test-a': 'a', - 'test-b': 'b' + 'test-b': 'b', } const server = http.createServer((req, res) => { t.is(req.method, 'GET') res.writeHead(200, { 'content-type': 'application/json', }) - res.write(JSON.stringify({ - 'test-a': req.headers['test-a'], - 'test-b': req.headers['test-b'] - })) + res.write( + JSON.stringify({ + 'test-a': req.headers['test-a'], + 'test-b': req.headers['test-b'], + }), + ) res.end() res.socket?.unref() }) @@ -1517,7 +1521,7 @@ test('Should be merge headers', async (t) => { server.listen() - const baseURL = `http://localhost:${(server.address() as AddressInfo)?.port}` + const baseURL = getBaseUrlOf(server) const pool = new Pool(baseURL) const dataSource = new (class extends HTTPDataSource { @@ -1526,15 +1530,17 @@ test('Should be merge headers', async (t) => { pool, requestOptions: { headers: { - 'test-a': mockHeaders['test-a'] - } - } + 'test-a': mockHeaders['test-a'], + }, + }, }) } getFoo() { - return this.get(path, {headers: { - 'test-b': mockHeaders['test-b'] - }}) + return this.get(path, { + headers: { + 'test-b': mockHeaders['test-b'], + }, + }) } })() @@ -1542,3 +1548,8 @@ test('Should be merge headers', async (t) => { t.deepEqual(response.body, mockHeaders) }) + +// Utils +function getBaseUrlOf(server: http.Server) { + return `http://localhost:${(server.address() as AddressInfo)?.port}` +} From f4b4eb7201fc5c4201577ad7b5c7b28a4e6b048a Mon Sep 17 00:00:00 2001 From: Harrison Lopes Date: Thu, 13 Jan 2022 18:07:11 -0300 Subject: [PATCH 02/10] feat: add request memoize option --- src/http-data-source.ts | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/src/http-data-source.ts b/src/http-data-source.ts index 77748fe..688e96c 100644 --- a/src/http-data-source.ts +++ b/src/http-data-source.ts @@ -48,6 +48,8 @@ export type Request = { origin: string path: string method: HttpMethod + // Indicates if the response of this request should be memoized + memoize?: boolean headers: Dictionary } & CacheTTLOptions @@ -141,6 +143,16 @@ export abstract class HTTPDataSource extends DataSource { return statusCodeCacheableByDefault.has(response.statusCode) && request.method === 'GET' } + /** + * Checks if the GET request is memoizable. This validation is performed before the + * response is set in **memoisedResults**. + * @param request + * @returns + */ + protected isRequestMemoizable(request: Request): boolean { + return Boolean(request.memoize) + } + /** * onCacheKeyCalculation returns the key for the GET request. * The key is used to memoize the request in the LRU cache. @@ -193,6 +205,7 @@ export abstract class HTTPDataSource extends DataSource { headers: {}, query: {}, body: null, + memoize: true, context: {}, ...requestOptions, method: 'GET', @@ -390,7 +403,10 @@ export abstract class HTTPDataSource extends DataSource { return cachedResponse } const response = this.performRequest(options, cacheKey) - this.memoizedResults.set(cacheKey, response) + if (request && this.isRequestMemoizable(request)) { + this.memoizedResults.set(cacheKey, response) + } + return response } catch (error: any) { this.logger?.error(`Cache item '${cacheKey}' could not be loaded: ${error.message}`) @@ -398,7 +414,9 @@ export abstract class HTTPDataSource extends DataSource { } const response = this.performRequest(options, cacheKey) - this.memoizedResults.set(cacheKey, response) + if (this.isRequestMemoizable(request)) { + this.memoizedResults.set(cacheKey, response) + } return response } From 8772c21d9d909fa8aaf6e34476bdbfbc98c74b11 Mon Sep 17 00:00:00 2001 From: Harrison Lopes Date: Thu, 13 Jan 2022 18:08:47 -0300 Subject: [PATCH 03/10] test: add tests for request memoize option --- test/http-data-source.test.ts | 144 ++++++++++++++++++++++++++++++++++ 1 file changed, 144 insertions(+) diff --git a/test/http-data-source.test.ts b/test/http-data-source.test.ts index 3a1234e..88a95d5 100644 --- a/test/http-data-source.test.ts +++ b/test/http-data-source.test.ts @@ -445,6 +445,150 @@ test('Should memoize subsequent GET calls to the same endpoint', async (t) => { t.falsy(response.maxTtl) }) +test('Should memoize subsequent GET calls to the same endpoint when the memoize option is undefined', async (t) => { + t.plan(17) + + const path = '/' + + const MAX_SUBSEQUENT_CALLS = 3 + + const wanted = { name: 'foo' } + + const server = http.createServer((req, res) => { + t.is(req.method, 'GET') + res.writeHead(200, { + 'content-type': 'application/json', + }) + res.write(JSON.stringify(wanted)) + setTimeout(() => res.end(), 50).unref() + res.socket?.unref() + }) + + t.teardown(server.close.bind(server)) + + server.listen() + + const baseURL = getBaseUrlOf(server) + + const dataSource = new (class extends HTTPDataSource { + constructor() { + super(baseURL) + } + getFoo() { + return this.get(path) + } + })() + + const firstCall = await dataSource.getFoo() + t.deepEqual(firstCall.body, wanted) + t.false(firstCall.isFromCache) + t.false(firstCall.memoized) + t.falsy(firstCall.maxTtl) + + for (let currentCall = 0; currentCall < MAX_SUBSEQUENT_CALLS; currentCall++) { + const subsequentCall = await dataSource.getFoo() + + t.deepEqual(subsequentCall.body, wanted) + t.false(subsequentCall.isFromCache) + t.true(subsequentCall.memoized) + t.falsy(subsequentCall.maxTtl) + } +}) + +test('Should memoize subsequent GET calls to the same endpoint when the memoize option is true', async (t) => { + t.plan(17) + + const path = '/' + + const MAX_SUBSEQUENT_CALLS = 3 + + const wanted = { name: 'foo' } + + const server = http.createServer((req, res) => { + t.is(req.method, 'GET') + res.writeHead(200, { + 'content-type': 'application/json', + }) + res.write(JSON.stringify(wanted)) + setTimeout(() => res.end(), 50).unref() + res.socket?.unref() + }) + + t.teardown(server.close.bind(server)) + + server.listen() + + const baseURL = getBaseUrlOf(server) + + const dataSource = new (class extends HTTPDataSource { + constructor() { + super(baseURL) + } + getFoo() { + return this.get(path, { memoize: true }) + } + })() + + const firstCall = await dataSource.getFoo() + t.deepEqual(firstCall.body, wanted) + t.false(firstCall.isFromCache) + t.false(firstCall.memoized) + t.falsy(firstCall.maxTtl) + + for (let currentCall = 0; currentCall < MAX_SUBSEQUENT_CALLS; currentCall++) { + const subsequentCall = await dataSource.getFoo() + + t.deepEqual(subsequentCall.body, wanted) + t.false(subsequentCall.isFromCache) + t.true(subsequentCall.memoized) + t.falsy(subsequentCall.maxTtl) + } +}) + +test('Should not memoize subsequent GET calls to the same endpoint when the memoize option is false', async (t) => { + t.plan(15) + + const path = '/' + + const MAX_CALLS = 3 + + const wanted = { name: 'foo' } + + const server = http.createServer((req, res) => { + t.is(req.method, 'GET') + res.writeHead(200, { + 'content-type': 'application/json', + }) + res.write(JSON.stringify(wanted)) + setTimeout(() => res.end(), 50).unref() + res.socket?.unref() + }) + + t.teardown(server.close.bind(server)) + + server.listen() + + const baseURL = getBaseUrlOf(server) + + const dataSource = new (class extends HTTPDataSource { + constructor() { + super(baseURL) + } + getFoo() { + return this.get(path, { memoize: false }) + } + })() + + for (let currentCall = 0; currentCall < MAX_CALLS; currentCall++) { + const subsequentCall = await dataSource.getFoo() + + t.deepEqual(subsequentCall.body, wanted) + t.false(subsequentCall.isFromCache) + t.false(subsequentCall.memoized) + t.falsy(subsequentCall.maxTtl) + } +}) + test('Should not memoize subsequent GET calls for unsuccessful responses', async (t) => { t.plan(17) From f2cd7b52ede29c53db0cff44bfefaa3fe56f7c82 Mon Sep 17 00:00:00 2001 From: Harrison Lopes Date: Thu, 13 Jan 2022 18:45:46 -0300 Subject: [PATCH 04/10] fix: remove request validation --- src/http-data-source.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/http-data-source.ts b/src/http-data-source.ts index 688e96c..020864a 100644 --- a/src/http-data-source.ts +++ b/src/http-data-source.ts @@ -403,7 +403,7 @@ export abstract class HTTPDataSource extends DataSource { return cachedResponse } const response = this.performRequest(options, cacheKey) - if (request && this.isRequestMemoizable(request)) { + if (this.isRequestMemoizable(request)) { this.memoizedResults.set(cacheKey, response) } From 9d34556875ed63da0d4ca1a9a9d00d7ae23cacbd Mon Sep 17 00:00:00 2001 From: Harrison Lopes Date: Fri, 14 Jan 2022 10:06:14 -0300 Subject: [PATCH 05/10] docs: added isRequestMemoizable return documentation --- src/http-data-source.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/http-data-source.ts b/src/http-data-source.ts index 020864a..e746c65 100644 --- a/src/http-data-source.ts +++ b/src/http-data-source.ts @@ -147,7 +147,7 @@ export abstract class HTTPDataSource extends DataSource { * Checks if the GET request is memoizable. This validation is performed before the * response is set in **memoisedResults**. * @param request - * @returns + * @returns *true* if request should be memoized */ protected isRequestMemoizable(request: Request): boolean { return Boolean(request.memoize) From b9950b25d2be5f4b94910c76e2f7fcccb6149ffe Mon Sep 17 00:00:00 2001 From: Harrison Lopes Date: Wed, 19 Jan 2022 12:09:11 -0300 Subject: [PATCH 06/10] docs: fi typo --- src/http-data-source.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/http-data-source.ts b/src/http-data-source.ts index e746c65..85e6b81 100644 --- a/src/http-data-source.ts +++ b/src/http-data-source.ts @@ -145,7 +145,7 @@ export abstract class HTTPDataSource extends DataSource { /** * Checks if the GET request is memoizable. This validation is performed before the - * response is set in **memoisedResults**. + * response is set in **memoizedResults**. * @param request * @returns *true* if request should be memoized */ From fac09a6593ec6de9ee8cc31960004fff21d69c8c Mon Sep 17 00:00:00 2001 From: Harrison Lopes Date: Fri, 21 Jan 2022 14:27:18 -0300 Subject: [PATCH 07/10] feat: add memoize verification in performRequest --- src/http-data-source.ts | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/http-data-source.ts b/src/http-data-source.ts index 85e6b81..1ca0819 100644 --- a/src/http-data-source.ts +++ b/src/http-data-source.ts @@ -89,7 +89,7 @@ export abstract class HTTPDataSource extends DataSource { private logger?: Logger private cache!: KeyValueCache private globalRequestOptions?: RequestOptions - private readonly memoizedResults: QuickLRU>> + private readonly memoizedResults: QuickLRU> constructor(public readonly baseURL: string, private readonly options?: HTTPDataSourceOptions) { super() @@ -144,9 +144,9 @@ export abstract class HTTPDataSource extends DataSource { } /** - * Checks if the GET request is memoizable. This validation is performed before the + * Checks if the GET request is memoizable. This validation is performed before the * response is set in **memoizedResults**. - * @param request + * @param request * @returns *true* if request should be memoized */ protected isRequestMemoizable(request: Request): boolean { @@ -325,6 +325,10 @@ export abstract class HTTPDataSource extends DataSource { this.onResponse(request, response) + if (this.isRequestMemoizable(request)) { + this.memoizedResults.set(cacheKey, response) + } + // let's see if we can fill the shared cache if (request.requestCache && this.isResponseCacheable(request, response)) { response.maxTtl = request.requestCache.maxTtl @@ -403,9 +407,6 @@ export abstract class HTTPDataSource extends DataSource { return cachedResponse } const response = this.performRequest(options, cacheKey) - if (this.isRequestMemoizable(request)) { - this.memoizedResults.set(cacheKey, response) - } return response } catch (error: any) { @@ -414,9 +415,6 @@ export abstract class HTTPDataSource extends DataSource { } const response = this.performRequest(options, cacheKey) - if (this.isRequestMemoizable(request)) { - this.memoizedResults.set(cacheKey, response) - } return response } From 97dc6db24dcc4bafa36bc567779d1cacbbb9542e Mon Sep 17 00:00:00 2001 From: Harrison Lopes Date: Fri, 21 Jan 2022 15:52:22 -0300 Subject: [PATCH 08/10] feat: add request method validation on isRequestMemoizable --- src/http-data-source.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/http-data-source.ts b/src/http-data-source.ts index 1ca0819..cac46db 100644 --- a/src/http-data-source.ts +++ b/src/http-data-source.ts @@ -150,7 +150,7 @@ export abstract class HTTPDataSource extends DataSource { * @returns *true* if request should be memoized */ protected isRequestMemoizable(request: Request): boolean { - return Boolean(request.memoize) + return Boolean(request.memoize) && request.method === 'GET' } /** From f6462c0964a750f554cb92f3ffbfacd3f4d0a6b0 Mon Sep 17 00:00:00 2001 From: Harrison Lopes Date: Mon, 7 Feb 2022 17:00:58 -0300 Subject: [PATCH 09/10] test: add Response type usage --- test/http-data-source.test.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/test/http-data-source.test.ts b/test/http-data-source.test.ts index 88a95d5..3ca3bfb 100644 --- a/test/http-data-source.test.ts +++ b/test/http-data-source.test.ts @@ -1500,11 +1500,7 @@ test.serial('Global maxAge should be used when no maxAge was set or similar.', a const baseURL = getBaseUrlOf(server) - let testResponse: - | Response - | { - memoized: boolean - } = { + let testResponse: Pick, 'memoized'> = { memoized: false, } const maxAge = 10000 From 615fb9053336107e668afa8a3b481636121d8915 Mon Sep 17 00:00:00 2001 From: Harrison Lopes Date: Mon, 7 Feb 2022 17:50:42 -0300 Subject: [PATCH 10/10] docs: add docs for the get method --- src/http-data-source.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/http-data-source.ts b/src/http-data-source.ts index cac46db..1acf8d2 100644 --- a/src/http-data-source.ts +++ b/src/http-data-source.ts @@ -197,6 +197,14 @@ export abstract class HTTPDataSource extends DataSource { protected onError?(_error: Error, requestOptions: Request): void + /** + * Execute a HTTP GET request. + * Note that the **memoizedResults** and **cache** will be checked before request is made. + * By default the received response will be memoized. + * + * @param path the path to the resource + * @param requestOptions + */ public async get( path: string, requestOptions?: RequestOptions,