Attempt ordering exception handlers by exception class inheritance #1514
Comments
vytas7
added
needs contributor
|
Since this is relevant to the other work I did with error-handling, I can dig into werkzeug and try to figure out what's going on there. Don't promise to finish said research though 😅 |
|
Here are the relevant lines in Flask for using the most specific handler available. They have some additional levels of categorization for blueprints, or storing handlers by code instead of exception class, which don't seem relevant for Falcon. So, basically, the error handlers are stored in a dict of If y'all decide to do this, it seems like it should be pretty straightforward. |
|
Great investigation, thanks a lot for joining us at sprints, Clara! @csojinb there is unfortunately one thing that is complicating the matters a little bit: we have recently added support for handlers associated with more than one exception... 😬 [1]
|
|
@vytas7 I might be misunderstanding the issue here... what is the problem with having handlers associated with more than one exception? Can't you just store that as |
|
@csojinb No, there is no problem per se, that is what my suggestion 2 is basically aiming at. |
|
Where did we end up on this issue? Is #1527 still waiting on this? |
|
The relevance of this to #1527 is that the current LIFO order may result in surprising behavior if someone overrides the Personally, I don't think this issue needs to block #1527 . IIUC, Falcon users who add a custom handler for |
|
OK, I'd love to have this smarter ordering implemented for 3.0 if we can swing it, otherwise push to 3.1. |
|
Any volunteers? :) |
|
P.S. When this is implemented, don't forget to update the note under the |
|
I think Falcon users who (read: me 🙂 but I think @jmvrbanac also uses this pattern and has even advised users to do so) add a custom exception handler for Not sure if we need to do much about this now though. The problematic should be gone once we introduce exception ordering. |
Addresses falconry#1514 Rather than selecting error handlers in LIFO order of registration, select the error handler corresponding to the nearest direct ancestor of the exception type raised. So, for example, if an app only adds a custom error handler for the Python ``Exception`` class, and an ``HTTPForbidden`` error is raised, then we use the default handler for ``HTTPError`` rather than the more general ``Exception`` handler (which is the pre-existing behavior). This is implemented by storing error handlers on the API object as a dict rather than a list and looking them up using the method resolution order attribute (`__mro__`) on the raised exception class. NOTE: This commit only includes the actual implementation and does not address testing or documentation. I am seeking implementation feedback before completing those additional changes. BREAKING CHANGE: Registration of a new error handler for type E will no longer override previously-registered error handlers for subclasses of type E. Registration order will no longer matter *except* when multiple error handlers are registered for the exact same exception type, in which case the most recently registered error handler overrides the previous ones.
vytas7
removed
needs contributor
|
Hi, I put up a draft PR with an implementation for this. I was hoping to get some input on the approach before adding tests, etc. I went with the simpler implementation because I didn't totally understand @vytas7's first suggestion for managing shared error handlers. Should I do a benchmark comparison or something for this implementation? |
|
Scratch those suggestions. Your proposed approach is way more scalable in the general case I think 👍 (it is only adding complexity traversing the MRO, but the class lookup itself is Benchmark against the current implementation would nevertheless be good to take a look at. (As said, I think those are going to be in favour of your new approach) |
Addresses falconry#1514 Rather than selecting error handlers in LIFO order of registration, select the error handler corresponding to the nearest direct ancestor of the exception type raised. So, for example, if an app only adds a custom error handler for the Python ``Exception`` class, and an ``HTTPForbidden`` error is raised, then we use the default handler for ``HTTPError`` rather than the more general ``Exception`` handler (which is the pre-existing behavior). This is implemented by storing error handlers on the API object as a dict rather than a list and looking them up using the method resolution order attribute (`__mro__`) on the raised exception class. NOTE: This commit only includes the actual implementation and does not address testing or documentation. I am seeking implementation feedback before completing those additional changes. BREAKING CHANGE: Registration of a new error handler for type E will no longer override previously-registered error handlers for subclasses of type E. Registration order will no longer matter *except* when multiple error handlers are registered for the exact same exception type, in which case the most recently registered error handler overrides the previous ones.
Addresses falconry#1514 Rather than selecting error handlers in LIFO order of registration, select the error handler corresponding to the nearest direct ancestor of the exception type raised. So, for example, if an app only adds a custom error handler for the Python ``Exception`` class, and an ``HTTPForbidden`` error is raised, then we use the default handler for ``HTTPError`` rather than the more general ``Exception`` handler (which is the pre-existing behavior). This is implemented by storing error handlers on the API object as a dict rather than a list and looking them up using the method resolution order attribute (`__mro__`) on the raised exception class. NOTE: This commit only includes the actual implementation and does not address testing or documentation. I am seeking implementation feedback before completing those additional changes. BREAKING CHANGE: Registration of a new error handler for type E will no longer override previously-registered error handlers for subclasses of type E. Registration order will no longer matter *except* when multiple error handlers are registered for the exact same exception type, in which case the most recently registered error handler overrides the previous ones.
…#1603) * doc(_handle_error): update NOTE missed in #1527 `_handle_error` returns false when the exception does not match a custom handler and is not a subclass of ``HTTPError``, ``HTTPStatus``, or ``Exception``. The update to include ``Exception`` was missed in PR #1527, which added a default handler for ``Exception`` * feat(API): on exception, select most specific error handler available Addresses #1514 Rather than selecting error handlers in LIFO order of registration, select the error handler corresponding to the nearest direct ancestor of the exception type raised. So, for example, if an app only adds a custom error handler for the Python ``Exception`` class, and an ``HTTPForbidden`` error is raised, then we use the default handler for ``HTTPError`` rather than the more general ``Exception`` handler (which is the pre-existing behavior). This is implemented by storing error handlers on the API object as a dict rather than a list and looking them up using the method resolution order attribute (`__mro__`) on the raised exception class. NOTE: This commit only includes the actual implementation and does not address testing or documentation. I am seeking implementation feedback before completing those additional changes. BREAKING CHANGE: Registration of a new error handler for type E will no longer override previously-registered error handlers for subclasses of type E. Registration order will no longer matter *except* when multiple error handlers are registered for the exact same exception type, in which case the most recently registered error handler overrides the previous ones. * test: add test for new error-handling order * doc(App): Document new error handler precedence logic * doc(add_error_handler): add versionchanged directive For the change in error-handler-match strategy * doc(towncrier): add feature news fragment for error handler precedence
At the time of writing, exception handlers are matched in LIFO order, i.e. when searching for an error handler to match a raised exception, and more than one handler matches the exception type, the framework will choose the one that was most recently registered.
In case one has many error handlers for classes potentially inheriting from each other, this may become very error-prone and confusing in case a wrong handler swallows the exception.
Investigate if it is possible to order exceptions statically as they are being added (not at the time of catching).
Prior art: Flask/Werkzeug claim to handle this; investigate how it is done there.
The text was updated successfully, but these errors were encountered: