Fix response from cache metadata with tests (#381)

* Fix response from cache metadata with tests
apollographql · Dec 6, 2024 · 8a4d4ef · 8a4d4ef
1 parent 6cd274b
commit 8a4d4ef
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 32 deletions.
diff --git a/.changeset/fluffy-news-cry.md b/.changeset/fluffy-news-cry.md
@@ -0,0 +1,5 @@
+---
+'@apollo/datasource-rest': patch
+---
+
+Add responseFromCache to DataSourceFetchResult
diff --git a/package-lock.json b/package-lock.json
diff --git a/src/HTTPCache.ts b/src/HTTPCache.ts
@@ -29,7 +29,7 @@ interface SneakyCachePolicy extends CachePolicy {
 
 interface ResponseWithCacheWritePromise {
   response: FetcherResponse;
-  responseFromCache?: Boolean;
+  responseFromCache?: boolean;
   cacheWritePromise?: Promise<void>;
 }
 
@@ -247,6 +247,7 @@ export class HTTPCache<CO extends CacheOptions = CacheOptions> {
     const returnedResponse = response.clone();
     return {
       response: returnedResponse,
+      responseFromCache: false,
       cacheWritePromise: this.readResponseAndWriteToCache({
         response,
         policy,

diff --git a/src/RESTDataSource.ts b/src/RESTDataSource.ts
@@ -160,6 +160,7 @@ export interface DataSourceFetchResult<TResult> {
   response: FetcherResponse;
   requestDeduplication: RequestDeduplicationResult;
   httpCache: HTTPCacheResult;
+  responseFromCache?: boolean;
 }
 
 // RESTDataSource has two layers of caching. The first layer is purely in-memory
@@ -536,16 +537,13 @@ export abstract class RESTDataSource<CO extends CacheOptions = CacheOptions> {
           ? outgoingRequest.cacheOptions
           : this.cacheOptionsFor?.bind(this);
         try {
-          const { response, cacheWritePromise } = await this.httpCache.fetch(
-            url,
-            outgoingRequest,
-            {
+          const { response, responseFromCache, cacheWritePromise } =
+            await this.httpCache.fetch(url, outgoingRequest, {
               cacheKey,
               cacheOptions,
               httpCacheSemanticsCachePolicyOptions:
                 outgoingRequest.httpCacheSemanticsCachePolicyOptions,
-            },
-          );
+            });
 
           if (cacheWritePromise) {
             this.catchCacheWritePromiseErrors(cacheWritePromise);
@@ -566,6 +564,7 @@ export abstract class RESTDataSource<CO extends CacheOptions = CacheOptions> {
             httpCache: {
               cacheWritePromise,
             },
+            responseFromCache,
           };
         } catch (error) {
           this.didEncounterError(error as Error, outgoingRequest, url);

diff --git a/src/__tests__/RESTDataSource.test.ts b/src/__tests__/RESTDataSource.test.ts
@@ -1720,12 +1720,14 @@ describe('RESTDataSource', () => {
           'cache-control': 'public, max-age=31536000, immutable',
         });
         nock(apiUrl).get('/foo/2').reply(200);
-        const { httpCache } = await dataSource.getFoo(1);
-        expect(httpCache.cacheWritePromise).toBeDefined();
-        await httpCache.cacheWritePromise;
+        const firstResponse = await dataSource.getFoo(1);
+        expect(firstResponse.responseFromCache).toBeFalsy();
+        expect(firstResponse.httpCache.cacheWritePromise).toBeDefined();
+        await firstResponse.httpCache.cacheWritePromise;
 
         // Call a second time which should be cached
-        await dataSource.getFoo(1);
+        const secondResponse = await dataSource.getFoo(1);
+        expect(secondResponse.responseFromCache).toBe(true);
       });
 
       it('does not cache 302 responses', async () => {
@@ -1741,16 +1743,18 @@ describe('RESTDataSource', () => {
           'cache-control': 'public, max-age=31536000, immutable',
         });
         nock(apiUrl).get('/foo/2').reply(200);
-        const { httpCache } = await dataSource.getFoo(1);
-        expect(httpCache.cacheWritePromise).toBeUndefined();
+        const firstResponse = await dataSource.getFoo(1);
+        expect(firstResponse.responseFromCache).toBeFalsy();
+        expect(firstResponse.httpCache.cacheWritePromise).toBeUndefined();
 
         // Call a second time which should NOT be cached (it's a temporary redirect!).
         nock(apiUrl).get('/foo/1').reply(302, '', {
           location: 'https://api.example.com/foo/2',
           'cache-control': 'public, max-age=31536000, immutable',
         });
         nock(apiUrl).get('/foo/2').reply(200);
-        await dataSource.getFoo(1);
+        const secondResponse = await dataSource.getFoo(1);
+        expect(secondResponse.responseFromCache).toBeFalsy();
       });
 
       it('allows setting cache options for each request', async () => {
@@ -1773,12 +1777,14 @@ describe('RESTDataSource', () => {
         })();
 
         nock(apiUrl).get('/foo/1').reply(200);
-        const { httpCache } = await dataSource.getFoo(1);
-        expect(httpCache.cacheWritePromise).toBeDefined();
-        await httpCache.cacheWritePromise;
+        const firstResponse = await dataSource.getFoo(1);
+        expect(firstResponse.responseFromCache).toBeFalsy();
+        expect(firstResponse.httpCache.cacheWritePromise).toBeDefined();
+        await firstResponse.httpCache.cacheWritePromise;
 
         // Call a second time which should be cached
-        await dataSource.getFoo(1);
+        const secondResponse = await dataSource.getFoo(1);
+        expect(secondResponse.responseFromCache).toBe(true);
       });
 
       it('allows setting custom cache options for each request', async () => {
@@ -1800,16 +1806,17 @@ describe('RESTDataSource', () => {
         const spyOnHttpFetch = jest.spyOn(dataSource['httpCache'], 'fetch');
 
         nock(apiUrl).get('/foo/1').reply(200);
-        const { httpCache } = await dataSource.getFoo(1);
-        expect(httpCache.cacheWritePromise).toBeDefined();
-        await httpCache.cacheWritePromise;
+        const firstResponse = await dataSource.getFoo(1);
+        expect(firstResponse.httpCache.cacheWritePromise).toBeDefined();
+        await firstResponse.httpCache.cacheWritePromise;
         expect(spyOnHttpFetch.mock.calls[0][2]).toEqual({
           cacheKey: 'GET https://api.example.com/foo/1',
           cacheOptions: { ttl: 1000000, tags: ['foo', 'bar'] },
         });
 
         // Call a second time which should be cached
-        await dataSource.getFoo(1);
+        const secondResponse = await dataSource.getFoo(1);
+        expect(secondResponse.responseFromCache).toBe(true);
       });
 
       it('allows setting a short TTL for the cache', async () => {
@@ -1835,9 +1842,10 @@ describe('RESTDataSource', () => {
         })();
 
         nock(apiUrl).get('/foo/1').reply(200);
-        const { httpCache } = await dataSource.getFoo(1);
-        expect(httpCache.cacheWritePromise).toBeDefined();
-        await httpCache.cacheWritePromise;
+        const firstResponse = await dataSource.getFoo(1);
+        expect(firstResponse.responseFromCache).toBeFalsy();
+        expect(firstResponse.httpCache.cacheWritePromise).toBeDefined();
+        await firstResponse.httpCache.cacheWritePromise;
 
         // expire the cache (note: 999ms, just shy of the 1s ttl, will reliably fail this test)
         jest.advanceTimersByTime(1000);
@@ -1864,12 +1872,15 @@ describe('RESTDataSource', () => {
           'set-cookie':
             'whatever=blah; expires=Mon, 01-Jan-2050 00:00:00 GMT; path=/; domain=www.example.com',
         });
-        const { httpCache } = await dataSource.getFoo(1, false);
-        expect(httpCache.cacheWritePromise).toBeDefined();
-        await httpCache.cacheWritePromise;
+        const firstResponse = await dataSource.getFoo(1, false);
+        expect(firstResponse.responseFromCache).toBeFalsy();
+        expect(firstResponse.httpCache.cacheWritePromise).toBeDefined();
+        await firstResponse.httpCache.cacheWritePromise;
+
         // Call a second time which should be cached despite `set-cookie` due to
         // `shared: false`.
-        await dataSource.getFoo(1, false);
+        const secondResponse = await dataSource.getFoo(1, false);
+        expect(secondResponse.responseFromCache).toBe(true);
 
         nock(apiUrl).get('/foo/2').times(2).reply(200, '{}', {
           'Cache-Control': 'max-age=60,must-revalidate',
@@ -1883,7 +1894,8 @@ describe('RESTDataSource', () => {
         ).toBeUndefined();
         // Call a second time which should be not be cached because of
         // `set-cookie` with `shared: true`. (Note the `.times(2)` above.)
-        await dataSource.getFoo(2, true);
+        const cookieResponse = await dataSource.getFoo(2, true);
+        expect(cookieResponse.responseFromCache).toBeFalsy();
       });
 
       it('should not crash in revalidation flow header handling when sending non-array non-string headers', async () => {