New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf: update method matching #5419
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
18013fa
update method matching implementation
gurgunday b9af908
push back string comparison
gurgunday 3143036
extract encoding
gurgunday 44dc0a5
add missing method test
gurgunday 97ede71
check for transferEncoding as well
gurgunday 82c44f6
merge next into PR
gurgunday 62174d4
skip instead of removing test
gurgunday a064f50
Update test/delete.test.js
gurgunday 3d00de4
Update test/method-missing.test.js
gurgunday File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
] | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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() | ||
}) | ||
}) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the RFC etc.. but this stops the fastify adoption to big (legacy) companies and tools.
I would create a new issue/feature request as follow-up, to let the user to customise the new
bodylessMethods
propThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem is DELETE is also in the class of methods that is permitted to have a body, so in that case DELETE would be broken for requests with with a body — not the best tradeoff even if we let customization imo
The issue in that test is sending the Content-Type header
application/json
with an empty body, this is just an incorrect request — they should've sent something like'{}'
or not set the Content-Type header...But I can add a
'DELETE'
string literal here if you think this would really cause companies a headache:fastify/lib/handleRequest.js
Lines 45 to 49 in 97ede71
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But
null
is a valid JSON: is the body empty/missing from the request?I thought inject submits
null
as body payload.In this case let's skip it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean this passes: