Repository: mercurius-js/mercurius
Pull request #641 (2021-11-09 14:48:42)

feat: add modifiedSchema on preExecution hooks
Opening this PR to get some feedback.

I'm working on this issue: https://github.com/mercurius-js/auth/issues/43
The target of that issue is to modify the output schema for inspection queries like so

```js
app.graphql.addHook('preExecution', async function filterHook (schema, document, context) {

  // removes those objects that the user has not access to
  const filteredSchema = auth.filterDirectives(schema, authSchema)

  return {
    schema: filteredSchema
  }
})
```

The inspection query is a standard query that is being executed on the `app.graphql.schema` object.

To let the user (and in this case the mercurius/auth plugin) change the schema where the query is executed and affect the output results, the `preExecution` hook may manage a schema replacing.

Note that I did not use `app.graphql.transformSchema` because it would replace the initial/original schema: this is unwanted: the target here is to replace the schema only for a single request execution.

Doing so, every request may change the schema accordingly to its data (such as client's role&permissions).

This PR lacks documentation, but I would like to know if you are OK with this solution before add it.
Feel free to suggest some additional tests too

Thanks


mcollina: > To let the user (and in this case the mercurius/auth plugin) change the schema where the query is executed and affect the output results, the preExecution hook may manage a schema replacing.

This seems a really bad idea. What about concurrency issues?
Eomm: > What about concurrency issues?

I don't expect those issues since the "temporary-schema" is used on the request's scope and it must not affect the global schema that remains the default.

I can arrange a test for it as well

Eomm: > This seems a really bad idea.

I did not find a way to implement the desired logic using the actual API, such as the `onResolution` hook, mainly because there is not a reference to the (auth) resolver to execute to decide if a Graph FIELD or OBJECT should be returned or not.

I agree that this could be a dangerous feature to use with caution - the risk is a broken API.
Could we protect the user from misusing this feature?



Patch:
diff --git a/docs/hooks.md b/docs/hooks.md
index 943a1b1..f92c097 100644
--- a/docs/hooks.md
+++ b/docs/hooks.md
@@ -61,15 +61,17 @@ fastify.graphql.addHook('preValidation', async (schema, document, context) => {
 
 In the `preExecution` hook, you can modify the following items by returning them in the hook definition:
   - `document`
+  - `schema`
   - `errors`
 
-Note that if you modify the `document` object, the jit compilation will be disabled for the request.
+Note that if you modify the `schema` or the `document` object, the [jit](./api/options.md#plugin-options) compilation will be disabled for the request.
 
 ```js
 fastify.graphql.addHook('preExecution', async (schema, document, context) => {
-  const { modifiedDocument, errors } = await asyncMethod(document)
+  const { modifiedSchema, modifiedDocument, errors } = await asyncMethod(document)
 
   return {
+    schema: modifiedSchema, // ⚠️ changing the schema may break the query execution. Use it carefully.
     document: modifiedDocument,
     errors
   }
diff --git a/index.d.ts b/index.d.ts
index 958b6a0..64629d2 100644
--- a/index.d.ts
+++ b/index.d.ts
@@ -786,6 +786,7 @@ type ValidationRules =
     }) => ValidationRule[]);
 
 export interface PreExecutionHookResponse<TError extends Error> {
+  schema?: GraphQLSchema
   document?: DocumentNode
   errors?: TError[]
 }
diff --git a/index.js b/index.js
index 69be7d8..17b3db3 100644
--- a/index.js
+++ b/index.js
@@ -517,7 +517,6 @@ const plugin = fp(async function (app, opts) {
     }
 
     const shouldCompileJit = cached && cached.count++ === minJit
-
     // Validate variables
     if (variables !== undefined && !shouldCompileJit) {
       const executionContext = buildExecutionContext(fastifyGraphQl.schema, document, root, context, variables, operationName)
@@ -529,24 +528,30 @@ const plugin = fp(async function (app, opts) {
     }
 
     // Trigger preExecution hook
+    let modifiedSchema
     let modifiedDocument
     if (context.preExecution !== null) {
-      ({ modifiedDocument } = await preExecutionHandler({ schema: fastifyGraphQl.schema, document, context }))
+      ({ modifiedSchema, modifiedDocument } = await preExecutionHandler({ schema: fastifyGraphQl.schema, document, context }))
     }
 
     // minJit is 0 by default
-    if (shouldCompileJit && !modifiedDocument) {
-      cached.jit = compileQuery(fastifyGraphQl.schema, document, operationName)
+    if (shouldCompileJit) {
+      if (!modifiedSchema && !modifiedDocument) {
+        // can compile only when the schema and document are not modified
+        cached.jit = compileQuery(fastifyGraphQl.schema, document, operationName)
+      } else {
+        // the counter must decrease to ignore the query
+        cached && cached.count--
+      }
     }
 
-    if (cached && cached.jit !== null && !modifiedDocument) {
+    if (cached && cached.jit !== null && !modifiedSchema && !modifiedDocument) {
       const execution = await cached.jit.query(root, context, variables || {})
-
       return maybeFormatErrors(execution, context)
     }
 
     const execution = await execute(
-      fastifyGraphQl.schema,
+      modifiedSchema || fastifyGraphQl.schema,
       modifiedDocument || document,
       root,
       context,
diff --git a/lib/handlers.js b/lib/handlers.js
index 1c8d2b4..bdc29a0 100644
--- a/lib/handlers.js
+++ b/lib/handlers.js
@@ -29,16 +29,20 @@ async function preValidationHandler (request) {
 }
 
 async function preExecutionHandler (request) {
-  const { errors, modifiedDocument } = await preExecutionHooksRunner(
+  const { errors, modifiedDocument, modifiedSchema } = await preExecutionHooksRunner(
     request.context.preExecution,
     request
   )
   if (errors.length > 0) {
     addErrorsToContext(request.context, errors)
   }
-  if (typeof modifiedDocument !== 'undefined') {
-    return { modifiedDocument, modifiedQuery: print(request.document) }
+  if (typeof modifiedDocument !== 'undefined' || typeof modifiedSchema !== 'undefined') {
+    return Object.create(null, {
+      modifiedDocument: { value: modifiedDocument },
+      modifiedSchema: { value: modifiedSchema }
+    })
   }
+
   return {}
 }
 
@@ -51,7 +55,10 @@ async function preGatewayExecutionHandler (request) {
     addErrorsToContext(request.context, errors)
   }
   if (typeof modifiedDocument !== 'undefined') {
-    return { modifiedDocument, modifiedQuery: print(modifiedDocument) }
+    return Object.create(null, {
+      modifiedDocument: { value: modifiedDocument },
+      modifiedQuery: { get: () => print(modifiedDocument) }
+    })
   }
   return {}
 }
diff --git a/lib/hooks.js b/lib/hooks.js
index f059e6b..394f9b5 100644
--- a/lib/hooks.js
+++ b/lib/hooks.js
@@ -87,12 +87,19 @@ async function hooksRunner (functions, runner, request) {
 
 async function preExecutionHooksRunner (functions, request) {
   let errors = []
+  let modifiedSchema
   let modifiedDocument
 
   for (const fn of functions) {
-    const result = await fn(request.schema, modifiedDocument || request.document, request.context)
+    const result = await fn(modifiedSchema || request.schema,
+      modifiedDocument || request.document,
+      request.context
+    )
 
     if (result) {
+      if (typeof result.schema !== 'undefined') {
+        modifiedSchema = result.schema
+      }
       if (typeof result.document !== 'undefined') {
         modifiedDocument = result.document
       }
@@ -102,7 +109,7 @@ async function preExecutionHooksRunner (functions, request) {
     }
   }
 
-  return { errors, modifiedDocument }
+  return { errors, modifiedDocument, modifiedSchema }
 }
 
 async function preGatewayExecutionHooksRunner (functions, request) {
diff --git a/package.json b/package.json
index d2ef85f..2aa2ee7 100644
--- a/package.json
+++ b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "mercurius",
-  "version": "8.9.0",
+  "version": "8.9.1",
   "description": "Fastify GraphQL adapter with gateway and subscription support",
   "main": "index.js",
   "types": "index.d.ts",
@@ -9,6 +9,7 @@
     "unit": "tap test/*.js test/gateway/*.js test/internals/*.js",
     "cov": "tap  --coverage-report=html -J test/*.js test/gateway/*.js",
     "lint": "npm run lint:standard && npm run lint:typescript",
+    "lint:fix": "standard --fix",
     "lint:standard": "standard | snazzy",
     "lint:typescript": "standard --parser @typescript-eslint/parser --plugin @typescript-eslint/eslint-plugin test/types/*.ts",
     "typescript": "tsd",


Test patch:
diff --git a/test/hooks.js b/test/hooks.js
index 941a2e0..7b674d1 100644
--- a/test/hooks.js
+++ b/test/hooks.js
@@ -2,7 +2,9 @@
 
 const { test } = require('tap')
 const Fastify = require('fastify')
-const { parse, GraphQLSchema } = require('graphql')
+const proxyquire = require('proxyquire')
+const { mapSchema } = require('@graphql-tools/utils')
+const { parse, buildSchema, GraphQLSchema } = require('graphql')
 const { promisify } = require('util')
 const GQL = require('..')
 
@@ -525,6 +527,139 @@ test('preExecution hooks should be able to modify the query document AST', async
   })
 })
 
+test('preExecution hooks should be able to modify the schema document AST', async t => {
+  t.plan(8)
+  const app = await createTestServer(t)
+
+  const query = `{ 
+    __type(name:"Query") { 
+      name
+      fields {
+        name
+      }  
+    }
+  }`
+
+  app.graphql.addHook('preExecution', async (schema, document, context) => {
+    t.type(schema, GraphQLSchema)
+    t.same(document, parse(query))
+    t.type(context, 'object')
+
+    if (context.reply.request.headers.role === 'super-user') {
+      const modifiedSchema = `
+        type Query {
+          add(x: Int, y: Int): Int
+          subtract(x: Int, y: Int): Int
+        }
+      `
+
+      return {
+        schema: buildSchema(modifiedSchema)
+      }
+    }
+  })
+
+  const reqSuper = app.inject({
+    method: 'POST',
+    headers: { 'content-type': 'application/json', role: 'super-user' },
+    url: '/graphql',
+    body: JSON.stringify({ query })
+  }).then(res => {
+    t.same(JSON.parse(res.body), {
+      data: {
+        __type: {
+          name: 'Query',
+          fields: [
+            { name: 'add' },
+            { name: 'subtract' }
+          ]
+        }
+      }
+    })
+  })
+
+  const reqNotSuper = app.inject({
+    method: 'POST',
+    headers: { 'content-type': 'application/json', role: 'not-a-super-user' },
+    url: '/graphql',
+    body: JSON.stringify({ query })
+  }).then(res => {
+    t.same(JSON.parse(res.body), {
+      data: {
+        __type: {
+          name: 'Query',
+          fields: [
+            { name: 'add' }
+          ]
+        }
+      }
+    })
+  })
+
+  await Promise.all([reqSuper, reqNotSuper])
+})
+
+test('cache skipped when the GQL Schema has been changed', async t => {
+  t.plan(4)
+
+  const app = Fastify()
+  t.teardown(() => app.close())
+
+  const plugin = proxyquire('../index', {
+    'graphql-jit': {
+      compileQuery () {
+        t.pass('the jit is called once')
+        return null
+      }
+    }
+  })
+
+  await app.register(plugin, { schema, resolvers, jit: 1 })
+  await app
+
+  app.graphql.addHook('preExecution', async (schema, document, context) => {
+    if (context.reply.request.headers.original === 'ok') {
+      return
+    }
+
+    return {
+      schema: mapSchema(schema)
+    }
+  })
+
+  const query = '{ add(x:1, y:2) }'
+
+  {
+    const res = await app.inject({
+      method: 'POST',
+      headers: { 'content-type': 'application/json', original: 'ok' },
+      url: '/graphql',
+      body: JSON.stringify({ query })
+    })
+    t.same(res.json(), { data: { add: 3 } }, 'this call warm up the jit counter')
+  }
+
+  {
+    const res = await app.inject({
+      method: 'POST',
+      headers: { 'content-type': 'application/json', original: 'NO' },
+      url: '/graphql',
+      body: JSON.stringify({ query })
+    })
+    t.same(res.json(), { data: { add: 3 } }, 'this call MUST not trigger the jit')
+  }
+
+  {
+    const res = await app.inject({
+      method: 'POST',
+      headers: { 'content-type': 'application/json', original: 'ok' },
+      url: '/graphql',
+      body: JSON.stringify({ query })
+    })
+    t.same(res.json(), { data: { add: 3 } }, 'this call triggers the jit cache')
+  }
+})
+
 test('preExecution hooks should be able to add to the errors array', async t => {
   t.plan(7)
   const app = await createTestServer(t)
diff --git a/test/types/index.ts b/test/types/index.ts
index ce6050e..d93bf42 100644
--- a/test/types/index.ts
+++ b/test/types/index.ts
@@ -537,6 +537,7 @@ app.graphql.addHook('preExecution', async function (schema, document, context) {
   expectAssignable<DocumentNode>(document)
   expectAssignable<MercuriusContext>(context)
   return {
+    schema,
     document,
     errors: [
       new Error('foo')