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
Self-handling exceptions should not have priority in Exception handler #718
Comments
So there are 3 levels.
All above are ordered top to bottom in their priority. How do u expect them to work? |
You're right. I had to go back to a previous project to remind myself what exact problem I had. Before Adonis I was using Laravel as my primary framework. Laravel has a "central" exception handler which is used as a catch-all for application errors. It can be used to format errors in a way that is required by the app. I thought that this is the role of Now comes the stupid me part - I forgot the exact details of the cause of this situation. It can be even possible that right now I'm talking nonsense. Due to this - I'll close the issue. In the upcoming days, I'll have the chance to validate this issue on my own. If it really exists I'll re-open this issue with more details. Apologies for taking your time on this. |
@thetutlage I'm back - hopefully with more valid details Case scenario: auth attempt Controller: class AuthController {
login ({ request, auth }) {
const { email, password } = request.all()
return auth.attempt(email, password)
}
} Global exception handler: (just async handle (error, { request, response }) {
response.status(error.status).send('FAIL :(')
} Sending incorrect credentials... Response: [
{
"field": "email",
"message": "Cannot find user with provided email"
}
] Expected response: 'FAIL :(' You mentioned that global exception handler is first in the priority list. The only way to add a custom response to such event is to add handler via I think that this behavior is incorrect. If an application has defined global exception handler it should be the only one responsible for handling exceptions. |
You took it the other way, I listed the point from top of bottom and bottom one overrides the top one. |
Ah, now it makes sense. Thanks. IMO if the general exception handler is defined it should take care of all exceptions. What do you think about it? |
Nope, it's there to catch what's left behind. |
This approach makes handling error responses more complicated. Let's say that application has a custom error message format. As said before - I've started new Adonis project. Till now (after just a few days) I had to declare 4 error handlers. One global handler, and three smaller for self-handling exceptions. All of them are casting exceptions to the same format. Every time, when a new exception is thrown I have to make sure that it's handled correctly. TBH this doesn't bring me developer joy. |
It depends on how you structure things, but first what do u think should happen in this case?
class ExceptionHandler {
async handle () {
}
} Now what should Adonis do? |
Do as is stated in the method - exactly nothing.
I guess that |
So let's say I put some code inside this method, what should happen now?
class ExceptionHandler {
async handle (error, { response }) {
if (error.name === 'ValidationException') {
response.send(error.message)
}
}
} |
In this case, only
Sorry for repeating myself yet, I'll try to explain myself once more. Short version Long version
It could even make use of The way I see such handler would be something like this: class BaseHandler {
async handle(error, { response }) {
if ('handle' in error) {
error.handle(response)
} else {
response.status(500).send(error.message)
}
}
}
class ExceptionHandler extends BaseHandler {
async handle(error, ctx) {
// here's a place for user to handle error
if (error.name === 'ValidationException') {
ctx.response.status(500).send('Be crazy!')
} else {
super.handle(error, ctx)
}
}
} Does it make sense? :) |
Okay so you are actually doing it the wrong way, lemme explain why.
The global handler is not their to build the flow, by writing Exception.bind('ValidationException', 'ExceptionHandler.validationException')
Exception.bind('SomeOtherException', 'ExceptionHandler.someOtherException') and then inside module.exports = {
validationException () {
},
someOtherException () {
}
} |
That's why I suggested that
Not source of truth, but error response builder. Unified for the application.
And that's the problem. If I'm using a custom error response format I need to bind a handler for each exception. That's quite cumbersome. |
Again, as an example, I'll use application that I'm writing right now. I've defined my error response format. The same Instead, I have to add to hooks custom handlers for those exceptions which basically do the same. Now, if there will be any new exceptions that I need to handle I'll have to remember about adding them in hooks. This could be done entirely by |
I can introduce an option, which forward all the exceptions to the GlobalHandler when exists, but I am really intersted in knowing, how you can have a single response for |
Handler for both of them looks like this: module.exports = async (error, { response }) =>
response.status(error.status)
.send({
error: error.name,
details: error.messages
})
If possible I could help you with that. I would suggest something like this:
Does it make sense? |
@radmen It totally makes sense. Reading the docs, I also understood it: Once you create this file, AdonisJs hands over all the exceptions occurred during HTTP lifecycle to it. It should have the handle method with an optional report method on it. It would also makes sense what you suggest above that it simply passes response. @thetutlage What can you say about this? this is totally valid. |
I already said it, the flow is simple and doesn't need more complications.
Again, you are thinking |
Again, what I suggest:
|
I don't think it is feasible to have one flow for all the exceptions. I will never handle all the following exceptions in the same way
So it completely makes sense to keep the flow flexible and where you want same flow for all the exceptions, you can go one step further and simply override them. const exceptionsToOverride = [
'UserNotFoundException',
'PasswordMisMatchException',
'ValidationException',
]
exceptionsToOverride.forEach((name) => {
Exception.handle(name, 'App/Exceptions/Handler.handle')
}) Now everything is in one place as you want. Also this is only required when the particular validation has a |
@thetutlage So when a new library comes out we keep adding it to the Example: ValidationException
We wish to have a format on any error it returns, like:
What do you think? smaller in code, more generic. |
No, it's not. Those bindings have to be put in
And you don't have to. As I said - The thing is that once everything will go through one handler it will be easier for developers to customize it for what they need (they won't be required to define custom hooks etc). Exceptions handler is already a global place for reporting stuff (via |
start/hooks.js
does not work either.
Not the expected result. Shows:
|
What’s the file name for |
@thetutlage |
@thetutlage I debug the app, Its not really working, its not registering the handler.. Exception |
Works fine for me, and hard to know, where you are writing this code and even it is getting executed or not. start/hooks.js const { hooks } = require('@adonisjs/ignitor')
hooks.after.providersBooted(() => {
const Exception = use('Exception')
Exception.handle('PasswordMisMatchException', (error, { response }) => {
response.send('Bad password')
})
}) and inside route. I am using wrong password intentionally. Route.get('/', async ({ auth }) => {
await auth.attempt('foo@bar.com', 'thisiswrongpassword')
}) |
@thetutlage Bummer, yup I got it working too by following the hooks sample for |
Yes, I always assume that, but half baked issues are kind of harder for me to make assumptions and form answers on top it. 😄 |
After reading the hole discussion, I thought as Radmen. I explain:
Sorry if there are obvious doubts but I am starting with Adonis and I am trying to do it the right way |
@AyozeVera the whole point of this discussion is that I suggest that exception handler should unify handling of all exceptions (like in Laravel) :)
If you're defining own exceptions generic exception handler will be enough. Just create new one (using For Adonis bultin exceptions (specially the ones which are self-handling) you need to define custom handlers. Here's the excerpt of const { hooks } = require('@adonisjs/ignitor')
const requestMacro = require('../app/Validators/Macro/request')
hooks.after.providersBooted(() => {
const Exception = use('Exception')
const genericExceptionsHandler = use('App/Exceptions/Handler/genericExceptionsHandler')
Exception.handle('PasswordMisMatchException', genericExceptionsHandler)
Exception.handle('UserNotFoundException', genericExceptionsHandler)
Exception.handle('ValidationException', genericExceptionsHandler)
}) To be complete, here're contents of module.exports = async (error, { response }) =>
response.status(error.status)
.send({
error: error.name,
details: error.messages
}) |
@AyozeVera If you set Recently you shared this snippet, https://gist.github.com/AyozeVera/af89c828270b304d532a1f23c98dde17#file-handler-js-L24 I am not sure how you can enjoy writing 20 switch/case clauses to handle exceptions, vs handling them nicely in their own files |
I can say the same for adding separate handlers for each self-handling exception which requires different response format. I understand that it's convenient if you don't bother about response format. In this case self-handling exceptions are fine. The way how one wants to model exception handler is their own business. If they want to use 20 switch/case (or if/else) statements it should be possible. Problem is that right now it's not possible. Taken that self-handling exceptions have higher priority it's required to write 1+N (where N is the number of exceptions which response have to be altered) handlers for exceptions. |
Hi @radmen, I guess you need to submit a PR. |
@KevMT I'd like to. First I need to make sure if I and @thetutlage are on the same page. In previous comments I've written a draft of how could exception handler work: |
How about using a flag. So on the global exception handler, you need to have a getter, which tells Adonis to use it always over anything else. Which literally means it will not call class ExceptionHandler {
static get overrideAll () {
return true
}
} |
Actually what I wanted to say is that the Exception handler should make use of exceptions Something like: class ExceptionHandler {
async handle (error, ctx) {
if ('handle' in error) {
error.handle(ctx)
} else {
// something different
}
} This way all error handling goes through the handler and is redirected to exceptions Having this, developer will only have to overwrite Overall I'd say that this will change the priority of error handling yet, the behaviour (for defaults) will remain the same. |
I believe we should have an This will check if the exception can handle itself as @radmen propose and fallback to the Youch screen if it's not possible. Then, the The flow will be basically.
This provides a good way to hook overall exceptions you can have on the system and doesn't feel like magic. |
Exactly. Plus, it should simplify a bit code base in the framework. |
Okay, So I am sync with you guys now. I will working on it in a way that it should not introduce any breaking changes, since every app will be using custom exception handlers. @radmen Thanks for staying patient 😄 |
@thetutlage no problem mate :) |
I just wanted to catch and format all unexpected errors that occured on my adonis api server. So I made Exceptions/Handler.js. This is little different what I expected. |
@swkimA Yes this is what going to happen in the future. For now I recommend using the workaround mentioned here #718 (comment) |
Exception handler now gives preference to the app exception handler over finding the best match BREAKING CHANGE: `Exception.bind` or `Exception.handle('*')` will stop working re #718
Thanks! Works like charm :) |
Hey guys, so this is an old issue, but I had understood error handling the way @thetutlage designed it originally. In my case, I wanted the error handler as a fallback to log only unhandled errors to the database. All my custom exceptions, I don't want to log, as they are handled. This way, my log only contains Exceptions I haven't handled yet. Turns out, since I added the global handlers, the self handle method is never called by my custom exceptions. Is there still a way to make it so that self handled exceptions don't fall back to the global handler? Edit: Nevermind! I figured it out. So I kinda went back on the design of this; in the global handler, instead of calling I added the following:
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Exception handler should be a central place to define custom handlers for exceptions thrown in application.
Yet, turns out that exceptions with
handle()
method have higher priority than custom handlers:https://github.com/adonisjs/adonis-framework/blob/d6a9d893e5ddc1e89c5df105175a39df80db463d/src/Exception/index.js#L70-L86
To handle such exception it's required to add a custom hook.
IMO
handle()
method of exception should have lower priority then custom handlers bound in main handler.The text was updated successfully, but these errors were encountered: