Repository: mcollina/fastify-gql (js)

Issue:
Error propagation/serialization when using Gateway mode
## Situation

I've added a new resolver to the `lib/gateway.js` example, in addition to the `me` resolver
there's now also a `you` resolver that looks like this:

```js
Query: {
  // ...
  you: (root, args, context, info) => {
    throw new ErrorWithProps(`Can't fetch other users data`, { code: 'NOT_ALLOWED' })
  },
  // ...
},
```

I've, of course, also extended the schema:

```js
  await createService(4001, `
    extend type Query {
      me: User
      you: User
      hello: String
    }
    // ...`
```

## Expectation

I've looked for a common pattern/specification on how a federated service should deal with these kind of errors,
which are being propagated from a federated service, via the gateway, to a client. But either my "google-foo" has become weak,
or there is just nothing out there describing what should happen in this case.

My personal expectation would be, that errors from a federated services would be collected by the gateway & propagated "as is" as part of the `errors` property of the final response which is being dispatched from the gateway to the client.
Taking the example from above, I'd expect to see the following as the result of calling the gateway with this query:

```
{
  you {
    id
    name
  }
}

```

```json
{
  "data": {
    "me": null
  },
  "errors": [
    {
      "message": "Can't fetch other users data",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "you"
      ],
      "extensions": {
        "code": "NOT_ALLOWED"
      }
    }
  ]
}
```

## Problem

Unfortunately that's not the case, even worse, I'll get different results when running the query for the first time (e.g. non jit'ed),
and for subsequent runs (when graphql-jit kicked in):

### Non jit'ed result

```json
{
  "data": {
    "you": null
  },
  "errors": [
    {
      "message": "Unexpected error value: [{ message: \"Can't fetch other users data\", locations: [Array], path: [Array], extensions: [Object] }]",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "you"
      ]
    }
  ]
}
```

### Result of subsequent runs with active graphql-jit

```json
{
  "data": {
    "you": null
  },
  "errors": [
    {
      "message": "[object Object]",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "you"
      ]
    }
  ]
}
```

## Reason behind this behaviour

`graphql` expects an error (or a rejected result) to be an instance of `Error`; see https://github.com/graphql/graphql-js/blob/74ce729eddf7c777a1c500d63f1c4fce35e9f1cf/src/execution/execute.js#L683-L720

Otherwise it converts it to an error on its own `return new Error('Unexpected error value: ' + inspect(error));`, while `inspect` serializes the error to a string, stripping out all other defined fields. `fastify-gql` on the other hand just `rejects` in an error case with the raw `json.errors` array, triggering the unfortunate serialization behaviour that way; see https://github.com/mcollina/fastify-gql/blob/master/lib/gateway/request.js#L92

Interestingly, `graphql-jit` is happy if https://github.com/mcollina/fastify-gql/blob/master/lib/gateway/request.js#L92 would reject with an array of `GraphQLError` instances (I first tried to apply the mapping there), responding with the desired result, but `graphql` does not like anything except something extending `Error`, and still producing a badly serialized result.

## Solution (well, kind of)

I've forked `fastify-gql` and added a branch with the changes that lead to my desired result (with comments for each change), but I guess it might be helpful to walk through them one by one.

* As `graphql` doesn't cope well with errors not extending the built-in `Error`, I've added a `FederatedError` (https://github.com/asciidisco/fastify-gql/blob/fix/federated-error-propagation/lib/errors.js#L23) that holds the errors array from the response of the federated service as a property (https://github.com/asciidisco/fastify-gql/blob/fix/federated-error-propagation/lib/gateway/request.js#L95)

* I've modified the `defaultErrorHandler` so that an instance of a `FederatedError` can be flattened into a list of actual errors happened at the service level (https://github.com/asciidisco/fastify-gql/blob/fix/federated-error-propagation/lib/routes.js#L61-L74). As the original location of the error can't be mapped easily when creating a new `GraphQLError` (at least I had no success in doing so) and the `locations` property of a `GraphQLError` is read-only, I'm constructing a new error that just takes the original error values and inherits the prototype of `GraphQLError` to satisfy subsequent checks (https://github.com/asciidisco/fastify-gql/blob/698d31c1bc616cb54aef4388a8eeb61dae205c05/lib/errors.js#L35-L69)

* As `graphql-jit` isn't aware of our mapping in the `defaultErrorHandler` (also wan't sure here if it's intentional that the `defaultErrorHandler` isn't invoked in case we have a jit'ed result), we have to apply a similar transformation before sending the jit'ed result (https://github.com/asciidisco/fastify-gql/blob/fix/federated-error-propagation/index.js#L370-L380)

* To validate the solution, I've added the `you` query mentioned above to `examples/gateway`. (https://github.com/asciidisco/fastify-gql/blob/fix/federated-error-propagation/examples/gateway.js#L87-L89)

## Why not a PR right away?

* First of all, I'm not sure if I'm missing something obvious here (just started out with fastify-gql), so it could be very well that this is a problem on my side, missing how to properly propagate issues from a federated service, via the gateway, to the client

* As there seems to be no spec around this, I'm not sure if my desired result/expectation matches with what other users need/want. Therefore I believe there should be a discussion first, to define the desired outcome

* My solution does not seem very PR'able to me. Aside from missing tests & docs (which I would add if it would come to a PR): I'm absolutely not happy with my solution, codewise. It seems to work (produce my desired result), but the modifications I've made to the codebase do not seem ideal to me (nonetheless, I wasn't able to find a more elegant way to achieve the result); so I'm kinda relying on external input here.

Thanks for bearing with me through this all this prose, I hope you find some time to review this issue & share your thoughts with me.
Thank you.

Cheers
Sebastian

Pull request:
fix(gateway): propagate errors from sub-services in gateway mode
Fixes #171 

Patch:
diff --git a/examples/gateway.js b/examples/gateway.js
index 2e20441..08578e9 100644
--- a/examples/gateway.js
+++ b/examples/gateway.js
@@ -1,6 +1,7 @@
 'use strict'
 const Fastify = require('fastify')
 const GQL = require('..')
+const { ErrorWithProps } = GQL
 
 async function createService (port, schema, resolvers = {}) {
   const service = Fastify()
@@ -61,6 +62,7 @@ async function start () {
   await createService(4001, `
     extend type Query {
       me: User
+      you: User
       hello: String
     }
 
@@ -82,6 +84,9 @@ async function start () {
       me: (root, args, context, info) => {
         return users.u1
       },
+      you: (root, args, context, info) => {
+        throw new ErrorWithProps('Can\'t fetch other users data', { code: 'NOT_ALLOWED' })
+      },
       hello: () => 'world'
     },
     User: {
diff --git a/index.js b/index.js
index bb3c8dc..1469273 100644
--- a/index.js
+++ b/index.js
@@ -28,7 +28,7 @@ const buildFederationSchema = require('./lib/federation')
 const buildGateway = require('./lib/gateway')
 const mq = require('mqemitter')
 const { PubSub } = require('./lib/subscriber')
-const { ErrorWithProps } = require('./lib/errors')
+const { ErrorWithProps, FEDERATED_ERROR } = require('./lib/errors')
 
 const kLoaders = Symbol('fastify-gql.loaders')
 
@@ -360,6 +360,20 @@ 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), [])
+      }
+
       return res
     }
 
diff --git a/lib/errors.js b/lib/errors.js
index 86adc1b..fc0fb54 100644
--- a/lib/errors.js
+++ b/lib/errors.js
@@ -1,5 +1,7 @@
 'use strict'
 
+const { GraphQLError } = require('graphql')
+
 class ErrorWithProps extends Error {
   constructor (message, extensions) {
     super(message)
@@ -7,6 +9,59 @@ class ErrorWithProps extends Error {
   }
 }
 
+const FEDERATED_ERROR = Symbol('FEDERATED_ERROR')
+
+// a specialized `Error` which extends the `Error` built-in
+// to satisfy the `graphql` error handler
+class FederatedError extends Error {
+  constructor (errors) {
+    super(FEDERATED_ERROR.toString())
+    this.extensions = { errors }
+  }
+}
+
+// converts an error to a `GraphQLError` compatible
+// allows to copy the `path` & `locations` properties
+// from the already serialized error
+function toGraphQLError (err) {
+  return Object.create(GraphQLError, {
+    name: {
+      value: err.name
+    },
+    message: {
+      value: err.message,
+      enumerable: true,
+      writable: true
+    },
+    locations: {
+      value: err.locations || undefined,
+      enumerable: true
+    },
+    path: {
+      value: err.path || undefined,
+      enumerable: true
+    },
+    nodes: {
+      value: err.nodes || undefined
+    },
+    source: {
+      value: err.source || undefined
+    },
+    positions: {
+      value: err.positions || undefined
+    },
+    originalError: {
+      value: err.originalError || undefined
+    },
+    extensions: {
+      value: err.extensions || undefined
+    }
+  })
+}
+
 module.exports = {
-  ErrorWithProps: ErrorWithProps
+  FEDERATED_ERROR: FEDERATED_ERROR,
+  ErrorWithProps: ErrorWithProps,
+  FederatedError: FederatedError,
+  toGraphQLError: toGraphQLError
 }
diff --git a/lib/gateway/request.js b/lib/gateway/request.js
index 36a6bff..6b15901 100644
--- a/lib/gateway/request.js
+++ b/lib/gateway/request.js
@@ -2,6 +2,7 @@
 const http = require('http')
 const https = require('https')
 const eos = require('end-of-stream')
+const { FederatedError } = require('../errors')
 
 function agentOption (opts) {
   return {
@@ -89,7 +90,9 @@ function sendRequest (request, url) {
             const json = JSON.parse(data.toString())
 
             if (json.errors && json.errors.length) {
-              return reject(json.errors)
+              // return a `FederatedError` instance to keep `graphql` happy
+              // e.g. have something that derives from `Error`
+              return reject(new FederatedError(json.errors))
             }
 
             resolve({
diff --git a/lib/routes.js b/lib/routes.js
index 122abbc..8e13a98 100644
--- a/lib/routes.js
+++ b/lib/routes.js
@@ -5,6 +5,7 @@ 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 responseSchema = {
   '2xx': {
@@ -57,9 +58,15 @@ async function defaultErrorHandler (err, request, reply) {
   }
 
   if (err.errors) {
-    const errors = err.errors.map(error => {
+    const 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 { errors, data: err.data || null }
   } else {


Test patch:
diff --git a/test/errors.js b/test/errors.js
index ee8c60d..652ad27 100644
--- a/test/errors.js
+++ b/test/errors.js
@@ -4,6 +4,7 @@ const { test } = require('tap')
 const Fastify = require('fastify')
 const GQL = require('..')
 const { ErrorWithProps } = GQL
+const { FederatedError } = require('../lib/errors')
 
 test('errors - multiple extended errors', async (t) => {
   const schema = `
@@ -253,3 +254,131 @@ test('errors - errors with jit enabled', async (t) => {
     ]
   })
 })
+
+test('errors - federated errors with jit enabled', async (t) => {
+  const schema = `
+    type Query {
+      error: String
+      successful: String
+    }
+  `
+
+  const resolvers = {
+    Query: {
+      error () {
+        throw new FederatedError([{
+          message: 'Invalid operation',
+          locations: [{ column: 3, line: 2 }],
+          path: ['error'],
+          extensions: {
+            code: 'ERROR',
+            additional: 'information',
+            other: 'data'
+          }
+        }])
+      },
+      successful () {
+        return 'Runs OK'
+      }
+    }
+  }
+
+  const app = Fastify()
+
+  app.register(GQL, {
+    schema,
+    resolvers,
+    jit: 1
+  })
+
+  await app.ready()
+
+  const res = await app.inject({
+    method: 'GET',
+    url: '/graphql?query={error,successful}'
+  })
+
+  const jitres = await app.inject({
+    method: 'GET',
+    url: '/graphql?query={error,successful}'
+  })
+
+  const expectedResult = {
+    data: {
+      error: null,
+      successful: 'Runs OK'
+    },
+    errors: [
+      {
+        message: 'Invalid operation',
+        locations: [{ column: 3, line: 2 }],
+        path: ['error'],
+        extensions: {
+          code: 'ERROR',
+          additional: 'information',
+          other: 'data'
+        }
+      }
+    ]
+  }
+
+  t.equal(res.statusCode, 200)
+  t.deepEqual(JSON.parse(res.payload), expectedResult)
+
+  t.equal(jitres.statusCode, 200)
+  t.deepEqual(JSON.parse(jitres.payload), expectedResult)
+})
+
+test('errors - federated errors without locations, path and extensions', async (t) => {
+  const schema = `
+    type Query {
+      error: String
+      successful: String
+    }
+  `
+
+  const resolvers = {
+    Query: {
+      error () {
+        throw new FederatedError([{ message: 'Invalid operation' }])
+      },
+      successful () {
+        return 'Runs OK'
+      }
+    }
+  }
+
+  const app = Fastify()
+
+  app.register(GQL, {
+    schema,
+    resolvers,
+    jit: 1
+  })
+
+  await app.ready()
+
+  const res = await app.inject({
+    method: 'GET',
+    url: '/graphql?query={error,successful}'
+  })
+
+  const jitres = await app.inject({
+    method: 'GET',
+    url: '/graphql?query={error,successful}'
+  })
+
+  const expectedResult = {
+    data: {
+      error: null,
+      successful: 'Runs OK'
+    },
+    errors: [{ message: 'Invalid operation' }]
+  }
+
+  t.equal(res.statusCode, 200)
+  t.deepEqual(JSON.parse(res.payload), expectedResult)
+
+  t.equal(jitres.statusCode, 200)
+  t.deepEqual(JSON.parse(jitres.payload), expectedResult)
+})
diff --git a/test/routes.js b/test/routes.js
index 1a696cd..cb13095 100644
--- a/test/routes.js
+++ b/test/routes.js
@@ -6,6 +6,7 @@ const querystring = require('querystring')
 const WebSocket = require('ws')
 const { GraphQLError } = require('graphql')
 const GQL = require('..')
+const { FederatedError } = require('../lib/errors')
 
 test('POST route', async (t) => {
   const app = Fastify()
@@ -1179,6 +1180,53 @@ test('route validation is catched and parsed to graphql error', async (t) => {
   t.strictDeepEqual(JSON.parse(res.body), expectedResult)
 })
 
+test('Error handler flattens federated errors', async (t) => {
+  const app = Fastify()
+  const schema = `
+    type Query {
+      add(x: Int, y: Int): Int
+    }
+  `
+
+  const resolvers = {
+    add: async ({ x, y }) => {
+      throw new FederatedError([{
+        message: 'Service error',
+        path: ['add'],
+        locations: [{ column: 3, line: 2 }],
+        extensions: { code: 'NOT_IMPLEMENTED' }
+      }])
+    }
+  }
+
+  app.register(GQL, {
+    schema,
+    resolvers,
+    errorHandler: true
+  })
+
+  // Invalid query
+  const res = await app.inject({
+    method: 'GET',
+    url: '/graphql?query={add(x:2,y:2)}'
+  })
+
+  const expectedResult = {
+    errors: [{
+      message: 'Service error',
+      path: ['add'],
+      locations: [{ column: 3, line: 2 }],
+      extensions: { code: 'NOT_IMPLEMENTED' }
+    }],
+    data: {
+      add: null
+    }
+  }
+
+  t.strictEqual(res.statusCode, 200)
+  t.strictDeepEqual(JSON.parse(res.body), expectedResult)
+})
+
 test('routes with custom context', async (t) => {
   const app = Fastify()