From a643f00808fadffa51c94f522f3c43df43003576 Mon Sep 17 00:00:00 2001 From: Jesse Rosenberger Date: Thu, 24 Jan 2019 22:21:07 +0200 Subject: [PATCH] Don't write to persisted query cache until execution will begin. Since we already write to the persisted query cache asynchronously (and intentionally do not await the Promise) this won't have any affect on the current behavior of when the persisted query cache is written to in the event of an executable request, but if an operation comes in and doesn't parse or validate, we'll avoid wasting cache space on an operation that will never execute. Additionally, in a similar light, if a plugin throws an error which stops execution, we can avoid the side-effect of writing to the persisted query cache. In terms of the APQ behavior, while this could cause additional round-trips for a client which repeatedly sends an invalid operation, that operation is never going to successfully finish anyway. While developer tooling will help avoid this problem in the first place, this refusal to store an invalid operation in the APQ cache could actually help diagnose a failure since the full operation (rather than just the SHA256 of that document) will appear in the browser's dev-tools on the retry. If we're looking to spare parsing and validating documents which we know are going to fail, I think that's going to be a better use of the (new) `documentStore` cache (#2111), since its in-memory and can accommodate a more complex data structure which includes the validation errors from the previous attempt which will, of course, be the same when validating the same operation against the same schema again. As the PR that introduced that feature shows, sparing those additional parses and validations (cached APQ documents still needs to be parsed and validated, currently) will provide a more successful performance win overall. Ref: https://github.com/apollographql/apollo-server/pull/2111 --- CHANGELOG.md | 1 + .../apollo-server-core/src/requestPipeline.ts | 26 +++++++++++++++---- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 71b41dec8a2..a2295737c68 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ### vNEXT - Switch from `json-stable-stringify` to `fast-json-stable-stringify`. [PR #2065](https://github.com/apollographql/apollo-server/pull/2065) +- Don't write to the persisted query cache until execution will begin. [PR #2227](https://github.com/apollographql/apollo-server/pull/2227) ### v2.3.1 diff --git a/packages/apollo-server-core/src/requestPipeline.ts b/packages/apollo-server-core/src/requestPipeline.ts index 7c4429c3f60..03ca7128715 100644 --- a/packages/apollo-server-core/src/requestPipeline.ts +++ b/packages/apollo-server-core/src/requestPipeline.ts @@ -44,6 +44,7 @@ import { } from 'apollo-server-plugin-base'; import { Dispatcher } from './utils/dispatcher'; +import { KeyValueCache } from 'apollo-server-caching'; export { GraphQLRequest, @@ -102,6 +103,7 @@ export async function processGraphQLRequest( let queryHash: string; + let persistedQueryCache: KeyValueCache | undefined; let persistedQueryHit = false; let persistedQueryRegister = false; @@ -116,10 +118,14 @@ export async function processGraphQLRequest( ); } + // We'll store a reference to the persisted query cache so we can actually + // do the write at a later point in the request pipeline processing. + persistedQueryCache = config.persistedQueries.cache; + queryHash = extensions.persistedQuery.sha256Hash; if (query === undefined) { - query = await config.persistedQueries.cache.get(`apq:${queryHash}`); + query = await persistedQueryCache.get(`apq:${queryHash}`); if (query) { persistedQueryHit = true; } else { @@ -134,11 +140,11 @@ export async function processGraphQLRequest( ); } + // We won't write to the persisted query cache until later. + // Defering the writing gives plugins the ability to "win" from use of + // the cache, but also have their say in whether or not the cache is + // written to (by interrupting the request with an error). persistedQueryRegister = true; - - Promise.resolve( - config.persistedQueries.cache.set(`apq:${queryHash}`, query), - ).catch(console.warn); } } else if (query) { // FIXME: We'll compute the APQ query hash to use as our cache key for @@ -212,6 +218,16 @@ export async function processGraphQLRequest( >, ); + // Now that we've gone through the pre-execution phases of the request + // pipeline, and given plugins appropriate ability to object (by throwing + // an error) and not actually write, we'll write to the cache if it was + // determined earlier in the request pipeline that we should do so. + if (persistedQueryRegister && persistedQueryCache) { + Promise.resolve(persistedQueryCache.set(`apq:${queryHash}`, query)).catch( + console.warn, + ); + } + const executionDidEnd = await dispatcher.invokeDidStartHook( 'executionDidStart', requestContext as WithRequired<