Repository: falconry/falcon
Pull request #1603 (2019-11-02 15:26:06)

feat(API): on exception, select most specific error handler available
# Summary of Changes

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.~~ Tests and documentation have been added. Thanks @vytas7 for implementation notes!

### 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. 

# Related Issues

Addresses #1514.

~~I believe it would also allow reversion of https://github.com/falconry/falcon/pull/1599.~~ Not true. I misremembered why #1599 was needed.

# Pull Request Checklist

This is just a reminder about the most common mistakes.  Please make sure that you tick all *appropriate* boxes.  But please read our [contribution guide](https://github.com/falconry/falcon/blob/master/CONTRIBUTING.md) at least once; it will save you a few review cycles!

If an item doesn't apply to your pull request, **check it anyway** to make it apparent that there's nothing to do.

- [x] Added **tests** for changed code.
- [x] Prefixed code comments with GitHub nick and an appropriate prefix.
- [x] Coding style is consistent with the rest of the framework.
- [x] Updated **documentation** for changed code.
    - [x] Added docstrings for any new classes, functions, or modules.
    - [x] Updated docstrings for any modifications to existing code.
    - [x] Added references to new classes, functions, or modules to the relevant RST file under `docs/`.
    - [x] Updated all relevant supporting documentation files under `docs/`.
    - [x] A copyright notice is included at the top of any new modules (using your own name or the name of your organization).
    - [x] Changed/added classes/methods/functions have appropriate `versionadded`, `versionchanged`, or `deprecated` [directives](http://www.sphinx-doc.org/en/stable/usage/restructuredtext/directives.html?highlight=versionadded#directive-versionadded).
- [x] Changes (and possible deprecations) have [towncrier](https://pypi.org/project/towncrier/) news fragments under `docs/_newsfragments/`. (Run `towncrier --draft` to ensure it renders correctly.)

If you have *any* questions to *any* of the points above, just **submit and ask**! This checklist is here to *help* you, not to deter you from contributing!

*PR template inspired by the attrs project.*


csojinb: Would this change warrant a towncrier entry?
codecov[bot]: # [Codecov](https://codecov.io/gh/falconry/falcon/pull/1603?src=pr&el=h1) Report
> Merging [#1603](https://codecov.io/gh/falconry/falcon/pull/1603?src=pr&el=desc) into [master](https://codecov.io/gh/falconry/falcon/commit/4df7706eb69f2064ee00a4d5280505d893f27638?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/falconry/falcon/pull/1603/graphs/tree.svg?width=650&token=zOtkfNQ3h6&height=150&src=pr)](https://codecov.io/gh/falconry/falcon/pull/1603?src=pr&el=tree)

```diff
@@          Coverage Diff           @@
##           master   #1603   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          40      40           
  Lines        2595    2595           
  Branches      394     394           
======================================
  Hits         2595    2595
```



------

[Continue to review full report at Codecov](https://codecov.io/gh/falconry/falcon/pull/1603?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/falconry/falcon/pull/1603?src=pr&el=footer). Last update [4df7706...97e297e](https://codecov.io/gh/falconry/falcon/pull/1603?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

vytas7: @csojinb Re Towncrier I would think, yes, we need an entry (or actually two?) since it is both a breaking change, and a new feature. 
csojinb: Yes, I can try to do it this weekend

On Thu, Dec 12, 2019 at 13:44 Kurt Griffiths <notifications@github.com>
wrote:

> *@kgriffs* requested changes on this pull request.
>
> This looks good, do you think you can finish this up soon? I'd love to get
> this merged for the alpha release. AFAICT the remaining TODO items include:
>
>    - Add towncrier news fragments
>    - Rebase on master (fix conflicts)
>    - Tweak docstrings (optional)
>    - Tweak line wrapping (optional)
>
> ------------------------------
>
> In falcon/api.py
> <https://github.com/falconry/falcon/pull/1603#discussion_r357306845>:
>
> >              app.add_error_handler(falcon.HTTPError, custom_handle_http_error)
> -            app.add_error_handler(CustomException)
> +            app.add_error_handler(Exception, custom_handle_uncaught_exception)
> +            app.add_error_handler(falcon.HTTPNotFound, custom_handle_404)
> +
> +        If an instance of ``falcon.HTTPForbidden`` is raised, it will be
>
> double backticks are fine, although I tend to add parens to the function
> name myself to help make it obvious what it is.
>
> Re class names, I'm fine either way TBH, since you are referencing example
> code here. But it couldn't hurt to add the :class: directive.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/falconry/falcon/pull/1603?email_source=notifications&email_token=ABRVDSYBXSEUQM545YHV3I3QYKA7HA5CNFSM4JIGEACKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCPAWL7Q#pullrequestreview-331441662>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABRVDS5R4LKUAQNI3GFPSEDQYKA7HANCNFSM4JIGEACA>
> .
>

csojinb: Okay, I rebased, undid the overzealous pep8-ing, tweaked the `add_error_handler` docs, and added a towncrier fragment. I think that's everything. cc @kgriffs @vytas7 
csojinb: Oh, so it is. I guess kdiff made that decision for me and I didn't look closely enough. I'll fix it and re-push
csojinb: @vytas7 fixed
csojinb: Moving my copied TODO list out of the PR description into this comment:

### TODOs:
- [x] Add towncrier news fragments
- [x] Rebase on master (fix conflicts)
- [x] Tweak docstrings (optional)
- [x] Tweak line wrapping (optional)

(Turns out you can't just check off checkboxes in someone else's comment.)

Patch:
diff --git a/docs/_newsfragments/1603-smart-exception-handler-precedence.feature.rst b/docs/_newsfragments/1603-smart-exception-handler-precedence.feature.rst
new file mode 100644
index 0000000..ba23500
--- /dev/null
+++ b/docs/_newsfragments/1603-smart-exception-handler-precedence.feature.rst
@@ -0,0 +1,1 @@
+Exceptions are now handled by the registered handler for the most specific matching exception class, rather than in reverse order of registration. "Specificity" is determined by the method resolution order of the raised exception type. (See :meth:`~.App.add_error_handler` for more details.)
diff --git a/falcon/app.py b/falcon/app.py
index be2ef5d..9313cca 100644
--- a/falcon/app.py
+++ b/falcon/app.py
@@ -207,7 +207,7 @@ class App:
         self._request_type = request_type
         self._response_type = response_type
 
-        self._error_handlers = []
+        self._error_handlers = {}
         self._serialize_error = helpers.default_serialize_error
 
         self.req_options = RequestOptions()
@@ -538,18 +538,30 @@ class App:
         the client.  Alternatively, a handler may modify `resp`
         directly.
 
-        Error handlers are matched in LIFO order. In other words, 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.
-        Therefore, more general error handlers (e.g., for the
-        standard ``Exception`` type) should be added first, to avoid
-        masking more specific handlers for subclassed types. For example::
+        An error handler "matches" a raised exception if the exception is an
+        instance of the corresponding exception type. If more than one error
+        handler matches the raised exception, the framework will choose the
+        most specific one, as determined by the method resolution order of the
+        raised exception type. If multiple error handlers are registered for the
+        *same* exception class, then the most recently-registered handler is
+        used.
 
-            app = falcon.App()
-            app.add_error_handler(Exception, custom_handle_uncaught_exception)
+        For example, suppose we register error handlers as follows::
+
+            app = falcon.API()
+            app.add_error_handler(falcon.HTTPNotFound, custom_handle_not_found)
             app.add_error_handler(falcon.HTTPError, custom_handle_http_error)
-            app.add_error_handler(CustomException)
+            app.add_error_handler(Exception, custom_handle_uncaught_exception)
+            app.add_error_handler(falcon.HTTPNotFound, custom_handle_404)
+
+        If an instance of ``falcon.HTTPForbidden`` is raised, it will be
+        handled by ``custom_handle_http_error()``. ``falcon.HTTPError`` is a
+        superclass of ``falcon.HTTPForbidden`` and a subclass of ``Exception``,
+        so it is the most specific exception type with a registered handler.
+
+        If an instance of ``falcon.HTTPNotFound`` is raised, it will be handled
+        by ``custom_handle_404()``, not by ``custom_handle_not_found()``, because
+        ``custom_handle_404()`` was registered more recently.
 
         .. Note::
 
@@ -559,11 +571,6 @@ class App:
             exceptions to the WSGI server. These can be overridden by adding a
             custom error handler method for the exception type in question.
 
-            Be aware that both :class:`~.HTTPError` and :class:`~.HTTPStatus`
-            inherit from the standard ``Exception`` type, so overriding the
-            default ``Exception`` handler will override all three default
-            handlers, due to the LIFO ordering of handler-matching.
-
         Args:
             exception (type or iterable of types): When handling a request,
                 whenever an error occurs that is an instance of the specified
@@ -589,6 +596,11 @@ class App:
                 If an iterable of exception types is specified instead of
                 a single type, the handler must be explicitly specified.
 
+        .. versionchanged:: 3.0
+            Breaking change: error handler now selected by most specific
+            matching error class, rather than most recently registered matching
+            error class.
+
         """
         def wrap_old_handler(old_handler):
             @wraps(old_handler)
@@ -616,18 +628,11 @@ class App:
         except TypeError:
             exception_tuple = (exception, )
 
-        if all(issubclass(exc, BaseException) for exc in exception_tuple):
-            # Insert at the head of the list in case we get duplicate
-            # adds (will cause the most recently added one to win).
-            if len(exception_tuple) == 1:
-                # In this case, insert only the single exception type
-                # (not a tuple), to avoid unnnecessary overhead in the
-                # exception handling path.
-                self._error_handlers.insert(0, (exception_tuple[0], handler))
-            else:
-                self._error_handlers.insert(0, (exception_tuple, handler))
-        else:
-            raise TypeError('"exception" must be an exception type.')
+        for exc in exception_tuple:
+            if not issubclass(exc, BaseException):
+                raise TypeError('"exception" must be an exception type.')
+
+            self._error_handlers[exc] = handler
 
     def set_error_serializer(self, serializer):
         """Override the default serializer for instances of :class:`~.HTTPError`.
@@ -799,6 +804,22 @@ class App:
         self._compose_error_response(
             req, resp, falcon.HTTPInternalServerError())
 
+    def _find_error_handler(self, ex):
+        # NOTE(csojinb): The `__mro__` class attribute returns the method
+        # resolution order tuple, i.e. the complete linear inheritance chain
+        # ``(type(ex), ..., object)``. For a valid exception class, the last
+        # two entries in the tuple will always be ``BaseException``and
+        # ``object``, so here we iterate over the lineage of exception types,
+        # from most to least specific.
+
+        # PERF(csojinb): The expression ``type(ex).__mro__[:-1]`` here is not
+        # super readable, but we inline it to avoid function call overhead.
+        for exc in type(ex).__mro__[:-1]:
+            handler = self._error_handlers.get(exc)
+
+            if handler is not None:
+                return handler
+
     def _handle_exception(self, req, resp, ex, params):
         """Handle an exception raised from mw or a responder.
 
@@ -815,17 +836,17 @@ class App:
             bool: ``True`` if a handler was found and called for the
             exception, ``False`` otherwise.
         """
+        err_handler = self._find_error_handler(ex)
 
-        for err_type, err_handler in self._error_handlers:
-            if isinstance(ex, err_type):
-                try:
-                    err_handler(req, resp, ex, params)
-                except HTTPStatus as status:
-                    self._compose_status_response(req, resp, status)
-                except HTTPError as error:
-                    self._compose_error_response(req, resp, error)
+        if err_handler is not None:
+            try:
+                err_handler(req, resp, ex, params)
+            except HTTPStatus as status:
+                self._compose_status_response(req, resp, status)
+            except HTTPError as error:
+                self._compose_error_response(req, resp, error)
 
-                return True
+            return True
 
         # NOTE(kgriffs): No error handlers are defined for ex
         # and it is not one of (HTTPStatus, HTTPError), since it


Test patch:
diff --git a/tests/test_error_handlers.py b/tests/test_error_handlers.py
index b46d721..35aac39 100644
--- a/tests/test_error_handlers.py
+++ b/tests/test_error_handlers.py
@@ -91,14 +91,14 @@ class TestErrorHandler:
         assert result.status_code == 723
         assert result.text == 'error: CustomException'
 
-    def test_error_order_duplicate(self, client):
+    def test_error_precedence_duplicate(self, client):
         client.app.add_error_handler(Exception, capture_error)
         client.app.add_error_handler(Exception, handle_error_first)
 
         result = client.simulate_get()
         assert result.text == 'first error handler'
 
-    def test_error_order_subclass(self, client):
+    def test_error_precedence_subclass(self, client):
         client.app.add_error_handler(Exception, capture_error)
         client.app.add_error_handler(CustomException, handle_error_first)
 
@@ -110,13 +110,13 @@ class TestErrorHandler:
         assert result.status_code == 723
         assert result.text == 'error: Plain Exception'
 
-    def test_error_order_subclass_masked(self, client):
+    def test_error_precedence_subclass_order_indifference(self, client):
         client.app.add_error_handler(CustomException, handle_error_first)
         client.app.add_error_handler(Exception, capture_error)
 
         result = client.simulate_delete()
-        assert result.status_code == 723
-        assert result.text == 'error: CustomException'
+        assert result.status_code == 200
+        assert result.text == 'first error handler'
 
     @pytest.mark.parametrize('exceptions', [
         (Exception, CustomException),