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) {