Invoke didEncounterErrors for known errors during pre-parse. (#3614)

* tests: Assert `didEncounterErrors` is invoked for execution errors.

Previously a missing test.

* tests: Assert error conditions for existing APQ logic (prior to improvements).

Previously, a number of APQ errors did not have tests for errors which could
arise at runtime.  These include:

- When the APQ `version` is not a version that we support.  (Currently, the
  only supported `version` is `1`.
- When the APQ `version` is entirely omitted.  (It must be `1`.)
- When the provided APQ `sha256Hash` doesn't match the provided `query`
  during APQ registration.

I'm adding these tests which assert the existing logic without making any
changes to the logic itself prior to making a number of changes to said
logic which will ensure that these (again, existing) errors are _also_
propagated to the `didEncounterErrors` hook (They currently are NOT!).

* Invoke `didEncounterErrors` for errors during pre-parse phases.

Roughly, these "pre-parse" errors come in two forms: When the query
was entirely omitted, or APQ (automated persisted query) errors.

Arguably, there is a third form of pre-parse errors, but it would be
unexpected and potentially out of scope of the `didEncounterErrors`
life-cycle hook.  Such an error would still be caught by `runHttpQuery.ts`,
but we wouldn't have a guarantee that our plugins have been initialized.
This is unlikely to improve until we re-work the entirety of the request
pipeline in Apollo Server 3.x.

While these errors were in-fact causing errors at runtime that were being
delivered to the client, those errors were not being delivered to the
plugin's `didEncounterErrors` hooks, causing reduced visibility by reporting
tools which utilize that life-cycle hook.

This commit changes the parent class of `InvaidGraphQLRequestError` from
`Error` to `GraphQLError` in order to allow such an error to be received by
`didEncounterErrors`, which requires `GraphQLError`s.  The `GraphQLError`
itself is still an `instanceof Error`, so any existing code that leverages
such a condition should still work as expected, and I suspect that most
other conditions that could be checked on either an `Error` or
`GraphQLError` should also still remain the same.  Despite any uncertain
here, I'd call this a net-win for reporting errors more reliably to the hook
on which they're expected to be received.

* Add CHANGELOG.md for #3614.

CHANGELOG.md

@@ -6,6 +6,7 @@ The version headers in this history reflect the versions of Apollo Server itself
 
 > The changes noted within this `vNEXT` section have not been released yet.  New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development.  When a release is being prepared, a new header will be (manually) created below and the the appropriate changes within that release will be moved into the new section.
 
+- `apollo-server-core`: Ensure that plugin's `didEncounterErrors` hooks are invoked for known automated persisted query (APQ) errors. [#3614](https://github.com/apollographql/apollo-server/pull/3614)
 - `apollo-server-plugin-base`: Move `TContext` generic from `requestDidStart` method to `ApolloServerPlugin` Interface. [#3525](https://github.com/apollographql/apollo-server/pull/3525)
 
 ### v2.9.13

packages/apollo-server-core/src/__tests__/runQuery.test.ts

@@ -514,19 +514,19 @@ describe('runQuery', () => {
 
     describe('didEncounterErrors', () => {
       const didEncounterErrors = jest.fn();
+      const plugins: ApolloServerPlugin[] = [
+        {
+          requestDidStart() {
+            return { didEncounterErrors };
+          },
+        },
+      ];
+
       it('called when an error occurs', async () => {
         await runQuery({
           schema,
           queryString: '{ testStringWithParseError: }',
-          plugins: [
-            {
-              requestDidStart() {
-                return {
-                  didEncounterErrors,
-                };
-              },
-            },
-          ],
+          plugins,
           request: new MockReq(),
         });
 
@@ -537,19 +537,32 @@ describe('runQuery', () => {
         );
       });
 
+      it('called when an error occurs in execution', async () => {
+        const response = await runQuery({
+          schema,
+          queryString: '{ testError }',
+          plugins,
+          request: new MockReq(),
+        });
+
+        expect(response).toHaveProperty(
+          'errors.0.message','Secret error message');
+        expect(response).toHaveProperty('data.testError', null);
+
+        expect(didEncounterErrors).toBeCalledWith(
+          expect.objectContaining({
+            errors: expect.arrayContaining([expect.objectContaining({
+              message: 'Secret error message',
+            })]),
+          }),
+        );
+      });
+
       it('not called when an error does not occur', async () => {
         await runQuery({
           schema,
           queryString: '{ testString }',
-          plugins: [
-            {
-              requestDidStart() {
-                return {
-                  didEncounterErrors,
-                };
-              },
-            },
-          ],
+          plugins,
           request: new MockReq(),
         });
 

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

@@ -135,11 +135,10 @@ export async function processGraphQLRequest<TContext>(
     // It looks like we've received a persisted query. Check if we
     // support them.
     if (!config.persistedQueries || !config.persistedQueries.cache) {
-      throw new PersistedQueryNotSupportedError();
+      return await emitErrorAndThrow(new PersistedQueryNotSupportedError());
     } else if (extensions.persistedQuery.version !== 1) {
-      throw new InvalidGraphQLRequestError(
-        'Unsupported persisted query version',
-      );
+      return await emitErrorAndThrow(
+        new InvalidGraphQLRequestError('Unsupported persisted query version'));
     }
 
     // We'll store a reference to the persisted query cache so we can actually
@@ -164,15 +163,14 @@ export async function processGraphQLRequest<TContext>(
       if (query) {
         metrics.persistedQueryHit = true;
       } else {
-        throw new PersistedQueryNotFoundError();
+        return await emitErrorAndThrow(new PersistedQueryNotFoundError());
       }
     } else {
       const computedQueryHash = computeQueryHash(query);
 
       if (queryHash !== computedQueryHash) {
-        throw new InvalidGraphQLRequestError(
-          'provided sha does not match query',
-        );
+        return await emitErrorAndThrow(
+          new InvalidGraphQLRequestError('provided sha does not match query'));
       }
 
       // We won't write to the persisted query cache until later.
@@ -186,7 +184,8 @@ export async function processGraphQLRequest<TContext>(
     // now, but this should be replaced with the new operation ID algorithm.
     queryHash = computeQueryHash(query);
   } else {
-    throw new InvalidGraphQLRequestError('Must provide query string.');
+    return await emitErrorAndThrow(
+      new InvalidGraphQLRequestError('Must provide query string.'));
   }
 
   requestContext.queryHash = queryHash;
@@ -313,6 +312,14 @@ export async function processGraphQLRequest<TContext>(
       // for the time-being this just maintains existing behavior for what
       // happens when `throw`-ing an `HttpQueryError` in `didResolveOperation`.
       if (err instanceof HttpQueryError) {
+        // In order to report this error reliably to the request pipeline, we'll
+        // have to regenerate it with the original error message and stack for
+        // the purposes of the `didEncounterErrors` life-cycle hook (which
+        // expects `GraphQLError`s), but still throw the `HttpQueryError`, so
+        // the appropriate status code is enforced by `runHttpQuery.ts`.
+        const graphqlError = new GraphQLError(err.message);
+        graphqlError.stack = err.stack;
+        await didEncounterErrors([graphqlError]);
         throw err;
       }
       return await sendErrorResponse(err);
@@ -493,6 +500,26 @@ export async function processGraphQLRequest<TContext>(
     return requestContext.response!;
   }
 
+  /**
+   * Report an error via `didEncounterErrors` and then `throw` it.
+   *
+   * Prior to the introduction of this function, some errors were being thrown
+   * within the request pipeline and going directly to handling within
+   * the `runHttpQuery.ts` module, rather than first being reported to the
+   * plugin API's `didEncounterErrors` life-cycle hook (where they are to be
+   * expected!).
+   *
+   * @param error The error to report to the request pipeline plugins prior
+   *              to being thrown.
+   *
+   * @throws
+   *
+   */
+  async function emitErrorAndThrow(error: GraphQLError): Promise<never> {
+    await didEncounterErrors([error]);
+    throw error;
+  }
+
   async function didEncounterErrors(errors: ReadonlyArray<GraphQLError>) {
     requestContext.errors = errors;
     extensionStack.didEncounterErrors(errors);