From 20217eeea93937763dea1937924edd09ef5440af Mon Sep 17 00:00:00 2001 From: Fabian Schneider Date: Thu, 23 May 2019 16:23:58 +0200 Subject: [PATCH 1/5] Prevent Cache-Control header from being set if response has errors --- .../__tests__/cacheControlExtension.test.ts | 60 ++++++++++++++++++- packages/apollo-cache-control/src/index.ts | 21 ++++--- 2 files changed, 73 insertions(+), 8 deletions(-) diff --git a/packages/apollo-cache-control/src/__tests__/cacheControlExtension.test.ts b/packages/apollo-cache-control/src/__tests__/cacheControlExtension.test.ts index 127071f55e9..c0fd3b17d2b 100644 --- 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,62 @@ 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' }, + }; + jest.spyOn(graphqlResponse.http!.headers, 'set'); + }); + + it('sets cache-control header', () => { + cacheControlExtension.willSendResponse && + cacheControlExtension.willSendResponse({ graphqlResponse }); + expect(graphqlResponse.http!.headers.set).toHaveBeenCalled(); + }); + + it('does not set cache-control header if graphqlResponse has errors', () => { + graphqlResponse.errors = [new GraphQLError('Test Error')]; + cacheControlExtension.willSendResponse && + cacheControlExtension.willSendResponse({ graphqlResponse }); + expect(graphqlResponse.http!.headers.set).not.toHaveBeenCalled(); + }); + + it('does not set cache-control header if there is no data', () => { + graphqlResponse.data = undefined; + cacheControlExtension.willSendResponse && + cacheControlExtension.willSendResponse({ graphqlResponse }); + expect(graphqlResponse.http!.headers.set).not.toHaveBeenCalled(); + }); + + it('does not set cache-control header if there is no overall cache policy', () => { + cacheControlExtension.computeOverallCachePolicy = () => undefined; + cacheControlExtension.willSendResponse && + cacheControlExtension.willSendResponse({ graphqlResponse }); + expect(graphqlResponse.http!.headers.set).not.toHaveBeenCalled(); + }); + + it('does not set cache-control header if the overall cache policy has max-age set to 0', () => { + cacheControlExtension.computeOverallCachePolicy = () => ({ + maxAge: 0, + scope: CacheScope.Public, + }); + cacheControlExtension.willSendResponse && + cacheControlExtension.willSendResponse({ graphqlResponse }); + expect(graphqlResponse.http!.headers.set).not.toHaveBeenCalled(); + }); + }); + 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 index ff719f75d96..2b62239560d 100644 --- a/packages/apollo-cache-control/src/index.ts +++ b/packages/apollo-cache-control/src/index.ts @@ -156,14 +156,21 @@ export class CacheControlExtension 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 ( + o.graphqlResponse.errors || + !o.graphqlResponse.data || + !overallCachePolicy || + overallCachePolicy.maxAge <= 0 + ) { + return; } + + o.graphqlResponse.http.headers.set( + 'Cache-Control', + `max-age=${ + overallCachePolicy.maxAge + }, ${overallCachePolicy.scope.toLowerCase()}`, + ); } } From 03b052ab58fe144d464be57404ee546388b3c899 Mon Sep 17 00:00:00 2001 From: Fabian Schneider Date: Thu, 23 May 2019 16:53:46 +0200 Subject: [PATCH 2/5] Improve Cache-Control tests --- .../src/__tests__/cacheControlExtension.test.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/apollo-cache-control/src/__tests__/cacheControlExtension.test.ts b/packages/apollo-cache-control/src/__tests__/cacheControlExtension.test.ts index c0fd3b17d2b..ccd5ad38861 100644 --- a/packages/apollo-cache-control/src/__tests__/cacheControlExtension.test.ts +++ b/packages/apollo-cache-control/src/__tests__/cacheControlExtension.test.ts @@ -25,34 +25,35 @@ describe('CacheControlExtension', () => { }, data: { test: 'test' }, }; - jest.spyOn(graphqlResponse.http!.headers, 'set'); }); it('sets cache-control header', () => { cacheControlExtension.willSendResponse && cacheControlExtension.willSendResponse({ graphqlResponse }); - expect(graphqlResponse.http!.headers.set).toHaveBeenCalled(); + expect(graphqlResponse.http!.headers.get('Cache-Control')).toBe( + 'max-age=300, public', + ); }); it('does not set cache-control header if graphqlResponse has errors', () => { graphqlResponse.errors = [new GraphQLError('Test Error')]; cacheControlExtension.willSendResponse && cacheControlExtension.willSendResponse({ graphqlResponse }); - expect(graphqlResponse.http!.headers.set).not.toHaveBeenCalled(); + expect(graphqlResponse.http!.headers.get('Cache-Control')).toBeNull(); }); it('does not set cache-control header if there is no data', () => { graphqlResponse.data = undefined; cacheControlExtension.willSendResponse && cacheControlExtension.willSendResponse({ graphqlResponse }); - expect(graphqlResponse.http!.headers.set).not.toHaveBeenCalled(); + expect(graphqlResponse.http!.headers.get('Cache-Control')).toBeNull(); }); it('does not set cache-control header if there is no overall cache policy', () => { cacheControlExtension.computeOverallCachePolicy = () => undefined; cacheControlExtension.willSendResponse && cacheControlExtension.willSendResponse({ graphqlResponse }); - expect(graphqlResponse.http!.headers.set).not.toHaveBeenCalled(); + expect(graphqlResponse.http!.headers.get('Cache-Control')).toBeNull(); }); it('does not set cache-control header if the overall cache policy has max-age set to 0', () => { @@ -62,7 +63,7 @@ describe('CacheControlExtension', () => { }); cacheControlExtension.willSendResponse && cacheControlExtension.willSendResponse({ graphqlResponse }); - expect(graphqlResponse.http!.headers.set).not.toHaveBeenCalled(); + expect(graphqlResponse.http!.headers.get('Cache-Control')).toBeNull(); }); }); From 5f95c6f6df052ed2c5a583b8739a74f186e28102 Mon Sep 17 00:00:00 2001 From: Fabian Schneider Date: Thu, 23 May 2019 17:39:02 +0200 Subject: [PATCH 3/5] Remove code duplication in Cache-Control tests --- .../__tests__/cacheControlExtension.test.ts | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/packages/apollo-cache-control/src/__tests__/cacheControlExtension.test.ts b/packages/apollo-cache-control/src/__tests__/cacheControlExtension.test.ts index ccd5ad38861..30c1d3d37f8 100644 --- a/packages/apollo-cache-control/src/__tests__/cacheControlExtension.test.ts +++ b/packages/apollo-cache-control/src/__tests__/cacheControlExtension.test.ts @@ -35,25 +35,25 @@ describe('CacheControlExtension', () => { ); }); - it('does not set cache-control header if graphqlResponse has errors', () => { - graphqlResponse.errors = [new GraphQLError('Test Error')]; + const shouldNotSetCacheControlHeaders = () => { cacheControlExtension.willSendResponse && cacheControlExtension.willSendResponse({ graphqlResponse }); expect(graphqlResponse.http!.headers.get('Cache-Control')).toBeNull(); + }; + + it('does not set cache-control header if graphqlResponse has errors', () => { + graphqlResponse.errors = [new GraphQLError('Test Error')]; + shouldNotSetCacheControlHeaders(); }); it('does not set cache-control header if there is no data', () => { graphqlResponse.data = undefined; - cacheControlExtension.willSendResponse && - cacheControlExtension.willSendResponse({ graphqlResponse }); - expect(graphqlResponse.http!.headers.get('Cache-Control')).toBeNull(); + shouldNotSetCacheControlHeaders(); }); it('does not set cache-control header if there is no overall cache policy', () => { cacheControlExtension.computeOverallCachePolicy = () => undefined; - cacheControlExtension.willSendResponse && - cacheControlExtension.willSendResponse({ graphqlResponse }); - expect(graphqlResponse.http!.headers.get('Cache-Control')).toBeNull(); + shouldNotSetCacheControlHeaders(); }); it('does not set cache-control header if the overall cache policy has max-age set to 0', () => { @@ -61,9 +61,7 @@ describe('CacheControlExtension', () => { maxAge: 0, scope: CacheScope.Public, }); - cacheControlExtension.willSendResponse && - cacheControlExtension.willSendResponse({ graphqlResponse }); - expect(graphqlResponse.http!.headers.get('Cache-Control')).toBeNull(); + shouldNotSetCacheControlHeaders(); }); }); From 8954792a5544a1c11a84e37554209f09127d76e3 Mon Sep 17 00:00:00 2001 From: Fabian Schneider Date: Fri, 24 May 2019 15:40:49 +0200 Subject: [PATCH 4/5] Use guard clause for Cache-Control checks --- .../__tests__/cacheControlExtension.test.ts | 24 +++++++------------ packages/apollo-cache-control/src/index.ts | 21 ++++++++-------- 2 files changed, 18 insertions(+), 27 deletions(-) diff --git a/packages/apollo-cache-control/src/__tests__/cacheControlExtension.test.ts b/packages/apollo-cache-control/src/__tests__/cacheControlExtension.test.ts index 30c1d3d37f8..29a6e0d195d 100644 --- a/packages/apollo-cache-control/src/__tests__/cacheControlExtension.test.ts +++ b/packages/apollo-cache-control/src/__tests__/cacheControlExtension.test.ts @@ -35,33 +35,25 @@ describe('CacheControlExtension', () => { ); }); - const shouldNotSetCacheControlHeaders = () => { + const shouldNotSetCacheControlHeader = () => { cacheControlExtension.willSendResponse && cacheControlExtension.willSendResponse({ graphqlResponse }); expect(graphqlResponse.http!.headers.get('Cache-Control')).toBeNull(); }; - it('does not set cache-control header if graphqlResponse has errors', () => { - graphqlResponse.errors = [new GraphQLError('Test Error')]; - shouldNotSetCacheControlHeaders(); + 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 there is no data', () => { - graphqlResponse.data = undefined; - shouldNotSetCacheControlHeaders(); + 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; - shouldNotSetCacheControlHeaders(); - }); - - it('does not set cache-control header if the overall cache policy has max-age set to 0', () => { - cacheControlExtension.computeOverallCachePolicy = () => ({ - maxAge: 0, - scope: CacheScope.Public, - }); - shouldNotSetCacheControlHeaders(); + shouldNotSetCacheControlHeader(); }); }); diff --git a/packages/apollo-cache-control/src/index.ts b/packages/apollo-cache-control/src/index.ts index 2b62239560d..bf818adf2aa 100644 --- a/packages/apollo-cache-control/src/index.ts +++ b/packages/apollo-cache-control/src/index.ts @@ -153,18 +153,17 @@ export class CacheControlExtension } public willSendResponse?(o: { graphqlResponse: GraphQLResponse }) { - if (this.options.calculateHttpHeaders && o.graphqlResponse.http) { - const overallCachePolicy = this.computeOverallCachePolicy(); - - if ( - o.graphqlResponse.errors || - !o.graphqlResponse.data || - !overallCachePolicy || - overallCachePolicy.maxAge <= 0 - ) { - return; - } + 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=${ From 6bd53f802c6bd715d25fef776bac7c55a8afe738 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Fri, 24 May 2019 17:38:31 +0300 Subject: [PATCH 5/5] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7bb572c6e73..1bb286d10cb 100644 --- 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