Remove memoized results when performing non-GET requests

Closes #1408.

packages/apollo-datasource-rest/src/RESTDataSource.ts

@@ -248,24 +248,18 @@ export abstract class RESTDataSource<TContext = any> extends DataSource {
     };
 
     if (request.method === 'GET') {
-      return this.memoize(cacheKey, performRequest);
+      let promise = this.memoizedResults.get(cacheKey);
+      if (promise) return promise;
+
+      promise = performRequest();
+      this.memoizedResults.set(cacheKey, promise);
+      return promise;
     } else {
+      this.memoizedResults.delete(cacheKey);
       return performRequest();
     }
   }
 
-  private async memoize<TResult>(
-    cacheKey: string,
-    fn: () => Promise<TResult>,
-  ): Promise<TResult> {
-    let promise = this.memoizedResults.get(cacheKey);
-    if (promise) return promise;
-
-    promise = fn();
-    this.memoizedResults.set(cacheKey, promise);
-    return promise;
-  }
-
   private async trace<TResult>(
     label: string,
     fn: () => Promise<TResult>,

packages/apollo-datasource-rest/src/__tests__/RESTDataSource.test.ts

@@ -434,8 +434,8 @@ describe('RESTDataSource', () => {
       const dataSource = new class extends RESTDataSource {
         baseURL = 'https://api.example.com';
 
-        getFoo(a: number) {
-          return this.get('foo', { a });
+        getFoo(id: number) {
+          return this.get(`foo/${id}`);
         }
       }();
 
@@ -447,7 +447,7 @@ describe('RESTDataSource', () => {
 
       expect(fetch.mock.calls.length).toEqual(1);
       expect(fetch.mock.calls[0][0].url).toEqual(
-        'https://api.example.com/foo?a=1',
+        'https://api.example.com/foo/1',
       );
     });
 
@@ -455,8 +455,8 @@ describe('RESTDataSource', () => {
       const dataSource = new class extends RESTDataSource {
         baseURL = 'https://api.example.com';
 
-        getFoo(a: number) {
-          return this.get('foo', { a });
+        getFoo(id: number) {
+          return this.get(`foo/${id}`);
         }
       }();
 
@@ -469,10 +469,10 @@ describe('RESTDataSource', () => {
 
       expect(fetch.mock.calls.length).toEqual(2);
       expect(fetch.mock.calls[0][0].url).toEqual(
-        'https://api.example.com/foo?a=1',
+        'https://api.example.com/foo/1',
       );
       expect(fetch.mock.calls[1][0].url).toEqual(
-        'https://api.example.com/foo?a=2',
+        'https://api.example.com/foo/2',
       );
     });
 
@@ -480,8 +480,8 @@ describe('RESTDataSource', () => {
       const dataSource = new class extends RESTDataSource {
         baseURL = 'https://api.example.com';
 
-        postFoo(a: number) {
-          return this.post('foo', { a });
+        postFoo(id: number) {
+          return this.post(`foo/${id}`);
         }
       }();
 
@@ -495,6 +495,40 @@ describe('RESTDataSource', () => {
       expect(fetch.mock.calls.length).toEqual(2);
     });
 
+    it('non-GET request removes memoized request with the same cache key', async () => {
+      const dataSource = new class extends RESTDataSource {
+        baseURL = 'https://api.example.com';
+
+        getFoo(id: number) {
+          return this.get(`foo/${id}`);
+        }
+
+        postFoo(id: number) {
+          return this.post(`foo/${id}`);
+        }
+      }();
+
+      dataSource.httpCache = httpCache;
+
+      fetch.mockJSONResponseOnce();
+      fetch.mockJSONResponseOnce();
+      fetch.mockJSONResponseOnce();
+
+      await Promise.all([
+        dataSource.getFoo(1),
+        dataSource.postFoo(1),
+        dataSource.getFoo(1),
+      ]);
+
+      expect(fetch.mock.calls.length).toEqual(3);
+      expect(fetch.mock.calls[0][0].url).toEqual(
+        'https://api.example.com/foo/1',
+      );
+      expect(fetch.mock.calls[2][0].url).toEqual(
+        'https://api.example.com/foo/1',
+      );
+    });
+
     it('allows specifying a custom cache key', async () => {
       const dataSource = new class extends RESTDataSource {
         baseURL = 'https://api.example.com';
@@ -505,8 +539,8 @@ describe('RESTDataSource', () => {
           return url.toString();
         }
 
-        getFoo(a: number) {
-          return this.get('foo', { a });
+        getFoo(id: number, apiKey: string) {
+          return this.get(`foo/${id}`, { api_key: apiKey });
         }
       }();
 
@@ -514,11 +548,14 @@ describe('RESTDataSource', () => {
 
       fetch.mockJSONResponseOnce();
 
-      await Promise.all([dataSource.getFoo(1), dataSource.getFoo(2)]);
+      await Promise.all([
+        dataSource.getFoo(1, 'secret'),
+        dataSource.getFoo(1, 'anotherSecret'),
+      ]);
 
       expect(fetch.mock.calls.length).toEqual(1);
       expect(fetch.mock.calls[0][0].url).toEqual(
-        'https://api.example.com/foo?a=1',
+        'https://api.example.com/foo/1?api_key=secret',
       );
     });
   });