From 588e2de61a3b5fecb96c15e37aba16d1219b4dea Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Tue, 1 Oct 2024 00:04:03 +0200 Subject: [PATCH 1/5] feat: add option to enable xml serialization on errors. The option is on by default --- docs/_newsfragments/2355.newandimproved.rst | 10 +++ falcon/app_helpers.py | 18 ++++-- falcon/http_error.py | 25 +++++--- falcon/response.py | 19 +++++- tests/test_error_handlers.py | 1 + tests/test_httperror.py | 70 ++++++++++++++++++--- 6 files changed, 121 insertions(+), 22 deletions(-) create mode 100644 docs/_newsfragments/2355.newandimproved.rst diff --git a/docs/_newsfragments/2355.newandimproved.rst b/docs/_newsfragments/2355.newandimproved.rst new file mode 100644 index 000000000..2be5ae4d5 --- /dev/null +++ b/docs/_newsfragments/2355.newandimproved.rst @@ -0,0 +1,10 @@ +Added a new flag :attr:`~falcon.ResponseOptions.enable_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. +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. diff --git a/falcon/app_helpers.py b/falcon/app_helpers.py index 86654aa1a..7ba586bd8 100644 --- a/falcon/app_helpers.py +++ b/falcon/app_helpers.py @@ -291,8 +291,13 @@ def default_serialize_error(req: Request, resp: Response, exception: HTTPError) resp: Instance of ``falcon.Response`` exception: Instance of ``falcon.HTTPError`` """ - predefined = [MEDIA_XML, 'text/xml', MEDIA_JSON] - media_handlers = [mt for mt in resp.options.media_handlers if mt not in predefined] + options = resp.options + predefined = ( + [MEDIA_XML, 'text/xml', MEDIA_JSON] + if options.enable_xml_error_serialization + else [MEDIA_JSON] + ) + media_handlers = [mt for mt in 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 @@ -315,10 +320,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 enable_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: @@ -331,8 +339,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.enable_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. diff --git a/falcon/http_error.py b/falcon/http_error.py index 508bd2524..cc6226fb9 100644 --- a/falcon/http_error.py +++ b/falcon/http_error.py @@ -20,6 +20,7 @@ from falcon.constants import MEDIA_JSON from falcon.util import code_to_http_status +from falcon.util import deprecated from falcon.util import http_status_to_code from falcon.util import uri @@ -44,8 +45,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. @@ -215,13 +215,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') @@ -243,6 +238,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. diff --git a/falcon/response.py b/falcon/response.py index d468b0db3..f102b4405 100644 --- a/falcon/response.py +++ b/falcon/response.py @@ -1387,7 +1387,7 @@ 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] @@ -1395,18 +1395,35 @@ class ResponseOptions: Defaults to ``mimetypes.types_map`` after calling ``mimetypes.init()``. """ + enable_xml_error_serialization: bool + """When ``False`` disables support for serializing http errors using XML when using + the default error serializer. Defaults to ``True``. + + In any case the request ``Accept`` headers it taken into consideration when + selecting the serialization format of the errors. + + Note: + + The default value will change to ``False`` in Falcon 5.0. + + Note: + + Has no effect when using a custom error serializers. + """ __slots__ = ( 'secure_cookies_by_default', 'default_media_type', 'media_handlers', 'static_media_types', + 'enable_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.enable_xml_error_serialization = True if not mimetypes.inited: mimetypes.init() diff --git a/tests/test_error_handlers.py b/tests/test_error_handlers.py index 5bac0842a..89c85bf2a 100644 --- a/tests/test_error_handlers.py +++ b/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.enable_xml_error_serialization = True result = client.simulate_get(headers=get_headers) assert result.status_code == 500 assert result.headers['content-type'] == resp_content_type diff --git a/tests/test_httperror.py b/tests/test_httperror.py index fb59c0f85..937cae28c 100644 --- a/tests/test_httperror.py +++ b/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.enable_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.enable_xml_error_serialization = True response = client.simulate_patch( path='/fail', headers={'Accept': 'application/xml'} ) @@ -316,7 +335,33 @@ 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' + + def test_xml_enable(self, client, enable_xml): + has_xml = enable_xml(client.app) + client.app.resp_options.default_media_type = 'app/foo' + response = client.simulate_patch( + path='/fail', headers={'Accept': 'application/xml'} + ) + assert response.status == falcon.HTTP_400 + + if has_xml: + expected_xml = ( + b'' + b'400 Bad Request' + ) + assert response.content == expected_xml + assert response.content_type == 'application/xml' + else: + assert response.content == b'' + 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 +544,7 @@ def test_epic_fail_json(self, client): ], ) def test_epic_fail_xml(self, client, media_type): + client.app.resp_options.enable_xml_error_serialization = True headers = {'Accept': media_type} expected_body = ( @@ -547,6 +593,7 @@ def test_unicode_json(self, client): assert expected_body == response.json def test_unicode_xml(self, client): + client.app.resp_options.enable_xml_error_serialization = True unicode_resource = UnicodeFaultyResource() expected_body = ( @@ -738,11 +785,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.enable_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'' b'416 Range Not Satisfiable' @@ -1015,6 +1063,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.enable_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 +1090,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 +1118,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 From 023eb690fd76f2dae038d4129fdf6f6a71e2cd61 Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Fri, 4 Oct 2024 22:37:06 +0200 Subject: [PATCH 2/5] chore: add coverage to missing branch --- tests/test_httperror.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tests/test_httperror.py b/tests/test_httperror.py index 937cae28c..c0c72b975 100644 --- a/tests/test_httperror.py +++ b/tests/test_httperror.py @@ -337,12 +337,12 @@ def test_no_description_xml(self, client): assert response.content == expected_xml assert response.content_type == 'application/xml' - def test_xml_enable(self, client, enable_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' - response = client.simulate_patch( - path='/fail', headers={'Accept': 'application/xml'} - ) + 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: @@ -1046,6 +1046,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: From 97fb4c74957a4f307d1b69be508897978875c619 Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Sat, 5 Oct 2024 09:16:59 +0200 Subject: [PATCH 3/5] test: fix failing test --- tests/test_httperror.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_httperror.py b/tests/test_httperror.py index c0c72b975..f81cb979b 100644 --- a/tests/test_httperror.py +++ b/tests/test_httperror.py @@ -351,9 +351,11 @@ def test_xml_enable(self, client, enable_xml, custom_xml): b'400 Bad Request' ) assert response.content == expected_xml - assert response.content_type == 'application/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): From 44a045961bc2c8c9cdd7ea8848b0924031318bd5 Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Sat, 5 Oct 2024 21:25:49 +0200 Subject: [PATCH 4/5] chore: review feedback --- docs/_newsfragments/2355.newandimproved.rst | 2 +- falcon/app_helpers.py | 6 +++--- falcon/response.py | 23 +++++++++++---------- tests/test_error_handlers.py | 2 +- tests/test_httperror.py | 12 +++++------ 5 files changed, 23 insertions(+), 22 deletions(-) diff --git a/docs/_newsfragments/2355.newandimproved.rst b/docs/_newsfragments/2355.newandimproved.rst index 2be5ae4d5..a1cbee0bd 100644 --- a/docs/_newsfragments/2355.newandimproved.rst +++ b/docs/_newsfragments/2355.newandimproved.rst @@ -1,4 +1,4 @@ -Added a new flag :attr:`~falcon.ResponseOptions.enable_xml_error_serialization` that +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 diff --git a/falcon/app_helpers.py b/falcon/app_helpers.py index 7ba586bd8..8c70122cd 100644 --- a/falcon/app_helpers.py +++ b/falcon/app_helpers.py @@ -294,7 +294,7 @@ def default_serialize_error(req: Request, resp: Response, exception: HTTPError) options = resp.options predefined = ( [MEDIA_XML, 'text/xml', MEDIA_JSON] - if options.enable_xml_error_serialization + if options.xml_error_serialization else [MEDIA_JSON] ) media_handlers = [mt for mt in options.media_handlers if mt not in predefined] @@ -320,7 +320,7 @@ def default_serialize_error(req: Request, resp: Response, exception: HTTPError) if '+json' in accept: preferred = MEDIA_JSON elif '+xml' in accept: - # NOTE(caselit): ignore enable_xml_error_serialization when + # 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 @@ -339,7 +339,7 @@ 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() - elif options.enable_xml_error_serialization: + elif options.xml_error_serialization: resp.data = exception._to_xml() # NOTE(kgriffs): No need to append the charset param, since diff --git a/falcon/response.py b/falcon/response.py index f102b4405..9c8094d68 100644 --- a/falcon/response.py +++ b/falcon/response.py @@ -1395,20 +1395,21 @@ class ResponseOptions: Defaults to ``mimetypes.types_map`` after calling ``mimetypes.init()``. """ - enable_xml_error_serialization: bool - """When ``False`` disables support for serializing http errors using XML when using - the default error serializer. Defaults to ``True``. + xml_error_serialization: bool + """Set to ``False`` to disable automatic inclusion of the XML handler + in the default error serializer (:ref:`errors`) (default ``True``). - In any case the request ``Accept`` headers it taken into consideration when - selecting the serialization format of the errors. + 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: - - The default value will change to ``False`` in Falcon 5.0. + This option will be removed in Falcon 5.0 (with XML error + serialization disabled by default). Note: - - Has no effect when using a custom error serializers. + This option has no effect when a custom error serializer, set using + :meth:`~falcon.App.set_error_serializer`, is in use. """ __slots__ = ( @@ -1416,14 +1417,14 @@ class ResponseOptions: 'default_media_type', 'media_handlers', 'static_media_types', - 'enable_xml_error_serialization', + '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.enable_xml_error_serialization = True + self.xml_error_serialization = True if not mimetypes.inited: mimetypes.init() diff --git a/tests/test_error_handlers.py b/tests/test_error_handlers.py index 89c85bf2a..0fad68920 100644 --- a/tests/test_error_handlers.py +++ b/tests/test_error_handlers.py @@ -77,7 +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.enable_xml_error_serialization = True + 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 diff --git a/tests/test_httperror.py b/tests/test_httperror.py index f81cb979b..9441f32b3 100644 --- a/tests/test_httperror.py +++ b/tests/test_httperror.py @@ -40,7 +40,7 @@ def client(asgi, util): def enable_xml(request): def go(app): if request.param is not None: - app.resp_options.enable_xml_error_serialization = request.param + app.resp_options.xml_error_serialization = request.param # return bool(request.param) return request.param is not False @@ -323,7 +323,7 @@ def test_no_description_json(self, client): assert response.content_type == 'application/json' def test_no_description_xml(self, client): - client.app.resp_options.enable_xml_error_serialization = True + client.app.resp_options.xml_error_serialization = True response = client.simulate_patch( path='/fail', headers={'Accept': 'application/xml'} ) @@ -546,7 +546,7 @@ def test_epic_fail_json(self, client): ], ) def test_epic_fail_xml(self, client, media_type): - client.app.resp_options.enable_xml_error_serialization = True + client.app.resp_options.xml_error_serialization = True headers = {'Accept': media_type} expected_body = ( @@ -595,7 +595,7 @@ def test_unicode_json(self, client): assert expected_body == response.json def test_unicode_xml(self, client): - client.app.resp_options.enable_xml_error_serialization = True + client.app.resp_options.xml_error_serialization = True unicode_resource = UnicodeFaultyResource() expected_body = ( @@ -787,7 +787,7 @@ 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.enable_xml_error_serialization = True + client.app.resp_options.xml_error_serialization = True client.app.add_route('/416', RangeNotSatisfiableResource()) response = client.simulate_request(path='/416', headers={'accept': 'text/xml'}) @@ -1071,7 +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.enable_xml_error_serialization = True + 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() From a864d09e049e27fb8631c04734a62bb59897a149 Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Sun, 6 Oct 2024 10:02:12 +0200 Subject: [PATCH 5/5] docs: improve documentation --- docs/_newsfragments/2355.newandimproved.rst | 2 +- falcon/response.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/_newsfragments/2355.newandimproved.rst b/docs/_newsfragments/2355.newandimproved.rst index a1cbee0bd..8cbc812ec 100644 --- a/docs/_newsfragments/2355.newandimproved.rst +++ b/docs/_newsfragments/2355.newandimproved.rst @@ -3,7 +3,7 @@ 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. +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. diff --git a/falcon/response.py b/falcon/response.py index 6344964da..afcc8941a 100644 --- a/falcon/response.py +++ b/falcon/response.py @@ -1404,8 +1404,8 @@ class ResponseOptions: and other configured media handlers. Note: - This option will be removed in Falcon 5.0 (with XML error - serialization disabled by default). + 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