From be9c418324a92c0ad76d4bfb68b5203ddf4bb05c Mon Sep 17 00:00:00 2001 From: Sergei Zharinov Date: Thu, 18 May 2023 18:55:48 +0300 Subject: [PATCH 1/7] feat(http): Etag support --- lib/util/http/bitbucket-server.ts | 9 ++++-- lib/util/http/bitbucket.ts | 8 +++-- lib/util/http/gitea.ts | 9 ++++-- lib/util/http/github.ts | 3 +- lib/util/http/gitlab.ts | 9 ++++-- lib/util/http/index.spec.ts | 54 +++++++++++++++++++++++++++++++ lib/util/http/index.ts | 39 +++++++++++++++++----- lib/util/http/jira.ts | 9 ++++-- lib/util/http/types.ts | 9 ++++++ 9 files changed, 129 insertions(+), 20 deletions(-) diff --git a/lib/util/http/bitbucket-server.ts b/lib/util/http/bitbucket-server.ts index 449754f4cecea7..f9a7fa3ac74d0d 100644 --- a/lib/util/http/bitbucket-server.ts +++ b/lib/util/http/bitbucket-server.ts @@ -1,5 +1,10 @@ import { resolveBaseUrl } from '../url'; -import type { HttpOptions, HttpResponse, InternalHttpOptions } from './types'; +import type { + HttpOptions, + HttpRequestOptions, + HttpResponse, + InternalHttpOptions, +} from './types'; import { Http } from '.'; let baseUrl: string; @@ -14,7 +19,7 @@ export class BitbucketServerHttp extends Http { protected override request( path: string, - options?: InternalHttpOptions + options?: InternalHttpOptions & HttpRequestOptions ): Promise> { const url = resolveBaseUrl(baseUrl, path); const opts = { diff --git a/lib/util/http/bitbucket.ts b/lib/util/http/bitbucket.ts index 15bea99b04860a..e836cded3838e2 100644 --- a/lib/util/http/bitbucket.ts +++ b/lib/util/http/bitbucket.ts @@ -1,7 +1,7 @@ import is from '@sindresorhus/is'; import type { PagedResult } from '../../modules/platform/bitbucket/types'; import { parseUrl, resolveBaseUrl } from '../url'; -import type { HttpOptions, HttpResponse } from './types'; +import type { HttpOptions, HttpRequestOptions, HttpResponse } from './types'; import { Http } from '.'; let baseUrl = 'https://api.bitbucket.org/'; @@ -21,13 +21,15 @@ export class BitbucketHttp extends Http { protected override async request( path: string, - options?: BitbucketHttpOptions + options?: BitbucketHttpOptions & HttpRequestOptions ): Promise> { const opts = { baseUrl, ...options }; const result = await super.request(path, opts); if (opts.paginate && isPagedResult(result.body)) { + delete opts.etagCache; + const resultBody = result.body as PagedResult; let nextPage = getPageFromURL(resultBody.next); @@ -42,7 +44,7 @@ export class BitbucketHttp extends Http { const nextResult = await super.request>( nextPath, - options + options as BitbucketHttpOptions ); resultBody.values.push(...nextResult.body.values); diff --git a/lib/util/http/gitea.ts b/lib/util/http/gitea.ts index 394f3589e4b0c4..32e2ea27d7aed1 100644 --- a/lib/util/http/gitea.ts +++ b/lib/util/http/gitea.ts @@ -1,6 +1,11 @@ import is from '@sindresorhus/is'; import { resolveBaseUrl } from '../url'; -import type { HttpOptions, HttpResponse, InternalHttpOptions } from './types'; +import type { + HttpOptions, + HttpRequestOptions, + HttpResponse, + InternalHttpOptions, +} from './types'; import { Http } from '.'; let baseUrl: string; @@ -36,7 +41,7 @@ export class GiteaHttp extends Http { protected override async request( path: string, - options?: InternalHttpOptions & GiteaHttpOptions + options?: InternalHttpOptions & GiteaHttpOptions & HttpRequestOptions ): Promise> { const resolvedUrl = resolveUrl(path, options?.baseUrl ?? baseUrl); const opts = { diff --git a/lib/util/http/github.ts b/lib/util/http/github.ts index 9dfb182d673673..440a042d3cc122 100644 --- a/lib/util/http/github.ts +++ b/lib/util/http/github.ts @@ -19,6 +19,7 @@ import type { GotLegacyError } from './legacy'; import type { GraphqlOptions, HttpOptions, + HttpRequestOptions, HttpResponse, InternalHttpOptions, } from './types'; @@ -271,7 +272,7 @@ export class GithubHttp extends Http { protected override async request( url: string | URL, - options?: InternalHttpOptions & GithubHttpOptions, + options?: InternalHttpOptions & GithubHttpOptions & HttpRequestOptions, okToRetry = true ): Promise> { const opts: GithubHttpOptions = { diff --git a/lib/util/http/gitlab.ts b/lib/util/http/gitlab.ts index 6a72c31cd8f7b6..9078bbeee0c08e 100644 --- a/lib/util/http/gitlab.ts +++ b/lib/util/http/gitlab.ts @@ -2,7 +2,12 @@ import is from '@sindresorhus/is'; import { logger } from '../../logger'; import { ExternalHostError } from '../../types/errors/external-host-error'; import { parseLinkHeader, parseUrl } from '../url'; -import type { HttpOptions, HttpResponse, InternalHttpOptions } from './types'; +import type { + HttpOptions, + HttpRequestOptions, + HttpResponse, + InternalHttpOptions, +} from './types'; import { Http } from '.'; let baseUrl = 'https://gitlab.com/api/v4/'; @@ -21,7 +26,7 @@ export class GitlabHttp extends Http { protected override async request( url: string | URL, - options?: InternalHttpOptions & GitlabHttpOptions + options?: InternalHttpOptions & GitlabHttpOptions & HttpRequestOptions ): Promise> { const opts = { baseUrl, diff --git a/lib/util/http/index.spec.ts b/lib/util/http/index.spec.ts index 9563f2995654d0..76a591e3bca2a2 100644 --- a/lib/util/http/index.spec.ts +++ b/lib/util/http/index.spec.ts @@ -425,4 +425,58 @@ describe('util/http/index', () => { expect(t2 - t1).toBeGreaterThanOrEqual(4000); }); }); + + describe('Etag caching', () => { + beforeEach(() => { + jest.resetAllMocks(); + memCache.init(); + }); + + afterEach(() => { + memCache.reset(); + }); + + type FooBar = { foo: string; bar: string }; + + it('returns cached data for status=304', async () => { + const data: FooBar = { foo: 'foo', bar: 'bar' }; + httpMock + .scope(baseUrl, { reqheaders: { 'If-None-Match': 'foobar' } }) + .get('/foo') + .reply(304); + + const res = await http.getJson(`/foo`, { + baseUrl, + etagCache: { + etag: 'foobar', + data, + }, + }); + + expect(res.statusCode).toBe(304); + expect(res.body).toEqual(data); + expect(res.body).not.toBe(data); + }); + + it('returns new data for status=200', async () => { + const oldData: FooBar = { foo: 'foo', bar: 'bar' }; + const newData: FooBar = { foo: 'FOO', bar: 'BAR' }; + httpMock + .scope(baseUrl, { reqheaders: { 'If-None-Match': 'foobar' } }) + .get('/foo') + .reply(200, newData); + + const res = await http.getJson(`/foo`, { + baseUrl, + etagCache: { + etag: 'foobar', + data: oldData, + }, + }); + + expect(res.statusCode).toBe(200); + expect(res.body).toEqual(newData); + expect(res.body).not.toBe(newData); + }); + }); }); diff --git a/lib/util/http/index.ts b/lib/util/http/index.ts index 20bca08a33bf96..53e51b746dcde8 100644 --- a/lib/util/http/index.ts +++ b/lib/util/http/index.ts @@ -18,6 +18,7 @@ import type { GotJSONOptions, GotOptions, HttpOptions, + HttpRequestOptions, HttpResponse, InternalHttpOptions, RequestStats, @@ -28,7 +29,7 @@ import './legacy'; export { RequestError as HttpError }; type JsonArgs< - Opts extends HttpOptions, + Opts extends HttpOptions & HttpRequestOptions, ResT = unknown, Schema extends ZodType = ZodType > = { @@ -120,7 +121,7 @@ export class Http { protected async request( requestUrl: string | URL, - httpOptions: InternalHttpOptions = {} + httpOptions: InternalHttpOptions & HttpRequestOptions = {} ): Promise> { let url = requestUrl.toString(); if (httpOptions?.baseUrl) { @@ -136,6 +137,17 @@ export class Http { httpOptions ); + const etagCache = + httpOptions.etagCache && options.method === 'get' + ? httpOptions.etagCache + : null; + if (etagCache) { + options.headers = { + ...options.headers, + 'If-None-Match': etagCache.etag, + }; + } + if (process.env.NODE_ENV === 'test') { options.retry = 0; } @@ -203,9 +215,14 @@ export class Http { } try { - const res = await resPromise; + const res = memCacheKey + ? cloneResponse(await resPromise) + : await resPromise; res.authorization = !!options?.headers?.authorization; - return cloneResponse(res); + if (etagCache && res.statusCode === 304) { + res.body = clone(etagCache.data); + } + return res; } catch (err) { const { abortOnError, abortIgnoreStatusCodes } = options; if (abortOnError && !abortIgnoreStatusCodes?.includes(err.statusCode)) { @@ -215,7 +232,10 @@ export class Http { } } - get(url: string, options: HttpOptions = {}): Promise { + get( + url: string, + options: HttpOptions & HttpRequestOptions = {} + ): Promise { return this.request(url, options); } @@ -281,19 +301,22 @@ export class Http { return res; } - getJson(url: string, options?: Opts): Promise>; + getJson( + url: string, + options?: Opts & HttpRequestOptions + ): Promise>; getJson = ZodType>( url: string, schema: Schema ): Promise>>; getJson = ZodType>( url: string, - options: Opts, + options: Opts & HttpRequestOptions, schema: Schema ): Promise>>; getJson = ZodType>( arg1: string, - arg2?: Opts | Schema, + arg2?: (Opts & HttpRequestOptions) | Schema, arg3?: Schema ): Promise> { const args = this.resolveArgs(arg1, arg2, arg3); diff --git a/lib/util/http/jira.ts b/lib/util/http/jira.ts index df51c152a47c08..1137cc33cce8eb 100644 --- a/lib/util/http/jira.ts +++ b/lib/util/http/jira.ts @@ -1,4 +1,9 @@ -import type { HttpOptions, HttpResponse, InternalHttpOptions } from './types'; +import type { + HttpOptions, + HttpRequestOptions, + HttpResponse, + InternalHttpOptions, +} from './types'; import { Http } from '.'; let baseUrl: string; @@ -14,7 +19,7 @@ export class JiraHttp extends Http { protected override request( url: string | URL, - options?: InternalHttpOptions + options?: InternalHttpOptions & HttpRequestOptions ): Promise> { const opts = { baseUrl, ...options }; return super.request(url, opts); diff --git a/lib/util/http/types.ts b/lib/util/http/types.ts index 4b232982585ea2..861959a9ac122e 100644 --- a/lib/util/http/types.ts +++ b/lib/util/http/types.ts @@ -65,6 +65,15 @@ export interface HttpOptions { memCache?: boolean; } +export interface EtagCache { + etag: string; + data: T; +} + +export interface HttpRequestOptions { + etagCache?: EtagCache; +} + export interface InternalHttpOptions extends HttpOptions { json?: HttpOptions['body']; responseType?: 'json' | 'buffer'; From 724a735bc293d9cffeb4b803ef1414632ecd2bbc Mon Sep 17 00:00:00 2001 From: Sergei Zharinov Date: Thu, 18 May 2023 19:20:56 +0300 Subject: [PATCH 2/7] Fix schema signature --- lib/util/http/index.spec.ts | 23 +++++++++++++++++++++++ lib/util/http/index.ts | 2 +- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/lib/util/http/index.spec.ts b/lib/util/http/index.spec.ts index 76a591e3bca2a2..4c3032cf796c1d 100644 --- a/lib/util/http/index.spec.ts +++ b/lib/util/http/index.spec.ts @@ -478,5 +478,28 @@ describe('util/http/index', () => { expect(res.body).toEqual(newData); expect(res.body).not.toBe(newData); }); + + it('throws if old data does not match the schema', async () => { + const FooBar = z.object({ foo: z.string(), bar: z.string() }); + const data = { foo: 'foo' }; + httpMock + .scope(baseUrl, { reqheaders: { 'If-None-Match': 'foobar' } }) + .get('/foo') + .reply(304); + + await expect( + http.getJson( + `/foo`, + { + baseUrl, + etagCache: { + etag: 'foobar', + data: data as never, + }, + }, + FooBar + ) + ).rejects.toThrow(z.ZodError); + }); }); }); diff --git a/lib/util/http/index.ts b/lib/util/http/index.ts index 53e51b746dcde8..a79bbcd8935d6b 100644 --- a/lib/util/http/index.ts +++ b/lib/util/http/index.ts @@ -311,7 +311,7 @@ export class Http { ): Promise>>; getJson = ZodType>( url: string, - options: Opts & HttpRequestOptions, + options: Opts & HttpRequestOptions>, schema: Schema ): Promise>>; getJson = ZodType>( From c85a78ccb0d8c467fab4e08cbc44da55a7fd37d0 Mon Sep 17 00:00:00 2001 From: Sergei Zharinov Date: Thu, 18 May 2023 20:45:06 +0300 Subject: [PATCH 3/7] Fix --- lib/util/http/index.spec.ts | 65 ++++++++++++++++++------------------- lib/util/http/index.ts | 55 +++++++++++++++++++++---------- 2 files changed, 68 insertions(+), 52 deletions(-) diff --git a/lib/util/http/index.spec.ts b/lib/util/http/index.spec.ts index 4c3032cf796c1d..7e8668154cf204 100644 --- a/lib/util/http/index.spec.ts +++ b/lib/util/http/index.spec.ts @@ -427,18 +427,8 @@ describe('util/http/index', () => { }); describe('Etag caching', () => { - beforeEach(() => { - jest.resetAllMocks(); - memCache.init(); - }); - - afterEach(() => { - memCache.reset(); - }); - - type FooBar = { foo: string; bar: string }; - it('returns cached data for status=304', async () => { + type FooBar = { foo: string; bar: string }; const data: FooBar = { foo: 'foo', bar: 'bar' }; httpMock .scope(baseUrl, { reqheaders: { 'If-None-Match': 'foobar' } }) @@ -458,7 +448,37 @@ describe('util/http/index', () => { expect(res.body).not.toBe(data); }); + it('bypasses schema parsing', async () => { + const FooBar = z + .object({ foo: z.string(), bar: z.string() }) + .transform(({ foo, bar }) => ({ + foobar: `${foo}${bar}`.toUpperCase(), + })); + const data = FooBar.parse({ foo: 'foo', bar: 'bar' }); + httpMock + .scope(baseUrl, { reqheaders: { 'If-None-Match': 'foobar' } }) + .get('/foo') + .reply(304); + + const res = await http.getJson( + `/foo`, + { + baseUrl, + etagCache: { + etag: 'foobar', + data, + }, + }, + FooBar + ); + + expect(res.statusCode).toBe(304); + expect(res.body).toEqual(data); + expect(res.body).not.toBe(data); + }); + it('returns new data for status=200', async () => { + type FooBar = { foo: string; bar: string }; const oldData: FooBar = { foo: 'foo', bar: 'bar' }; const newData: FooBar = { foo: 'FOO', bar: 'BAR' }; httpMock @@ -478,28 +498,5 @@ describe('util/http/index', () => { expect(res.body).toEqual(newData); expect(res.body).not.toBe(newData); }); - - it('throws if old data does not match the schema', async () => { - const FooBar = z.object({ foo: z.string(), bar: z.string() }); - const data = { foo: 'foo' }; - httpMock - .scope(baseUrl, { reqheaders: { 'If-None-Match': 'foobar' } }) - .get('/foo') - .reply(304); - - await expect( - http.getJson( - `/foo`, - { - baseUrl, - etagCache: { - etag: 'foobar', - data: data as never, - }, - }, - FooBar - ) - ).rejects.toThrow(z.ZodError); - }); }); }); diff --git a/lib/util/http/index.ts b/lib/util/http/index.ts index a79bbcd8935d6b..301eecb6f7225e 100644 --- a/lib/util/http/index.ts +++ b/lib/util/http/index.ts @@ -40,18 +40,26 @@ type JsonArgs< type Task = () => Promise>; -function cloneResponse( - response: HttpResponse +// Copying will help to avoid circular structure +// and mutation of the cached response. +function copyResponse( + response: HttpResponse, + deep: boolean ): HttpResponse { const { body, statusCode, headers } = response; - // clone body and headers so that the cached result doesn't get accidentally mutated - // Don't use json clone for buffers - return { - statusCode, - body: body instanceof Buffer ? (body.slice() as T) : clone(body), - headers: clone(headers), - authorization: !!response.authorization, - }; + return deep + ? { + statusCode, + body: body instanceof Buffer ? (body.slice() as T) : clone(body), + headers: clone(headers), + authorization: !!response.authorization, + } + : { + statusCode, + body, + headers, + authorization: !!response.authorization, + }; } function applyDefaultHeaders(options: Options): void { @@ -215,13 +223,8 @@ export class Http { } try { - const res = memCacheKey - ? cloneResponse(await resPromise) - : await resPromise; + const res = copyResponse(await resPromise, !!memCacheKey); res.authorization = !!options?.headers?.authorization; - if (etagCache && res.statusCode === 304) { - res.body = clone(etagCache.data); - } return res; } catch (err) { const { abortOnError, abortIgnoreStatusCodes } = options; @@ -255,7 +258,11 @@ export class Http { private async requestJson( method: InternalHttpOptions['method'], - { url, httpOptions: requestOptions, schema }: JsonArgs + { + url, + httpOptions: requestOptions, + schema, + }: JsonArgs, ResT> ): Promise> { const { body, ...httpOptions } = { ...requestOptions }; const opts: InternalHttpOptions = { @@ -273,11 +280,23 @@ export class Http { } const res = await this.request(url, opts); + const etagCacheHit = + httpOptions.etagCache && res.statusCode === 304 + ? clone(httpOptions.etagCache.data) + : null; + if (!schema) { + if (etagCacheHit) { + res.body = etagCacheHit; + } return res; } - res.body = await schema.parseAsync(res.body); + if (etagCacheHit) { + res.body = etagCacheHit; + } else { + res.body = await schema.parseAsync(res.body); + } return res; } From 7bf66d984feeb9c5484e9687958385863eaafadb Mon Sep 17 00:00:00 2001 From: Sergei Zharinov Date: Thu, 18 May 2023 20:50:02 +0300 Subject: [PATCH 4/7] Fix --- lib/util/http/index.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/util/http/index.ts b/lib/util/http/index.ts index 301eecb6f7225e..16c5a303f49947 100644 --- a/lib/util/http/index.ts +++ b/lib/util/http/index.ts @@ -146,7 +146,8 @@ export class Http { ); const etagCache = - httpOptions.etagCache && options.method === 'get' + httpOptions.etagCache && + (options.method === 'get' || options.method === 'head') ? httpOptions.etagCache : null; if (etagCache) { From b8730c00dc25e6e599262848e7a55b22e9f59df6 Mon Sep 17 00:00:00 2001 From: Sergei Zharinov Date: Fri, 19 May 2023 11:08:37 +0300 Subject: [PATCH 5/7] Fix --- lib/util/http/index.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/util/http/index.ts b/lib/util/http/index.ts index 16c5a303f49947..fd60a1082112b5 100644 --- a/lib/util/http/index.ts +++ b/lib/util/http/index.ts @@ -52,13 +52,11 @@ function copyResponse( statusCode, body: body instanceof Buffer ? (body.slice() as T) : clone(body), headers: clone(headers), - authorization: !!response.authorization, } : { statusCode, body, headers, - authorization: !!response.authorization, }; } From ea957a2848d2ed14bd2909c66ec5ad5e5af2746a Mon Sep 17 00:00:00 2001 From: Sergei Zharinov Date: Fri, 19 May 2023 11:13:52 +0300 Subject: [PATCH 6/7] Don't deep clone for 304 --- lib/util/http/index.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/util/http/index.ts b/lib/util/http/index.ts index fd60a1082112b5..f435c274609190 100644 --- a/lib/util/http/index.ts +++ b/lib/util/http/index.ts @@ -222,9 +222,11 @@ export class Http { } try { - const res = copyResponse(await resPromise, !!memCacheKey); - res.authorization = !!options?.headers?.authorization; - return res; + const res = await resPromise; + const deepCopyNeeded = !!memCacheKey && res.statusCode !== 304; + const resCopy = copyResponse(res, deepCopyNeeded); + resCopy.authorization = !!options?.headers?.authorization; + return resCopy; } catch (err) { const { abortOnError, abortIgnoreStatusCodes } = options; if (abortOnError && !abortIgnoreStatusCodes?.includes(err.statusCode)) { From 3ac66eb271fb0853acdff9e8bb01a9efbad7f9cc Mon Sep 17 00:00:00 2001 From: Sergei Zharinov Date: Thu, 25 May 2023 14:40:37 +0300 Subject: [PATCH 7/7] Remove deletion of `cacheTag` for paginated result --- lib/util/http/bitbucket.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/util/http/bitbucket.ts b/lib/util/http/bitbucket.ts index 2638a1b80afe53..34c5166ca8cb7a 100644 --- a/lib/util/http/bitbucket.ts +++ b/lib/util/http/bitbucket.ts @@ -46,8 +46,6 @@ export class BitbucketHttp extends Http { const result = await super.request(resolvedURL.toString(), opts); if (opts.paginate && isPagedResult(result.body)) { - delete opts.etagCache; // Etag cache isn't designed for paginated requests - const resultBody = result.body as PagedResult; let page = 1; let nextURL = resultBody.next;