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
feat(#1930): added option to override onBadUrl callback #2106
feat(#1930): added option to override onBadUrl callback #2106
Conversation
You should to the validator changes in https://github.com/fastify/fastify/blob/master/build/build-validation.js and re-run that file. |
Why did you use a frameworkErrors wrapping object? |
There are other places where we might want the user to be able to override the default behavior of the error handler so i thought it might have been good to wrap those handlers to replace the content-type header or for other reasons. I just thought it could be clearer to wrap those handlers. |
The current pattern is to have separate options. I would keep it consistent for now. |
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 don't agree @mcollina, I think that for this case it would be better to have a single function that we call, the user can either decide to use our default handling or provide their own, in the same way we are doing for the default error handler.
const fastify = require('fastify')({
frameworkErrors (error, req, reply) {
// custom handling
}
})
Then, the error object should use our error objects, so the user can either specify a custom handling or return the error directly.
For example:
const fastify = require('fastify')({
frameworkErrors (error, req, reply) {
if (err instanceof BadUrlError) {
// custom handling
} else {
reply.send(err)
}
}
})
I added an object instead of a simple function because there are cases which, in my opinion, are completely different like clientError and it I think it looks cleaner for the user to override some behavior and leave the default for others. |
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.
@alemagio the reason why there was this proposal in the first place is that someone might want to return the body with a different content type.
Having a single callback makes it quite easy while having multiple keys to override could be annoying.
I could argue that having a simple function allows you to override a single case as well, as described in my previous comment.
Ok, thanks for the explanation @delvedor . |
Updated the code. In any case I'm a noob guys but I enjoyed working on this project and using 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.
Good work! This would just needs docs now.
Thanks |
docs/Server.md
Outdated
|
||
+ Default: `null` | ||
|
||
Configure custom error handlers. |
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.
This could use some more information. What is the purpose of configuring such handlers? If I configure a handler, what problem am I solving?
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.
What about:
Fastify provides default error handlers for the most common use cases.
Using this option it is possible to override one or more of those handlers with custom code.
const fastify = require('fastify')({
frameworkErrors (error, req, reply) {
if (err instanceof BadUrlError) {
const headers = {
'Content-Type': 'text/plain'
}
res.writeHead(400, headers)
return res.end("Provided url is not valid")
} else {
reply.send(err)
}
}
})
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.
That's better.
docs/Server.md
Outdated
res.writeHead(400, headers) | ||
return res.end("Provided url is not valid") | ||
} else { | ||
reply.send(err) |
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.
we don't have reply here
test/router-options.test.js
Outdated
fastify.inject({ | ||
method: 'GET', | ||
url: '/test/%world' | ||
}) |
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.
we should check the result here
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'm sorry I don't understand what do you mean
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
fastify.inject({
method: 'GET',
url: '/test/%world'
}, (err, res) => {
t.error(err)
t.deepEquals(res.json(), ....)
} )
fastify.js
Outdated
@@ -229,6 +234,7 @@ function build (options) { | |||
// custom error handling | |||
setNotFoundHandler: setNotFoundHandler, | |||
setErrorHandler: setErrorHandler, | |||
frameworkErrors: null, |
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.
this line is not used
Code updated I think I fixed all the comments |
… bad url error in errors section
96a57f8
to
002e69d
Compare
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.
lgtm
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.
Add more detail to improve the parameter function signature
test/router-options.test.js
Outdated
}, | ||
(err, res) => { | ||
t.error(err) | ||
t.deepEquals(res.body, 'FST_ERR_BAD_URL: \'%world\' is not a valid url component') |
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.
t.deepEquals(res.body, 'FST_ERR_BAD_URL: \'%world\' is not a valid url component') | |
t.equals(res.body, 'FST_ERR_BAD_URL: \'%world\' is not a valid url component') |
deepEquals
is for object
fastify.js
Outdated
@@ -431,6 +436,9 @@ function build (options) { | |||
} | |||
|
|||
function onBadUrl (path, req, res) { | |||
if (options.frameworkErrors) { | |||
return options.frameworkErrors(new FST_ERR_BAD_URL(path), req, res) |
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.
as suggested by delvedor, here we could manage the req and res object like this the following code.
In this way, we will provide a solid and plain API to all the developers, where the frameworkErrors
function will receive every time the same input: one fastify error, the enhanced request and the reply.
Lines 167 to 182 in 911c8c0
function fourOhFourFallBack (req, res) { | |
// if this happen, we have a very bad bug | |
// we might want to do some hard debugging | |
// here, let's print out as much info as | |
// we can | |
req.id = genReqId(req) | |
req.originalUrl = req.url | |
var childLogger = logger.child({ reqId: req.id }) | |
if (modifyCoreObjects) { | |
req.log = res.log = childLogger | |
} | |
childLogger.info({ req }, 'incoming request') | |
var request = new Request(null, req, null, req.headers, childLogger) | |
var reply = new Reply(res, { onSend: [], onError: [] }, request, childLogger) |
The main goal would be the logger: everyone wants to log the errors, and if we deliver the plain HTTP-REQ and HTTP-RES the user will have issue to archive this
You should have all the parameters you need in that scope to reflect that logic 💪
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 believe I have everything i need in the scope.
So basically after those lines I should pass the newly created request and reply to the frameworkErrors function to expose them to the user right?
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.
can you also do:
const frameworkErrors = options.frameworkErrors
// in the function
if (frameworkErrors) {
return frameworkErrors(new FST_ERR_BAD_URL(path), req, res)
}
This will avoid frameworkErrors
to be called with this
as the options
object.
Code and doc and test updated. |
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.
@alemagio , please don't hate me 😓 : last suggestion to fix the readme ✨ 😇
Updated @Eomm |
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.
lgtm
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. |
Checklist
npm run test
andnpm run benchmark
I added this draft to have a feedback.
I think this could be a possible solution to allow the user to configure error handling of the framework. As you can see it is still incomplete I hope the idea is clear.