Repository: mcollina/fastify-gql
Pull request #248 (2020-08-11 11:28:26)

Fix inconsistency in handling errors
Fixes https://github.com/fastify/help/issues/227

mcollina: cc @paul590 @RafaelGSS 
pscarey: Other than comment, looks good to me.

Patch:
diff --git a/index.js b/index.js
index af29577..34093ef 100644
--- a/index.js
+++ b/index.js
@@ -3,14 +3,13 @@
 const fp = require('fastify-plugin')
 const LRU = require('tiny-lru')
 const routes = require('./lib/routes')
-const { BadRequest, MethodNotAllowed, InternalServerError } = require('http-errors')
+const { BadRequest, MethodNotAllowed } = require('http-errors')
 const { compileQuery } = require('graphql-jit')
 const { Factory } = require('single-user-cache')
 const {
   parse,
   buildSchema,
   getOperationAST,
-  GraphQLError,
   GraphQLObjectType,
   GraphQLScalarType,
   GraphQLEnumType,
@@ -425,15 +424,9 @@ const plugin = fp(async function (app, opts) {
     }
 
     if (cached && cached.jit !== null) {
-      const res = await cached.jit.query(root, context, variables || {})
+      const execution = await cached.jit.query(root, context, variables || {})
 
-      if (res.errors) {
-        const { response: { data, errors } } = errorFormatter(res)
-        res.data = data
-        res.errors = errors
-      }
-
-      return res
+      return maybeFormatErrors(execution, reply)
     }
 
     // Validate variables
@@ -455,19 +448,18 @@ const plugin = fp(async function (app, opts) {
       operationName
     )
 
+    return maybeFormatErrors(execution, reply)
+  }
+
+  function maybeFormatErrors (execution, reply) {
     if (execution.errors) {
-      execution.errors = execution.errors.map(e => {
-        if (e.originalError && (Object.prototype.hasOwnProperty.call(e.originalError, 'extensions'))) {
-          return new GraphQLError(e.originalError.message, e.nodes, e.source, e.positions, e.path, e.originalError, e.originalError.extensions)
-        }
-        return e
-      })
-      const err = new InternalServerError()
-      err.errors = execution.errors
-      err.data = execution.data
-      throw err
+      const { statusCode, response: { data, errors } } = errorFormatter(execution)
+      execution.data = data
+      execution.errors = errors
+      if (reply) {
+        reply.code(statusCode)
+      }
     }
-
     return execution
   }
 }, {


Test patch:
diff --git a/tap-snapshots/test-errors.js-TAP.test.js b/tap-snapshots/test-errors.js-TAP.test.js
new file mode 100644
index 0000000..d56bbbd
--- /dev/null
+++ b/tap-snapshots/test-errors.js-TAP.test.js
@@ -0,0 +1,50 @@
+/* IMPORTANT
+ * This snapshot file is auto-generated, but designed for humans.
+ * It should be checked into source control and tracked carefully.
+ * Re-generate by setting TAP_SNAPSHOT=1 and running tests.
+ * Make sure to inspect the output below.  Do not ignore changes!
+ */
+'use strict'
+exports['test/errors.js TAP POST query which throws, with JIT enabled, twice > must match snapshot 1'] = `
+{
+  "data": {
+    "bad": null
+  },
+  "errors": [
+    {
+      "message": "Bad Resolver",
+      "locations": [
+        {
+          "line": 3,
+          "column": 15
+        }
+      ],
+      "path": [
+        "bad"
+      ]
+    }
+  ]
+}
+`
+
+exports['test/errors.js TAP POST query which throws, with JIT enabled, twice > must match snapshot 2'] = `
+{
+  "data": {
+    "bad": null
+  },
+  "errors": [
+    {
+      "message": "Int cannot represent non-integer value: [function bad]",
+      "locations": [
+        {
+          "line": 3,
+          "column": 15
+        }
+      ],
+      "path": [
+        "bad"
+      ]
+    }
+  ]
+}
+`
diff --git a/tap-snapshots/test-routes.js-TAP.test.js b/tap-snapshots/test-routes.js-TAP.test.js
index bad52c7..dea5d8c 100644
--- a/tap-snapshots/test-routes.js-TAP.test.js
+++ b/tap-snapshots/test-routes.js-TAP.test.js
@@ -66,17 +66,6 @@ exports['test/routes.js TAP POST return 400 on error > must match snapshot 1'] =
 }
 `
 
-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."
-    }
-  ]
-}
-`
-
 exports['test/routes.js TAP if ide is graphiql, serve config.js with the correct endpoint > must match snapshot 1'] = `
 window.GRAPHQL_ENDPOINT = '/app/graphql'
 `
@@ -125,7 +114,6 @@ 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",
@@ -139,6 +127,7 @@ exports['test/routes.js TAP server should return 500 on graphql errors (if field
         "hello"
       ]
     }
-  ]
+  ],
+  "data": null
 }
 `
diff --git a/test/errors.js b/test/errors.js
index 7d7fe5a..436370d 100644
--- a/test/errors.js
+++ b/test/errors.js
@@ -255,6 +255,65 @@ test('errors - errors with jit enabled', async (t) => {
   })
 })
 
+test('errors - errors with jit enabled using the app decorator', async (t) => {
+  const schema = `
+    type Query {
+      error: String
+      successful: String
+    }
+  `
+
+  const resolvers = {
+    Query: {
+      error () {
+        throw new ErrorWithProps('Error', { code: 'ERROR', additional: 'information', other: 'data' })
+      },
+      successful () {
+        return 'Runs OK'
+      }
+    }
+  }
+
+  const app = Fastify()
+
+  app.register(GQL, {
+    schema,
+    resolvers,
+    jit: 1
+  })
+
+  await app.ready()
+
+  // jit it
+  await app.graphql('{error,successful}')
+
+  const payload = await app.graphql('{error,successful}')
+
+  t.deepEqual(payload, {
+    data: {
+      error: null,
+      successful: 'Runs OK'
+    },
+    errors: [
+      {
+        message: 'Error',
+        locations: [
+          {
+            line: 1,
+            column: 2
+          }
+        ],
+        path: ['error'],
+        extensions: {
+          code: 'ERROR',
+          additional: 'information',
+          other: 'data'
+        }
+      }
+    ]
+  })
+})
+
 test('errors - federated errors with jit enabled', async (t) => {
   const schema = `
     type Query {
@@ -485,3 +544,55 @@ test('POST query which throws, with custom error formatter and JIT enabled, twic
   t.equal(res.statusCode, 200)
   t.deepEqual(JSON.parse(res.body), { data: null, errors: [{ message: 'Internal Server Error' }] })
 })
+
+test('POST query which throws, with 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,
+    jit: 1
+  })
+
+  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.matchSnapshot(JSON.stringify(JSON.parse(res.body), null, 2))
+
+  res = await app.inject({
+    method: 'POST',
+    url: '/graphql',
+    body: {
+      operationName: 'BadQuery',
+      variables: { x: 1 },
+      query: `
+          query BadQuery {
+              bad
+          }`
+    }
+  })
+
+  t.equal(res.statusCode, 200)
+  t.matchSnapshot(JSON.stringify(JSON.parse(res.body), null, 2))
+})