Repository: falconry/falcon
Pull request #1418 (2019-01-25 11:12:10)

allow passing iterable of exceptions to add_error_handler
This PR extends the `add_error_handler` to accept a generic iterable of exceptions.

Todo:

- [x] update docs
- [x] update tests

kgriffs: Hi @carlodri, do you think you'll be able to get this over the finish line over the next day or two? If not, we'll need to push this to the next release after 2.0.
carlodri: @kgriffs sorry but I'm quite busy with my job this week... Maybe it's better if we push it forward. If I find the time in the next few days, I will let you know here ASAP.
carlodri: Now that I think of it I'm actually puzzled as to _why_ this even works with exception tuples as it is, because 
```python
isinstance(MyException, (MyBaseException, AnotherException))
```
is `False`... What am I missing here?
vytas7: @carlodri Could you show a code example where it happens?
Might it be that you happened to compare a *type* against these exceptions classes?
You would want to use `issubclass` in that case, such as the example in my previous comment, see also, for instance:
```python
>>> class MyException(KeyboardInterrupt):
...     """Press that Ctrl+C!"""
... 
>>> issubclass(MyException, (ValueError, BaseException))
True
>>> isinstance(MyException, (ValueError, BaseException))
False
>>> # But:
... 
>>> isinstance(MyException(), (ValueError, BaseException))
True
```
carlodri: @vytas7 thanks for the example above, in the rush I was indeed mixing up _instances_ and _classes_... I need to think for a moment of which is the best solution. I also like @kgriffs's idea above of normalizing to tuple upstream in `add_error_handler`.
codecov[bot]: # [Codecov](https://codecov.io/gh/falconry/falcon/pull/1418?src=pr&el=h1) Report
> Merging [#1418](https://codecov.io/gh/falconry/falcon/pull/1418?src=pr&el=desc) into [master](https://codecov.io/gh/falconry/falcon/commit/35d5220c7ee359ac6b03788107a194644eb00344?src=pr&el=desc) will **decrease** coverage by `2.23%`.
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff             @@
##           master    #1418      +/-   ##
==========================================
- Coverage     100%   97.76%   -2.24%     
==========================================
  Files          39       39              
  Lines        2550     2550              
  Branches      397      382      -15     
==========================================
- Hits         2550     2493      -57     
- Misses          0       47      +47     
- Partials        0       10      +10
```


| [Impacted Files](https://codecov.io/gh/falconry/falcon/pull/1418?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [falcon/media/validators/jsonschema.py](https://codecov.io/gh/falconry/falcon/pull/1418/diff?src=pr&el=tree#diff-ZmFsY29uL21lZGlhL3ZhbGlkYXRvcnMvanNvbnNjaGVtYS5weQ==) | `50% <0%> (-50%)` | :arrow_down: |
| [falcon/util/compat.py](https://codecov.io/gh/falconry/falcon/pull/1418/diff?src=pr&el=tree#diff-ZmFsY29uL3V0aWwvY29tcGF0LnB5) | `58.33% <0%> (-41.67%)` | :arrow_down: |
| [falcon/util/uri.py](https://codecov.io/gh/falconry/falcon/pull/1418/diff?src=pr&el=tree#diff-ZmFsY29uL3V0aWwvdXJpLnB5) | `86.04% <0%> (-13.96%)` | :arrow_down: |
| [falcon/response\_helpers.py](https://codecov.io/gh/falconry/falcon/pull/1418/diff?src=pr&el=tree#diff-ZmFsY29uL3Jlc3BvbnNlX2hlbHBlcnMucHk=) | `92.72% <0%> (-7.28%)` | :arrow_down: |
| [falcon/routing/static.py](https://codecov.io/gh/falconry/falcon/pull/1418/diff?src=pr&el=tree#diff-ZmFsY29uL3JvdXRpbmcvc3RhdGljLnB5) | `96.22% <0%> (-3.78%)` | :arrow_down: |
| [falcon/util/misc.py](https://codecov.io/gh/falconry/falcon/pull/1418/diff?src=pr&el=tree#diff-ZmFsY29uL3V0aWwvbWlzYy5weQ==) | `96.93% <0%> (-3.07%)` | :arrow_down: |
| [falcon/testing/helpers.py](https://codecov.io/gh/falconry/falcon/pull/1418/diff?src=pr&el=tree#diff-ZmFsY29uL3Rlc3RpbmcvaGVscGVycy5weQ==) | `97.36% <0%> (-2.64%)` | :arrow_down: |
| [falcon/response.py](https://codecov.io/gh/falconry/falcon/pull/1418/diff?src=pr&el=tree#diff-ZmFsY29uL3Jlc3BvbnNlLnB5) | `98.95% <0%> (-1.05%)` | :arrow_down: |
| [falcon/request.py](https://codecov.io/gh/falconry/falcon/pull/1418/diff?src=pr&el=tree#diff-ZmFsY29uL3JlcXVlc3QucHk=) | `99.21% <0%> (-0.79%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/falconry/falcon/pull/1418?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/1418?src=pr&el=footer). Last update [35d5220...72bcd64](https://codecov.io/gh/falconry/falcon/pull/1418?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

carlodri: @vytas7 @kgriffs this is ready for review, let me know your thoughts!
vytas7: This is really shaping up, thanks!
(I would just like to understand the design choice behind the current EAFP implementation, but I am not opposed to it).
vytas7: Let's do that indeed! 
Great effort btw @carlodri ! 
carlodri: @vytas7 I also fixed the last issue you raised concerning the unwanted overhead in the case of as single-element tuple. Let me know if it's ok!

Patch:
diff --git a/falcon/api.py b/falcon/api.py
index 358639a..6898ac3 100644
--- a/falcon/api.py
+++ b/falcon/api.py
@@ -522,9 +522,9 @@ class API(object):
             exception type in question.
 
         Args:
-            exception (type): Whenever an error occurs when handling a request
-                that is an instance of this exception class, the associated
-                handler will be called.
+            exception (type or iterable of types): When handling a request,
+                whenever an error occurs that is an instance of one of these
+                exception types, the associated handler will be called.
             handler (callable): A function or callable object taking the form
                 ``func(req, resp, ex, params)``.
 
@@ -542,6 +542,8 @@ class API(object):
                             # Convert to an instance of falcon.HTTPError
                             raise falcon.HTTPError(falcon.HTTP_792)
 
+                If an iterable of exception types is specified instead of
+                a single type, the handler must be explicitly specified.
 
         """
 
@@ -554,9 +556,23 @@ class API(object):
                                      'method named "handle" that is a '
                                      'member of the given exception class.')
 
-        # Insert at the head of the list in case we get duplicate
-        # adds (will cause the most recently added one to win).
-        self._error_handlers.insert(0, (exception, handler))
+        try:
+            exception_tuple = tuple(exception)
+        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.')
 
     def set_error_serializer(self, serializer):
         """Override the default serializer for instances of :class:`~.HTTPError`.


Test patch:
diff --git a/tests/test_error_handlers.py b/tests/test_error_handlers.py
index af478af..b21cb33 100644
--- a/tests/test_error_handlers.py
+++ b/tests/test_error_handlers.py
@@ -114,3 +114,35 @@ class TestErrorHandler(object):
         result = client.simulate_delete()
         assert result.status_code == 723
         assert result.text == 'error: CustomException'
+
+    @pytest.mark.parametrize('exceptions', [
+        (Exception, CustomException),
+        [Exception, CustomException],
+    ])
+    def test_handler_multiple_exception_iterable(self, client, exceptions):
+        client.app.add_error_handler(exceptions, capture_error)
+
+        result = client.simulate_get()
+        assert result.status_code == 723
+
+        result = client.simulate_delete()
+        assert result.status_code == 723
+
+    def test_handler_single_exception_iterable(self, client):
+        def exception_list_generator():
+            yield CustomException
+
+        client.app.add_error_handler(exception_list_generator(), capture_error)
+
+        result = client.simulate_delete()
+        assert result.status_code == 723
+
+    @pytest.mark.parametrize('exceptions', [
+        NotImplemented,
+        'Hello, world!',
+        frozenset([ZeroDivisionError, int, NotImplementedError]),
+        iter([float, float]),
+    ])
+    def test_invalid_add_exception_handler_input(self, client, exceptions):
+        with pytest.raises(TypeError):
+            client.app.add_error_handler(exceptions, capture_error)