Fix regression caused by #3614 when using APQ with Graph Manage… (#3638)

The fix in #3614 changed behavior which was not surfacing errors to the
extensions and request pipeline plugins when those errors occurred during
APQ negotiation.

However, it failed to consider - nor were there any tests - which ensured
that the `apollo-engine-reporting`'s mechanism didn't receive an error
earlier in the request pipeline than previously allowed.

This applies a fix which special-cases the APQ error in this case, and
avoids reporting it to Apollo Graph Manager (which is the same behavior as
before).  We also introduce a test for exercising this regression, though it's
certainly a test which makes sure that the A-E-R functionality works, not
something that tests that errors are not thrown before the expected time
in a more general "extensions"-based sense.

CHANGELOG.md

@@ -9,6 +9,7 @@ The version headers in this history reflect the versions of Apollo Server itself
 - `apollo-server-core`: Upgrade TS to 3.7.3 [#3618](https://github.com/apollographql/apollo-server/pull/3618)
 - `apollo-server-cloud-functions`: Transmit CORS headers on `OPTIONS` request. [PR #3557](https://github.com/apollographql/apollo-server/pull/3557)
 - `apollo-server-caching`: De-compose options interface for `KeyValueCache.prototype.set` to accommodate better TSDoc annotations for its properties (e.g. to specify that `ttl` is defined in _seconds_). [PR #3619](https://github.com/apollographql/apollo-server/pull/3619)
+- `apollo-engine-reporting`: Fix regression introduced by [#3614](https://github.com/apollographql/apollo-server/pull/3614) which caused `PersistedQueryNotFoundError`, `PersistedQueryNotSupportedError` and `InvalidGraphQLRequestError` errors to be triggered before the `requestDidStart` handler triggered `treeBuilder`'s `startTiming` method. This fix preserves the existing behavior by special-casing these specific errors.  [PR #3638](https://github.com/apollographql/apollo-server/pull/3638) [Issue #3627](https://github.com/apollographql/apollo-server/issues/3627)
 
 ### v2.9.14
 

packages/apollo-engine-reporting/package.json

@@ -15,6 +15,7 @@
     "apollo-graphql": "^0.3.4",
     "apollo-server-caching": "file:../apollo-server-caching",
     "apollo-server-env": "file:../apollo-server-env",
+    "apollo-server-errors": "file:../apollo-server-errors",
     "apollo-server-types": "file:../apollo-server-types",
     "async-retry": "^1.2.1",
     "graphql-extensions": "file:../graphql-extensions"

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

@@ -5,6 +5,11 @@ import {
   responsePathAsArray,
 } from 'graphql';
 import { Trace, google } from 'apollo-engine-reporting-protobuf';
+import {
+  PersistedQueryNotFoundError,
+  PersistedQueryNotSupportedError,
+} from 'apollo-server-errors';
+import { InvalidGraphQLRequestError } from "apollo-server-types";
 
 function internalError(message: string) {
   return new Error(`[internal apollo-server error] ${message}`);
@@ -77,6 +82,12 @@ export class EngineReportingTreeBuilder {
 
   public didEncounterErrors(errors: GraphQLError[]) {
     errors.forEach(err => {
+      if (err instanceof PersistedQueryNotFoundError ||
+        err instanceof PersistedQueryNotSupportedError ||
+        err instanceof InvalidGraphQLRequestError) {
+        return;
+      }
+
       // This is an error from a federated service. We will already be reporting
       // it in the nested Trace in the query plan.
       //

packages/apollo-engine-reporting/tsconfig.json

@@ -8,6 +8,7 @@
   "exclude": ["**/__tests__", "**/__mocks__"],
   "references": [
     { "path": "../graphql-extensions" },
+    { "path": "../apollo-server-errors" },
     { "path": "../apollo-server-types" },
   ]
 }

packages/apollo-server-integration-testsuite/src/ApolloServer.ts

@@ -1084,6 +1084,55 @@ export function testApolloServer<AS extends ApolloServerBase>(
             );
           });
 
+          it("doesn't internal server error on an APQ", async () => {
+            await setupApolloServerAndFetchPair();
+
+            const TEST_STRING_QUERY = `
+              { onlyForThisApqTest${
+                Math.random()
+                  .toString()
+                  .split('.')[1]
+              }: justAField }
+            `;
+            const hash = sha256
+              .create()
+              .update(TEST_STRING_QUERY)
+              .hex();
+
+            const result = await apolloFetch({
+            // @ts-ignore The `ApolloFetch` types don't allow `extensions` to be
+            // passed in, in the same way as `variables`, with a request. This
+            // is a typing omission in `apollo-fetch`, as can be seen here:
+            // https://git.io/Jeb63  This will all be going away soon (and
+            // that package is already archived and deprecated.
+              extensions: {
+                persistedQuery: {
+                  version: VERSION,
+                  sha256Hash: hash,
+                }
+              },
+            });
+
+            // Having a persisted query not found error is fine.
+            expect(result.errors).toContainEqual(
+              expect.objectContaining({
+                extensions: expect.objectContaining({
+                  code: "PERSISTED_QUERY_NOT_FOUND",
+                })
+              })
+            );
+
+            // However, having an internal server error is not okay!
+            expect(result.errors).not.toContainEqual(
+              expect.objectContaining({
+                extensions: expect.objectContaining({
+                  code: "INTERNAL_SERVER_ERROR",
+                })
+              })
+            );
+
+          });
+
           describe('error munging', () => {
             describe('rewriteError', () => {
               it('new error', async () => {