feat(resp): add option to disable XML serialization of errors (#2356)

* feat: add option to enable xml serialization on errors.

The option is on by default

* chore: add coverage to missing branch

* test: fix failing test

* chore: review feedback

* docs: improve documentation

docs/_newsfragments/2355.newandimproved.rst

@@ -0,0 +1,10 @@
+Added a new flag :attr:`~falcon.ResponseOptions.xml_error_serialization` that
+can be used to disable XML serialization of :class:`~falcon.HTTPError` when
+using the default error serializer (and the client preferred it).
+This flag currently defaults to ``True``, keeping the same behavior as the previous
+Falcon series; Falcon 5.0 will change the default to ``False``, with the objective of
+completely removing support for it in a future version.
+User that whish to retain support for XML serialization of the errors in the default
+error serializer should add an XML media handler.
+
+Deprecated the :meth:`~falcon.HTTPError.to_xml` method.

falcon/app_helpers.py

@@ -291,9 +291,13 @@ def default_serialize_error(req: Request, resp: Response, exception: HTTPError)
         resp: Instance of ``falcon.Response``
         exception: Instance of ``falcon.HTTPError``
     """
-
-    predefined = [MEDIA_JSON, 'text/xml', MEDIA_XML]
-    media_handlers = [mt for mt in resp.options.media_handlers if mt not in predefined]
+    options = resp.options
+    predefined = (
+        [MEDIA_JSON, 'text/xml', MEDIA_XML]
+        if options.xml_error_serialization
+        else [MEDIA_JSON]
+    )
+    media_handlers = [mt for mt in options.media_handlers if mt not in predefined]
     # NOTE(caselit,vytas): Add the registered handlers after the predefined
     #   ones. This ensures that in the case of an equal match, the first one
     #   (JSON) is selected and that the q parameter is taken into consideration
@@ -317,10 +321,13 @@ def default_serialize_error(req: Request, resp: Response, exception: HTTPError)
         if '+json' in accept:
             preferred = MEDIA_JSON
         elif '+xml' in accept:
+            # NOTE(caselit): ignore xml_error_serialization when
+            # checking if the media should be xml. This gives a chance to
+            # a xml media handler, if any, to be used
             preferred = MEDIA_XML
 
     if preferred is not None:
-        handler, _, _ = resp.options.media_handlers._resolve(
+        handler, _, _ = options.media_handlers._resolve(
             preferred, MEDIA_JSON, raise_not_found=False
         )
         if preferred == MEDIA_JSON:
@@ -333,8 +340,8 @@ def default_serialize_error(req: Request, resp: Response, exception: HTTPError)
             # 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()
+        elif options.xml_error_serialization:
+            resp.data = exception._to_xml()
 
         # NOTE(kgriffs): No need to append the charset param, since
         #   utf-8 is the default for both JSON and XML.

falcon/http_error.py

@@ -19,6 +19,7 @@
 import xml.etree.ElementTree as et
 
 from falcon.constants import MEDIA_JSON
+from falcon.util import deprecated
 from falcon.util import misc
 from falcon.util import uri
 
@@ -43,8 +44,7 @@ class HTTPError(Exception):
 
     To customize what data is passed to the serializer, subclass
     ``HTTPError`` and override the ``to_dict()`` method (``to_json()``
-    is implemented via ``to_dict()``). To also support XML, override
-    the ``to_xml()`` method.
+    is implemented via ``to_dict()``).
 
     `status` is the only positional argument allowed,
     the other arguments are defined as keyword-only.
@@ -214,13 +214,8 @@ def to_json(self, handler: Optional[BaseHandler] = None) -> bytes:
         # NOTE: the json handler requires the sync serialize interface
         return handler.serialize(obj, MEDIA_JSON)
 
-    def to_xml(self) -> bytes:
-        """Return an XML-encoded representation of the error.
-
-        Returns:
-            bytes: An XML document for the error.
-
-        """
+    def _to_xml(self) -> bytes:
+        """Return an XML-encoded representation of the error."""
 
         error_element = et.Element('error')
 
@@ -242,6 +237,18 @@ def to_xml(self) -> bytes:
             error_element, encoding='utf-8'
         )
 
+    @deprecated(
+        'The internal serialization to xml is deprecated. A similar output can be '
+        'obtained by serializing to xml the output of ``.to_dict()``'
+    )
+    def to_xml(self) -> bytes:
+        """Return an XML-encoded representation of the error.
+
+        Returns:
+            bytes: An XML document for the error.
+        """
+        return self._to_xml()
+
 
 # NOTE: initialized in falcon.media.json, that is always imported since Request/Response
 # are imported by falcon init.

falcon/response.py

@@ -1387,26 +1387,44 @@ class ResponseOptions:
     media_handlers: Handlers
     """A dict-like object for configuring the media-types to handle.
 
-    default, handlers are provided for the ``application/json``,
+    Default handlers are provided for the ``application/json``,
     ``application/x-www-form-urlencoded`` and ``multipart/form-data`` media types.
     """
     static_media_types: Dict[str, str]
     """A mapping of dot-prefixed file extensions to Internet media types (RFC 2046).
 
     Defaults to ``mimetypes.types_map`` after calling ``mimetypes.init()``.
     """
+    xml_error_serialization: bool
+    """Set to ``False`` to disable automatic inclusion of the XML handler
+    in the default error serializer (:ref:`errors`) (default ``True``).
+
+    Enabling this option does not automatically render all error response in XML,
+    but only if the client prefers (via the ``Accept`` request header) XML to JSON
+    and other configured media handlers.
+
+    Note:
+        This option will default to ``False`` in Falcon 5.0 disabling XML error
+        serialization by default).
+
+    Note:
+        This option has no effect when a custom error serializer, set using
+        :meth:`~falcon.App.set_error_serializer`, is in use.
+    """
 
     __slots__ = (
         'secure_cookies_by_default',
         'default_media_type',
         'media_handlers',
         'static_media_types',
+        'xml_error_serialization',
     )
 
     def __init__(self) -> None:
         self.secure_cookies_by_default = True
         self.default_media_type = DEFAULT_MEDIA_TYPE
         self.media_handlers = Handlers()
+        self.xml_error_serialization = True
 
         if not mimetypes.inited:
             mimetypes.init()

tests/test_error_handlers.py

@@ -77,6 +77,7 @@ def test_caught_error(self, client):
     def test_uncaught_python_error(
         self, client, get_headers, resp_content_type, resp_start
     ):
+        client.app.resp_options.xml_error_serialization = True
         result = client.simulate_get(headers=get_headers)
         assert result.status_code == 500
         assert result.headers['content-type'] == resp_content_type

tests/test_httperror.py

@@ -12,6 +12,7 @@
 from falcon.constants import MEDIA_YAML
 from falcon.media import BaseHandler
 import falcon.testing as testing
+from falcon.util.deprecation import DeprecatedWarning
 
 try:
     import yaml
@@ -29,6 +30,23 @@ def client(asgi, util):
     return testing.TestClient(app)
 
 
+@pytest.fixture(
+    params=[
+        pytest.param(True, id='with_xml'),
+        pytest.param(False, id='without_xml'),
+        pytest.param(None, id='default_xml'),
+    ]
+)
+def enable_xml(request):
+    def go(app):
+        if request.param is not None:
+            app.resp_options.xml_error_serialization = request.param
+        # return bool(request.param)
+        return request.param is not False
+
+    return go
+
+
 class FaultyResource:
     def on_get(self, req, resp):
         status = req.get_header('X-Error-Status')
@@ -302,9 +320,10 @@ def test_no_description_json(self, client):
         response = client.simulate_patch('/fail')
         assert response.status == falcon.HTTP_400
         assert response.json == {'title': '400 Bad Request'}
-        assert response.headers['Content-Type'] == 'application/json'
+        assert response.content_type == 'application/json'
 
     def test_no_description_xml(self, client):
+        client.app.resp_options.xml_error_serialization = True
         response = client.simulate_patch(
             path='/fail', headers={'Accept': 'application/xml'}
         )
@@ -316,7 +335,35 @@ def test_no_description_xml(self, client):
         )
 
         assert response.content == expected_xml
-        assert response.headers['Content-Type'] == 'application/xml'
+        assert response.content_type == 'application/xml'
+
+    @pytest.mark.parametrize('custom_xml', [True, False])
+    def test_xml_enable(self, client, enable_xml, custom_xml):
+        has_xml = enable_xml(client.app)
+        client.app.resp_options.default_media_type = 'app/foo'
+        accept = 'app/falcon+xml' if custom_xml else 'application/xml'
+        response = client.simulate_patch(path='/fail', headers={'Accept': accept})
+        assert response.status == falcon.HTTP_400
+
+        if has_xml:
+            expected_xml = (
+                b'<?xml version="1.0" encoding="UTF-8"?><error>'
+                b'<title>400 Bad Request</title></error>'
+            )
+            assert response.content == expected_xml
+        else:
+            assert response.content == b''
+        if has_xml or custom_xml:
+            assert response.content_type == 'application/xml'
+        else:
+            assert response.content_type == 'app/foo'
+
+    def test_to_xml_deprecated(self):
+        with pytest.warns(
+            DeprecatedWarning, match='The internal serialization to xml is deprecated.'
+        ):
+            res = falcon.HTTPGone().to_xml()
+        assert res == falcon.HTTPGone()._to_xml()
 
     def test_client_does_not_accept_json_or_xml(self, client):
         headers = {
@@ -499,6 +546,7 @@ def test_epic_fail_json(self, client):
         ],
     )
     def test_epic_fail_xml(self, client, media_type):
+        client.app.resp_options.xml_error_serialization = True
         headers = {'Accept': media_type}
 
         expected_body = (
@@ -547,6 +595,7 @@ def test_unicode_json(self, client):
         assert expected_body == response.json
 
     def test_unicode_xml(self, client):
+        client.app.resp_options.xml_error_serialization = True
         unicode_resource = UnicodeFaultyResource()
 
         expected_body = (
@@ -738,11 +787,12 @@ def test_414_with_custom_kwargs(self, client):
 
     def test_416(self, client, asgi, util):
         client.app = util.create_app(asgi)
+        client.app.resp_options.xml_error_serialization = True
         client.app.add_route('/416', RangeNotSatisfiableResource())
         response = client.simulate_request(path='/416', headers={'accept': 'text/xml'})
 
         assert response.status == falcon.HTTP_416
-        assert response.content == falcon.HTTPRangeNotSatisfiable(123456).to_xml()
+        assert response.content == falcon.HTTPRangeNotSatisfiable(123456)._to_xml()
         exp = (
             b'<?xml version="1.0" encoding="UTF-8"?><error>'
             b'<title>416 Range Not Satisfiable</title></error>'
@@ -998,6 +1048,12 @@ def client(self, util, asgi):
         app.add_route('/', GoneResource())
         return testing.TestClient(app)
 
+    def test_unknown_accept(self, client):
+        res = client.simulate_get(headers={'Accept': 'foo/bar'})
+        assert res.content_type == 'application/json'
+        assert res.headers['vary'] == 'Accept'
+        assert res.content == b''
+
     @pytest.mark.parametrize('has_json_handler', [True, False])
     def test_defaults_to_json(self, client, has_json_handler):
         if not has_json_handler:
@@ -1015,6 +1071,7 @@ def test_defaults_to_json(self, client, has_json_handler):
     def test_serializes_error_to_preferred_by_sender(
         self, accept, content_type, content, client, asgi
     ):
+        client.app.resp_options.xml_error_serialization = True
         client.app.resp_options.media_handlers[MEDIA_YAML] = FakeYamlMediaHandler()
         client.app.resp_options.media_handlers[ASYNC_WITH_SYNC[0]] = (
             SyncInterfaceMediaHandler()
@@ -1041,9 +1098,11 @@ def test_json_async_only_error(self, util):
         with pytest.raises(NotImplementedError, match='requires the sync interface'):
             client.simulate_get()
 
-    def test_add_xml_handler(self, client):
+    @pytest.mark.parametrize('accept', [MEDIA_XML, 'application/xhtml+xml'])
+    def test_add_xml_handler(self, client, enable_xml, accept):
+        enable_xml(client.app)
         client.app.resp_options.media_handlers[MEDIA_XML] = FakeYamlMediaHandler()
-        res = client.simulate_get(headers={'Accept': 'application/xhtml+xml'})
+        res = client.simulate_get(headers={'Accept': accept})
         assert res.content_type == MEDIA_XML
         assert res.content == YAML[-1]
 
@@ -1067,7 +1126,12 @@ def test_add_xml_handler(self, client):
             (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):
+    def test_hard_content_types(self, client, accept, content_type, enable_xml):
+        has_xml = enable_xml(client.app)
+        client.app.resp_options.default_media_type = 'my_type'
         client.app.resp_options.media_handlers[MEDIA_YAML] = FakeYamlMediaHandler()
         res = client.simulate_get(headers={'Accept': accept})
-        assert res.content_type == content_type
+        if has_xml or content_type != MEDIA_XML:
+            assert res.content_type == content_type
+        else:
+            assert res.content_type == MEDIA_JSON