Repository: mcollina/fastify-gql (js)

Issue:
Incorrect Graphql Error processing
The current implementation of processing Graphql Errors is not compatible with [Graphql Specification](http://spec.graphql.org/June2018/#sec-Errors).

From https://github.com/rmosolgo/graphql-ruby/issues/1130#issuecomment-347373937

> It's common practice to use 200 for all GraphQL requests. When errors happen, they're put in the "errors" key. Non-200 response codes indicate that some issue occurred at the HTTP transport layer, not the GraphQL layer.

Current implementation includes next validation for 200 status code

```js
const responseSchema = {
  '2xx': {
    type: 'object',
    properties: {
      data: {
        type: 'object',
        additionalProperties: true
      },
      errors: {
        type: 'object', // <------- Problem is here. Errors should have type array
        properties: {
          message: { type: 'string' },
          locations: {
            type: 'array',
            items: {
              type: 'object',
              properties: {
                line: { type: 'integer' },
                column: { type: 'integer' }
              }
            }
          },
          path: {
            type: 'array',
            items: { type: 'string' }
          }
        }
      }
    }
  }
}
```

Also, I noticed, that `data` can't be null.

Pull request description:
Fix: Incorrect Graphql Errors processing
PR closes #106 

Patch:
diff --git a/README.md b/README.md
index 329dc9a..4e99fcc 100644
--- a/README.md
+++ b/README.md
@@ -379,6 +379,21 @@ payload must conform to the following JSON schema:
 }
 ```
 
+For code from [example](#example) use:
+```sh
+curl -H "Content-Type:application/json" -XPOST -d '{"query": "query { add(x: 2, y: 2) }"}' http://localhost:3000/graphql
+```
+
+#### POST /graphql with Content-type: application/graphql
+
+Executes the GraphQL query or mutation described in the body. `operationName` and `variables` can not be passed using this method. The
+payload contains the GraphQL query.
+
+For code from [example](#example) use:
+```sh
+curl -H "Content-Type:application/graphql" -XPOST -d "query { add(x: 2, y: 2) }" http://localhost:3000/graphql
+```
+
 #### GET /graphiql
 
 Serves [GraphiQL](https://www.npmjs.com/package/graphiql) if enabled by
diff --git a/index.js b/index.js
index 37d730a..32033d8 100644
--- a/index.js
+++ b/index.js
@@ -298,6 +298,7 @@ module.exports = fp(async function (app, opts) {
     if (execution.errors) {
       const err = new InternalServerError()
       err.errors = execution.errors
+      err.data = execution.data
       throw err
     }
 
diff --git a/lib/routes.js b/lib/routes.js
index 317fc75..1a4ad51 100644
--- a/lib/routes.js
+++ b/lib/routes.js
@@ -13,26 +13,37 @@ const responseSchema = {
     type: 'object',
     properties: {
       data: {
-        type: 'object',
+        type: ['object', 'null'],
         additionalProperties: true
       },
       errors: {
-        type: 'object',
-        properties: {
-          message: { type: 'string' },
-          locations: {
-            type: 'array',
-            items: {
+        type: 'array',
+        items: {
+          type: 'object',
+          required: ['message'],
+          properties: {
+            message: { type: 'string' },
+            locations: {
+              type: 'array',
+              items: {
+                type: 'object',
+                properties: {
+                  line: { type: 'integer' },
+                  column: { type: 'integer' }
+                }
+              }
+            },
+            path: {
+              type: 'array',
+              items: { type: 'string' }
+            },
+            extensions: {
               type: 'object',
               properties: {
-                line: { type: 'integer' },
-                column: { type: 'integer' }
+                code: { type: 'string' },
+                timestamp: { type: 'string' }
               }
             }
-          },
-          path: {
-            type: 'array',
-            items: { type: 'string' }
           }
         }
       }
@@ -41,18 +52,24 @@ const responseSchema = {
 }
 
 async function defaultErrorHandler (err, request, reply) {
-  reply.code(err.statusCode)
+  if (err.data) {
+    reply.code(200)
+  } else {
+    reply.code(err.statusCode)
+  }
+
   if (err.errors) {
     const errors = err.errors.map(error => {
       return error instanceof GraphQLError ? formatError(error) : { message: error.message }
     })
 
-    return { errors }
+    return { errors, data: err.data || null }
   } else {
     return {
       errors: [
         { message: err.message }
-      ]
+      ],
+      data: err.data || null
     }
   }
 }
@@ -151,6 +168,15 @@ module.exports = async function (app, opts) {
     app.route(getOptions)
   }
 
+  app.addContentTypeParser('application/graphql', function (req, done) {
+    req.setEncoding('utf8')
+    var data = ''
+    req.on('data', chunk => { data += chunk })
+    req.on('end', _ => {
+      done(null, { query: data })
+    })
+  })
+
   app.post(graphqlPath, {
     schema: {
       body: {


Test patch:
diff --git a/tap-snapshots/test-routes.js-TAP.test.js b/tap-snapshots/test-routes.js-TAP.test.js
index 856d10b..ed9809d 100644
--- a/tap-snapshots/test-routes.js-TAP.test.js
+++ b/tap-snapshots/test-routes.js-TAP.test.js
@@ -5,8 +5,33 @@
  * Make sure to inspect the output below.  Do not ignore changes!
  */
 'use strict'
-exports['test/routes.js TAP GET return 500 on resolver error > must match snapshot 1'] = `
+exports['test/routes.js TAP GET return 200 on resolver error > must match snapshot 1'] = `
 {
+  "data": {
+    "add": null
+  },
+  "errors": [
+    {
+      "message": "this is a dummy error",
+      "locations": [
+        {
+          "line": 1,
+          "column": 2
+        }
+      ],
+      "path": [
+        "add"
+      ]
+    }
+  ]
+}
+`
+
+exports['test/routes.js TAP POST return 200 on resolver error > must match snapshot 1'] = `
+{
+  "data": {
+    "add": null
+  },
   "errors": [
     {
       "message": "this is a dummy error",
@@ -36,35 +61,60 @@ 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 resolver error > must match snapshot 1'] = `
+exports['test/routes.js TAP mutation with GET errors > must match snapshot 1'] = `
 {
   "errors": [
     {
-      "message": "this is a dummy error",
+      "message": "Operation cannot be perfomed via a GET request"
+    }
+  ],
+  "data": null
+}
+`
+
+exports['test/routes.js TAP server should return 200 on graphql errors (if field can be null) > must match snapshot 1'] = `
+{
+  "data": {
+    "hello": null
+  },
+  "errors": [
+    {
+      "message": "Simple error",
       "locations": [
         {
-          "line": 1,
-          "column": 2
+          "line": 3,
+          "column": 7
         }
       ],
       "path": [
-        "add"
+        "hello"
       ]
     }
   ]
 }
 `
 
-exports['test/routes.js TAP mutation with GET errors > must match snapshot 1'] = `
+exports['test/routes.js TAP server should return 500 on graphql errors (if field can not be null) > must match snapshot 1'] = `
 {
   "errors": [
     {
-      "message": "Operation cannot be perfomed via a GET request"
+      "message": "Simple error",
+      "locations": [
+        {
+          "line": 3,
+          "column": 7
+        }
+      ],
+      "path": [
+        "hello"
+      ]
     }
-  ]
+  ],
+  "data": null
 }
 `
diff --git a/test/routes.js b/test/routes.js
index d40f976..6e90239 100644
--- a/test/routes.js
+++ b/test/routes.js
@@ -4,6 +4,7 @@ const { test } = require('tap')
 const Fastify = require('fastify')
 const querystring = require('querystring')
 const WebSocket = require('ws')
+const { GraphQLError } = require('graphql')
 const GQL = require('..')
 
 test('POST route', async (t) => {
@@ -41,6 +42,40 @@ test('POST route', async (t) => {
   })
 })
 
+test('POST route application/graphql', async (t) => {
+  const app = Fastify()
+  const schema = `
+    type Query {
+      add(x: Int, y: Int): Int
+    }
+  `
+
+  const resolvers = {
+    add: async ({ x, y }) => x + y
+  }
+
+  app.register(GQL, {
+    schema,
+    resolvers
+  })
+
+  const query = '{ add(x: 2, y: 2) }'
+
+  const res = await app.inject({
+    method: 'POST',
+    headers: { 'content-type': 'application/graphql' },
+    url: '/graphql',
+    body: query
+  })
+
+  t.equal(res.statusCode, 200)
+  t.deepEqual(JSON.parse(res.body), {
+    data: {
+      add: 4
+    }
+  })
+})
+
 test('custom route', async (t) => {
   const app = Fastify()
   const schema = `
@@ -311,7 +346,7 @@ test('disable routes', async (t) => {
   t.deepEqual(res.statusCode, 404)
 })
 
-test('GET return 500 on resolver error', async (t) => {
+test('GET return 200 on resolver error', async (t) => {
   const app = Fastify()
   const schema = `
     type Query {
@@ -333,11 +368,11 @@ test('GET return 500 on resolver error', async (t) => {
     url: '/graphql?query={add(x:2,y:2)}'
   })
 
-  t.equal(res.statusCode, 500) // Internal Server Error
+  t.equal(res.statusCode, 200)
   t.matchSnapshot(JSON.stringify(JSON.parse(res.body), null, 2))
 })
 
-test('POST return 500 on resolver error', async (t) => {
+test('POST return 200 on resolver error', async (t) => {
   const app = Fastify()
   const schema = `
     type Query {
@@ -363,7 +398,7 @@ test('POST return 500 on resolver error', async (t) => {
     }
   })
 
-  t.equal(res.statusCode, 500) // Internal Server Error
+  t.equal(res.statusCode, 200)
   t.matchSnapshot(JSON.stringify(JSON.parse(res.body), null, 2))
 })
 
@@ -442,6 +477,49 @@ test('mutation with POST', async (t) => {
   t.equal(msg, 'hello world')
 })
 
+test('mutation with POST application/graphql', async (t) => {
+  const app = Fastify()
+  const schema = `
+    type Mutation {
+      setMessage(message: String): String
+    }
+
+    type Query {
+      getMessage: String
+    }
+  `
+
+  let msg = 'hello'
+  const resolvers = {
+    setMessage: async ({ message }) => {
+      msg = message
+      return message
+    },
+    async getMessage () { return msg }
+  }
+
+  app.register(GQL, {
+    schema,
+    resolvers
+  })
+
+  const query = 'mutation { setMessage(message: "hello world") }'
+
+  const res = await app.inject({
+    method: 'POST',
+    headers: { 'content-type': 'application/graphql' },
+    url: '/graphql',
+    body: query
+  })
+
+  t.deepEqual(JSON.parse(res.body), {
+    data: {
+      setMessage: 'hello world'
+    }
+  })
+  t.equal(msg, 'hello world')
+})
+
 test('mutation with GET errors', async (t) => {
   const app = Fastify()
   const schema = `
@@ -849,6 +927,72 @@ test('Custom error handler', async (t) => {
   t.strictEqual(res.statusCode, 403)
 })
 
+test('server should return 200 on graphql errors (if field can be null)', async (t) => {
+  const app = Fastify()
+  const schema = `
+    type Query {
+      hello: String
+    }
+  `
+
+  app.register(GQL, {
+    schema,
+    resolvers: {
+      Query: {
+        hello: () => { throw new GraphQLError('Simple error') }
+      }
+    }
+  })
+
+  const query = `
+    query {
+      hello
+    }
+  `
+
+  const response = await app.inject({
+    method: 'POST',
+    url: '/graphql',
+    body: { query }
+  })
+
+  t.equal(response.statusCode, 200)
+  t.matchSnapshot(JSON.stringify(JSON.parse(response.body), null, 2))
+})
+
+test('server should return 500 on graphql errors (if field can not be null)', async (t) => {
+  const app = Fastify()
+  const schema = `
+    type Query {
+      hello: String!
+    }
+  `
+
+  app.register(GQL, {
+    schema,
+    resolvers: {
+      Query: {
+        hello: () => { throw new GraphQLError('Simple error') }
+      }
+    }
+  })
+
+  const query = `
+    query {
+      hello
+    }
+  `
+
+  const response = await app.inject({
+    method: 'POST',
+    url: '/graphql',
+    body: { query }
+  })
+
+  t.equal(response.statusCode, 500)
+  t.matchSnapshot(JSON.stringify(JSON.parse(response.body), null, 2))
+})
+
 test('Error handler set to true should not change default behavior', async (t) => {
   const app = Fastify()
   const schema = `
@@ -880,7 +1024,8 @@ test('Error handler set to true should not change default behavior', async (t) =
         line: 1,
         column: 8
       }]
-    }]
+    }],
+    data: null
   }
 
   t.strictEqual(res.statusCode, 400)
@@ -943,7 +1088,7 @@ test('route validation is catched and parsed to graphql error', async (t) => {
     url: '/graphql'
   })
 
-  const expectedResult = { errors: [{ message: 'body should be object' }] }
+  const expectedResult = { errors: [{ message: 'body should be object' }], data: null }
 
   t.strictEqual(res.statusCode, 400)
   t.strictDeepEqual(JSON.parse(res.body), expectedResult)
@@ -1101,7 +1246,8 @@ test('cached errors', async (t) => {
           }
         ]
       }
-    ]
+    ],
+    data: null
   })
 
   const post = await app.inject({
@@ -1123,6 +1269,7 @@ test('cached errors', async (t) => {
           }
         ]
       }
-    ]
+    ],
+    data: null
   })
 })