Repository: mercurius-js/mercurius (js)

Issue:
Allow setting graphql-jit's customJSONSerializer / fast-json-stringify
I'd like to use graphql-jit's `customJSONSerializer` option, to use `fast-json-stringify` instead of `JSON.stringify`, but it looks like that isn't possible:

https://github.com/mercurius-js/mercurius/blob/361e9534f8c5e0dd693e16c7943b74b0d4c20e1e/index.js#L483

Could this be supported by mercurius's API?

I'm also a little curious if `customJSONSerializer` should just always be passed true, i.e. if its faster, just have that be the default. I had actually assumed it was, and was surprised that neither graphql-jit nor mercurius default it to on.

I guess `fast-json-stringify` does have the disclaimer that its slower on large output (...or at least "not as dramatically faster"), so maybe that is why. 

Kind of random/tangential, I also noticed the `fast-json-stringify` disclaimer about it dove-tailing really well with `flatstr`, but I don't see `flatstr` used in either mercurius or graphql-jit; is that a missed opportunity for a performance win?


Pull request:
Added graphql-jit compiler options
This PR adds new options in mercurius which can be passed to the graphql-jit compiler.

Closes #390 

Patch:
diff --git a/docs/api/options.md b/docs/api/options.md
index 90a8e96..abcfc33 100644
--- a/docs/api/options.md
+++ b/docs/api/options.md
@@ -97,6 +97,8 @@
   - `notSupportedError?: string`: An error message to return when a query matches `isPersistedQuery`, but returns no valid hash from `getHash`. Defaults to `Bad Request`.
 - `allowBatchedQueries`: Boolean. Flag to control whether to allow batched queries. When `true`, the server supports recieving an array of queries and returns an array of results.
 
+- `compilerOptions`: Object. Configurable options for the graphql-jit compiler. For more details check https://github.com/zalando-incubator/graphql-jit
+
 #### queryDepth example
 
 ```
diff --git a/docs/hooks.md b/docs/hooks.md
index f92c097..66039a7 100644
--- a/docs/hooks.md
+++ b/docs/hooks.md
@@ -49,7 +49,7 @@ fastify.graphql.addHook('preParsing', async (schema, source, context) => {
 
 ### preValidation
 
-By the time the `preValidation` hook triggers, the query string has been parsed into a GraphQL Document AST.
+By the time the `preValidation` hook triggers, the query string has been parsed into a GraphQL Document AST. The hook will not be triggered for cached queries, as they are not validated.
 
 ```js
 fastify.graphql.addHook('preValidation', async (schema, document, context) => {
diff --git a/index.js b/index.js
index 74f3f0e..5d0f0be 100644
--- a/index.js
+++ b/index.js
@@ -554,7 +554,7 @@ const plugin = fp(async function (app, opts) {
     if (shouldCompileJit) {
       if (!modifiedSchema && !modifiedDocument) {
         // can compile only when the schema and document are not modified
-        cached.jit = compileQuery(fastifyGraphQl.schema, document, operationName)
+        cached.jit = compileQuery(fastifyGraphQl.schema, document, operationName, opts.compilerOptions)
       } else {
         // the counter must decrease to ignore the query
         cached && cached.count--
diff --git a/lib/errors.js b/lib/errors.js
index e82c633..58d934f 100644
--- a/lib/errors.js
+++ b/lib/errors.js
@@ -68,16 +68,18 @@ function defaultErrorFormatter (err, ctx) {
 
   if (err.errors) {
     errors = err.errors.map((error, idx) => {
-      log.error({ err: error }, error.message)
+      log.info({ err: error }, error.message)
 
       // parses, converts & combines errors if they are the result of a federated request
-      if (error.message === FEDERATED_ERROR.toString()) {
+      if (error.message === FEDERATED_ERROR.toString() && error.extensions) {
         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), [])
+  } else {
+    log.info({ err }, err.message)
   }
 
   return {
diff --git a/lib/routes.js b/lib/routes.js
index 7921f86..8846130 100644
--- a/lib/routes.js
+++ b/lib/routes.js
@@ -124,26 +124,51 @@ function tryJSONParse (request, value) {
   try {
     return sJSON.parse(value)
   } catch (err) {
-    request.log.info({ err: err })
-    throw new MER_ERR_GQL_VALIDATION(err.message)
+    const wrap = new MER_ERR_GQL_VALIDATION()
+    err.code = wrap.code
+    err.statusCode = wrap.statusCode
+    throw err
   }
 }
 
 module.exports = async function (app, opts) {
   const errorFormatter = typeof opts.errorFormatter === 'function' ? opts.errorFormatter : defaultErrorFormatter
+  const contextFn = opts.context
 
   if (typeof opts.errorHandler === 'function') {
-    app.setErrorHandler(opts.errorHandler)
+    app.setErrorHandler(async (error, request, reply) => {
+      const errorHandler = opts.errorHandler
+      if (!request[kRequestContext]) {
+        // Generate the context for this request
+        if (contextFn) {
+          request[kRequestContext] = await contextFn(request, reply)
+          Object.assign(request[kRequestContext], { reply, app })
+        } else {
+          request[kRequestContext] = { reply, app }
+        }
+      }
+
+      return errorHandler(error, request, reply)
+    })
   } else if (opts.errorHandler === true || opts.errorHandler === undefined) {
-    app.setErrorHandler((error, request, reply) => {
+    app.setErrorHandler(async (error, request, reply) => {
+      if (!request[kRequestContext]) {
+        // Generate the context for this request
+        if (contextFn) {
+          request[kRequestContext] = await contextFn(request, reply)
+          Object.assign(request[kRequestContext], { reply, app })
+        } else {
+          request[kRequestContext] = { reply, app }
+        }
+      }
+
       const { statusCode, response } = errorFormatter(
         error,
         request[kRequestContext]
       )
-      reply.code(statusCode).send(response)
+      return reply.code(statusCode).send(response)
     })
   }
-  const contextFn = opts.context
   const { subscriptionContextFn } = opts
 
   app.decorateRequest(kRequestContext)
diff --git a/package.json b/package.json
index feb2277..650e20e 100644
--- a/package.json
+++ b/package.json
@@ -46,6 +46,7 @@
     "graphql-tools": "^8.0.0",
     "pre-commit": "^1.2.2",
     "proxyquire": "^2.1.3",
+    "sinon": "^12.0.1",
     "snazzy": "^9.0.0",
     "split2": "^4.0.0",
     "standard": "^16.0.3",


Test patch:
diff --git a/test/cache.js b/test/cache.js
index 109d82d..c5c28ab 100644
--- a/test/cache.js
+++ b/test/cache.js
@@ -34,7 +34,11 @@ test('cache skipped when the GQL Schema has been changed', async t => {
   const app = Fastify()
   t.teardown(() => app.close())
 
-  await app.register(GQL, { schema, resolvers, jit: 1 })
+  await app.register(GQL, {
+    schema,
+    resolvers,
+    jit: 1
+  })
 
   app.graphql.addHook('preExecution', async (schema, document, context) => {
     if (context.reply.request.headers.super === 'true') {
diff --git a/test/errors.js b/test/errors.js
index d01010a..005cde6 100644
--- a/test/errors.js
+++ b/test/errors.js
@@ -5,6 +5,7 @@ const Fastify = require('fastify')
 const GQL = require('..')
 const { ErrorWithProps } = GQL
 const { FederatedError } = require('../lib/errors')
+const { kRequestContext } = require('../lib/symbols')
 const split = require('split2')
 
 test('ErrorWithProps - support status code in the constructor', async (t) => {
@@ -767,3 +768,188 @@ test('errors - should override statusCode to 200 if the data is present', async
 
   t.equal(res.statusCode, 200)
 })
+
+test('bad json', async (t) => {
+  const schema = `
+    type Query {
+      successful: String
+    }
+  `
+
+  const resolvers = {
+    Query: {
+      successful () {
+        t.fail('Should not be called')
+        return 'Runs OK'
+      }
+    }
+  }
+
+  const app = Fastify()
+
+  app.register(GQL, {
+    schema,
+    resolvers
+  })
+
+  await app.ready()
+
+  const res = await app.inject({
+    method: 'POST',
+    headers: {
+      'content-type': 'application/json'
+    },
+    body: 'this is not a json',
+    url: '/graphql'
+  })
+
+  t.equal(res.statusCode, 400)
+  t.same(res.json(),
+    { data: null, errors: [{ message: 'Unexpected token h in JSON at position 1' }] }
+  )
+})
+
+test('bad json with custom error formatter and custom context', async (t) => {
+  const schema = `
+    type Query {
+      successful: String
+    }
+  `
+
+  const resolvers = {
+    Query: {
+      successful () {
+        t.fail('Should not be called')
+        return 'Runs OK'
+      }
+    }
+  }
+
+  const app = Fastify()
+
+  app.register(GQL, {
+    schema,
+    resolvers,
+    context: (_request, _reply) => ({ customValue: true }),
+    errorFormatter: (_execution, context) => {
+      t.equal(context.customValue, true)
+      t.pass('custom error formatter called')
+      return {
+        statusCode: 400,
+        response: { data: null, errors: [{ message: 'Unexpected token h' }] }
+      }
+    }
+  })
+
+  await app.ready()
+
+  const res = await app.inject({
+    method: 'POST',
+    headers: {
+      'content-type': 'application/json'
+    },
+    body: 'this is not a json',
+    url: '/graphql'
+  })
+
+  t.equal(res.statusCode, 400)
+  t.same(res.json(),
+    { data: null, errors: [{ message: 'Unexpected token h' }] }
+  )
+})
+
+test('bad json with custom error handler', async (t) => {
+  t.plan(3)
+  const schema = `
+    type Query {
+      successful: String
+    }
+  `
+
+  const resolvers = {
+    Query: {
+      successful () {
+        t.fail('Should not be called')
+        return 'Runs OK'
+      }
+    }
+  }
+
+  const app = Fastify()
+
+  app.register(GQL, {
+    schema,
+    resolvers,
+    errorHandler: (_, request, reply) => {
+      t.pass('custom error handler called')
+      reply.code(400).send({
+        is: 'error'
+      })
+    }
+  })
+
+  await app.ready()
+
+  const res = await app.inject({
+    method: 'POST',
+    headers: {
+      'content-type': 'application/json'
+    },
+    body: 'this is not a json',
+    url: '/graphql'
+  })
+
+  t.equal(res.statusCode, 400)
+  t.same(res.json(), {
+    is: 'error'
+  })
+})
+
+test('bad json with custom error handler, custom error formatter and custom context', async (t) => {
+  const schema = `
+    type Query {
+      successful: String
+    }
+  `
+
+  const resolvers = {
+    Query: {
+      successful () {
+        t.fail('Should not be called')
+        return 'Runs OK'
+      }
+    }
+  }
+
+  const app = Fastify()
+
+  app.register(GQL, {
+    schema,
+    resolvers,
+    context: (_request, _reply) => ({ customValue: true }),
+
+    errorHandler: (_, request, reply) => {
+      t.equal(request[kRequestContext].customValue, true)
+      t.pass('custom error handler called')
+      reply.code(400).send({
+        is: 'error'
+      })
+    }
+  })
+
+  await app.ready()
+
+  const res = await app.inject({
+    method: 'POST',
+    headers: {
+      'content-type': 'application/json'
+    },
+    body: 'this is not a json',
+    url: '/graphql'
+  })
+
+  t.equal(res.statusCode, 400)
+  t.same(res.json(), {
+    is: 'error'
+  })
+})
diff --git a/test/options.js b/test/options.js
new file mode 100644
index 0000000..bf294f6
--- /dev/null
+++ b/test/options.js
@@ -0,0 +1,86 @@
+'use strict'
+
+const proxyquire = require('proxyquire')
+const sinon = require('sinon')
+const { test } = require('tap')
+const Fastify = require('fastify')
+
+const schema = `
+type User {
+  name: String!
+  password: String!
+}
+
+type Query {
+  read: [User]
+}
+`
+
+const resolvers = {
+  Query: {
+    read: async (_, obj) => {
+      return [
+        {
+          name: 'foo',
+          password: 'bar'
+        }
+      ]
+    }
+  }
+}
+
+test('call compileQuery with correct options if compilerOptions specified', async t => {
+  t.plan(1)
+
+  const app = Fastify()
+  t.teardown(() => app.close())
+
+  const compileQueryStub = sinon.stub()
+
+  const GQL = proxyquire('../index', {
+    'graphql-jit': {
+      compileQuery: compileQueryStub
+    }
+  })
+
+  await app.register(GQL, {
+    schema,
+    resolvers,
+    jit: 1,
+    compilerOptions: {
+      customJSONSerializer: true
+    }
+  })
+
+  const queryStub = sinon.stub()
+
+  compileQueryStub.returns({
+    query: queryStub
+  })
+
+  queryStub.resolves({ errors: [] })
+
+  const query = `{
+    read {
+      name
+      password
+    }
+  }`
+
+  // warm up the jit counter
+  await app.inject({
+    method: 'POST',
+    headers: { 'content-type': 'application/json', super: 'false' },
+    url: '/graphql',
+    body: JSON.stringify({ query })
+  })
+
+  await app.inject({
+    method: 'POST',
+    headers: { 'content-type': 'application/json', super: 'false' },
+    url: '/graphql',
+    body: JSON.stringify({ query })
+  })
+
+  t.ok(compileQueryStub.calledOnceWith(sinon.match.any, sinon.match.any, sinon.match.any, { customJSONSerializer: true }))
+})
diff --git a/test/routes.js b/test/routes.js
index 7c6c01a..c32977c 100644
--- a/test/routes.js
+++ b/test/routes.js
@@ -2,6 +2,7 @@
 
 const { test } = require('tap')
 const Fastify = require('fastify')
+const split = require('split2')
 const querystring = require('querystring')
 const WebSocket = require('ws')
 const { GraphQLError } = require('graphql')
@@ -319,8 +320,14 @@ test('GET route with extensions', async (t) => {
   })
 })
 
-test('GET route with bad JSON extensions', async (t) => {
-  const app = Fastify()
+test('GET route with bad JSON extensions', { only: true }, async (t) => {
+  t.plan(3)
+  const lines = split(JSON.parse)
+  const app = Fastify({
+    logger: {
+      stream: lines
+    }
+  })
   const schema = `
     type Query {
       add(x: Int, y: Int): Int
@@ -345,6 +352,14 @@ test('GET route with bad JSON extensions', async (t) => {
   })
 
   t.equal(res.statusCode, 400)
+
+  for await (const line of lines) {
+    if (line.err) {
+      t.equal(line.err.message, 'Unexpected token o in JSON at position 1')
+      t.equal(line.err.code, 'MER_ERR_GQL_VALIDATION')
+      break
+    }
+  }
 })
 
 test('POST route variables', async (t) => {