fix: exposeHeadRoute should pass "onSend" hook handlers provided to G…

…ET handler if method is not uppercased (fastify#5766) * fix: normalize route methods early * add test for onSend fix * fix lint * Update lib/route.js Co-authored-by: Manuel Spigolon <[email protected]> Signed-off-by: Aras Abbasi <[email protected]> --------- Signed-off-by: Aras Abbasi <[email protected]> Co-authored-by: Manuel Spigolon <[email protected]>
dancastillo · Oct 25, 2024 · d59ed98 · d59ed98
1 parent 37fc3b0
commit d59ed98
Show file tree

Hide file tree

Showing 6 changed files with 243 additions and 27 deletions.
diff --git a/lib/route.js b/lib/route.js
@@ -181,44 +181,46 @@ function buildRouting (options) {
    * @param {{ options: import('../fastify').RouteOptions, isFastify: boolean }}
    */
   function route ({ options, isFastify }) {
+    throwIfAlreadyStarted('Cannot add route!')
+
     // Since we are mutating/assigning only top level props, it is fine to have a shallow copy using the spread operator
     const opts = { ...options }
 
-    const { exposeHeadRoute } = opts
-    const hasRouteExposeHeadRouteFlag = exposeHeadRoute != null
-    const shouldExposeHead = hasRouteExposeHeadRouteFlag ? exposeHeadRoute : globalExposeHeadRoutes
+    const path = opts.url || opts.path || ''
 
-    const isGetRoute = opts.method === 'GET' ||
-      (Array.isArray(opts.method) && opts.method.includes('GET'))
-    const isHeadRoute = opts.method === 'HEAD' ||
-      (Array.isArray(opts.method) && opts.method.includes('HEAD'))
+    if (!opts.handler) {
+      throw new FST_ERR_ROUTE_MISSING_HANDLER(opts.method, path)
+    }
 
-    // we need to clone a set of initial options for HEAD route
-    const headOpts = shouldExposeHead && isGetRoute ? { ...options } : null
+    if (opts.errorHandler !== undefined && typeof opts.errorHandler !== 'function') {
+      throw new FST_ERR_ROUTE_HANDLER_NOT_FN(opts.method, path)
+    }
 
-    throwIfAlreadyStarted('Cannot add route!')
+    validateBodyLimitOption(opts.bodyLimit)
 
-    const path = opts.url || opts.path || ''
+    const shouldExposeHead = opts.exposeHeadRoute ?? globalExposeHeadRoutes
+
+    let isGetRoute = false
+    let isHeadRoute = false
 
     if (Array.isArray(opts.method)) {
       for (let i = 0; i < opts.method.length; ++i) {
         opts.method[i] = normalizeAndValidateMethod.call(this, opts.method[i])
         validateSchemaBodyOption.call(this, opts.method[i], path, opts.schema)
+
+        isGetRoute = opts.method.includes('GET')
+        isHeadRoute = opts.method.includes('HEAD')
       }
     } else {
       opts.method = normalizeAndValidateMethod.call(this, opts.method)
       validateSchemaBodyOption.call(this, opts.method, path, opts.schema)
-    }
 
-    if (!opts.handler) {
-      throw new FST_ERR_ROUTE_MISSING_HANDLER(opts.method, path)
+      isGetRoute = opts.method === 'GET'
+      isHeadRoute = opts.method === 'HEAD'
     }
 
-    if (opts.errorHandler !== undefined && typeof opts.errorHandler !== 'function') {
-      throw new FST_ERR_ROUTE_HANDLER_NOT_FN(opts.method, path)
-    }
-
-    validateBodyLimitOption(opts.bodyLimit)
+    // we need to clone a set of initial options for HEAD route
+    const headOpts = shouldExposeHead && isGetRoute ? { ...options } : null
 
     const prefix = this[kRoutePrefix]
 

diff --git a/test/request-error.test.js b/test/request-error.test.js
@@ -363,6 +363,128 @@ test('request.routeOptions should be immutable', t => {
   })
 })
 
+test('request.routeOptions.method is an uppercase string /1', t => {
+  t.plan(4)
+  const fastify = Fastify()
+  const handler = function (req, res) {
+    t.equal('POST', req.routeOptions.method)
+    res.send({})
+  }
+
+  fastify.post('/', {
+    bodyLimit: 1000,
+    handler
+  })
+  fastify.listen({ port: 0 }, function (err) {
+    t.error(err)
+    t.teardown(() => { fastify.close() })
+
+    sget({
+      method: 'POST',
+      url: 'http://localhost:' + fastify.server.address().port,
+      headers: { 'Content-Type': 'application/json' },
+      body: [],
+      json: true
+    }, (err, response, body) => {
+      t.error(err)
+      t.equal(response.statusCode, 200)
+    })
+  })
+})
+
+test('request.routeOptions.method is an uppercase string /2', t => {
+  t.plan(4)
+  const fastify = Fastify()
+  const handler = function (req, res) {
+    t.equal('POST', req.routeOptions.method)
+    res.send({})
+  }
+
+  fastify.route({
+    url: '/',
+    method: 'POST',
+    bodyLimit: 1000,
+    handler
+  })
+  fastify.listen({ port: 0 }, function (err) {
+    t.error(err)
+    t.teardown(() => { fastify.close() })
+
+    sget({
+      method: 'POST',
+      url: 'http://localhost:' + fastify.server.address().port,
+      headers: { 'Content-Type': 'application/json' },
+      body: [],
+      json: true
+    }, (err, response, body) => {
+      t.error(err)
+      t.equal(response.statusCode, 200)
+    })
+  })
+})
+
+test('request.routeOptions.method is an uppercase string /3', t => {
+  t.plan(4)
+  const fastify = Fastify()
+  const handler = function (req, res) {
+    t.equal('POST', req.routeOptions.method)
+    res.send({})
+  }
+
+  fastify.route({
+    url: '/',
+    method: 'pOSt',
+    bodyLimit: 1000,
+    handler
+  })
+  fastify.listen({ port: 0 }, function (err) {
+    t.error(err)
+    t.teardown(() => { fastify.close() })
+
+    sget({
+      method: 'POST',
+      url: 'http://localhost:' + fastify.server.address().port,
+      headers: { 'Content-Type': 'application/json' },
+      body: [],
+      json: true
+    }, (err, response, body) => {
+      t.error(err)
+      t.equal(response.statusCode, 200)
+    })
+  })
+})
+
+test('request.routeOptions.method is an array with uppercase string', t => {
+  t.plan(4)
+  const fastify = Fastify()
+  const handler = function (req, res) {
+    t.strictSame(['POST'], req.routeOptions.method)
+    res.send({})
+  }
+
+  fastify.route({
+    url: '/',
+    method: ['pOSt'],
+    bodyLimit: 1000,
+    handler
+  })
+  fastify.listen({ port: 0 }, function (err) {
+    t.error(err)
+    t.teardown(() => { fastify.close() })
+
+    sget({
+      method: 'POST',
+      url: 'http://localhost:' + fastify.server.address().port,
+      headers: { 'Content-Type': 'application/json' },
+      body: [],
+      json: true
+    }, (err, response, body) => {
+      t.error(err)
+      t.equal(response.statusCode, 200)
+    })
+  })
+})
+
 test('test request.routeOptions.version', t => {
   t.plan(7)
   const fastify = Fastify()

diff --git a/test/route.8.test.js b/test/route.8.test.js
@@ -169,3 +169,65 @@ test('Adding manually HEAD route after GET with the same path throws Fastify dup
     t.equal(error.code, 'FST_ERR_DUPLICATED_ROUTE')
   }
 })
+
+test('Will pass onSend hook to HEAD method if exposeHeadRoutes is true /1', async (t) => {
+  t.plan(1)
+
+  const fastify = Fastify({ exposeHeadRoutes: true })
+
+  await fastify.register((scope, opts, next) => {
+    scope.route({
+      method: 'GET',
+      path: '/route',
+      handler: (req, reply) => {
+        reply.send({ ok: true })
+      },
+      onSend: (req, reply, payload, done) => {
+        reply.header('x-content-type', 'application/fastify')
+        done(null, payload)
+      }
+    })
+
+    next()
+  }, { prefix: '/prefix' })
+
+  await fastify.ready()
+
+  const result = await fastify.inject({
+    url: '/prefix/route',
+    method: 'HEAD'
+  })
+
+  t.equal(result.headers['x-content-type'], 'application/fastify')
+})
+
+test('Will pass onSend hook to HEAD method if exposeHeadRoutes is true /2', async (t) => {
+  t.plan(1)
+
+  const fastify = Fastify({ exposeHeadRoutes: true })
+
+  await fastify.register((scope, opts, next) => {
+    scope.route({
+      method: 'get',
+      path: '/route',
+      handler: (req, reply) => {
+        reply.send({ ok: true })
+      },
+      onSend: (req, reply, payload, done) => {
+        reply.header('x-content-type', 'application/fastify')
+        done(null, payload)
+      }
+    })
+
+    next()
+  }, { prefix: '/prefix' })
+
+  await fastify.ready()
+
+  const result = await fastify.inject({
+    url: '/prefix/route',
+    method: 'HEAD'
+  })
+
+  t.equal(result.headers['x-content-type'], 'application/fastify')
+})
diff --git a/test/types/route.test-d.ts b/test/types/route.test-d.ts
@@ -398,9 +398,21 @@ type LowerCaseHTTPMethods = 'delete' | 'get' | 'head' | 'patch' | 'post' | 'put'
   })
 })
 
-expectError(fastify().route({
+expectType<FastifyInstance>(fastify().route({
   url: '/',
-  method: 'CONNECT', // not a valid method
+  method: 'CONNECT', // not a valid method but could be implemented by the user
+  handler: routeHandler
+}))
+
+expectType<FastifyInstance>(fastify().route({
+  url: '/',
+  method: 'OPTIONS',
+  handler: routeHandler
+}))
+
+expectType<FastifyInstance>(fastify().route({
+  url: '/',
+  method: 'OPTION', // OPTION is a typo for OPTIONS
   handler: routeHandler
 }))
 
@@ -410,7 +422,7 @@ expectType<FastifyInstance>(fastify().route({
   handler: routeHandler
 }))
 
-expectError(fastify().route({
+expectType<FastifyInstance>(fastify().route({
   url: '/',
   method: ['GET', 'POST', 'OPTION'], // OPTION is a typo for OPTIONS
   handler: routeHandler
@@ -507,10 +519,20 @@ expectType<FastifyInstance>(fastify().route({
   handler: routeHandlerWithReturnValue
 }))
 
+expectType<FastifyInstance>(fastify().route({
+  url: '/',
+  method: 'GET',
+  handler: (req) => {
+    expectType<HTTPMethods | HTTPMethods[]>(req.routeOptions.method)
+    expectAssignable<string | Array<string>>(req.routeOptions.method)
+  }
+}))
+
 expectType<FastifyInstance>(fastify().route({
   url: '/',
   method: ['HEAD', 'GET'],
   handler: (req) => {
-    expectType<string | string[]>(req.routeOptions.method)
+    expectType<HTTPMethods | HTTPMethods[]>(req.routeOptions.method)
+    expectAssignable<string | Array<string>>(req.routeOptions.method)
   }
 }))
diff --git a/types/request.d.ts b/types/request.d.ts
@@ -5,7 +5,7 @@ import { FastifyBaseLogger } from './logger'
 import { FastifyRouteConfig, RouteGenericInterface, RouteHandlerMethod } from './route'
 import { FastifySchema } from './schema'
 import { FastifyRequestType, FastifyTypeProvider, FastifyTypeProviderDefault, ResolveFastifyRequestType } from './type-provider'
-import { ContextConfigDefault, RawRequestDefaultExpression, RawServerBase, RawServerDefault, RequestBodyDefault, RequestHeadersDefault, RequestParamsDefault, RequestQuerystringDefault } from './utils'
+import { ContextConfigDefault, HTTPMethods, RawRequestDefaultExpression, RawServerBase, RawServerDefault, RequestBodyDefault, RequestHeadersDefault, RequestParamsDefault, RequestQuerystringDefault } from './utils'
 
 type HTTPRequestPart = 'body' | 'query' | 'querystring' | 'params' | 'headers'
 export interface RequestGenericInterface {
@@ -21,7 +21,7 @@ export interface ValidationFunction {
 }
 
 export interface RequestRouteOptions<ContextConfig = ContextConfigDefault, SchemaCompiler = FastifySchema> {
-  method: string | string[];
+  method: HTTPMethods | HTTPMethods[];
   // `url` can be `undefined` for instance when `request.is404` is true
   url: string | undefined;
   bodyLimit: number;

diff --git a/types/utils.d.ts b/types/utils.d.ts
@@ -2,13 +2,22 @@ import * as http from 'http'
 import * as http2 from 'http2'
 import * as https from 'https'
 
+type AutocompletePrimitiveBaseType<T> =
+  T extends string ? string :
+    T extends number ? number :
+      T extends boolean ? boolean :
+        never
+
+export type Autocomplete<T> = T | (AutocompletePrimitiveBaseType<T> & Record<never, never>)
+
 /**
  * Standard HTTP method strings
+ * for internal use
  */
 type _HTTPMethods = 'DELETE' | 'GET' | 'HEAD' | 'PATCH' | 'POST' | 'PUT' | 'OPTIONS' |
 'PROPFIND' | 'PROPPATCH' | 'MKCOL' | 'COPY' | 'MOVE' | 'LOCK' | 'UNLOCK' | 'TRACE' | 'SEARCH' | 'REPORT' | 'MKCALENDAR'
 
-export type HTTPMethods = Uppercase<_HTTPMethods> | Lowercase<_HTTPMethods>
+export type HTTPMethods = Autocomplete<_HTTPMethods | Lowercase<_HTTPMethods>>
 
 /**
  * A union type of the Node.js server types from the http, https, and http2 modules.
@@ -56,7 +65,6 @@ type Digit = 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9
 type HttpCodes = StringAsNumber<`${CodeClasses}${Digit}${Digit}`>
 type HttpKeys = HttpCodes | `${Digit}xx`
 export type StatusCodeReply = {
-
   [Key in HttpKeys]?: unknown;
 }