(v3.x) fix: handle invalid url #3806
Conversation
* fix: handle invalid url * refactor: handle as 404 instead of 400 * fix: respect disableRequestLogging
climba03003
changed the title
lib/fourOhFour.js
Outdated
|
|
||
| // 404 router, used for handling encapsulated 404 handlers | ||
| const router = FindMyWay({ defaultRoute: fourOhFourFallBack }) | ||
| const router = FindMyWay({ onBadUrl, defaultRoute: fourOhFourFallBack }) |
There was a problem hiding this comment.
I am unclear how this is different from the default route. Both are handling the 404 case, correct?
There was a problem hiding this comment.
Okay, I don't understand why both are needed.
There was a problem hiding this comment.
You can check this issue #3127
There was a problem hiding this comment.
I don't think this is right. It's a 404. The 404 handler should handle it. As implemented, we should be exposing two 404 handlers for the Fastify user to override. Basically:
const server = fastify({
404Handler1: () => {},
404Handler2: () => {}
})This is not a good dev experience, and is confusing.
There was a problem hiding this comment.
When the url invalid, it cannot go to the correct /* handler. Then it fallback to defaultRoute.
The fourOhFourFallBack is actually giving the user warning in this case.
The below one should be the correct handler for 404 case.
https://github.com/fastify/fastify/blob/v3.x/lib/fourOhFour.js#L50-L59
https://github.com/fastify/fastify/blob/v3.x/lib/fourOhFour.js#L150-L151
fourOhFourFallBack should never be run.
https://github.com/fastify/fastify/blob/v3.x/lib/fourOhFour.js#L154-L171
There was a problem hiding this comment.
Correct. The 404 handler should handle any case where a route is not found.
There was a problem hiding this comment.
If the new approach is good, I will create a PR for v4.
There was a problem hiding this comment.
I have stepped through the new tests. Everything is working as I think it should. Without a custom 404 handler defined, the basic404 handler is invoked when a "bad url" is encountered. When a custom 404 handler is defined, it is invoked.
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Backport #3128 to
v3branch and some new changes as requested.After revisit the code, I think it should not break people but provide a better experience to the developer.
Checklist
npm run testandnpm run benchmarkand the Code of conduct