fix: onRoute hook should not be called when it registered after route

lib/context.js

@@ -9,7 +9,8 @@ const {
   kRequest,
   kBodyLimit,
   kLogLevel,
-  kContentTypeParser
+  kContentTypeParser,
+  kRouteByFastify
 } = require('./symbols.js')
 
 // Objects that holds the context of every request
@@ -25,7 +26,8 @@ function Context ({
   attachValidation,
   replySerializer,
   schemaErrorFormatter,
-  server
+  server,
+  isFastify
 }) {
   this.schema = schema
   this.handler = handler
@@ -50,6 +52,7 @@ function Context ({
   this.attachValidation = attachValidation
   this[kReplySerializerDefault] = replySerializer
   this.schemaErrorFormatter = schemaErrorFormatter || server[kSchemaErrorFormatter] || defaultSchemaErrorFormatter
+  this[kRouteByFastify] = isFastify
 
   this.server = server
 }

lib/route.js

@@ -8,7 +8,7 @@ const supportedMethods = ['DELETE', 'GET', 'HEAD', 'PATCH', 'POST', 'PUT', 'OPTI
 const { normalizeSchema } = require('./schemas')
 const { parseHeadOnSendHandlers } = require('./headRoute')
 const warning = require('./warnings')
-const { kRequestAcceptVersion } = require('./symbols')
+const { kRequestAcceptVersion, kRouteByFastify } = require('./symbols')
 
 const {
   compileSchemasForValidation,
@@ -113,7 +113,7 @@ function buildRouting (options) {
   }
 
   // Convert shorthand to extended route declaration
-  function prepareRoute ({ method, url, options, handler }) {
+  function prepareRoute ({ method, url, options, handler, isFastify }) {
     if (typeof url !== 'string') {
       throw new FST_ERR_INVALID_URL(typeof url)
     }
@@ -140,11 +140,11 @@ function buildRouting (options) {
       handler: handler || (options && options.handler)
     })
 
-    return route.call(this, { options })
+    return route.call(this, { options, isFastify })
   }
 
   // Route management
-  function route ({ options }) {
+  function route ({ options, isFastify }) {
     // Since we are mutating/assigning only top level props, it is fine to have a shallow copy using the spread operator
     const opts = { ...options }
 
@@ -176,30 +176,30 @@ function buildRouting (options) {
     if (path === '/' && prefix.length > 0 && opts.method !== 'HEAD') {
       switch (opts.prefixTrailingSlash) {
         case 'slash':
-          addNewRoute.call(this, { path })
+          addNewRoute.call(this, { path, isFastify })
           break
         case 'no-slash':
-          addNewRoute.call(this, { path: '' })
+          addNewRoute.call(this, { path: '', isFastify })
           break
         case 'both':
         default:
-          addNewRoute.call(this, { path: '' })
+          addNewRoute.call(this, { path: '', isFastify })
           // If ignoreTrailingSlash is set to true we need to add only the '' route to prevent adding an incomplete one.
           if (ignoreTrailingSlash !== true && (ignoreDuplicateSlashes !== true || !prefix.endsWith('/'))) {
-            addNewRoute.call(this, { path, prefixing: true })
+            addNewRoute.call(this, { path, prefixing: true, isFastify })
           }
       }
     } else if (path[0] === '/' && prefix.endsWith('/')) {
       // Ensure that '/prefix/' + '/route' gets registered as '/prefix/route'
-      addNewRoute.call(this, { path: path.slice(1) })
+      addNewRoute.call(this, { path: path.slice(1), isFastify })
     } else {
-      addNewRoute.call(this, { path })
+      addNewRoute.call(this, { path, isFastify })
     }
 
     // chainable api
     return this
 
-    function addNewRoute ({ path, prefixing = false }) {
+    function addNewRoute ({ path, prefixing = false, isFastify = false }) {
       const url = prefix + path
 
       opts.url = url
@@ -241,21 +241,29 @@ function buildRouting (options) {
         attachValidation: opts.attachValidation,
         schemaErrorFormatter: opts.schemaErrorFormatter,
         replySerializer: this[kReplySerializerDefault],
-        server: this
+        server: this,
+        isFastify
       })
 
       if (opts.version) {
         warning.emit('FSTDEP008')
         constraints.version = opts.version
       }
 
-      const headRouteExists = opts.method === 'HEAD' && router.find(opts.method, opts.url, constraints) != null
+      const headHandler = router.find('HEAD', opts.url, constraints)
+      const hasHEADHandler = headHandler != null
 
-      // Check if the current route is not for a sibling HEAD one
-      if (!headRouteExists) {
-        try {
-          router.on(opts.method, opts.url, { constraints }, routeHandler, context)
-        } catch (error) {
+      // remove the head route created by fastify
+      if (hasHEADHandler && !context[kRouteByFastify] && headHandler.store[kRouteByFastify]) {
+        router.off(opts.method, opts.url, { constraints })
+      }
+
+      try {
+        router.on(opts.method, opts.url, { constraints }, routeHandler, context)
+      } catch (error) {
+        // any route insertion error created by fastify can be safely ignore
+        // because it only duplicate route for head
+        if (!context[kRouteByFastify]) {
           const isDuplicatedRoute = error.message.includes(`Method '${opts.method}' already declared for route '${opts.url}'`)
           if (isDuplicatedRoute) {
             throw new FST_ERR_DUPLICATED_ROUTE(opts.method, opts.url)
@@ -320,19 +328,21 @@ function buildRouting (options) {
           }
         })
 
-        const { exposeHeadRoute } = opts
-        const hasRouteExposeHeadRouteFlag = exposeHeadRoute != null
-        const shouldExposeHead = hasRouteExposeHeadRouteFlag ? exposeHeadRoute : globalExposeHeadRoutes
-
-        if (shouldExposeHead && options.method === 'GET' && !headRouteExists) {
-          const onSendHandlers = parseHeadOnSendHandlers(opts.onSend)
-          prepareRoute.call(this, { method: 'HEAD', url: path, options: { ...opts, onSend: onSendHandlers } })
-        } else if (headRouteExists && exposeHeadRoute) {
-          warning.emit('FSTDEP007')
-        }
-
         done(notHandledErr)
       })
+
+      // register head route in sync
+      // we must place it after the `this.after`
+      const { exposeHeadRoute } = opts
+      const hasRouteExposeHeadRouteFlag = exposeHeadRoute != null
+      const shouldExposeHead = hasRouteExposeHeadRouteFlag ? exposeHeadRoute : globalExposeHeadRoutes
+
+      if (shouldExposeHead && options.method === 'GET' && !hasHEADHandler) {
+        const onSendHandlers = parseHeadOnSendHandlers(opts.onSend)
+        prepareRoute.call(this, { method: 'HEAD', url: path, options: { ...opts, onSend: onSendHandlers }, isFastify: true })
+      } else if (hasHEADHandler && exposeHeadRoute) {
+        warning.emit('FSTDEP007')
+      }
     }
   }
 

lib/symbols.js

@@ -47,7 +47,8 @@ const keys = {
   kTestInternals: Symbol('fastify.testInternals'),
   kErrorHandler: Symbol('fastify.errorHandler'),
   kHasBeenDecorated: Symbol('fastify.hasBeenDecorated'),
-  kKeepAliveConnections: Symbol('fastify.keepAliveConnections')
+  kKeepAliveConnections: Symbol('fastify.keepAliveConnections'),
+  kRouteByFastify: Symbol('fastify.routeByFastify')
 }
 
 module.exports = keys

test/hooks.test.js

@@ -799,6 +799,27 @@ test('onRoute hook with many prefix', t => {
   fastify.ready(err => { t.error(err) })
 })
 
+test('onRoute hook should not be called when it registered after route', t => {
+  t.plan(3)
+  const fastify = Fastify()
+
+  fastify.addHook('onRoute', () => {
+    t.pass()
+  })
+
+  fastify.get('/', function (req, reply) {
+    reply.send()
+  })
+
+  fastify.addHook('onRoute', () => {
+    t.fail('should not be called')
+  })
+
+  fastify.ready(err => {
+    t.error(err)
+  })
+})
+
 test('onResponse hook should log request error', t => {
   t.plan(4)
 

test/pretty-print.test.js

@@ -144,7 +144,7 @@ test('pretty print - commonPrefix', t => {
 `
     const flatExpected = `└── / (-)
     ├── helicopter (GET, HEAD)
-    └── hello (GET, PUT, HEAD)
+    └── hello (GET, HEAD, PUT)
 `
     t.equal(typeof radixTree, 'string')
     t.equal(typeof flatTree, 'string')
@@ -200,7 +200,7 @@ test('pretty print - includeMeta, includeHooks', t => {
     │   • (onTimeout) ["onTimeout()"]
     │   • (onRequest) ["anonymous()"]
     │   • (errorHandler) "defaultErrorHandler()"
-    └── hello (GET, PUT, HEAD)
+    └── hello (GET, HEAD, PUT)
         • (onTimeout) ["onTimeout()"]
         • (onRequest) ["anonymous()"]
         • (errorHandler) "defaultErrorHandler()"
@@ -210,7 +210,7 @@ test('pretty print - includeMeta, includeHooks', t => {
     ├── helicopter (GET, HEAD)
     │   • (onTimeout) ["onTimeout()"]
     │   • (onRequest) ["anonymous()"]
-    └── hello (GET, PUT, HEAD)
+    └── hello (GET, HEAD, PUT)
         • (onTimeout) ["onTimeout()"]
         • (onRequest) ["anonymous()"]
 `