Repository: mcollina/fastify-gql
Pull request #152 (2020-05-05 10:21:10)

Variable validation before query execution
I found an inconsistency in returned error codes when handling user input errors. Syntax errors in queries will cause `Bad Request` (400) error, which is correct. However missing variables such as this example:

```js
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 = "query ($x: Int!, $y: Int!) { add(x: $x, y: $y) }";

const res = await app.inject({
  method: "GET",
  url: `/graphql?query=${query}&variables=${JSON.stringify({ x: 5 })}`, // notice there's no 'y' 
});

```

Along with other errors related to query variables like mis-typed variables would cause `Internal Server Error` (500) errors which isn't correct since this problem is coming from the client-side providing wrong variables. 

In this PR I added variable validation just before the execution of the query to produce `Bad Request` (400) error on invalid variables without affecting execution errors.

I also added a test case for requests with missing variables scenario I described above.

yndc: I benchmarked my changes and I'm not seeing any loss in performance in queries with variables despite effectively validating the variables twice (once in `buildExecutionContext` and `execute`).

However I see major performance loss in queries with wrong variables (about 75% request-per-second loss) on this `throw` line. 
```js
if (variables !== undefined) {
  const executionContext = buildExecutionContext(schema, document, root, context, variables, operationName)
  if (Array.isArray(executionContext)) {
    const err = new BadRequest()
    err.errors = executionContext
    throw err // line causing performance loss
  }
}
```
I don't know why `throw`-ing would be so expensive. 

Patch:
diff --git a/index.d.ts b/index.d.ts
index 0cfb907..bee519c 100644
--- a/index.d.ts
+++ b/index.d.ts
@@ -254,4 +254,10 @@ type Result = ExecutionResult & {
 
 interface IGraphQLToolsResolveInfo extends GraphQLResolveInfo {
   mergeInfo?: MergeInfo;
-}
\ No newline at end of file
+}
+
+type Request = {
+  document: DocumentNode;
+  variables: Record<string, any>;
+  extensions?: Record<string, any>;
+}; 
\ No newline at end of file
diff --git a/index.js b/index.js
index c5ba214..050e067 100644
--- a/index.js
+++ b/index.js
@@ -22,6 +22,7 @@ const {
   validateSchema,
   execute
 } = require('graphql')
+const { buildExecutionContext } = require('graphql/execution/execute')
 const queryDepth = require('./lib/queryDepth')
 const buildFederationSchema = require('./lib/federation')
 const buildGateway = require('./lib/gateway')
@@ -362,6 +363,16 @@ const plugin = fp(async function (app, opts) {
       return res
     }
 
+    // Validate variables
+    if (variables !== undefined) {
+      const executionContext = buildExecutionContext(schema, document, root, context, variables, operationName)
+      if (Array.isArray(executionContext)) {
+        const err = new BadRequest()
+        err.errors = executionContext
+        throw err
+      }
+    }
+
     const execution = await execute(
       schema,
       document,
@@ -373,7 +384,7 @@ const plugin = fp(async function (app, opts) {
 
     if (execution.errors) {
       execution.errors = execution.errors.map(e => {
-        if (Object.prototype.hasOwnProperty.call(e.originalError, 'code') || Object.prototype.hasOwnProperty.call(e.originalError, 'additionalProperties')) {
+        if (e.originalError && (Object.prototype.hasOwnProperty.call(e.originalError, 'code') || Object.prototype.hasOwnProperty.call(e.originalError, 'additionalProperties'))) {
           return new GraphQLError(e.originalError.message, e.nodes, e.source, e.positions, e.path, e.originalError, {
             code: e.originalError.code,
             ...e.originalError.additionalProperties
diff --git a/lib/gateway/request.js b/lib/gateway/request.js
index c07c082..36a6bff 100644
--- a/lib/gateway/request.js
+++ b/lib/gateway/request.js
@@ -87,6 +87,11 @@ function sendRequest (request, url) {
 
           try {
             const json = JSON.parse(data.toString())
+
+            if (json.errors && json.errors.length) {
+              return reject(json.errors)
+            }
+
             resolve({
               statusCode: response.statusCode,
               json
diff --git a/lib/routes.js b/lib/routes.js
index bd94aae..122abbc 100644
--- a/lib/routes.js
+++ b/lib/routes.js
@@ -256,7 +256,7 @@ module.exports = async function (app, opts) {
 
       app.get('/graphiql/config.js', (req, reply) => {
         reply
-          .header('Content-Type', 'applicatin/javascript')
+          .header('Content-Type', 'application/javascript')
           .send(`window.GRAPHQL_ENDPOINT = '${graphqlPath}'`)
       })
     }
diff --git a/package.json b/package.json
index ab13dde..af6e182 100644
--- a/package.json
+++ b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "fastify-gql",
-  "version": "3.5.0",
+  "version": "3.6.0",
   "description": "Fastify GraphQL adapter",
   "main": "index.js",
   "types": "index.d.ts",


Test patch:
diff --git a/test/gateway/request.js b/test/gateway/request.js
index 5844f59..f4d30a4 100644
--- a/test/gateway/request.js
+++ b/test/gateway/request.js
@@ -49,3 +49,38 @@ test('sendRequest method rejects when response is not valid json', async (t) =>
 
   t.end()
 })
+
+test('sendRequest method rejects when response contains errors', async (t) => {
+  const app = fastify()
+  app.post('/', async (request, reply) => {
+    return { errors: ['foo'] }
+  })
+
+  await app.listen(0)
+
+  const url = new URL(`http://localhost:${app.server.address().port}`)
+  const { request, close } = buildRequest({})
+  t.tearDown(() => {
+    close()
+    app.close()
+  })
+  t.rejects(
+    sendRequest(
+      request,
+      url
+    )({
+      method: 'POST',
+      body: JSON.stringify({
+        query: `
+      query ServiceInfo {
+        _service {
+          sdl
+        }
+      }
+      `
+      })
+    })
+  )
+
+  t.end()
+})
diff --git a/test/routes.js b/test/routes.js
index 2b18823..1a696cd 100644
--- a/test/routes.js
+++ b/test/routes.js
@@ -198,6 +198,60 @@ test('GET route with bad JSON variables', async (t) => {
   t.is(res.statusCode, 400)
 })
 
+test('GET route with missing variables', 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 = 'query ($x: Int!, $y: Int!) { add(x: $x, y: $y) }'
+
+  const res = await app.inject({
+    method: 'GET',
+    url: `/graphql?query=${query}&variables=${JSON.stringify({ x: 5 })}`
+  })
+
+  t.is(res.statusCode, 400)
+})
+
+test('GET route with mistyped variables', 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 = 'query ($x: Int!, $y: Int!) { add(x: $x, y: $y) }'
+
+  const res = await app.inject({
+    method: 'GET',
+    url: `/graphql?query=${query}&variables=${JSON.stringify({ x: 5, y: 'wrong data' })}`
+  })
+
+  t.is(res.statusCode, 400)
+})
+
 test('POST route variables', async (t) => {
   const app = Fastify()
   const schema = `
@@ -1393,6 +1447,7 @@ test('if ide is playground, serve init.js with the correct endpoint', async (t)
     url: '/playground/init.js'
   })
   t.strictEqual(res.statusCode, 200)
+  t.strictEqual(res.headers['content-type'], 'application/javascript')
   t.matchSnapshot(res.body)
 })
 
@@ -1408,6 +1463,7 @@ test('if ide is graphiql, serve config.js with the correct endpoint', async (t)
     url: '/graphiql/config.js'
   })
   t.strictEqual(res.statusCode, 200)
+  t.strictEqual(res.headers['content-type'], 'application/javascript')
   t.matchSnapshot(res.body)
 })