feat(API): on exception, select most specific error handler available #1603

csojinb · 2019-11-02T15:26:06Z

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

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.

falcon/api.py

csojinb · 2019-11-02T21:38:27Z

Would this change warrant a towncrier entry? Or a versionchanged in the docstring? Seems like probably a versionchanged would be good... ~~should I just assume 3.0 for now?~~ I see that #1514 is currently marked as version 3.0, so I'll go with that.

codecov · 2019-11-02T21:50:30Z

Codecov Report

Merging #1603 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1603   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          40      40           
  Lines        2692    2692           
  Branches      397     397           
======================================
  Hits         2692    2692

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b68c4ff...2cee9c1. Read the comment docs.

falcon/api.py

vytas7 · 2019-11-03T18:32:48Z

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

myusko · 2019-11-06T08:31:21Z

falcon/api.py

@@ -269,7 +269,8 @@ def __call__(self, env, start_response):  # noqa: C901
                    # next-hop child resource. In that case, the object
                    # being asked to dispatch to its child will raise an
                    # HTTP exception signalling the problem, e.g. a 404.
-                    responder, params, resource, req.uri_template = self._get_responder(req)
+                    responder, params, resource, req.uri_template = self._get_responder(
+                        req)


Not a big deal, but do we need additional space for self._get_responder?

You mean, move it to another line because it's past 80 characters? Or, are you asking if this change shouldn't have occurred?

I didn't intentionally make this change here, I think it must have been my auto-pep8. It looks like Falcon's pep8-checker rules allow 99-character lines, so probably I should revert this newline change. I guess my tool didn't pick up the config.

Yeah, falcon allows 99-characters lines, flake8 config.

In that case, I think would be better to revert that newline change

myusko · 2019-11-06T08:36:24Z

falcon/api.py

@@ -879,7 +898,8 @@ def _get_body(self, resp, wsgi_file_wrapper=None):
                    iterable = wsgi_file_wrapper(stream,
                                                 self._STREAM_BLOCK_SIZE)
                else:
-                    iterable = helpers.CloseableStreamIterator(stream, self._STREAM_BLOCK_SIZE)
+                    iterable = helpers.CloseableStreamIterator(
+                        stream, self._STREAM_BLOCK_SIZE)


And the same here:)

kgriffs

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)

falcon/api.py

csojinb · 2019-12-12T23:04:13Z

Yes, I can try to do it this weekend

…

On Thu, Dec 12, 2019 at 13:44 Kurt Griffiths ***@***.***> wrote: ***@***.**** 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 <#1603 (comment)>: > 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 <#1603?email_source=notifications&email_token=ABRVDSYBXSEUQM545YHV3I3QYKA7HA5CNFSM4JIGEACKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCPAWL7Q#pullrequestreview-331441662>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABRVDS5R4LKUAQNI3GFPSEDQYKA7HANCNFSM4JIGEACA> .

`_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 falconry#1527, which added a default handler for ``Exception``

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.

csojinb · 2019-12-15T23:01:35Z

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

vytas7

Looking great 👍

Just one thing, not sure if the GH PR interface is playing tricks on me, or did you happen to reintroduce falcon/api.py by manual master merge? This file is now removed in master.

csojinb · 2019-12-15T23:16:00Z

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

For the change in error-handler-match strategy

csojinb · 2019-12-15T23:28:42Z

@vytas7 fixed

csojinb · 2019-12-15T23:30:03Z

Moving my copied TODO list out of the PR description into this comment:

TODOs:

Add towncrier news fragments
Rebase on master (fix conflicts)
Tweak docstrings (optional)
Tweak line wrapping (optional)

(Turns out you can't just check off checkboxes in someone else's comment.)

vytas7

👍

kgriffs · 2019-12-16T22:41:35Z

🎉

csojinb commented Nov 2, 2019

View reviewed changes