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

@@ -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.

src/RESTDataSource.ts

@@ -104,6 +104,12 @@ export interface DataSourceConfig {
   cache?: KeyValueCache;
   fetch?: Fetcher;
 }
+export interface DataSourceFetchResult<TResult> {
+  parsedBody: TResult;
+  response: FetcherResponse;
+  // This is primarily returned so that tests can be deterministic.
+  cacheWritePromise: Promise<void> | 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<object | string> {
     const contentType = response.headers.get('Content-Type');
     const contentLength = response.headers.get('Content-Length');
@@ -247,6 +256,23 @@ export abstract class RESTDataSource {
     }
   }
 
+  private cloneDataSourceFetchResult<TResult>(
+    dataSourceFetchResult: DataSourceFetchResult<TResult>,
+  ): DataSourceFetchResult<TResult> {
+    return {
+      ...dataSourceFetchResult,
+      parsedBody: this.cloneParsedBody(dataSourceFetchResult.parsedBody),
+    };
+  }
+
+  protected cloneParsedBody<TResult>(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<TResult>(
     path: string,
     incomingRequest: DataSourceRequest,
-  ): Promise<{
-    parsedBody: TResult;
-    response: FetcherResponse;
-    // This is primarily returned so that tests can be deterministic.
-    cacheWritePromise: Promise<void> | undefined;
-  }> {
+  ): Promise<DataSourceFetchResult<TResult>> {
     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);

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';