feat(errors): default error serializer content negotiation (#2190)

* Add unit test for default_serialize_json

* Extract options to variables for improved readability

* Extract prefered response content type negotiation to function

* Use media handlers to serialize errors by accepted content type

* refactor: remove use of ordered dict since it's no longer required

* feat: add content_type to testing result

* chore: improve implementation

* docs: add news fragment

* chore: add missing tests and improve docs

* fix: take into account async only handlers

* chore: fix mypy errors

* chore: simplify code a bit, avoiding unnecessary complexity

* fix: avoid change to app code

* chore: review feedback

* fix: properly account for q= when parting the accept header

* docs(newsfragments): improve spelling/grammar

* docs(newsfragments): revert changes to one newsfr since the impl was changed!

---------

Co-authored-by: Federico Caselli <cfederico87@gmail.com>
Co-authored-by: Vytautas Liuolia <vytautas.liuolia@gmail.com>

docs/_newsfragments/2023.newandimproved.rst

@@ -0,0 +1,4 @@
+The default error serializer will now use the response media handlers
+to better negotiate the response serialization with the client.
+The implementation still defaults to JSON if the client does not indicate any
+preference.

docs/_newsfragments/2349.newandimproved.rst

@@ -0,0 +1,3 @@
+Added :attr:`falcon.testing.Result.content_type` and
+:attr:`falcon.testing.StreamedResult.content_type` as a utility accessor
+for the ``Content-Type`` header.

falcon/app_helpers.py

@@ -291,7 +291,12 @@ def default_serialize_error(req: Request, resp: Response, exception: HTTPError)
         resp: Instance of ``falcon.Response``
         exception: Instance of ``falcon.HTTPError``
     """
-    preferred = req.client_prefers((MEDIA_XML, 'text/xml', MEDIA_JSON))
+    predefined = [MEDIA_XML, 'text/xml', MEDIA_JSON]
+    media_handlers = [mt for mt in resp.options.media_handlers if mt not in predefined]
+    # NOTE(caselit) add all the registered before the predefined ones. This ensures that
+    # in case of equal match the last one (json) is selected and that the q= is taken
+    # into consideration when selecting the media
+    preferred = req.client_prefers(media_handlers + predefined)
 
     if preferred is None:
         # NOTE(kgriffs): See if the client expects a custom media
@@ -313,11 +318,19 @@ def default_serialize_error(req: Request, resp: Response, exception: HTTPError)
             preferred = MEDIA_XML
 
     if preferred is not None:
+        handler, _, _ = resp.options.media_handlers._resolve(
+            preferred, MEDIA_JSON, raise_not_found=False
+        )
         if preferred == MEDIA_JSON:
-            handler, _, _ = resp.options.media_handlers._resolve(
-                MEDIA_JSON, MEDIA_JSON, raise_not_found=False
-            )
+            # NOTE(caselit): special case json to ensure that it's always possible to
+            # serialize an error in json even if no handler is set in the
+            # media_handlers.
             resp.data = exception.to_json(handler)
+        elif handler:
+            # NOTE(caselit): Let the app serialize the response even if it needs
+            # to re-get the handler, since async handlers may not have a sync
+            # version available.
+            resp.media = exception.to_dict()
         else:
             resp.data = exception.to_xml()
 

falcon/http_error.py

@@ -15,7 +15,6 @@
 
 from __future__ import annotations
 
-from collections import OrderedDict
 from typing import MutableMapping, Optional, Type, TYPE_CHECKING, Union
 import xml.etree.ElementTree as et
 
@@ -144,10 +143,11 @@ def __init__(
         self.code = code
 
         if href:
-            link = self.link = OrderedDict()
-            link['text'] = href_text or 'Documentation related to this error'
-            link['href'] = uri.encode(href)
-            link['rel'] = 'help'
+            self.link = {
+                'text': href_text or 'Documentation related to this error',
+                'href': uri.encode(href),
+                'rel': 'help',
+            }
         else:
             self.link = None
 
@@ -209,9 +209,10 @@ def to_json(self, handler: Optional[BaseHandler] = None) -> bytes:
 
         """
 
-        obj = self.to_dict(OrderedDict)
+        obj = self.to_dict()
         if handler is None:
             handler = _DEFAULT_JSON_HANDLER
+        # NOTE: the json handler requires the sync serialize interface
         return handler.serialize(obj, MEDIA_JSON)
 
     def to_xml(self) -> bytes:

falcon/request.py

@@ -22,12 +22,12 @@
     Callable,
     ClassVar,
     Dict,
+    Iterable,
     List,
     Literal,
     Mapping,
     Optional,
     overload,
-    Sequence,
     TextIO,
     Tuple,
     Type,
@@ -1171,7 +1171,7 @@ def client_accepts(self, media_type: str) -> bool:
         except ValueError:
             return False
 
-    def client_prefers(self, media_types: Sequence[str]) -> Optional[str]:
+    def client_prefers(self, media_types: Iterable[str]) -> Optional[str]:
         """Return the client's preferred media type, given several choices.
 
         Args:

falcon/testing/client.py

@@ -274,6 +274,11 @@ def encoding(self) -> Optional[str]:
         """
         return self._encoding
 
+    @property
+    def content_type(self) -> Optional[str]:
+        """Return the ``Content-Type`` header or ``None`` if missing."""
+        return self.headers.get('Content-Type')
+
 
 class ResultBodyStream:
     """Simple forward-only reader for a streamed test result body.
@@ -369,7 +374,7 @@ def json(self) -> Any:
         return json_module.loads(self.text)
 
     def __repr__(self) -> str:
-        content_type = self.headers.get('Content-Type', '')
+        content_type = self.content_type or ''
 
         if len(self.content) > 40:
             content = self.content[:20] + b'...' + self.content[-20:]

tests/test_httperror.py

@@ -7,6 +7,10 @@
 import pytest
 
 import falcon
+from falcon.constants import MEDIA_JSON
+from falcon.constants import MEDIA_XML
+from falcon.constants import MEDIA_YAML
+from falcon.media import BaseHandler
 import falcon.testing as testing
 
 try:
@@ -943,7 +947,127 @@ def test_MediaMalformedError(self):
         err.__cause__ = ValueError('some error')
         assert err.description == 'Could not parse foo-media body - some error'
 
+    def test_kw_only(self):
+        with pytest.raises(TypeError, match='positional argument'):
+            falcon.HTTPError(falcon.HTTP_BAD_REQUEST, 'foo', 'bar')
 
-def test_kw_only():
-    with pytest.raises(TypeError, match='positional argument'):
-        falcon.HTTPError(falcon.HTTP_BAD_REQUEST, 'foo', 'bar')
+
+JSON_CONTENT = b'{"title": "410 Gone"}'
+JSON = (MEDIA_JSON, MEDIA_JSON, JSON_CONTENT)
+CUSTOM_JSON = ('custom/any+json', MEDIA_JSON, JSON_CONTENT)
+
+XML_CONTENT = (
+    b'<?xml version="1.0" encoding="UTF-8"?><error><title>410 Gone</title></error>'
+)
+XML = (MEDIA_XML, MEDIA_XML, XML_CONTENT)
+CUSTOM_XML = ('custom/any+xml', MEDIA_XML, XML_CONTENT)
+
+YAML = (MEDIA_YAML, MEDIA_YAML, b'title: 410 Gone!')
+ASYNC_ONLY = ('application/only_async', 'application/only_async', b'this is async')
+ASYNC_WITH_SYNC = (
+    'application/async_with_sync',
+    'application/async_with_sync',
+    b'this is sync instead',
+)
+
+
+class FakeYamlMediaHandler(BaseHandler):
+    def serialize(self, media, content_type=None):
+        assert media == {'title': '410 Gone'}
+        return b'title: 410 Gone!'
+
+
+class AsyncOnlyMediaHandler(BaseHandler):
+    async def serialize_async(self, media, content_type=None):
+        assert media == {'title': '410 Gone'}
+        return b'this is async'
+
+
+class SyncInterfaceMediaHandler(AsyncOnlyMediaHandler):
+    def serialize(self, media, content_type=None):
+        assert media == {'title': '410 Gone'}
+        return b'this is sync instead'
+
+    _serialize_sync = serialize  # type: ignore[assignment]
+
+
+class TestDefaultSerializeError:
+    @pytest.fixture
+    def client(self, util, asgi):
+        app = util.create_app(asgi)
+        app.add_route('/', GoneResource())
+        return testing.TestClient(app)
+
+    @pytest.mark.parametrize('has_json_handler', [True, False])
+    def test_defaults_to_json(self, client, has_json_handler):
+        if not has_json_handler:
+            client.app.req_options.media_handlers.pop(MEDIA_JSON)
+            client.app.resp_options.media_handlers.pop(MEDIA_JSON)
+        res = client.simulate_get()
+        assert res.content_type == 'application/json'
+        assert res.headers['vary'] == 'Accept'
+        assert res.content == JSON_CONTENT
+
+    @pytest.mark.parametrize(
+        'accept, content_type, content',
+        (JSON, XML, CUSTOM_JSON, CUSTOM_XML, YAML, ASYNC_ONLY, ASYNC_WITH_SYNC),
+    )
+    def test_serializes_error_to_preferred_by_sender(
+        self, accept, content_type, content, client, asgi
+    ):
+        client.app.resp_options.media_handlers[MEDIA_YAML] = FakeYamlMediaHandler()
+        client.app.resp_options.media_handlers[ASYNC_WITH_SYNC[0]] = (
+            SyncInterfaceMediaHandler()
+        )
+        if asgi:
+            client.app.resp_options.media_handlers[ASYNC_ONLY[0]] = (
+                AsyncOnlyMediaHandler()
+            )
+        res = client.simulate_get(headers={'Accept': accept})
+        assert res.headers['vary'] == 'Accept'
+        if content_type == ASYNC_ONLY[0] and not asgi:
+            # media-json is the default content type
+            assert res.content_type == MEDIA_JSON
+            assert res.content == b''
+        else:
+            assert res.content_type == content_type
+            assert res.content == content
+
+    def test_json_async_only_error(self, util):
+        app = util.create_app(True)
+        app.add_route('/', GoneResource())
+        app.resp_options.media_handlers[MEDIA_JSON] = AsyncOnlyMediaHandler()
+        client = testing.TestClient(app)
+        with pytest.raises(NotImplementedError, match='requires the sync interface'):
+            client.simulate_get()
+
+    def test_add_xml_handler(self, client):
+        client.app.resp_options.media_handlers[MEDIA_XML] = FakeYamlMediaHandler()
+        res = client.simulate_get(headers={'Accept': 'application/xhtml+xml'})
+        assert res.content_type == MEDIA_XML
+        assert res.content == YAML[-1]
+
+    @pytest.mark.parametrize(
+        'accept, content_type',
+        [
+            (
+                # firefox
+                'text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,'
+                'image/webp,image/png,image/svg+xml,*/*;q=0.8',
+                MEDIA_XML,
+            ),
+            (
+                # safari / chrome
+                'text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,'
+                'image/apng,*/*;q=0.8',
+                MEDIA_XML,
+            ),
+            ('text/html, application/xhtml+xml, image/jxr, */*', MEDIA_JSON),  # edge
+            (f'text/html,{MEDIA_YAML};q=0.8,*/*;q=0.7', MEDIA_YAML),
+            (f'text/html,{MEDIA_YAML};q=0.8,{MEDIA_JSON};q=0.8', MEDIA_JSON),
+        ],
+    )
+    def test_hard_content_types(self, client, accept, content_type):
+        client.app.resp_options.media_handlers[MEDIA_YAML] = FakeYamlMediaHandler()
+        res = client.simulate_get(headers={'Accept': accept})
+        assert res.content_type == content_type

tests/test_media_handlers.py

@@ -9,6 +9,8 @@
 from falcon import media
 from falcon import testing
 from falcon.asgi.stream import BoundedStream
+from falcon.constants import MEDIA_JSON
+from falcon.constants import MEDIA_YAML
 
 mujson = None
 orjson = None
@@ -372,6 +374,16 @@ def on_get(self, req, resp):
     assert result.json == falcon.HTTPForbidden().to_dict()
 
 
+def test_handlers_include_new_media_handlers_in_resolving() -> None:
+    handlers = media.Handlers({falcon.MEDIA_URLENCODED: media.URLEncodedFormHandler()})
+    assert handlers._resolve(MEDIA_YAML, MEDIA_JSON, raise_not_found=False)[0] is None
+
+    js_handler = media.JSONHandler()
+    handlers[MEDIA_YAML] = js_handler
+    resolved = handlers._resolve(MEDIA_YAML, MEDIA_JSON, raise_not_found=False)[0]
+    assert resolved is js_handler
+
+
 class TestBaseHandler:
     def test_defaultError(self):
         h = media.BaseHandler()

tests/test_testing.py

@@ -4,6 +4,7 @@
 from falcon import App
 from falcon import status_codes
 from falcon import testing
+from falcon.util.sync import async_to_sync
 
 
 class CustomCookies:
@@ -103,6 +104,32 @@ def on_post(self, req, resp):
     assert result.text == falcon.MEDIA_JSON
 
 
+@pytest.mark.parametrize('mode', ['wsgi', 'asgi', 'asgi-stream'])
+def test_content_type(util, mode):
+    class Responder:
+        def on_get(self, req, resp):
+            resp.content_type = req.content_type
+
+    app = util.create_app('asgi' in mode)
+    app.add_route('/', Responder())
+
+    if 'stream' in mode:
+
+        async def go():
+            async with testing.ASGIConductor(app) as ac:
+                async with ac.simulate_get_stream(
+                    '/', content_type='my-content-type'
+                ) as r:
+                    assert r.content_type == 'my-content-type'
+            return 1
+
+        assert async_to_sync(go) == 1
+    else:
+        client = testing.TestClient(app)
+        res = client.simulate_get('/', content_type='foo-content')
+        assert res.content_type == 'foo-content'
+
+
 @pytest.mark.parametrize('cookies', [{'foo': 'bar', 'baz': 'foo'}, CustomCookies()])
 def test_create_environ_cookies(cookies):
     environ = testing.create_environ(cookies=cookies)