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

packages/apollo-engine-reporting/src/extension.ts

@@ -46,7 +46,7 @@ export class EngineReportingExtension<TContext = any>
   public trace = new Trace();
   private nodes = new Map<string, Trace.Node>();
   private startHrTime!: [number, number];
-  private operationName?: string | null;
+  private explicitOperationName?: string | null;
   private queryString?: string;
   private documentAST?: DocumentNode;
   private options: EngineReportingOptions<TContext>;
@@ -203,12 +203,15 @@ export class EngineReportingExtension<TContext = any>
       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<TContext = any>
   }
 
   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<TContext = any>
     _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();

packages/apollo-server-core/src/requestPipeline.ts

@@ -287,7 +287,10 @@ export async function processGraphQLRequest<TContext>(
     );
 
     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;
 

packages/apollo-server-core/src/requestPipelineAPI.ts

@@ -71,9 +71,10 @@ export interface GraphQLRequestContext<TContext = Record<string, any>> {
   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;
 

packages/graphql-extensions/src/index.ts

@@ -49,9 +49,6 @@ export class GraphQLExtension<TContext = any> {
     executionArgs: ExecutionArgs;
   }): EndHandler | void;
 
-  public didResolveOperation?(o: {
-    requestContext: GraphQLRequestContext<TContext>;
-  }): void;
   public didEncounterErrors?(errors: ReadonlyArray<GraphQLError>): void;
 
   public willSendResponse?(o: {
@@ -113,15 +110,6 @@ export class GraphQLExtensionStack<TContext = any> {
     );
   }
 
-  public didResolveOperation(o: {
-    requestContext: GraphQLRequestContext<TContext>;
-  }) {
-    this.extensions.forEach(extension => {
-      if (extension.didResolveOperation) {
-        extension.didResolveOperation(o);
-      }
-    });
-  }
   public didEncounterErrors(errors: ReadonlyArray<GraphQLError>) {
     this.extensions.forEach(extension => {
       if (extension.didEncounterErrors) {