Repository: mercurius-js/mercurius
Pull request #921 (2022-11-25 06:53:21)

feat: Allow preExecution hook to modify variables
I saw discussions about this on https://github.com/mercurius-js/mercurius/issues/876, which mainly talked about **accessing** variables, but there is a comment that also mentions updating them.

I require this use-case, where hooks can modify the incoming variables based on logic or contacting external systems. See the test included with the PR for an example. 

This could have be achieved by putting the logic in a plugin earlier in the chain than Mercurius, but preventing double query parsing would be difficult, as the hooks need to be query-aware. 

I'll admit I don't have a complete understanding of this codebase so forgive me if there are any things I've missed here, it's mainly to frame the requirement I have and start a discussion. 

If there is any modifications you'd like here, let me know and I'd be happy to contribute; I can update any documentation as necessary, but wanted to wait until this feature was discussed. 

curtiswilkinson: Apologies @mcollina, I didn't expect the "re-review" button to drop the reviewers you requested, but apparently it does for some reason

Patch:
diff --git a/docs/hooks.md b/docs/hooks.md
index 66039a7..9946b27 100644
--- a/docs/hooks.md
+++ b/docs/hooks.md
@@ -62,17 +62,24 @@ 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`
+  - `variables`
   - `errors`
 
 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 { modifiedSchema, modifiedDocument, errors } = await asyncMethod(document)
+fastify.graphql.addHook('preExecution', async (schema, document, context, variables) => {
+  const {
+    modifiedSchema,
+    modifiedDocument,
+    modifiedVariables,
+    errors
+  } = await asyncMethod(document)
 
   return {
     schema: modifiedSchema, // ⚠️ changing the schema may break the query execution. Use it carefully.
     document: modifiedDocument,
+    variables: modifiedVariables,
     errors
   }
 })
diff --git a/index.d.ts b/index.d.ts
index 5673216..e4f03d1 100644
--- a/index.d.ts
+++ b/index.d.ts
@@ -104,14 +104,17 @@ export interface preValidationHookHandler<TContext = MercuriusContext> {
 /**
  * `preExecution` is the third hook to be executed in the GraphQL request lifecycle. The previous hook was `preValidation`, the next hook will be `preGatewayExecution`.
  * Notice: in the `preExecution` hook, you can modify the following items by returning them in the hook definition:
+ *  - `schema`
  *  - `document`
  *  - `errors`
+ *  - `variables`
  */
 export interface preExecutionHookHandler<TContext = MercuriusContext, TError extends Error = Error> {
   (
     schema: GraphQLSchema,
     source: DocumentNode,
     context: TContext,
+    variables: Record<string, any>,
   ): Promise<PreExecutionHookResponse<TError> | void> | PreExecutionHookResponse<TError> | void;
 }
 
@@ -831,5 +834,6 @@ type ValidationRules =
 export interface PreExecutionHookResponse<TError extends Error> {
   schema?: GraphQLSchema
   document?: DocumentNode
-  errors?: TError[]
+  errors?: TError[],
+  variables?: Record<string, any>,
 }
diff --git a/index.js b/index.js
index 790a835..14021e2 100644
--- a/index.js
+++ b/index.js
@@ -522,11 +522,13 @@ const plugin = fp(async function (app, opts) {
     // Trigger preExecution hook
     let modifiedSchema
     let modifiedDocument
+    let modifiedVariables
     if (context.preExecution !== null) {
-      ({ modifiedSchema, modifiedDocument } = await preExecutionHandler({
+      ({ modifiedSchema, modifiedDocument, modifiedVariables } = await preExecutionHandler({
         schema: fastifyGraphQl.schema,
         document,
-        context
+        context,
+        variables
       }))
     }
 
@@ -542,7 +544,7 @@ const plugin = fp(async function (app, opts) {
     }
 
     if (cached && cached.jit !== null && !modifiedSchema && !modifiedDocument && isCompiledQuery(cached.jit)) {
-      const execution = await cached.jit.query(root, context, variables || {})
+      const execution = await cached.jit.query(root, context, modifiedVariables || variables || {})
       return maybeFormatErrors(execution, context)
     }
 
@@ -551,7 +553,7 @@ const plugin = fp(async function (app, opts) {
       document: modifiedDocument || document,
       rootValue: root,
       contextValue: context,
-      variableValues: variables,
+      variableValues: modifiedVariables || variables,
       operationName
     })
 
diff --git a/lib/handlers.js b/lib/handlers.js
index bdc29a0..a070873 100644
--- a/lib/handlers.js
+++ b/lib/handlers.js
@@ -29,17 +29,28 @@ async function preValidationHandler (request) {
 }
 
 async function preExecutionHandler (request) {
-  const { errors, modifiedDocument, modifiedSchema } = await preExecutionHooksRunner(
+  const {
+    errors,
+    modifiedDocument,
+    modifiedSchema,
+    modifiedVariables
+  } = await preExecutionHooksRunner(
     request.context.preExecution,
     request
   )
+
   if (errors.length > 0) {
     addErrorsToContext(request.context, errors)
   }
-  if (typeof modifiedDocument !== 'undefined' || typeof modifiedSchema !== 'undefined') {
+  if (
+    typeof modifiedDocument !== 'undefined' ||
+    typeof modifiedSchema !== 'undefined' ||
+    typeof modifiedVariables !== 'undefined'
+  ) {
     return Object.create(null, {
       modifiedDocument: { value: modifiedDocument },
-      modifiedSchema: { value: modifiedSchema }
+      modifiedSchema: { value: modifiedSchema },
+      modifiedVariables: { value: modifiedVariables }
     })
   }
 
diff --git a/lib/hooks.js b/lib/hooks.js
index 394f9b5..4de9971 100644
--- a/lib/hooks.js
+++ b/lib/hooks.js
@@ -89,11 +89,14 @@ async function preExecutionHooksRunner (functions, request) {
   let errors = []
   let modifiedSchema
   let modifiedDocument
+  let modifiedVariables
 
   for (const fn of functions) {
-    const result = await fn(modifiedSchema || request.schema,
+    const result = await fn(
+      modifiedSchema || request.schema,
       modifiedDocument || request.document,
-      request.context
+      request.context,
+      modifiedVariables || request.variables
     )
 
     if (result) {
@@ -103,13 +106,16 @@ async function preExecutionHooksRunner (functions, request) {
       if (typeof result.document !== 'undefined') {
         modifiedDocument = result.document
       }
+      if (typeof result.variables !== 'undefined') {
+        modifiedVariables = result.variables
+      }
       if (typeof result.errors !== 'undefined') {
         errors = errors.concat(result.errors)
       }
     }
   }
 
-  return { errors, modifiedDocument, modifiedSchema }
+  return { errors, modifiedDocument, modifiedSchema, modifiedVariables }
 }
 
 async function preGatewayExecutionHooksRunner (functions, request) {
diff --git a/lib/subscription-connection.js b/lib/subscription-connection.js
index 8bd7d30..f4cfaf2 100644
--- a/lib/subscription-connection.js
+++ b/lib/subscription-connection.js
@@ -1,6 +1,6 @@
 'use strict'
 
-const on = require('events.on')
+const { on } = require('events')
 const { subscribe, parse, print, getOperationAST } = require('graphql')
 const { SubscriptionContext } = require('./subscriber')
 const { kEntityResolvers } = require('./gateway/make-resolver')
diff --git a/package.json b/package.json
index d9068f3..07aae2d 100644
--- a/package.json
+++ b/package.json
@@ -59,7 +59,6 @@
     "@fastify/error": "^3.0.0",
     "@fastify/static": "^6.0.0",
     "@fastify/websocket": "^7.0.0",
-    "events.on": "^1.0.1",
     "fastify-plugin": "^4.2.0",
     "graphql": "^16.0.0",
     "graphql-jit": "^0.7.3",


Test patch:
diff --git a/test/hooks.js b/test/hooks.js
index f71e1b7..cc3323e 100644
--- a/test/hooks.js
+++ b/test/hooks.js
@@ -813,13 +813,43 @@ test('preExecution hooks should be able to add to the errors array that is alrea
   })
 })
 
+test('preExecution hooks should be able to modify variables', async t => {
+  t.plan(1)
+  const app = await createTestServer(t)
+
+  app.graphql.addHook('preExecution', async (schema, document, context, variables) => {
+    // double x, leave y
+    return { variables: { x: variables.x * 2, y: variables.y } }
+  })
+
+  app.graphql.addHook('preExecution', async (schema, document, context, variables) => {
+    // leave x, double y
+    return { variables: { x: variables.x, y: variables.y * 2 } }
+  })
+
+  const res = await app.inject({
+    method: 'POST',
+    headers: { 'content-type': 'application/json' },
+    url: '/graphql',
+    body: JSON.stringify({
+      query: 'query ($x: Int!, $y: Int!) { add(x: $x, y: $y) }',
+      variables: { x: 1, y: 2 }
+    })
+  })
+
+  t.same(JSON.parse(res.body), {
+    data: {
+      add: 6
+    }
+  })
+})
+
 // -------------
 // preGatewayExecution
 // -------------
 test('preGatewayExecution hooks should never be called in a non-gateway graphql server', async t => {
   t.plan(1)
   const app = await createTestServer(t)
-
   app.graphql.addHook('preGatewayExecution', async (schema, document, context) => {
     t.fail('this should not be called')
   })
diff --git a/test/types/index.ts b/test/types/index.ts
index 3e56828..8c0dd36 100644
--- a/test/types/index.ts
+++ b/test/types/index.ts
@@ -691,36 +691,41 @@ app.graphql.addHook('preValidation', function (schema, document, context) {
   expectAssignable<MercuriusContext>(context)
 })
 
-app.graphql.addHook('preExecution', async function (schema, document, context) {
+app.graphql.addHook('preExecution', async function (schema, document, context, variables) {
   expectAssignable<GraphQLSchema>(schema)
   expectAssignable<DocumentNode>(document)
   expectAssignable<MercuriusContext>(context)
+  expectAssignable<Record<string, any>>(context)
   return {
     schema,
     document,
+    variables,
     errors: [
       new Error('foo')
     ]
   }
 })
 
-app.graphql.addHook('preExecution', function (schema, document, context) {
+app.graphql.addHook('preExecution', function (schema, document, context, variables) {
   expectAssignable<GraphQLSchema>(schema)
   expectAssignable<DocumentNode>(document)
   expectAssignable<MercuriusContext>(context)
+  expectAssignable<Record<string, any>>(context)
   return {
     schema,
     document,
+    variables,
     errors: [
       new Error('foo')
     ]
   }
 })
 
-app.graphql.addHook('preExecution', function (schema, document, context) {
+app.graphql.addHook('preExecution', function (schema, document, context, variables) {
   expectAssignable<GraphQLSchema>(schema)
   expectAssignable<DocumentNode>(document)
   expectAssignable<MercuriusContext>(context)
+  expectAssignable<Record<string, any>>(context)
 })
 
 app.graphql.addHook('preGatewayExecution', async function (schema, document, context) {