Skip to content

Commit

Permalink
feat(API): on exception, select most specific error handler available
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
csojinb committed Nov 2, 2019
1 parent 2e2d31d commit 0dc25fc
Showing 1 changed file with 41 additions and 29 deletions.
70 changes: 41 additions & 29 deletions falcon/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ def __init__(self, media_type=DEFAULT_MEDIA_TYPE,
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()
Expand Down Expand Up @@ -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)
except Exception as ex:
if not self._handle_exception(req, resp, ex, params):
raise
Expand Down Expand Up @@ -610,18 +611,11 @@ def handler(req, resp, ex, params):
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`.
Expand Down Expand Up @@ -793,6 +787,22 @@ def _python_error_handler(self, req, resp, error, params):
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.
Expand All @@ -809,21 +819,22 @@ def _handle_exception(self, req, resp, ex, params):
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)

return True

# NOTE(kgriffs): No error handlers are defined for ex and it
# is not one of (HTTPStatus, HTTPError, Exception), since it
# would have matched one of the corresponding default handlers.
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

# NOTE(kgriffs): No error handlers are defined for ex
# and it is not one of (HTTPStatus, HTTPError), since it
# would have matched one of the corresponding default
# handlers.
return False

# PERF(kgriffs): Moved from api_helpers since it is slightly faster
Expand Down Expand Up @@ -878,7 +889,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)
else:
iterable = stream

Expand Down

0 comments on commit 0dc25fc

Please sign in to comment.