Errors Lifecycle: user extensions > engine reporting > formatError (#…

…1272)

* enable willSendResponse to return a modified response

* add formatError as an extension that wraps engine reporting

* ensure that formatError once on every error path

* move old formatError express tests into integration suite

* add error lifecycle with minimal engine reporting check

* increase granularity of formatError test

* return 400 error for GraphQL error created by context

* add check for internal server error for errors thrown in context

* comment about context error status code

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

@@ -39,6 +39,8 @@ import {
   FileUploadOptions,
 } from './types';
 
+import { FormatErrorExtension } from './formatters';
+
 import { gql } from './index';
 
 const NoIntrospection = (context: ValidationContext) => ({
@@ -185,11 +187,23 @@ export class ApolloServerBase {
     // or cacheControl.
     this.extensions = [];
 
+    // Error formatting should happen after the engine reporting agent, so that
+    // engine gets the unmasked errors if necessary
+    if (this.requestOptions.formatError) {
+      this.extensions.push(
+        () =>
+          new FormatErrorExtension(
+            this.requestOptions.formatError,
+            this.requestOptions.debug,
+          ),
+      );
+    }
+
     if (engine || (engine !== false && process.env.ENGINE_API_KEY)) {
       this.engineReportingAgent = new EngineReportingAgent(
         engine === true ? {} : engine,
       );
-      // Let's keep this extension first so it wraps everything.
+      // Let's keep this extension second so it wraps everything, except error formatting
       this.extensions.push(() => this.engineReportingAgent.newExtension());
     }
 

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

@@ -0,0 +1,29 @@
+import { GraphQLExtension, GraphQLResponse } from 'graphql-extensions';
+import { formatApolloErrors } from 'apollo-server-errors';
+
+export class FormatErrorExtension extends GraphQLExtension {
+  private formatError: Function;
+  private debug: boolean;
+
+  public constructor(formatError: Function, debug: boolean = false) {
+    super();
+    this.formatError = formatError;
+    this.debug = debug;
+  }
+
+  public willSendResponse(o: {
+    graphqlResponse: GraphQLResponse;
+  }): void | { graphqlResponse: GraphQLResponse } {
+    if (o.graphqlResponse.errors) {
+      return {
+        graphqlResponse: {
+          ...o.graphqlResponse,
+          errors: formatApolloErrors(o.graphqlResponse.errors, {
+            formatter: this.formatError,
+            debug: this.debug,
+          }),
+        },
+      };
+    }
+  }
+}

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

@@ -69,18 +69,23 @@ export class HttpQueryError extends Error {
   }
 }
 
-function throwHttpGraphQLError(
-  statusCode,
-  errors: Array<Error>,
-  optionsObject,
+/**
+ * If optionsObject is specified, then the errors array will be formatted
+ */
+function throwHttpGraphQLError<E extends Error>(
+  statusCode: number,
+  errors: Array<E>,
+  optionsObject?: Partial<GraphQLOptions>,
 ) {
   throw new HttpQueryError(
     statusCode,
     prettyJSONStringify({
-      errors: formatApolloErrors(errors, {
-        debug: optionsObject.debug,
-        formatter: optionsObject.formatError,
-      }),
+      errors: optionsObject
+        ? formatApolloErrors(errors, {
+            debug: optionsObject.debug,
+            formatter: optionsObject.formatError,
+          })
+        : errors,
     }),
     true,
     {
@@ -302,7 +307,17 @@ export async function runHttpQuery(
           context = await context();
         } catch (e) {
           e.message = `Context creation failed: ${e.message}`;
-          throwHttpGraphQLError(500, [e], optionsObject);
+          // For errors that are not internal, such as authentication, we
+          // should provide a 400 response
+          if (
+            e.extensions &&
+            e.extensions.code &&
+            e.extensions.code !== 'INTERNAL_SERVER_ERROR'
+          ) {
+            throwHttpGraphQLError(400, [e], optionsObject);
+          } else {
+            throwHttpGraphQLError(500, [e], optionsObject);
+          }
         }
       } else {
         // Always clone the context if it's not a function, because that preserves
@@ -402,16 +417,23 @@ export async function runHttpQuery(
         throw e;
       }
 
+      // This error will be uncaught, so we need to wrap it and treat it as an
+      // internal server error
       return {
-        errors: formatApolloErrors([e], {
-          formatter: optionsObject.formatError,
-          debug: optionsObject.debug,
-        }),
+        errors: formatApolloErrors([e], optionsObject),
       };
     }
   }) as Array<Promise<ExecutionResult & { extensions?: Record<string, any> }>>;
 
-  const responses = await Promise.all(requests);
+  let responses;
+  try {
+    responses = await Promise.all(requests);
+  } catch (e) {
+    if (e.name === 'HttpQueryError') {
+      throw e;
+    }
+    throwHttpGraphQLError(500, [e], optionsObject);
+  }
 
   const responseInit: ApolloServerHttpResponse = {
     headers: {
@@ -449,7 +471,8 @@ export async function runHttpQuery(
     // This code is run on parse/validation errors and any other error that
     // doesn't reach GraphQL execution
     if (graphqlResponse.errors && typeof graphqlResponse.data === 'undefined') {
-      throwHttpGraphQLError(400, graphqlResponse.errors as any, optionsObject);
+      // don't include optionsObject, since the errors have already been formatted
+      throwHttpGraphQLError(400, graphqlResponse.errors as any);
     }
     const stringified = prettyJSONStringify(graphqlResponse);
 

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

@@ -159,7 +159,6 @@ function doRunQuery(options: QueryOptions): Promise<GraphQLResponse> {
               }),
             ],
             {
-              formatter: options.formatError,
               debug,
             },
           );
@@ -199,7 +198,6 @@ function doRunQuery(options: QueryOptions): Promise<GraphQLResponse> {
                 fromGraphQLError(err, { errorClass: ValidationError }),
               ),
               {
-                formatter: options.formatError,
                 debug,
               },
             );
@@ -250,7 +248,6 @@ function doRunQuery(options: QueryOptions): Promise<GraphQLResponse> {
 
           if (result.errors) {
             response.errors = formatApolloErrors([...result.errors], {
-              formatter: options.formatError,
               debug,
             });
           }
@@ -277,8 +274,8 @@ function doRunQuery(options: QueryOptions): Promise<GraphQLResponse> {
       throw err;
     })
     .then(graphqlResponse => {
-      extensionStack.willSendResponse({ graphqlResponse });
+      const response = extensionStack.willSendResponse({ graphqlResponse });
       requestDidEnd();
-      return graphqlResponse;
+      return response.graphqlResponse;
     });
 }

packages/apollo-server-express/src/apolloServerHttp.test.ts

@@ -417,74 +417,6 @@ describe(`GraphQL-HTTP (apolloServer) tests for ${version} express`, () => {
       expect(response.status).to.equal(400);
     });
 
-    it('allows for custom error formatting to sanitize', async () => {
-      const app = express();
-
-      app.use('/graphql', bodyParser.json());
-      app.use(
-        '/graphql',
-        graphqlExpress({
-          schema: TestSchema,
-          formatError(error) {
-            return { message: 'Custom error format: ' + error.message };
-          },
-        }),
-      );
-
-      const response = await request(app)
-        .post('/graphql')
-        .send({
-          query: '{thrower}',
-        });
-
-      expect(response.status).to.equal(200);
-      expect(JSON.parse(response.text)).to.deep.equal({
-        data: null,
-        errors: [
-          {
-            message: 'Custom error format: Throws!',
-          },
-        ],
-      });
-    });
-
-    it('allows for custom error formatting to elaborate', async () => {
-      const app = express();
-
-      app.use('/graphql', bodyParser.json());
-      app.use(
-        '/graphql',
-        graphqlExpress({
-          schema: TestSchema,
-          formatError(error) {
-            return {
-              message: error.message,
-              locations: error.locations,
-              stack: 'Stack trace',
-            };
-          },
-        }),
-      );
-
-      const response = await request(app)
-        .post('/graphql')
-        .send({
-          query: '{thrower}',
-        });
-
-      expect(response.status).to.equal(200);
-      expect(JSON.parse(response.text)).to.deep.equal({
-        data: null,
-        errors: [
-          {
-            message: 'Throws!',
-            locations: [{ line: 1, column: 2 }],
-            stack: 'Stack trace',
-          },
-        ],
-      });
-    });
-
     it('handles unsupported HTTP methods', async () => {
       const app = express();
 

packages/apollo-server-hapi/src/ApolloServer.test.ts

@@ -212,7 +212,6 @@ describe('apollo-server-hapi', () => {
 
       const apolloFetch = createApolloFetch({ uri }).useAfter(
         (response, next) => {
-          console.log(response.response.headers);
           expect(
             response.response.headers.get('access-control-expose-headers'),
           ).to.deep.equal(

packages/apollo-server-integration-testsuite/package.json

@@ -26,11 +26,15 @@
     "apollo-server-core": "^2.0.0-rc.5"
   },
   "devDependencies": {
+    "@types/body-parser": "1.17.0",
+    "apollo-engine-reporting-protobuf": "0.0.0-beta.7",
     "apollo-fetch": "^0.7.0",
     "apollo-link": "^1.2.2",
     "apollo-link-http": "^1.5.4",
     "apollo-link-persisted-queries": "^0.2.1",
     "apollo-server-env": "^2.0.0-rc.3",
+    "body-parser": "^1.18.3",
+    "graphql-extensions": "^0.1.0-beta.15",
     "graphql-subscriptions": "^0.5.8",
     "graphql-tag": "^2.9.2",
     "js-sha256": "^0.9.0",