From f3ae806d60224cf94a8314d84c47627d7ef44da4 Mon Sep 17 00:00:00 2001 From: David Glasser Date: Mon, 24 Jun 2019 13:16:51 -0700 Subject: [PATCH] Simplify apollo-engine-reporting operationName capturing (#2899) The full query caching change (#2437) intended to introduce didResolveOperation to the old graphql-extensions API used by apollo-engine-reporting ("backporting" it from the newer plugin API). However, that change accidentally forgot to invoke didResolveOperation from the request pipeline! This meant that the operation name never got reported. The change was backed out in #2557. But this unfortunately re-introduced the exact bug that the change in #2437 was intended to fix: operationName was no longer set when a result is served from the cache! Additionally, it was not set if a *plugin* didResolveOperation call threw, which is what happens when the operation registry plugin forbids an operation. While we could have fixed this by reintroducing the didResolveOperation extension API, there would be a subtle requirement that the apollo-engine-reporting extension didResolveOperation be run before the possibly-throwing operation registry didResolveOperation. So instead, @abernix implemented #2711. This used `requestContext.operationName` as a fallback if neither executionDidStart nor willResolveField gets called. This will be set if the operation properly parsed, validates, and either has a specified operationName that is found in the document, or there is no specified operationName and there is exactly one operation in the document and it has a name. (Note that no version of this code ever sent the user-provided operationName in case of parse or validation errors.) The existing code is correct, but this PR cleans up a few things: - #2557 reverted the one *implementation* of the didResolveOperation extension API, and #2437 accidentally didn't contain any *callers* of the API, but it was still declared on GraphQLExtension and GraphQLExtensionStack. This PR removes those declarations (which have never been useful). - We currently look for the operation name in willResolveField. But in any case where fields are successfully being resolved, the pipeline must have managed to successfully resolve the operation and set requestContext.operationName. So we don't actually need the willResolveField code, because the "fallback" in the requestDidStart end-callback will have the same value. So take this code away. (This change is the motivation for this PR; for federation metrics I'm trying to disengage the "calculate times for fields" part of trace generation from the rest of it.) - Fix the comment in "requestDidEnd" that implied incorrectly that requestContext.operationName was the user-provided name rather than the pipeline-calculated name. Be explicit both there and in requestPipeline.ts that we are relying on the fact that the RequestContext passed to requestDidStart is mutated to add operationName before its end handler is called. This change is intended to be a no-op change (other than the removal of the never-used APIs). --- .../apollo-engine-reporting/src/extension.ts | 39 +++++++++---------- .../apollo-server-core/src/requestPipeline.ts | 5 ++- .../src/requestPipelineAPI.ts | 7 ++-- packages/graphql-extensions/src/index.ts | 12 ------ 4 files changed, 26 insertions(+), 37 deletions(-) diff --git a/packages/apollo-engine-reporting/src/extension.ts b/packages/apollo-engine-reporting/src/extension.ts index f734fa15fd1..c3f3aaba780 100644 --- a/packages/apollo-engine-reporting/src/extension.ts +++ b/packages/apollo-engine-reporting/src/extension.ts @@ -46,7 +46,7 @@ export class EngineReportingExtension public trace = new Trace(); private nodes = new Map(); private startHrTime!: [number, number]; - private operationName?: string | null; + private explicitOperationName?: string | null; private queryString?: string; private documentAST?: DocumentNode; private options: EngineReportingOptions; @@ -203,12 +203,15 @@ export class EngineReportingExtension this.trace.registeredOperation = !!o.requestContext.metrics .registeredOperation; - // If the `operationName` was not already set elsewhere, for example, - // through the `executionDidStart` or the `willResolveField` hooks, then - // we'll resort to using the `operationName` which was requested to be - // executed by the client. + // If the user did not explicitly specify an operation name (which we + // would have saved in `executionDidStart`), but the request pipeline made + // it far enough to figure out what the operation name must be and store + // it on requestContext.operationName, use that name. (Note that this + // depends on the assumption that the RequestContext passed to + // requestDidStart, which does not yet have operationName, will be mutated + // to add operationName later.) const operationName = - this.operationName || o.requestContext.operationName || ''; + this.explicitOperationName || o.requestContext.operationName || ''; const documentAST = this.documentAST || o.requestContext.document; this.addTrace({ @@ -222,18 +225,17 @@ export class EngineReportingExtension } public executionDidStart(o: { executionArgs: ExecutionArgs }) { - // If the operationName is explicitly provided, save it. If there's just one - // named operation, the client doesn't have to provide it, but we still want - // to know the operation name so that the server can identify the query by - // it without having to parse a signature. + // If the operationName is explicitly provided, save it. Note: this is the + // operationName provided by the user. It might be empty if they're relying on + // the "just use the only operation I sent" behavior, even if that operation + // has a name. // - // Fortunately, in the non-error case, we can just pull this out of - // the first call to willResolveField's `info` argument. In an - // error case (eg, the operationName isn't found, or there are more - // than one operation and no specified operationName) it's OK to continue - // to file this trace under the empty operationName. + // It's possible that execution is about to fail because this operation + // isn't actually in the document. We want to know the name in that case + // too, which is why it's important that we save the name now, and not just + // rely on requestContext.operationName (which will be null in this case). if (o.executionArgs.operationName) { - this.operationName = o.executionArgs.operationName; + this.explicitOperationName = o.executionArgs.operationName; } this.documentAST = o.executionArgs.document; } @@ -244,11 +246,6 @@ export class EngineReportingExtension _context: TContext, info: GraphQLResolveInfo, ): ((error: Error | null, result: any) => void) | void { - if (this.operationName === undefined) { - this.operationName = - (info.operation.name && info.operation.name.value) || ''; - } - const path = info.path; const node = this.newNode(path); node.type = info.returnType.toString(); diff --git a/packages/apollo-server-core/src/requestPipeline.ts b/packages/apollo-server-core/src/requestPipeline.ts index 1c806cda1dd..af179b3896f 100644 --- a/packages/apollo-server-core/src/requestPipeline.ts +++ b/packages/apollo-server-core/src/requestPipeline.ts @@ -287,7 +287,10 @@ export async function processGraphQLRequest( ); requestContext.operation = operation || undefined; - // We'll set `operationName` to `null` for anonymous operations. + // We'll set `operationName` to `null` for anonymous operations. Note that + // apollo-engine-reporting relies on the fact that the requestContext passed + // to requestDidStart is mutated to add this field before requestDidEnd is + // called requestContext.operationName = (operation && operation.name && operation.name.value) || null; diff --git a/packages/apollo-server-core/src/requestPipelineAPI.ts b/packages/apollo-server-core/src/requestPipelineAPI.ts index 9e835194109..83f5e8fd3ab 100644 --- a/packages/apollo-server-core/src/requestPipelineAPI.ts +++ b/packages/apollo-server-core/src/requestPipelineAPI.ts @@ -71,9 +71,10 @@ export interface GraphQLRequestContext> { readonly document?: DocumentNode; readonly source?: string; - // `operationName` is set based on the operation AST, so it is defined - // even if no `request.operationName` was passed in. - // It will be set to `null` for an anonymous operation. + // `operationName` is set based on the operation AST, so it is defined even if + // no `request.operationName` was passed in. It will be set to `null` for an + // anonymous operation, or if `requestName.operationName` was passed in but + // doesn't resolve to an operation in the document. readonly operationName?: string | null; readonly operation?: OperationDefinitionNode; diff --git a/packages/graphql-extensions/src/index.ts b/packages/graphql-extensions/src/index.ts index c1b051d9439..ea609fc125c 100644 --- a/packages/graphql-extensions/src/index.ts +++ b/packages/graphql-extensions/src/index.ts @@ -49,9 +49,6 @@ export class GraphQLExtension { executionArgs: ExecutionArgs; }): EndHandler | void; - public didResolveOperation?(o: { - requestContext: GraphQLRequestContext; - }): void; public didEncounterErrors?(errors: ReadonlyArray): void; public willSendResponse?(o: { @@ -113,15 +110,6 @@ export class GraphQLExtensionStack { ); } - public didResolveOperation(o: { - requestContext: GraphQLRequestContext; - }) { - this.extensions.forEach(extension => { - if (extension.didResolveOperation) { - extension.didResolveOperation(o); - } - }); - } public didEncounterErrors(errors: ReadonlyArray) { this.extensions.forEach(extension => { if (extension.didEncounterErrors) {