Repository: mcollina/fastify-gql (js)

Issue:
Error Formatting Breaks with GraphQL JIT Enabled
Error formatting and GraphQL JIT cannot both be enabled.

**Observed behaviour:**
- Apply custom error formatter
- Enable GraphQL JIT (`jit: 1`)
- First error is formatted correctly
- Following errors are returned directly i.e. not formatted

Happy to look into this when I get some time later in the week, JIT is showing a worthwhile performance improvement for us.


Pull request description:
Add and fix test case for error formatting with JIT enabled
Fix #223 

Patch:
diff --git a/index.js b/index.js
index 9242808..1aa31f5 100644
--- a/index.js
+++ b/index.js
@@ -29,7 +29,7 @@ const buildFederationSchema = require('./lib/federation')
 const buildGateway = require('./lib/gateway')
 const mq = require('mqemitter')
 const { PubSub } = require('./lib/subscriber')
-const { ErrorWithProps, FEDERATED_ERROR } = require('./lib/errors')
+const { ErrorWithProps, defaultErrorFormatter } = require('./lib/errors')
 const persistedQueryDefaults = require('./lib/persistedQueryDefaults')
 
 const kLoaders = Symbol('fastify-gql.loaders')
@@ -61,6 +61,7 @@ const plugin = fp(async function (app, opts) {
 
   const minJit = opts.jit || 0
   const queryDepthLimit = opts.queryDepth
+  const errorFormatter = typeof opts.errorFormatter === 'function' ? opts.errorFormatter : defaultErrorFormatter
 
   if (opts.persistedQueries) {
     if (opts.onlyPersisted) {
@@ -399,17 +400,10 @@ const plugin = fp(async function (app, opts) {
     if (cached && cached.jit !== null) {
       const res = await cached.jit.query(root, context, variables || {})
 
-      // as `graphql-jit` takes the original response that's dispatched
-      // by the request handler none of the transformations applied do have an effect,
-      // so `FederatedError` needs to be exchanged
-      // with the contents of its `extensions.errors` property and flattened afterwards
       if (res.errors) {
-        res.errors = res.errors.map((error, idx) => {
-          if (error.message === FEDERATED_ERROR.toString()) {
-            return error.extensions.errors
-          }
-          return error
-        }).reduce((acc, val) => acc.concat(val), [])
+        const { response: { data, errors } } = errorFormatter(res)
+        res.data = data
+        res.errors = errors
       }
 
       return res
diff --git a/lib/errors.js b/lib/errors.js
index fc0fb54..ba99c97 100644
--- a/lib/errors.js
+++ b/lib/errors.js
@@ -1,6 +1,6 @@
 'use strict'
 
-const { GraphQLError } = require('graphql')
+const { formatError, GraphQLError } = require('graphql')
 
 class ErrorWithProps extends Error {
   constructor (message, extensions) {
@@ -59,9 +59,34 @@ function toGraphQLError (err) {
   })
 }
 
+function defaultErrorFormatter (err) {
+  let errors = [{ message: err.message }]
+
+  if (err.errors) {
+    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)))
+      }
+      return error instanceof GraphQLError ? formatError(error) : { message: error.message }
+      // 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 {
+    statusCode: err.data ? 200 : /* istanbul ignore next */ (err.statusCode || 500),
+    response: {
+      data: err.data || null,
+      errors
+    }
+  }
+}
+
 module.exports = {
   FEDERATED_ERROR: FEDERATED_ERROR,
   ErrorWithProps: ErrorWithProps,
   FederatedError: FederatedError,
-  toGraphQLError: toGraphQLError
+  toGraphQLError: toGraphQLError,
+  defaultErrorFormatter: defaultErrorFormatter
 }
diff --git a/lib/routes.js b/lib/routes.js
index 8b14a1f..65a6060 100644
--- a/lib/routes.js
+++ b/lib/routes.js
@@ -3,9 +3,8 @@
 const { join } = require('path')
 const Static = require('fastify-static')
 const { BadRequest } = require('http-errors')
-const { formatError, GraphQLError } = require('graphql')
 const subscription = require('./subscription')
-const { FEDERATED_ERROR, toGraphQLError } = require('./errors')
+const { defaultErrorFormatter } = require('./errors')
 
 const responseProperties = {
   data: {
@@ -97,30 +96,6 @@ const postSchema = (allowBatchedQueries) => ({
   }
 })
 
-function defaultErrorFormatter (err) {
-  let errors = [{ message: err.message }]
-
-  if (err.errors) {
-    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)))
-      }
-      return error instanceof GraphQLError ? formatError(error) : { message: error.message }
-      // 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 {
-    statusCode: err.data ? 200 : /* istanbul ignore next */ (err.statusCode || 500),
-    response: {
-      data: err.data || null,
-      errors
-    }
-  }
-}
-
 function validationHandler (validationError) {
   if (validationError) {
     const err = new BadRequest()


Test patch:
diff --git a/test/errors.js b/test/errors.js
index 81d2cd9..7d7fe5a 100644
--- a/test/errors.js
+++ b/test/errors.js
@@ -425,3 +425,63 @@ test('POST query with a resolver which which throws and a custom error formatter
   t.equal(res.statusCode, 200)
   t.deepEqual(JSON.parse(res.body), { data: null, errors: [{ message: 'Internal Server Error' }] })
 })
+
+test('POST query which throws, with custom error formatter and JIT enabled, twice', 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,
+    jit: 1,
+    errorFormatter: () => ({
+      statusCode: 200,
+      response: {
+        data: null,
+        errors: [{ message: 'Internal Server Error' }]
+      }
+    })
+  })
+
+  let 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' }] })
+
+  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' }] })
+})