From 609ba1fc03fd1581176d41a1f666436db37acd0c Mon Sep 17 00:00:00 2001 From: David Glasser Date: Tue, 13 Dec 2022 22:27:39 -0800 Subject: [PATCH] Clone deduplicated response bodies Previously, the deduplication logic shared the response body across both responses; if you mutate that response, it will affect other deduplicated responses. Now we clone the body. If you override `parseBody` to return new data types, you should override `cloneParsedBody` to properly clone them. There's probably an optimization that can be done to skip the cloning if there was no duplicate operation (and it's in `deduplicate-during-request-lifetime` mode). Fixes #18. --- .changeset/dry-ties-relax.md | 5 ++++ src/RESTDataSource.ts | 44 +++++++++++++++++++++++----- src/__tests__/RESTDataSource.test.ts | 19 ++++++++++++ 3 files changed, 60 insertions(+), 8 deletions(-) create mode 100644 .changeset/dry-ties-relax.md diff --git a/.changeset/dry-ties-relax.md b/.changeset/dry-ties-relax.md new file mode 100644 index 0000000..5bf1625 --- /dev/null +++ b/.changeset/dry-ties-relax.md @@ -0,0 +1,5 @@ +--- +'@apollo/datasource-rest': patch +--- + +When de-duplicating requests, the returned parsed body is now cloned rather than shared across duplicate requests. If you override the `parseBody` method, you should also override `cloneParsedBody` to match. diff --git a/src/RESTDataSource.ts b/src/RESTDataSource.ts index 172b2ab..6c1f233 100644 --- a/src/RESTDataSource.ts +++ b/src/RESTDataSource.ts @@ -104,6 +104,12 @@ export interface DataSourceConfig { cache?: KeyValueCache; fetch?: Fetcher; } +export interface DataSourceFetchResult { + parsedBody: TResult; + response: FetcherResponse; + // This is primarily returned so that tests can be deterministic. + cacheWritePromise: Promise | undefined; +} // RESTDataSource has two layers of caching. The first layer is purely in-memory // within a single RESTDataSource object and is called "request deduplication". @@ -229,6 +235,9 @@ export abstract class RESTDataSource { // method. It's important that the body always read in full (otherwise the // clone of this response that is being read to write to the HTTPCache could // block and lead to a memory leak). + // + // If you override this to return interesting new mutable data types, override + // cloneParsedBody too. protected parseBody(response: FetcherResponse): Promise { const contentType = response.headers.get('Content-Type'); const contentLength = response.headers.get('Content-Length'); @@ -247,6 +256,23 @@ export abstract class RESTDataSource { } } + private cloneDataSourceFetchResult( + dataSourceFetchResult: DataSourceFetchResult, + ): DataSourceFetchResult { + return { + ...dataSourceFetchResult, + parsedBody: this.cloneParsedBody(dataSourceFetchResult.parsedBody), + }; + } + + protected cloneParsedBody(parsedBody: TResult) { + if (typeof parsedBody === 'string') { + return parsedBody; + } else { + return JSON.parse(JSON.stringify(parsedBody)); + } + } + protected shouldJSONSerializeBody(body: RequestWithBody['body']): boolean { return !!( // We accept arbitrary objects and arrays as body and serialize them as JSON. @@ -388,12 +414,7 @@ export abstract class RESTDataSource { public async fetch( path: string, incomingRequest: DataSourceRequest, - ): Promise<{ - parsedBody: TResult; - response: FetcherResponse; - // This is primarily returned so that tests can be deterministic. - cacheWritePromise: Promise | undefined; - }> { + ): Promise> { const augmentedRequest: AugmentedRequest = { ...incomingRequest, // guarantee params and headers objects before calling `willSendRequest` for convenience @@ -488,7 +509,10 @@ export abstract class RESTDataSource { const previousRequestPromise = this.deduplicationPromises.get( policy.deduplicationKey, ); - if (previousRequestPromise) return previousRequestPromise; + if (previousRequestPromise) + return previousRequestPromise.then((result) => + this.cloneDataSourceFetchResult(result), + ); const thisRequestPromise = performRequest(); this.deduplicationPromises.set( @@ -501,7 +525,11 @@ export abstract class RESTDataSource { // from the cache. Additionally, the use of finally here guarantees the // deduplication cache is cleared in the event of an error during the // request. - return await thisRequestPromise; + // + // Note: we could try to get fancy and only clone if no de-duplication + // happened (and we're "deduplicate-during-request-lifetime") but we + // haven't quite bothered yet. + return this.cloneDataSourceFetchResult(await thisRequestPromise); } finally { if (policy.policy === 'deduplicate-during-request-lifetime') { this.deduplicationPromises.delete(policy.deduplicationKey); diff --git a/src/__tests__/RESTDataSource.test.ts b/src/__tests__/RESTDataSource.test.ts index 03f61ae..faece90 100644 --- a/src/__tests__/RESTDataSource.test.ts +++ b/src/__tests__/RESTDataSource.test.ts @@ -746,6 +746,25 @@ describe('RESTDataSource', () => { await Promise.all([dataSource.getFoo(1), dataSource.getFoo(2)]); }); + it('returns a copy of the results and not a reference in case of modification', async () => { + const dataSource = new (class extends RESTDataSource { + override baseURL = 'https://api.example.com'; + + async getFoo(id: number) { + let data = await this.get(`foo/${id}`); + data.foo.shift(); + expect(data.foo.length).toEqual(1); + return data; + } + })(); + + nock(apiUrl) + .get('/foo/1') + .reply(200, { foo: [{}, {}] }); + + await Promise.all([dataSource.getFoo(1), dataSource.getFoo(1)]); + }); + it('does not deduplicate non-GET requests by default', async () => { const dataSource = new (class extends RESTDataSource { override baseURL = 'https://api.example.com';