Core: Self-handling exceptions should not have priority in Exception handler

[radmen]

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.

[thetutlage]

So there are 3 levels.

1. Global exception handler. ie app/Exceptions/Handler.js
2. The exception handle method
3. Custom exception hook Exception.handle('NAME',callback)

All above are ordered top to bottom in their priority. How do u expect them to work?

[radmen]

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 App/Exceptions/Handler. Yet it turns out that self-handling errors are not processed by App/Exceptions/Handler::handle().

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.

[radmen]

@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 handle method)

  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.
In this case, priority took Exceptions.UserNotFoundException::handle() (from @adonisjs/auth).

The only way to add a custom response to such event is to add handler via Exception.handle().

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.

[thetutlage]

You took it the other way, I listed the point from top of bottom and bottom one overrides the top one.

[radmen]

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?

[thetutlage]

Nope, it's there to catch what's left behind.

[radmen]

This approach makes handling error responses more complicated.

Let's say that application has a custom error message format.
Now, to format those errors it's required to create global exception handler (for the "leftovers") and custom handler for each self-handling exception.

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.

[thetutlage]

It depends on how you structure things, but first what do u think should happen in this case?

1. The routeValidator throws a ValidationException.
2. You have created a file called app/Exceptions/Handler.js with following content

class ExceptionHandler {
  async handle () {
  }
}

Now what should Adonis do?

[radmen]

Now what should Adonis do?

Do as is stated in the method - exactly nothing.

ExceptionHandler is an optional thing - one who creates it should be aware that all exceptions will be handled by this method. If it will be left empty - nothing will be returned.

I guess that make:ehandler could put some default behavior to this method.

[thetutlage]

So let's say I put some code inside this method, what should happen now?

Trying to find if I get your issue or not

class ExceptionHandler {
  async handle (error, { response }) {
    if (error.name === 'ValidationException') {
       response.send(error.message)
    }
  }
}

[radmen]

So let's say I put some code inside this method, what should happen now?

In this case, only ValidationException would be handled. Every other exception would be discarded (empty response would be returned)

Trying to find if I get your issue or not

Sorry for repeating myself yet, I'll try to explain myself once more.

Short version
Once defined, ExceptionHandler::handle() should be responsible for handling all exceptions cought by application

Long version
In general, I think that self-handling exceptions are bad design.

• they brake SRP - exception should act as a "transport" of error details; here they're also responsible for preparing error response
• each self-handling exception requires a separate handler to format it in a way one wants to.

ExceptionHandler should be the main spot in the application which takes care of _how_ exception should be returned to the end-client.

It could even make use of handle() method of an exception.

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? :)

[thetutlage]

Okay so you are actually doing it the wrong way, lemme explain why.

1. First I want exceptions to have the power of handling themselves, this is how they can build clean abstractions, so in short Exception.handle is not going away.

2. You are using the Global exception handler as the source of truth for handling all exceptions, which is wrong.

3. If I am at your place and want to manually handle the ValidationException, I will simply bind to it.

The global handler is not their to build the flow, by writing 20 if/else inside it, and that is why Exception.bind exists.

Exception.bind('ValidationException', 'ExceptionHandler.validationException')
Exception.bind('SomeOtherException', 'ExceptionHandler.someOtherException')

and then inside app/Exceptions/ExceptionHandler.js file.

module.exports = {
   validationException () {
   },

   someOtherException () {
   }
}

[radmen]

First I want exceptions to have the power of handling themselves, this is how they can build clean abstractions, so in short Exception.handle is not going away.

That's why I suggested that ExceptionHandler could delegate response to exceptions handle() method if it's defined.

You are using the Global exception handler as the source of truth for handling all exceptions, which is wrong.

Not source of truth, but error response builder. Unified for the application.

If I am at your place and want to manually handle the ValidationException, I will simply bind to it.

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.

[radmen]

The global handler is not their to build the flow, by writing 20 if/else inside it, and that is why Exception.bind exists.

Again, as an example, I'll use application that I'm writing right now. I've defined my error response format. ExceptionHandler simply casts exceptions to that format. It's quite simple, so I don't have to write any flow logic inside it.

The same ExceptionHandler could handle PasswordMisMatchException, UserNotFoundException, ValidationException without any modifications but it's not possible.

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 ExceptionHandler.

[thetutlage]

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 ValidationException and UserNotFoundException?

[radmen]

Handler for both of them looks like this:

module.exports = async (error, { response }) =>
  response.status(error.status)
    .send({
      error: error.name,
      details: error.messages
    })

I can introduce an option, which forward all the exceptions to the GlobalHandler

If possible I could help you with that.

I would suggest something like this:

• by default all exceptions go through BaseExceptionHandler::handle()
  
  
  
  • if exception has handle() method it simply passes response to that method so that exception can handle itself - basically nothing changes here
  
  
  • otherwise - return some generic error response
  
  
• make:ehandler creates ExceptionHandler which extends BaseExceptionHandler; handle() method calls simply super.handle() method. A developer can decide what they want to do with it.
• depending on state Adonis will use either ExceptionHandler or BaseExceptionHandler
• to avoid confusion I'd remove Exception.handle()

Does it make sense?

[kevyworks]

@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.

[thetutlage]

I already said it, the flow is simple and doesn't need more complications.

1. First priority to Exception.handle('ExceptionName'), you have the power to decide the flow.
2. 2nd priority to handle method on the exception. So that the developer of that module/exception can have a default exception handling logic.
3. Both are missing, then fallback to a generic handler that is app/Exceptions/Handler.js file.

Again, you are thinking app/Exceptions/Handler is the way to go for designing the flow, but all I am saying is, this is the fallback.

[radmen]

Again, what I suggest:

• let app/Exception/Handler do all the job
• by default it passes handling to handle method of exception
• developer may alter this behavior and change the whole flow in one place
• remove Exception.handle()

[thetutlage]

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

1. PasswordMisMatch
2. ValidationException
3. UserNotFound

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 .handle method and there are very few of them.

[kevyworks]

@thetutlage So when a new library comes out we keep adding it to the exceptionsToOverride? what we are trying to emphasize is simply make the Handler.js handle all exception. Above suggestion is not even on docs.

Example: ValidationException

[{ field: "email", message: "..." }]

We wish to have a format on any error it returns, like:

{ 
  success: false, 
  errors: [{ field: "email", message: "..." }] 
}

What do you think? smaller in code, more generic.

[radmen]

Now everything is in one place as you want.

No, it's not. Those bindings have to be put in start/hooks.js file.

I will never handle all the following exceptions in the same way

And you don't have to. As I said - Exception/Handler can simply make use of exceptions handle() method.

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 report() method). The same thing should apply to handle().

[kevyworks]

start/hooks.js

const exceptionsToOverride = [
 'UserNotFoundException',
 'PasswordMisMatchException',
 'ValidationException',
]

exceptionsToOverride.forEach((name) => {
  Exception.handle(name, 'App/Exceptions/Generic.handle')
})

does not work either.

class GenericException {
   async handle(error, { response }) {
     response.status(error.status).send('Oops!')
   }
}

Not the expected result. Shows:

[
    {
        "field": "password",
        "message": "Invalid user password"
    }
]

[thetutlage]

What’s the file name for GenericException?

[kevyworks]

@thetutlage GenericException.js typo at the post above, Generic.handle maybe? because even if I change it to GenericException.handle still wont catch PasswordMisMatchException

[kevyworks]

@thetutlage I debug the app, Its not really working, its not registering the handler.. Exception use('Exception') looses _handlers somehow what is there is just @provider:Adonis/Exceptions/Handler which is binding to *. And _handlers, _reporters are empty.

[thetutlage]

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('[email protected]', 'thisiswrongpassword')
})

[kevyworks]

@thetutlage Bummer, yup I got it working too by following the hooks sample for hooks.after.providersBooted() please do always assume that all users are noob on the framework :) Thanks, got it working as well. 👍

[thetutlage]

Yes, I always assume that, but half baked issues are kind of harder for me to make assumptions and form answers on top it. 😄

[AyozeVera]

After reading the hole discussion, I thought as Radmen. I explain:
I am trying Adonis to create a REST API, so I need my handler to response JSON instead of HTML, so as I am used to do in Laravel, I write all my logic in the App/Exception/Handler.js file, just like
this gists
It works, but after reading this issue I think I am not using the handler the way it is supposed to be used.
But I also has a doubt:

• I need to handle all the exceptions, to return them as JSON, where is the best place to do it?

Sorry if there are obvious doubts but I am starting with Adonis and I am trying to do it the right way

[radmen]

@AyozeVera the whole point of this discussion is that I suggest that exception handler should unify handling of all exceptions (like in Laravel) :)

I need to handle all the exceptions, to return them as JSON, where is the best place to do it?

If you're defining own exceptions generic exception handler will be enough. Just create new one (using adonis make:ehandler) and define response format in handle() method.

For Adonis bultin exceptions (specially the ones which are self-handling) you need to define custom handlers.

Here's the excerpt of start/hooks.js file from one of mine projects. Basically, you can copy&paste it to your project :)

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 App/Exceptions/Handler/genericExceptionsHandler

module.exports = async (error, { response }) =>
  response.status(error.status)
    .send({
      error: error.name,
      details: error.messages
    })

[thetutlage]

@AyozeVera If you set Accept: application/json header when making the API request, all internal exception handlers will return valid JSON automatically.

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

[radmen]

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.

[kevyworks]

Hi @radmen, I guess you need to submit a PR.

[radmen]

@kevmt I'd like to. First I need to make sure if I and @thetutlage are on the same page.
If not, my work may be discarded. I'd like to avoid it.

In previous comments I've written a draft of how could exception handler work:

• https://github.com/adonisjs/adonis-framework/issues/718#issuecomment-346137626
• https://github.com/adonisjs/adonis-framework/issues/718#issuecomment-346330447

[thetutlage]

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 handle method on the exceptions, even when those exceptions are created by you.

class ExceptionHandler {
   static get overrideAll () {
       return true
   }
}

[radmen]

Which literally means it will not call handle method on the exceptions, even when those exceptions are created by you.

Actually what I wanted to say is that the Exception handler should make use of exceptions handle() method.

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 handle() methods.

Having this, developer will only have to overwrite handle() method of ExceptionHandler if they want different results.

Overall I'd say that this will change the priority of error handling yet, the behaviour (for defaults) will remain the same.

[RomainLanz]

I believe we should have an ExceptionHandler that will reside on the core of the framework.

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 ExceptionHandler created by the developer/cli (let's say CustomExceptionHandler) will call super.handle(error, cli) to hook over every single exception.

The flow will be basically.

1. System throw an exception
2. Is CustomExceptionHandler available?
3. Call CustomExceptionHandler.handle(), if the exception is not hooked here --> super.handle()
4. ExceptionHandler.handle(), self-handled?, Fallback to Youch error page

This provides a good way to hook overall exceptions you can have on the system and doesn't feel like magic.

[radmen]

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.

[thetutlage]

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 😄

[radmen]

@thetutlage no problem mate :)

[swkimA]

I just wanted to catch and format all unexpected errors that occured on my adonis api server.

So I made Exceptions/Handler.js.
But when I use auth.attempt(), that Handler cannot catche some exceptions which are predefined by framework.

This is little different what I expected.
I expected that all of user's code have higher priority than predefined one.

[thetutlage]

@swkimA Yes this is what going to happen in the future. For now I recommend using the workaround mentioned here https://github.com/adonisjs/adonis-framework/issues/718#issuecomment-350831415

[radmen]

Thanks! Works like charm :)

[vaaguirre]

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
return super.handle(...arguments)

I added the following:

 if (typeof (error.handle) === 'function') {
      return error.handle(error, ctx)
    }

    // handle the rest

[lock[bot]]

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.