Repository: mcollina/fastify-gql (js)

Issue:
Error Formatting Improvements
The `errorHandler` option controls the Fastify App Error handler directly, meaning:

- Users must fully rewrite the formatting of GraphQL errors in order to do error formatting.
- Batched query errors can't be formatted, as the overall response is a success. Errors should be formatted for each individual query, and the current implementation just sends the message of the error.

I propose a new option, `errorFormatter`. The Fastify GQL plugin will then:

- Set the app `errorHandler` using the `errorHandler` option, if provided, or if not false, set the default error handler.
- The default error handler will use either the `errorFormatter` if provided, or the default error formatter.

The `errorFormatter` option will take the form:

```js
(err: Error | GraphQLError | FastifyError) => {
    statusCode: number
    response: {
      data: object
      errors: object[]
    }
 }
```

For regular queries, the returned `statusCode` will be set, and the response will be set accordingly.
For batched queries, the `statusCode` will be ignored (set to 200) and the individual responses will be set accordingly.

An example usage of this option is:

```js
const errorParser = (e: GraphQLError) => {
    const { originalError, locations, path } = e
    const error = originalError as PlatformError

    let message
    if (error.name === 'PlatformError') {
        // Custom error formatter for internal usage
        message = getMessageForError(error.code, error.metadata)
    }

    if (!message) {
        // Error monitoring and reporting for unrecognised errors
        captureError(error)
    }

    // Default to internal server error
    return {
        message: message || 'Internal Server Error',
        locations,
        path
    }
}

const errorFormatter = (error: GraphQLServerError) => {
    if (!error.data) {
        switch (error.message) {
            // Send persisted query errors back in a formatted fashion
            case 'PersistedQueryNotFound':
                return {
                    statusCode: 200,
                    response: { data: null, errors: [{ message: 'PersistedQueryNotFound' }] }
                }
            case 'PersistedQueryNotSupported':
                return {
                    statusCode: 500,
                    response: { data: null, errors: [{ message: 'PersistedQueryNotSupported' }] }
                }
            // Send middleware errors directly
            default:
                return {
                    statusCode: error.statusCode || 500,
                    response: error.message
                }
        }
    }

    // Format all other errors as a valid GraphQL response
    const errors = (error.errors || [error]).map(errorParser)
    const data = error.data || null

    return {
        statusCode: 200,
        response: { errors, data }
    }
}
```

Pull request description:
Error Formatter Option
Fix #196 

PR based on top of #187 (was required for internal usage and testing in production). 


Patch:
diff --git a/README.md b/README.md
index fd5b150..3bc474a 100644
--- a/README.md
+++ b/README.md
@@ -713,6 +713,7 @@ __fastify-gql__ supports the following options:
 * `prefix`: String. Change the route prefix of the graphql endpoint if enabled.
 * `defineMutation`: Boolean. Add the empty Mutation definition if schema is not defined (Default: `false`).
 * `errorHandler`: `Function`  or `boolean`. Change the default error handler (Default: `true`). _Note: If a custom error handler is defined, it should return the standardized response format according to [GraphQL spec](https://graphql.org/learn/serving-over-http/#response)._
+* `errorFormatter`: `Function`. Change the default error formatter. Allows the status code of the response to be set, and a GraphQL response for the error to be defined. This can be used to format errors for batched queries, which return a successful response overall but individual errors, or to obfuscate or format internal errors.
 * `queryDepth`: `Integer`. The maximum depth allowed for a single query. _Note: GraphiQL IDE (or Playground IDE) sends an introspection query when it starts up. This query has a depth of 7 so when the `queryDepth` value is smaller than 7 this query will fail with a `Bad Request` error_
 * `validationRules`: `Function` or `Function[]`. Optional additional validation rules that the queries must satisfy in addition to those defined by the GraphQL specification. When using `Function`, arguments include additional data from graphql request and the return value must be validation rules `Function[]`.
 * `subscription`: Boolean | Object. Enable subscriptions. It is uses [mqemitter](https://github.com/mcollina/mqemitter) when it is true. To use a custom emitter set the value to an object containing the emitter.
diff --git a/index.d.ts b/index.d.ts
index b85c1dc..9b2a954 100644
--- a/index.d.ts
+++ b/index.d.ts
@@ -12,6 +12,7 @@ import {
   DocumentNode,
   ExecutionResult,
   GraphQLSchema,
+  GraphQLError,
   Source,
   GraphQLResolveInfo,
   GraphQLIsTypeOfFn,
@@ -132,6 +133,18 @@ export interface FastifyGQLOptions {
         request: FastifyRequest,
         reply: FastifyReply
       ) => ExecutionResult);
+  /**
+   * Change the default error formatter.
+   */
+  errorFormatter?: ((
+    error: FastifyError | GraphQLError | Error
+  ) => {
+    statusCode?: number
+    response?: {
+      data?: object
+      errors: any[]
+    }
+  });
   /**
    * The maximum depth allowed for a single query.
    */
@@ -228,7 +241,6 @@ declare namespace fastifyGQL {
     notSupportedError?: string;
   }
 
- 
 
   /**
    * Extended errors for adding additional information in error responses
diff --git a/index.js b/index.js
index fc7d5cf..9242808 100644
--- a/index.js
+++ b/index.js
@@ -156,8 +156,10 @@ const plugin = fp(async function (app, opts) {
 
   app.addHook('onReady', async function () {
     const schemaValidationErrors = validateSchema(fastifyGraphQl.schema)
-    if (schemaValidationErrors.length > 0) {
-      const err = new Error('schema issues')
+    if (schemaValidationErrors.length === 1) {
+      throw schemaValidationErrors[0]
+    } else if (schemaValidationErrors.length > 1) {
+      const err = new Error('Schema issues, check out the .errors property on the Error.')
       err.errors = schemaValidationErrors
       throw err
     }
@@ -169,6 +171,7 @@ const plugin = fp(async function (app, opts) {
     const optsIde = opts.graphiql || opts.ide
     app.register(routes, {
       errorHandler: opts.errorHandler,
+      errorFormatter: opts.errorFormatter,
       ide: optsIde,
       prefix: opts.prefix,
       path: opts.path,
diff --git a/lib/routes.js b/lib/routes.js
index 167c242..293e703 100644
--- a/lib/routes.js
+++ b/lib/routes.js
@@ -97,23 +97,11 @@ const postSchema = (allowBatchedQueries) => ({
   }
 })
 
-async function defaultErrorHandler (err, request, reply) {
-  if (err.data) {
-    reply.code(200)
-  } else {
-    /* istanbul ignore if */
-    if (!err.statusCode) {
-      // The errors that lands here should have the statusCode set.
-      // If they don't, we have a bug to fix.
-      reply.log.trace({ err }, 'error without status code, report this as a bug')
-      reply.code(500)
-    } else {
-      reply.code(err.statusCode)
-    }
-  }
+function defaultErrorFormatter (err) {
+  let errors = [{ message: err.message }]
 
   if (err.errors) {
-    const errors = err.errors.map((error, idx) => {
+    errors = err.errors.map((error, idx) => {
       // parses, converts & combines errors if they are the result of a federated request
       if (error.message === FEDERATED_ERROR.toString()) {
         return error.extensions.errors.map(err => formatError(toGraphQLError(err)))
@@ -122,14 +110,13 @@ async function defaultErrorHandler (err, request, reply) {
       // as the result of the outer map could potentially contain arrays with federated errors
       // the result needs to be flattened
     }).reduce((acc, val) => acc.concat(val), [])
+  }
 
-    return { errors, data: err.data || null }
-  } else {
-    return {
-      errors: [
-        { message: err.message }
-      ],
-      data: err.data || null
+  return {
+    statusCode: err.data ? 200 : /* istanbul ignore next */ (err.statusCode || 500),
+    response: {
+      data: err.data || null,
+      errors
     }
   }
 }
@@ -143,10 +130,15 @@ function validationHandler (validationError) {
 }
 
 module.exports = async function (app, opts) {
+  const errorFormatter = typeof opts.errorFormatter === 'function' ? opts.errorFormatter : defaultErrorFormatter
+
   if (typeof opts.errorHandler === 'function') {
     app.setErrorHandler(opts.errorHandler)
   } else if (opts.errorHandler === true || opts.errorHandler === undefined) {
-    app.setErrorHandler(defaultErrorHandler)
+    app.setErrorHandler((error, _, reply) => {
+      const { statusCode, response } = errorFormatter(error)
+      reply.code(statusCode).send(response)
+    })
   }
   const contextFn = opts.context
 
@@ -296,7 +288,10 @@ module.exports = async function (app, opts) {
       // Batched query
       return Promise.all(request.body.map(r =>
         execute(r, request, reply)
-          .catch(e => ({ errors: [{ message: e.message }] }))
+          .catch(e => {
+            const { response } = errorFormatter(e)
+            return response
+          })
       ))
     } else {
       // Regular query
diff --git a/package.json b/package.json
index ca327fe..a1a5968 100644
--- a/package.json
+++ b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "fastify-gql",
-  "version": "5.0.0",
+  "version": "5.0.1",
   "description": "Fastify GraphQL adapter",
   "main": "index.js",
   "types": "index.d.ts",
@@ -29,7 +29,7 @@
     "@typescript-eslint/parser": "^3.6.1",
     "autocannon": "^5.0.0",
     "fastify": "^3.0.2",
-    "graphql-tools": "^6.0.8",
+    "graphql-tools": "^6.0.14",
     "pre-commit": "^1.2.2",
     "proxyquire": "^2.1.3",
     "snazzy": "^8.0.0",


Test patch:
diff --git a/tap-snapshots/test-routes.js-TAP.test.js b/tap-snapshots/test-routes.js-TAP.test.js
index ec7602a..bad52c7 100644
--- a/tap-snapshots/test-routes.js-TAP.test.js
+++ b/tap-snapshots/test-routes.js-TAP.test.js
@@ -51,6 +51,7 @@ exports['test/routes.js TAP POST return 200 on resolver error > must match snaps
 
 exports['test/routes.js TAP POST return 400 on error > must match snapshot 1'] = `
 {
+  "data": null,
   "errors": [
     {
       "message": "Syntax Error: Expected Name, found <EOF>.",
@@ -61,19 +62,18 @@ exports['test/routes.js TAP POST return 400 on error > must match snapshot 1'] =
         }
       ]
     }
-  ],
-  "data": null
+  ]
 }
 `
 
 exports['test/routes.js TAP POST return 500 on error without statusCode > must match snapshot 1'] = `
 {
+  "data": null,
   "errors": [
     {
       "message": "Interface field Event.Id expected but CustomEvent does not provide it."
     }
-  ],
-  "data": null
+  ]
 }
 `
 
@@ -92,12 +92,12 @@ window.addEventListener('load', function(event) {
 
 exports['test/routes.js TAP mutation with GET errors > must match snapshot 1'] = `
 {
+  "data": null,
   "errors": [
     {
       "message": "Operation cannot be perfomed via a GET request"
     }
-  ],
-  "data": null
+  ]
 }
 `
 
@@ -125,6 +125,7 @@ exports['test/routes.js TAP server should return 200 on graphql errors (if field
 
 exports['test/routes.js TAP server should return 500 on graphql errors (if field can not be null) > must match snapshot 1'] = `
 {
+  "data": null,
   "errors": [
     {
       "message": "Simple error",
@@ -138,7 +139,6 @@ exports['test/routes.js TAP server should return 500 on graphql errors (if field
         "hello"
       ]
     }
-  ],
-  "data": null
+  ]
 }
 `
diff --git a/test/app-decorator.js b/test/app-decorator.js
index b6cf57e..98b0807 100644
--- a/test/app-decorator.js
+++ b/test/app-decorator.js
@@ -1044,6 +1044,47 @@ test('Error in schema', async (t) => {
     })
     await app.ready()
   } catch (error) {
+    t.equal(error.message, 'Interface field Event.Id expected but CustomEvent does not provide it.')
+    t.equal(error.name, 'GraphQLError')
+  }
+})
+
+test('Multiple errors in schema', async (t) => {
+  const schema = `
+    interface Event {
+      Id: Int!
+    }
+    type CustomEvent implements Event {
+      # Id needs to be specified here
+      Name: String!
+    }
+    type AnotherEvent implements Event {
+      # Id needs to be specified here
+      Name: String!
+    }
+    type Query {
+      listEvent: [Event]
+    }
+  `
+
+  const resolvers = {
+    listEvent: async () => []
+  }
+
+  const app = Fastify()
+
+  try {
+    app.register(GQL, {
+      schema,
+      resolvers
+    })
+    await app.ready()
+  } catch (error) {
+    t.equal(error.message, 'Schema issues, check out the .errors property on the Error.')
     t.equal(error.name, 'Error')
+    t.equal(error.errors[0].message, 'Interface field Event.Id expected but CustomEvent does not provide it.')
+    t.equal(error.errors[0].name, 'GraphQLError')
+    t.equal(error.errors[1].message, 'Interface field Event.Id expected but AnotherEvent does not provide it.')
+    t.equal(error.errors[1].name, 'GraphQLError')
   }
 })
diff --git a/test/batched.js b/test/batched.js
index ce32a0e..3bd4af4 100644
--- a/test/batched.js
+++ b/test/batched.js
@@ -108,7 +108,7 @@ test('POST single bad batched query', async (t) => {
     ]
   })
 
-  t.deepEqual(JSON.parse(res.body), [{ errors: [{ message: 'Bad Request' }] }])
+  t.deepEqual(JSON.parse(res.body), [{ data: null, errors: [{ message: 'Syntax Error: Expected "$", found <EOF>.', locations: [{ line: 2, column: 37 }] }] }])
 })
 
 test('POST batched query', async (t) => {
@@ -195,7 +195,7 @@ test('POST good and bad batched query', async (t) => {
     ]
   })
 
-  t.deepEqual(JSON.parse(res.body), [{ data: { add: 3 } }, { errors: [{ message: 'Bad Request' }] }])
+  t.deepEqual(JSON.parse(res.body), [{ data: { add: 3 } }, { data: null, errors: [{ message: 'Syntax Error: Expected "$", found <EOF>.', locations: [{ line: 1, column: 20 }] }] }])
 })
 
 test('POST batched query with a resolver which succeeds and a resolver which throws', async (t) => {
@@ -242,5 +242,58 @@ test('POST batched query with a resolver which succeeds and a resolver which thr
     ]
   })
 
-  t.deepEqual(JSON.parse(res.body), [{ data: { add: 3 } }, { errors: [{ message: 'Internal Server Error' }] }])
+  t.deepEqual(JSON.parse(res.body), [{ data: { add: 3 } }, { data: { bad: null }, errors: [{ message: 'Bad Resolver', locations: [{ line: 3, column: 17 }], path: ['bad'] }] }])
+})
+
+test('POST batched query with a resolver which succeeds and a resolver which throws, with a custom error formatter', async (t) => {
+  const app = Fastify()
+
+  const schema = `
+      type Query {
+        add(x: Int, y: Int): Int
+        bad: Int
+      }
+    `
+
+  const resolvers = {
+    add: async ({ x, y }) => x + y,
+    bad: () => { throw new Error('Bad Resolver') }
+  }
+
+  app.register(GQL, {
+    schema,
+    resolvers,
+    allowBatchedQueries: true,
+    errorFormatter: () => ({
+      response: {
+        data: null,
+        errors: [{ message: 'Internal Server Error' }]
+      }
+    })
+  })
+
+  const res = await app.inject({
+    method: 'POST',
+    url: '/graphql',
+    body: [
+      {
+        operationName: 'AddQuery',
+        variables: { x: 1, y: 2 },
+        query: `
+            query AddQuery ($x: Int!, $y: Int!) {
+                add(x: $x, y: $y)
+            }`
+      },
+      {
+        operationName: 'BadQuery',
+        variables: { x: 1 },
+        query: `
+            query BadQuery {
+                bad
+            }`
+      }
+    ]
+  })
+
+  t.deepEqual(JSON.parse(res.body), [{ data: { add: 3 } }, { data: null, errors: [{ message: 'Internal Server Error' }] }])
 })
diff --git a/test/errors.js b/test/errors.js
index 652ad27..81d2cd9 100644
--- a/test/errors.js
+++ b/test/errors.js
@@ -382,3 +382,46 @@ test('errors - federated errors without locations, path and extensions', async (
   t.equal(jitres.statusCode, 200)
   t.deepEqual(JSON.parse(jitres.payload), expectedResult)
 })
+
+test('POST query with a resolver which which throws and a custom error formatter', async (t) => {
+  const app = Fastify()
+
+  const schema = `
+      type Query {
+        bad: Int
+      }
+    `
+
+  const resolvers = {
+    bad: () => { throw new Error('Bad Resolver') }
+  }
+
+  app.register(GQL, {
+    schema,
+    resolvers,
+    allowBatchedQueries: true,
+    errorFormatter: () => ({
+      statusCode: 200,
+      response: {
+        data: null,
+        errors: [{ message: 'Internal Server Error' }]
+      }
+    })
+  })
+
+  const res = await app.inject({
+    method: 'POST',
+    url: '/graphql',
+    body: {
+      operationName: 'BadQuery',
+      variables: { x: 1 },
+      query: `
+          query BadQuery {
+              bad
+          }`
+    }
+  })
+
+  t.equal(res.statusCode, 200)
+  t.deepEqual(JSON.parse(res.body), { data: null, errors: [{ message: 'Internal Server Error' }] })
+})