fix(async-hooks): mixing async and callback style in preHandler option now returns an error

fastify.js

@@ -74,6 +74,7 @@ const {
 } = errorCodes
 
 const { buildErrorHandler } = require('./lib/error-handler.js')
+const { validateAsyncHooks } = require('./lib/validation.js')
 
 function defaultBuildPrettyMeta (route) {
   // return a shallow copy of route's sanitized context
@@ -616,22 +617,8 @@ function fastify (options) {
       throw new errorCodes.FST_ERR_HOOK_INVALID_HANDLER(name, fn)
     }
 
-    if (name === 'onSend' || name === 'preSerialization' || name === 'onError' || name === 'preParsing') {
-      if (fn.constructor.name === 'AsyncFunction' && fn.length === 4) {
-        throw new errorCodes.FST_ERR_HOOK_INVALID_ASYNC_HANDLER()
-      }
-    } else if (name === 'onReady' || name === 'onListen') {
-      if (fn.constructor.name === 'AsyncFunction' && fn.length !== 0) {
-        throw new errorCodes.FST_ERR_HOOK_INVALID_ASYNC_HANDLER()
-      }
-    } else if (name === 'onRequestAbort') {
-      if (fn.constructor.name === 'AsyncFunction' && fn.length !== 1) {
-        throw new errorCodes.FST_ERR_HOOK_INVALID_ASYNC_HANDLER()
-      }
-    } else {
-      if (fn.constructor.name === 'AsyncFunction' && fn.length === 3) {
-        throw new errorCodes.FST_ERR_HOOK_INVALID_ASYNC_HANDLER()
-      }
+    if (!validateAsyncHooks(name, fn)) {
+      throw new errorCodes.FST_ERR_HOOK_INVALID_ASYNC_HANDLER()
     }
 
     if (name === 'onClose') {

lib/route.js

@@ -11,7 +11,8 @@ const warning = require('./warnings')
 
 const {
   compileSchemasForValidation,
-  compileSchemasForSerialization
+  compileSchemasForSerialization,
+  validateAsyncHooks
 } = require('./validation')
 
 const {
@@ -28,7 +29,8 @@ const {
   FST_ERR_ROUTE_METHOD_NOT_SUPPORTED,
   FST_ERR_ROUTE_METHOD_INVALID,
   FST_ERR_ROUTE_BODY_VALIDATION_SCHEMA_NOT_SUPPORTED,
-  FST_ERR_ROUTE_BODY_LIMIT_OPTION_NOT_INT
+  FST_ERR_ROUTE_BODY_LIMIT_OPTION_NOT_INT,
+  FST_ERR_HOOK_INVALID_ASYNC_HANDLER
 } = require('./errors')
 
 const {
@@ -268,6 +270,10 @@ function buildRouting (options) {
               if (typeof func !== 'function') {
                 throw new FST_ERR_HOOK_INVALID_HANDLER(hook, Object.prototype.toString.call(func))
               }
+
+              if (!validateAsyncHooks(hook, func)) {
+                throw new FST_ERR_HOOK_INVALID_ASYNC_HANDLER()
+              }
             }
           } else if (opts[hook] !== undefined && typeof opts[hook] !== 'function') {
             throw new FST_ERR_HOOK_INVALID_HANDLER(hook, Object.prototype.toString.call(opts[hook]))

lib/validation.js

@@ -217,6 +217,26 @@ function validateAsyncHeaders (validatePromise, context, request) {
     })
 }
 
+function validateAsyncHooks (name, fn) {
+  if (fn.constructor.name !== 'AsyncFunction') {
+    return true
+  }
+
+  const validateParametersLengthFunction = {
+    onSend: length => length !== 4,
+    preSerialization: length => length !== 4,
+    onError: length => length !== 4,
+    preParsing: length => length !== 4,
+    onReady: length => length === 0,
+    onListen: length => length === 0,
+    onRequestAbort: length => length === 1
+  }
+
+  const defaultValidator = length => !(length === 3)
+
+  return (validateParametersLengthFunction[name] ?? defaultValidator)(fn.length)
+}
+
 function wrapValidationError (result, dataVar, schemaErrorFormatter) {
   if (result instanceof Error) {
     result.statusCode = result.statusCode || 400
@@ -237,5 +257,6 @@ module.exports = {
   symbols: { bodySchema, querystringSchema, responseSchema, paramsSchema, headersSchema },
   compileSchemasForValidation,
   compileSchemasForSerialization,
-  validate
+  validate,
+  validateAsyncHooks
 }

test/hooks-async.test.js

@@ -745,8 +745,8 @@ test('Should log a warning if is an async function with `done`', t => {
     try {
       fastify.addHook('onRequestAbort', async (req, done) => {})
     } catch (e) {
-      t.ok(e.code, 'FST_ERR_HOOK_INVALID_ASYNC_HANDLER')
-      t.ok(e.message === 'Async function has too many arguments. Async hooks should not use the \'done\' argument.')
+      t.equal(e.code, 'FST_ERR_HOOK_INVALID_ASYNC_HANDLER')
+      t.equal(e.message, 'Async function has too many arguments. Async hooks should not use the \'done\' argument.')
     }
   })
 
@@ -757,8 +757,8 @@ test('Should log a warning if is an async function with `done`', t => {
     try {
       fastify.addHook('onRequest', async (req, reply, done) => {})
     } catch (e) {
-      t.ok(e.code, 'FST_ERR_HOOK_INVALID_ASYNC_HANDLER')
-      t.ok(e.message === 'Async function has too many arguments. Async hooks should not use the \'done\' argument.')
+      t.equal(e.code, 'FST_ERR_HOOK_INVALID_ASYNC_HANDLER')
+      t.equal(e.message, 'Async function has too many arguments. Async hooks should not use the \'done\' argument.')
     }
   })
 
@@ -769,20 +769,20 @@ test('Should log a warning if is an async function with `done`', t => {
     try {
       fastify.addHook('onSend', async (req, reply, payload, done) => {})
     } catch (e) {
-      t.ok(e.code, 'FST_ERR_HOOK_INVALID_ASYNC_HANDLER')
-      t.ok(e.message === 'Async function has too many arguments. Async hooks should not use the \'done\' argument.')
+      t.equal(e.code, 'FST_ERR_HOOK_INVALID_ASYNC_HANDLER')
+      t.equal(e.message, 'Async function has too many arguments. Async hooks should not use the \'done\' argument.')
     }
     try {
       fastify.addHook('preSerialization', async (req, reply, payload, done) => {})
     } catch (e) {
-      t.ok(e.code, 'FST_ERR_HOOK_INVALID_ASYNC_HANDLER')
-      t.ok(e.message === 'Async function has too many arguments. Async hooks should not use the \'done\' argument.')
+      t.equal(e.code, 'FST_ERR_HOOK_INVALID_ASYNC_HANDLER')
+      t.equal(e.message, 'Async function has too many arguments. Async hooks should not use the \'done\' argument.')
     }
     try {
       fastify.addHook('onError', async (req, reply, payload, done) => {})
     } catch (e) {
-      t.ok(e.code, 'FST_ERR_HOOK_INVALID_ASYNC_HANDLER')
-      t.ok(e.message === 'Async function has too many arguments. Async hooks should not use the \'done\' argument.')
+      t.equal(e.code, 'FST_ERR_HOOK_INVALID_ASYNC_HANDLER')
+      t.equal(e.message, 'Async function has too many arguments. Async hooks should not use the \'done\' argument.')
     }
   })
 
@@ -923,3 +923,28 @@ t.test('nested hooks to do not crash on 404', t => {
     t.equal(res.statusCode, 404)
   })
 })
+
+test('Register an hook as route option should fail if mixing async and callback style', t => {
+  t.plan(2)
+  const fastify = Fastify()
+
+  try {
+    fastify.get(
+      '/',
+      {
+        preHandler: [
+          async (request, reply, done) => {
+            done()
+          }
+        ]
+      },
+      async (request, reply) => {
+        return { hello: 'world' }
+      }
+    )
+    t.fail('preHandler mixing async and callback style')
+  } catch (e) {
+    t.equal(e.code, 'FST_ERR_HOOK_INVALID_ASYNC_HANDLER')
+    t.equal(e.message, 'Async function has too many arguments. Async hooks should not use the \'done\' argument.')
+  }
+})