Repository: falconry/falcon
Pull request #1427 (2019-02-04 23:50:11)

fix(errors): call custom serializers even for errors without represention
Addresses https://github.com/falconry/falcon/issues/1422

This is still somewhat work in progress, I am sharing this to speed things up for the impending 2.0 release.
If the general direction looks good, I will update `CHANGES.rst` etc.

codecov[bot]: # [Codecov](https://codecov.io/gh/falconry/falcon/pull/1427?src=pr&el=h1) Report
> Merging [#1427](https://codecov.io/gh/falconry/falcon/pull/1427?src=pr&el=desc) into [master](https://codecov.io/gh/falconry/falcon/commit/04decb9f7a7b7e5e09851f2bf990c180a2d7fd26?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

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

```diff
@@          Coverage Diff           @@
##           master   #1427   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          39      39           
  Lines        2568    2570    +2     
  Branches      396     397    +1     
======================================
+ Hits         2568    2570    +2
```


| [Impacted Files](https://codecov.io/gh/falconry/falcon/pull/1427?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [falcon/http\_error.py](https://codecov.io/gh/falconry/falcon/pull/1427/diff?src=pr&el=tree#diff-ZmFsY29uL2h0dHBfZXJyb3IucHk=) | `100% <ø> (ø)` | :arrow_up: |
| [falcon/api\_helpers.py](https://codecov.io/gh/falconry/falcon/pull/1427/diff?src=pr&el=tree#diff-ZmFsY29uL2FwaV9oZWxwZXJzLnB5) | `100% <100%> (ø)` | :arrow_up: |

------

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

codecov[bot]: # [Codecov](https://codecov.io/gh/falconry/falcon/pull/1427?src=pr&el=h1) Report
> Merging [#1427](https://codecov.io/gh/falconry/falcon/pull/1427?src=pr&el=desc) into [master](https://codecov.io/gh/falconry/falcon/commit/04decb9f7a7b7e5e09851f2bf990c180a2d7fd26?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `100%`.

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

```diff
@@          Coverage Diff           @@
##           master   #1427   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          39      39           
  Lines        2568    2570    +2     
  Branches      396     397    +1     
======================================
+ Hits         2568    2570    +2
```


| [Impacted Files](https://codecov.io/gh/falconry/falcon/pull/1427?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [falcon/http\_error.py](https://codecov.io/gh/falconry/falcon/pull/1427/diff?src=pr&el=tree#diff-ZmFsY29uL2h0dHBfZXJyb3IucHk=) | `100% <ø> (ø)` | :arrow_up: |
| [falcon/api\_helpers.py](https://codecov.io/gh/falconry/falcon/pull/1427/diff?src=pr&el=tree#diff-ZmFsY29uL2FwaV9oZWxwZXJzLnB5) | `100% <100%> (ø)` | :arrow_up: |

------

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


Patch:
diff --git a/falcon/api.py b/falcon/api.py
index e20d825..2537fc5 100644
--- a/falcon/api.py
+++ b/falcon/api.py
@@ -556,6 +556,15 @@ class API(object):
             "+json" or "+xml" suffix, the default serializer will
             convert the error to JSON or XML, respectively.
 
+        Note:
+            The default serializer will not render any response body for
+            :class:`~.HTTPError` instances where the `has_representation`
+            property evaluates to ``False`` (such as in the case of types
+            that subclass :class:`falcon.http_error.NoRepresentation`).
+            However a custom serializer will be called regardless of the
+            property value, and it may choose to override the
+            representation logic.
+
         The :class:`~.HTTPError` class contains helper methods,
         such as `to_json()` and `to_dict()`, that can be used from
         within custom serializers. For example::
@@ -566,7 +575,7 @@ class API(object):
                 preferred = req.client_prefers(('application/x-yaml',
                                                 'application/json'))
 
-                if preferred is not None:
+                if exception.has_representation and preferred is not None:
                     if preferred == 'application/json':
                         representation = exception.to_json()
                     else:
@@ -688,8 +697,7 @@ class API(object):
         if error.headers is not None:
             resp.set_headers(error.headers)
 
-        if error.has_representation:
-            self._serialize_error(req, resp, error)
+        self._serialize_error(req, resp, error)
 
     def _http_status_handler(self, req, resp, status, params):
         self._compose_status_response(req, resp, status)
diff --git a/falcon/api_helpers.py b/falcon/api_helpers.py
index 81cce04..fbaa2c8 100644
--- a/falcon/api_helpers.py
+++ b/falcon/api_helpers.py
@@ -137,6 +137,9 @@ def default_serialize_error(req, resp, exception):
         resp: Instance of ``falcon.Response``
         exception: Instance of ``falcon.HTTPError``
     """
+    if not exception.has_representation:
+        return
+
     representation = None
 
     preferred = req.client_prefers(('application/xml',
diff --git a/falcon/http_error.py b/falcon/http_error.py
index e8b30cd..21ac194 100644
--- a/falcon/http_error.py
+++ b/falcon/http_error.py
@@ -47,6 +47,11 @@ class HTTPError(Exception):
 
             (See also: :class:`falcon.http_error.NoRepresentation`)
 
+            Note:
+                A custom error serializer
+                (see :meth:`~.API.set_error_serializer`) may choose to set a
+                response body regardless of the value of this property.
+
         title (str): Error title to send to the client.
         description (str): Description of the error to send to the client.
         headers (dict): Extra headers to add to the response.


Test patch:
diff --git a/tests/test_httperror.py b/tests/test_httperror.py
index 54ed8e6..97dfc67 100644
--- a/tests/test_httperror.py
+++ b/tests/test_httperror.py
@@ -414,6 +414,38 @@ class TestHTTPError(object):
         _check('application/x-yaml', yaml.load)
         _check('application/json', json.loads)
 
+    @pytest.mark.parametrize('method,path,status', [
+        ('GET', '/404', 404),
+        ('GET', '/notfound', 404),
+        ('REPORT', '/404', 405),
+        ('BREW', '/notfound', 400),
+    ])
+    def test_custom_error_serializer_optional_representation(self, client, method, path, status):
+        def _simple_serializer(req, resp, exception):
+            representation = exception.to_dict()
+            representation.update(status=int(exception.status[:3]))
+
+            resp.content_type = falcon.MEDIA_JSON
+            resp.media = representation
+
+        client.app.add_route('/404', NotFoundResource())
+        client.app.add_route('/notfound', NotFoundResourceWithBody())
+        client.app.set_error_serializer(_simple_serializer)
+
+        resp = client.simulate_request(path=path, method=method)
+        assert resp.json['title']
+        assert resp.json['status'] == status
+
+    def test_custom_serializer_no_representation(self, client):
+        def _chatty_serializer(req, resp, exception):
+            resp.content_type = falcon.MEDIA_TEXT
+            resp.body = b'You might think this error should not have a body'
+
+        client.app.add_route('/416', RangeNotSatisfiableResource())
+        client.app.set_error_serializer(_chatty_serializer)
+        resp = client.simulate_get(path='/416')
+        assert resp.text == 'You might think this error should not have a body'
+
     def test_client_does_not_accept_anything(self, client):
         headers = {
             'Accept': '45087gigo;;;;',