From 68cbc938ff709078efc8a1e726300e61222f6b8b Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Mon, 30 Mar 2020 15:02:44 +0300 Subject: [PATCH] refactor: Introduce "cache-control" plugin and switch req. pipeline to it. Similar to 6009d8a00278e, which migrated the tracing package to the plugin API, this commit introduces a `plugin` (named) export from the `apollo-cache-control` module. It similarly accomplishes that by duplicating the behavior of the `CacheControlExtension` which leveraged the soon-to-be-deprecated `graphql-extensions`. The functionality, again, is intended to be identical in spirit. Since the delta of the commits was otherwise even _more_ confusing, I've left the `CacheControlExtension` present and exported and will remove it in a subsequent commit. Briefly summarizing what the necessary changes were: 1. We no longer use a class instance to house the extension, which was necessitated by the `graphql-extensions` API. This means that uses of `this` have been replaced with function scoped variables by the same name. 2. The logic which actually does the formatting (previously handled by the `format` method in `graphql-extension`, now takes place within the plugin API's `willSendResponse` method. 3. Rather than leaning on plugin-specific behavior from within the request pipeline, the `apollo-cache-control` plugin now makes a number of assertions throughout its various life-cycle methods to ensure that the `overallCachePolicy` is calculated. 4. Switch tests to use the new `pluginTestHarness` method for testing plugins which was introduced in eec87a6c6dead7fd (#3990). --- package-lock.json | 1 + packages/apollo-cache-control/package.json | 3 +- .../__tests__/cacheControlExtension.test.ts | 162 ++++++++++----- .../src/__tests__/collectCacheControlHints.ts | 33 +-- packages/apollo-cache-control/src/index.ts | 188 ++++++++++++++++++ packages/apollo-cache-control/tsconfig.json | 1 + .../apollo-server-core/src/ApolloServer.ts | 60 +++--- .../apollo-server-core/src/requestPipeline.ts | 25 --- .../apollo-server-core/src/runHttpQuery.ts | 4 - 9 files changed, 353 insertions(+), 124 deletions(-) diff --git a/package-lock.json b/package-lock.json index 20a701000e3..519c1acc2ed 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4277,6 +4277,7 @@ "version": "file:packages/apollo-cache-control", "requires": { "apollo-server-env": "file:packages/apollo-server-env", + "apollo-server-plugin-base": "file:packages/apollo-server-plugin-base", "graphql-extensions": "file:packages/graphql-extensions" } }, diff --git a/packages/apollo-cache-control/package.json b/packages/apollo-cache-control/package.json index a14de3e4b15..23b4e1f583a 100644 --- a/packages/apollo-cache-control/package.json +++ b/packages/apollo-cache-control/package.json @@ -12,7 +12,8 @@ }, "dependencies": { "apollo-server-env": "file:../apollo-server-env", - "graphql-extensions": "file:../graphql-extensions" + "graphql-extensions": "file:../graphql-extensions", + "apollo-server-plugin-base": "file:../apollo-server-plugin-base" }, "peerDependencies": { "graphql": "^0.12.0 || ^0.13.0 || ^14.0.0 || ^15.0.0" diff --git a/packages/apollo-cache-control/src/__tests__/cacheControlExtension.test.ts b/packages/apollo-cache-control/src/__tests__/cacheControlExtension.test.ts index 29a6e0d195d..ff4d1fb75eb 100644 --- a/packages/apollo-cache-control/src/__tests__/cacheControlExtension.test.ts +++ b/packages/apollo-cache-control/src/__tests__/cacheControlExtension.test.ts @@ -1,59 +1,110 @@ import { ResponsePath, GraphQLError } from 'graphql'; import { GraphQLResponse } from 'graphql-extensions'; import { Headers } from 'apollo-server-env'; -import { CacheControlExtension, CacheScope } from '../'; +import { + CacheScope, + CacheControlExtensionOptions, + CacheHint, + __testing__, + plugin, +} from '../'; +const { addHint, computeOverallCachePolicy } = __testing__; +import { + GraphQLRequestContextWillSendResponse, +} from 'apollo-server-plugin-base'; +import pluginTestHarness from 'apollo-server-core/dist/utils/pluginTestHarness'; describe('CacheControlExtension', () => { - let cacheControlExtension: CacheControlExtension; - - beforeEach(() => { - cacheControlExtension = new CacheControlExtension(); - }); - describe('willSendResponse', () => { - let graphqlResponse: GraphQLResponse; + function makePluginWithOptions({ + pluginInitializationOptions, + overallCachePolicy, + errors = false, + }: { + pluginInitializationOptions?: CacheControlExtensionOptions; + overallCachePolicy?: Required; + errors?: boolean; + } = Object.create(null)) { + const pluginInstance = plugin(pluginInitializationOptions); + + return pluginTestHarness({ + pluginInstance, + overallCachePolicy, + graphqlRequest: { query: 'does not matter' }, + executor: () => { + const response: GraphQLResponse = { + http: { + headers: new Headers(), + }, + data: { test: 'test' }, + }; + + if (errors) { + response.errors = [new GraphQLError('Test Error')]; + } + + return response; + }, + }); + } - beforeEach(() => { - cacheControlExtension.options.calculateHttpHeaders = true; - cacheControlExtension.computeOverallCachePolicy = () => ({ + describe('HTTP cache-control header', () => { + const overallCachePolicy: Required = { 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', - ); - }); + it('is set when calculateHttpHeaders is set to true', async () => { + const requestContext = await makePluginWithOptions({ + pluginInitializationOptions: { + calculateHttpHeaders: true, + }, + overallCachePolicy, + }); + expect(requestContext.response.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(); - }; + const shouldNotSetCacheControlHeader = ( + requestContext: GraphQLRequestContextWillSendResponse, + ) => { + expect( + requestContext.response.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('is not set when calculateHttpHeaders is set to false', async () => { + const requestContext = await makePluginWithOptions({ + pluginInitializationOptions: { + calculateHttpHeaders: false, + }, + overallCachePolicy, + }); + shouldNotSetCacheControlHeader(requestContext); + }); - it('does not set cache-control header if graphqlResponse has errors', () => { - graphqlResponse.errors = [new GraphQLError('Test Error')]; - shouldNotSetCacheControlHeader(); - }); + it('is not set if response has errors', async () => { + const requestContext = await makePluginWithOptions({ + pluginInitializationOptions: { + calculateHttpHeaders: false, + }, + overallCachePolicy, + errors: true, + }); + shouldNotSetCacheControlHeader(requestContext); + }); - it('does not set cache-control header if there is no overall cache policy', () => { - cacheControlExtension.computeOverallCachePolicy = () => undefined; - shouldNotSetCacheControlHeader(); + it('does not set cache-control header if there is no overall cache policy', async () => { + const requestContext = await makePluginWithOptions({ + pluginInitializationOptions: { + calculateHttpHeaders: false, + }, + overallCachePolicy: undefined, + errors: true, + }); + shouldNotSetCacheControlHeader(requestContext); + }); }); }); @@ -71,46 +122,49 @@ describe('CacheControlExtension', () => { prev: responseSubPath, }; + const hints = new Map(); + afterEach(() => hints.clear()); + it('returns undefined without cache hints', () => { - const cachePolicy = cacheControlExtension.computeOverallCachePolicy(); + const cachePolicy = computeOverallCachePolicy(hints); expect(cachePolicy).toBeUndefined(); }); it('returns lowest max age value', () => { - cacheControlExtension.addHint(responsePath, { maxAge: 10 }); - cacheControlExtension.addHint(responseSubPath, { maxAge: 20 }); + addHint(hints, responsePath, { maxAge: 10 }); + addHint(hints, responseSubPath, { maxAge: 20 }); - const cachePolicy = cacheControlExtension.computeOverallCachePolicy(); + const cachePolicy = computeOverallCachePolicy(hints); expect(cachePolicy).toHaveProperty('maxAge', 10); }); it('returns undefined if any cache hint has a maxAge of 0', () => { - cacheControlExtension.addHint(responsePath, { maxAge: 120 }); - cacheControlExtension.addHint(responseSubPath, { maxAge: 0 }); - cacheControlExtension.addHint(responseSubSubPath, { maxAge: 20 }); + addHint(hints, responsePath, { maxAge: 120 }); + addHint(hints, responseSubPath, { maxAge: 0 }); + addHint(hints, responseSubSubPath, { maxAge: 20 }); - const cachePolicy = cacheControlExtension.computeOverallCachePolicy(); + const cachePolicy = computeOverallCachePolicy(hints); expect(cachePolicy).toBeUndefined(); }); it('returns PUBLIC scope by default', () => { - cacheControlExtension.addHint(responsePath, { maxAge: 10 }); + addHint(hints, responsePath, { maxAge: 10 }); - const cachePolicy = cacheControlExtension.computeOverallCachePolicy(); + const cachePolicy = computeOverallCachePolicy(hints); expect(cachePolicy).toHaveProperty('scope', CacheScope.Public); }); it('returns PRIVATE scope if any cache hint has PRIVATE scope', () => { - cacheControlExtension.addHint(responsePath, { + addHint(hints, responsePath, { maxAge: 10, scope: CacheScope.Public, }); - cacheControlExtension.addHint(responseSubPath, { + addHint(hints, responseSubPath, { maxAge: 10, scope: CacheScope.Private, }); - const cachePolicy = cacheControlExtension.computeOverallCachePolicy(); + const cachePolicy = computeOverallCachePolicy(hints); expect(cachePolicy).toHaveProperty('scope', CacheScope.Private); }); }); diff --git a/packages/apollo-cache-control/src/__tests__/collectCacheControlHints.ts b/packages/apollo-cache-control/src/__tests__/collectCacheControlHints.ts index e69e8bc448e..fdadf7d7ca4 100644 --- a/packages/apollo-cache-control/src/__tests__/collectCacheControlHints.ts +++ b/packages/apollo-cache-control/src/__tests__/collectCacheControlHints.ts @@ -1,38 +1,41 @@ import { GraphQLSchema, graphql } from 'graphql'; - -import { - enableGraphQLExtensions, - GraphQLExtensionStack, -} from 'graphql-extensions'; import { - CacheControlExtension, CacheHint, CacheControlExtensionOptions, + plugin, } from '../'; +import pluginTestHarness from 'apollo-server-core/dist/utils/pluginTestHarness'; export async function collectCacheControlHints( schema: GraphQLSchema, source: string, options?: CacheControlExtensionOptions, ): Promise { - enableGraphQLExtensions(schema); // Because this test helper looks at the formatted extensions, we always want - // to include them. - const cacheControlExtension = new CacheControlExtension({ + // to include them in the response rather than allow them to be stripped + // out. + const pluginInstance = plugin({ ...options, stripFormattedExtensions: false, }); - const response = await graphql({ + const requestContext = await pluginTestHarness({ + pluginInstance, schema, - source, - contextValue: { - _extensionStack: new GraphQLExtensionStack([cacheControlExtension]), + graphqlRequest: { + query: source, }, + executor: async (requestContext) => { + return await graphql({ + schema, + source: requestContext.request.query, + contextValue: requestContext.context, + }); + } }); - expect(response.errors).toBeUndefined(); + expect(requestContext.response.errors).toBeUndefined(); - return cacheControlExtension.format()[1].hints; + return requestContext.response.extensions!.cacheControl.hints; } diff --git a/packages/apollo-cache-control/src/index.ts b/packages/apollo-cache-control/src/index.ts index fa37370f03e..f1a9a944e42 100644 --- a/packages/apollo-cache-control/src/index.ts +++ b/packages/apollo-cache-control/src/index.ts @@ -7,6 +7,7 @@ import { ResponsePath, responsePathAsArray, } from 'graphql'; +import { ApolloServerPlugin } from "apollo-server-plugin-base"; import { GraphQLExtension, GraphQLResponse } from 'graphql-extensions'; @@ -49,6 +50,151 @@ declare module 'apollo-server-types' { } } +type MapResponsePathHints = Map; + +export const plugin = ( + options: CacheControlExtensionOptions = Object.create(null), +): ApolloServerPlugin => ({ + requestDidStart(requestContext) { + const defaultMaxAge: number = options.defaultMaxAge || 0; + const hints: MapResponsePathHints = new Map(); + + + function setOverallCachePolicyWhenUnset() { + if (!requestContext.overallCachePolicy) { + requestContext.overallCachePolicy = computeOverallCachePolicy(hints); + } + } + + return { + willResolveField(...args) { + const [, , , info] = args; + let hint: CacheHint = {}; + + // If this field's resolver returns an object or interface, look for + // hints on that return type. + const targetType = getNamedType(info.returnType); + if ( + targetType instanceof GraphQLObjectType || + targetType instanceof GraphQLInterfaceType + ) { + if (targetType.astNode) { + hint = mergeHints( + hint, + cacheHintFromDirectives(targetType.astNode.directives), + ); + } + } + + // Look for hints on the field itself (on its parent type), taking + // precedence over previously calculated hints. + const fieldDef = info.parentType.getFields()[info.fieldName]; + if (fieldDef.astNode) { + hint = mergeHints( + hint, + cacheHintFromDirectives(fieldDef.astNode.directives), + ); + } + + // If this resolver returns an object or is a root field and we haven't + // seen an explicit maxAge hint, set the maxAge to 0 (uncached) or the + // default if specified in the constructor. (Non-object fields by + // default are assumed to inherit their cacheability from their parents. + // But on the other hand, while root non-object fields can get explicit + // hints from their definition on the Query/Mutation object, if that + // doesn't exist then there's no parent field that would assign the + // default maxAge, so we do it here.) + if ( + (targetType instanceof GraphQLObjectType || + targetType instanceof GraphQLInterfaceType || + !info.path.prev) && + hint.maxAge === undefined + ) { + hint.maxAge = defaultMaxAge; + } + + if (hint.maxAge !== undefined || hint.scope !== undefined) { + addHint(hints, info.path, hint); + } + + info.cacheControl = { + setCacheHint: (hint: CacheHint) => { + addHint(hints, info.path, hint); + }, + cacheHint: hint, + }; + }, + + responseForOperation() { + // We are not supplying an answer, we are only setting the cache + // policy if it's not set! Therefore, we return null. + setOverallCachePolicyWhenUnset(); + return null; + }, + + executionDidStart() { + return () => setOverallCachePolicyWhenUnset(); + }, + + willSendResponse(requestContext) { + const { + response, + overallCachePolicy: overallCachePolicyOverride, + } = requestContext; + + // If there are any errors, we don't consider this cacheable. + if (response.errors) { + return; + } + + // Use the override by default, but if it's not overridden, set our + // own computation onto the `requestContext` for other plugins to read. + const overallCachePolicy = + overallCachePolicyOverride || + (requestContext.overallCachePolicy = + computeOverallCachePolicy(hints)); + + if ( + overallCachePolicy && + options.calculateHttpHeaders && + response.http + ) { + response.http.headers.set( + 'Cache-Control', + `max-age=${ + overallCachePolicy.maxAge + }, ${overallCachePolicy.scope.toLowerCase()}`, + ); + } + + // We should have to explicitly ask to leave the formatted extension in, + // or pass the old-school `cacheControl: true` (as interpreted by + // apollo-server-core/ApolloServer), in order to include the + // engineproxy-aimed extensions. Specifically, we want users of + // apollo-server-plugin-response-cache to be able to specify + // `cacheControl: {defaultMaxAge: 600}` without accidentally turning on + // the extension formatting. + if (options.stripFormattedExtensions !== false) return; + + const extensions = + response.extensions || (response.extensions = Object.create(null)); + + if (typeof extensions.cacheControl !== 'undefined') { + throw new Error("The cacheControl information already existed."); + } + + extensions.cacheControl = { + version: 1, + hints: Array.from(hints).map(([path, hint]) => ({ + path: [...responsePathAsArray(path)], + ...hint, + })), + }; + } + } + } +}); + export class CacheControlExtension implements GraphQLExtension { private defaultMaxAge: number; @@ -255,3 +401,45 @@ function mergeHints( scope: otherHint.scope || hint.scope, }; } + +function computeOverallCachePolicy( + hints: MapResponsePathHints, +): Required | undefined { + let lowestMaxAge: number | undefined = undefined; + let scope: CacheScope = CacheScope.Public; + + for (const hint of hints.values()) { + if (hint.maxAge !== undefined) { + lowestMaxAge = + lowestMaxAge !== undefined + ? Math.min(lowestMaxAge, hint.maxAge) + : hint.maxAge; + } + if (hint.scope === CacheScope.Private) { + scope = CacheScope.Private; + } + } + + // If maxAge is 0, then we consider it uncacheable so it doesn't matter what + // the scope was. + return lowestMaxAge + ? { + maxAge: lowestMaxAge, + scope, + } + : undefined; +} + +function addHint(hints: MapResponsePathHints, path: ResponsePath, hint: CacheHint) { + const existingCacheHint = hints.get(path); + if (existingCacheHint) { + hints.set(path, mergeHints(existingCacheHint, hint)); + } else { + hints.set(path, hint); + } +} + +export const __testing__ = { + addHint, + computeOverallCachePolicy, +}; diff --git a/packages/apollo-cache-control/tsconfig.json b/packages/apollo-cache-control/tsconfig.json index 0de28001c29..1276353ea04 100644 --- a/packages/apollo-cache-control/tsconfig.json +++ b/packages/apollo-cache-control/tsconfig.json @@ -8,5 +8,6 @@ "exclude": ["**/__tests__", "**/__mocks__"], "references": [ { "path": "../graphql-extensions" }, + { "path": "../apollo-server-plugin-base" }, ] } diff --git a/packages/apollo-server-core/src/ApolloServer.ts b/packages/apollo-server-core/src/ApolloServer.ts index bea27fea464..8bddf49873a 100644 --- a/packages/apollo-server-core/src/ApolloServer.ts +++ b/packages/apollo-server-core/src/ApolloServer.ts @@ -70,6 +70,10 @@ import { import { Headers } from 'apollo-server-env'; import { buildServiceDefinition } from '@apollographql/apollo-tools'; import { Logger } from "apollo-server-types"; +import { + plugin as pluginCacheControl, + CacheControlExtensionOptions, +} from 'apollo-cache-control'; const NoIntrospection = (context: ValidationContext) => ({ Field(node: FieldDefinitionNode) { @@ -190,6 +194,7 @@ export class ApolloServerBase { playground, plugins, gateway, + cacheControl, experimental_approximateDocumentStoreMiB, ...requestOptions } = config; @@ -249,31 +254,6 @@ export class ApolloServerBase { : noIntro; } - if (requestOptions.cacheControl !== false) { - if ( - typeof requestOptions.cacheControl === 'boolean' && - requestOptions.cacheControl === true - ) { - // cacheControl: true means that the user needs the cache-control - // extensions. This means we are running the proxy, so we should not - // strip out the cache control extension and not add cache-control headers - requestOptions.cacheControl = { - stripFormattedExtensions: false, - calculateHttpHeaders: false, - defaultMaxAge: 0, - }; - } else { - // Default behavior is to run default header calculation and return - // no cacheControl extensions - requestOptions.cacheControl = { - stripFormattedExtensions: true, - calculateHttpHeaders: true, - defaultMaxAge: 0, - ...requestOptions.cacheControl, - }; - } - } - if (!requestOptions.cache) { requestOptions.cache = new InMemoryLRUCache(); } @@ -790,7 +770,37 @@ export class ApolloServerBase { // at the end of that list so they take precidence. // A follow-up commit will actually introduce this. + // Enable cache control unless it was explicitly disabled. + if (this.config.cacheControl !== false) { + let cacheControlOptions: CacheControlExtensionOptions = {}; + if ( + typeof this.config.cacheControl === 'boolean' && + this.config.cacheControl === true + ) { + // cacheControl: true means that the user needs the cache-control + // extensions. This means we are running the proxy, so we should not + // strip out the cache control extension and not add cache-control headers + cacheControlOptions = { + stripFormattedExtensions: false, + calculateHttpHeaders: false, + defaultMaxAge: 0, + }; + } else { + // Default behavior is to run default header calculation and return + // no cacheControl extensions + cacheControlOptions = { + stripFormattedExtensions: true, + calculateHttpHeaders: true, + defaultMaxAge: 0, + ...this.config.cacheControl, + }; + } + + pluginsToInit.push(pluginCacheControl(cacheControlOptions)); + } + pluginsToInit.push(...plugins); + this.plugins = pluginsToInit.map(plugin => { if (typeof plugin === 'function') { return plugin(); diff --git a/packages/apollo-server-core/src/requestPipeline.ts b/packages/apollo-server-core/src/requestPipeline.ts index 6393d82c95a..4751f44eb8f 100644 --- a/packages/apollo-server-core/src/requestPipeline.ts +++ b/packages/apollo-server-core/src/requestPipeline.ts @@ -22,10 +22,6 @@ import { symbolRequestListenerDispatcher, enablePluginsForSchemaResolvers, } from '.'; -import { - CacheControlExtension, - CacheControlExtensionOptions, -} from 'apollo-cache-control'; import { TracingExtension } from 'apollo-tracing'; import { ApolloError, @@ -97,7 +93,6 @@ export interface GraphQLRequestPipelineConfig { extensions?: Array<() => GraphQLExtension>; tracing?: boolean; persistedQueries?: PersistedQueryOptions; - cacheControl?: CacheControlExtensionOptions; formatError?: (error: GraphQLError) => GraphQLFormattedError; formatResponse?: ( @@ -126,7 +121,6 @@ export async function processGraphQLRequest( // all of our own machinery will certainly set it now. const logger = requestContext.logger || console; - let cacheControlExtension: CacheControlExtension | undefined; const extensionStack = initializeExtensionStack(); (requestContext.context as any)._extensionStack = extensionStack; @@ -390,20 +384,6 @@ export async function processGraphQLRequest( } } - if (cacheControlExtension) { - if (requestContext.overallCachePolicy) { - // If we read this response from a cache and it already has its own - // policy, teach that to cacheControlExtension so that it'll use the - // saved policy for HTTP headers. (If cacheControlExtension was a - // plugin, it could just read from the requestContext, but it isn't.) - cacheControlExtension.overrideOverallCachePolicy( - requestContext.overallCachePolicy, - ); - } else { - requestContext.overallCachePolicy = cacheControlExtension.computeOverallCachePolicy(); - } - } - const formattedExtensions = extensionStack.format(); if (Object.keys(formattedExtensions).length > 0) { response.extensions = formattedExtensions; @@ -604,11 +584,6 @@ export async function processGraphQLRequest( extensions.push(new TracingExtension()); } - if (config.cacheControl) { - cacheControlExtension = new CacheControlExtension(config.cacheControl); - extensions.push(cacheControlExtension); - } - return new GraphQLExtensionStack(extensions); } diff --git a/packages/apollo-server-core/src/runHttpQuery.ts b/packages/apollo-server-core/src/runHttpQuery.ts index c229fe87cf7..b02ce408fb0 100644 --- a/packages/apollo-server-core/src/runHttpQuery.ts +++ b/packages/apollo-server-core/src/runHttpQuery.ts @@ -17,7 +17,6 @@ import { GraphQLRequestContext, GraphQLResponse, } from './requestPipeline'; -import { CacheControlExtensionOptions } from 'apollo-cache-control'; import { ApolloServerPlugin } from 'apollo-server-plugin-base'; import { WithRequired, GraphQLExecutionResult } from 'apollo-server-types'; @@ -173,9 +172,6 @@ export async function runHttpQuery( // cacheControl defaults will also have been set if a boolean argument is // passed in. cache: options.cache!, - cacheControl: options.cacheControl as - | CacheControlExtensionOptions - | undefined, dataSources: options.dataSources, documentStore: options.documentStore,