perf: update method matching (#5419)

* update method matching implementation * push back string comparison * extract encoding * add missing method test * check for transferEncoding as well * skip instead of removing test * Update test/delete.test.js Co-authored-by: Manuel Spigolon <behemoth89@gmail.com> Signed-off-by: Gürgün Dayıoğlu <hey@gurgun.day> * Update test/method-missing.test.js Co-authored-by: Manuel Spigolon <behemoth89@gmail.com> Signed-off-by: Gürgün Dayıoğlu <hey@gurgun.day> --------- Signed-off-by: Gürgün Dayıoğlu <hey@gurgun.day> Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>
fastify · May 1, 2024 · ad89250 · ad89250
1 parent ccd16ac
commit ad89250
Show file tree

Hide file tree

Showing 5 changed files with 94 additions and 43 deletions.
diff --git a/lib/handleRequest.js b/lib/handleRequest.js
@@ -1,5 +1,6 @@
 'use strict'
 
+const { bodylessMethods, bodyMethods } = require('./httpMethods')
 const { validate: validateSchema } = require('./validation')
 const { preValidationHookRunner, preHandlerHookRunner } = require('./hooks')
 const wrapThenable = require('./wrapThenable')
@@ -20,42 +21,36 @@ function handleRequest (err, request, reply) {
   const headers = request.headers
   const context = request[kRouteContext]
 
-  if (method === 'GET' || method === 'HEAD') {
+  if (bodylessMethods.has(method)) {
     handler(request, reply)
     return
   }
 
-  const contentType = headers['content-type']
+  if (bodyMethods.has(method)) {
+    const contentType = headers['content-type']
+    const contentLength = headers['content-length']
+    const transferEncoding = headers['transfer-encoding']
 
-  if (method === 'POST' || method === 'PUT' || method === 'PATCH' || method === 'TRACE' || method === 'SEARCH' ||
-    method === 'PROPFIND' || method === 'PROPPATCH' || method === 'LOCK') {
     if (contentType === undefined) {
       if (
-        headers['transfer-encoding'] === undefined &&
-        (headers['content-length'] === '0' || headers['content-length'] === undefined)
-      ) { // Request has no body to parse
+        (contentLength === undefined || contentLength === '0') &&
+        transferEncoding === undefined
+      ) {
+        // Request has no body to parse
         handler(request, reply)
       } else {
         context.contentTypeParser.run('', handler, request, reply)
       }
     } else {
-      context.contentTypeParser.run(contentType, handler, request, reply)
-    }
-    return
-  }
+      if (contentLength === undefined && transferEncoding === undefined && method === 'OPTIONS') {
+        // OPTIONS can have a Content-Type header without a body
+        handler(request, reply)
+        return
+      }
 
-  if (method === 'OPTIONS' || method === 'DELETE') {
-    if (
-      contentType !== undefined &&
-      (
-        headers['transfer-encoding'] !== undefined ||
-        headers['content-length'] !== undefined
-      )
-    ) {
       context.contentTypeParser.run(contentType, handler, request, reply)
-    } else {
-      handler(request, reply)
     }
+
     return
   }
 

diff --git a/lib/httpMethods.js b/lib/httpMethods.js
@@ -1,22 +1,38 @@
 'use strict'
 
+const bodylessMethods = new Set([
+  // Standard
+  'GET',
+  'HEAD',
+  'TRACE',
+
+  // WebDAV
+  'UNLOCK'
+])
+
+const bodyMethods = new Set([
+  // Standard
+  'DELETE',
+  'OPTIONS',
+  'PATCH',
+  'PUT',
+  'POST',
+
+  // WebDAV
+  'COPY',
+  'LOCK',
+  'MOVE',
+  'MKCOL',
+  'PROPFIND',
+  'PROPPATCH',
+  'SEARCH'
+])
+
 module.exports = {
+  bodylessMethods,
+  bodyMethods,
   supportedMethods: [
-    'DELETE',
-    'GET',
-    'HEAD',
-    'PATCH',
-    'POST',
-    'PUT',
-    'OPTIONS',
-    'PROPFIND',
-    'PROPPATCH',
-    'MKCOL',
-    'COPY',
-    'MOVE',
-    'LOCK',
-    'UNLOCK',
-    'TRACE',
-    'SEARCH'
+    ...bodylessMethods,
+    ...bodyMethods
   ]
 }
diff --git a/test/delete.test.js b/test/delete.test.js
@@ -300,8 +300,28 @@ fastify.listen({ port: 0 }, err => {
   })
 })
 
+test('shorthand - delete with application/json Content-Type header and null body', t => {
+  t.plan(4)
+  const fastify = require('..')()
+  fastify.delete('/', {}, (req, reply) => {
+    t.equal(req.body, null)
+    reply.send(req.body)
+  })
+  fastify.inject({
+    method: 'DELETE',
+    url: '/',
+    headers: { 'Content-Type': 'application/json' },
+    body: 'null'
+  }, (err, response) => {
+    t.error(err)
+    t.equal(response.statusCode, 200)
+    t.same(response.payload.toString(), 'null')
+  })
+})
+
 // https://github.com/fastify/fastify/issues/936
-test('shorthand - delete with application/json Content-Type header and without body', t => {
+// Skip this test because this is an invalid request
+test('shorthand - delete with application/json Content-Type header and without body', { skip: 'https://github.com/fastify/fastify/pull/5419' }, t => {
   t.plan(4)
   const fastify = require('..')()
   fastify.delete('/', {}, (req, reply) => {

diff --git a/test/helper.js b/test/helper.js
@@ -216,11 +216,7 @@ module.exports.payloadMethod = function (method, t, isSetErrorHandler = false) {
 
       }, (err, response, body) => {
         t.error(err)
-        if (upMethod === 'OPTIONS') {
-          t.equal(response.statusCode, 200)
-        } else {
-          t.equal(response.statusCode, 415)
-        }
+        t.equal(response.statusCode, 415)
       })
     })
 

diff --git a/test/method-missing.test.js b/test/method-missing.test.js
@@ -0,0 +1,24 @@
+const http = require('http')
+const { test } = require('tap')
+const Fastify = require('../fastify')
+
+test('missing method from http client', t => {
+  t.plan(2)
+  const fastify = Fastify()
+
+  fastify.listen({ port: 3000 }, (err) => {
+    t.error(err)
+
+    const port = fastify.server.address().port
+    const req = http.request({
+      port,
+      method: 'REBIND',
+      path: '/'
+    }, (res) => {
+      t.equal(res.statusCode, 404)
+      fastify.close()
+    })
+
+    req.end()
+  })
+})