Prevent Cache-Control header from being set if response has errors (#…

…2715) * Prevent Cache-Control header from being set if response has errors * Improve Cache-Control tests * Remove code duplication in Cache-Control tests * Use guard clause for Cache-Control checks * Update CHANGELOG.md
apollographql · May 24, 2019 · d1a0d16 · d1a0d16
1 parent 0aaf421
commit d1a0d16
Show file tree

Hide file tree

Showing 3 changed files with 68 additions and 12 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,7 @@
 - `apollo-server`: Support `onHealthCheck` in the `ApolloServer` constructor in the same way as `cors` is supported.  This contrasts with the `-express`, `-hapi`, etc. variations which accept this parameter via their `applyMiddleware` methods and will remain as-is.  [PR #2672](https://github.com/apollographql/apollo-server/pull/2672)
 - core: Expose SHA-256 hex hash digest of the Engine API key to plugins, when available, as `engine.apiKeyHash`. [PR# 2685](https://github.com/apollographql/apollo-server/pull/2685)
 - `apollo-datasource-rest`: If another `Content-type` is already set on the response, don't overwrite it with `application/json`, allowing the user's initial `Content-type` to prevail. [PR #2520](https://github.com/apollographql/apollo-server/issues/2035)
+- `apollo-cache-control`: Do not respond with `Cache-control` headers if the HTTP response contains `errors`. [PR #2715](https://github.com/apollographql/apollo-server/pull/2715)
 
 ### v2.5.0
 

diff --git a/packages/apollo-cache-control/src/__tests__/cacheControlExtension.test.ts b/packages/apollo-cache-control/src/__tests__/cacheControlExtension.test.ts
@@ -1,4 +1,6 @@
-import { ResponsePath } from 'graphql';
+import { ResponsePath, GraphQLError } from 'graphql';
+import { GraphQLResponse } from 'graphql-extensions';
+import { Headers } from 'apollo-server-env';
 import { CacheControlExtension, CacheScope } from '../';
 
 describe('CacheControlExtension', () => {
@@ -8,6 +10,53 @@ describe('CacheControlExtension', () => {
     cacheControlExtension = new CacheControlExtension();
   });
 
+  describe('willSendResponse', () => {
+    let graphqlResponse: GraphQLResponse;
+
+    beforeEach(() => {
+      cacheControlExtension.options.calculateHttpHeaders = true;
+      cacheControlExtension.computeOverallCachePolicy = () => ({
+        maxAge: 300,
+        scope: CacheScope.Public,
+      });
+      graphqlResponse = {
+        http: {
+          headers: new Headers(),
+        },
+        data: { test: 'test' },
+      };
+    });
+
+    it('sets cache-control header', () => {
+      cacheControlExtension.willSendResponse &&
+        cacheControlExtension.willSendResponse({ graphqlResponse });
+      expect(graphqlResponse.http!.headers.get('Cache-Control')).toBe(
+        'max-age=300, public',
+      );
+    });
+
+    const shouldNotSetCacheControlHeader = () => {
+      cacheControlExtension.willSendResponse &&
+        cacheControlExtension.willSendResponse({ graphqlResponse });
+      expect(graphqlResponse.http!.headers.get('Cache-Control')).toBeNull();
+    };
+
+    it('does not set cache-control header if calculateHttpHeaders is set to false', () => {
+      cacheControlExtension.options.calculateHttpHeaders = false;
+      shouldNotSetCacheControlHeader();
+    });
+
+    it('does not set cache-control header if graphqlResponse has errors', () => {
+      graphqlResponse.errors = [new GraphQLError('Test Error')];
+      shouldNotSetCacheControlHeader();
+    });
+
+    it('does not set cache-control header if there is no overall cache policy', () => {
+      cacheControlExtension.computeOverallCachePolicy = () => undefined;
+      shouldNotSetCacheControlHeader();
+    });
+  });
+
   describe('computeOverallCachePolicy', () => {
     const responsePath: ResponsePath = {
       key: 'test',

diff --git a/packages/apollo-cache-control/src/index.ts b/packages/apollo-cache-control/src/index.ts
@@ -153,17 +153,23 @@ export class CacheControlExtension<TContext = any>
   }
 
   public willSendResponse?(o: { graphqlResponse: GraphQLResponse }) {
-    if (this.options.calculateHttpHeaders && o.graphqlResponse.http) {
-      const overallCachePolicy = this.computeOverallCachePolicy();
-
-      if (overallCachePolicy) {
-        o.graphqlResponse.http.headers.set(
-          'Cache-Control',
-          `max-age=${
-            overallCachePolicy.maxAge
-          }, ${overallCachePolicy.scope.toLowerCase()}`,
-        );
-      }
+    if (
+      !this.options.calculateHttpHeaders ||
+      !o.graphqlResponse.http ||
+      o.graphqlResponse.errors
+    ) {
+      return;
+    }
+
+    const overallCachePolicy = this.computeOverallCachePolicy();
+
+    if (overallCachePolicy) {
+      o.graphqlResponse.http.headers.set(
+        'Cache-Control',
+        `max-age=${
+          overallCachePolicy.maxAge
+        }, ${overallCachePolicy.scope.toLowerCase()}`,
+      );
     }
   }