From 1afc573e945c5dd7babba08326f4ef60337c40aa Mon Sep 17 00:00:00 2001 From: Piotr Kopalko Date: Sun, 19 Nov 2023 20:44:09 +0100 Subject: [PATCH 01/17] Add unit test for default_serialize_json --- tests/test_app_helpers.py | 45 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 tests/test_app_helpers.py diff --git a/tests/test_app_helpers.py b/tests/test_app_helpers.py new file mode 100644 index 000000000..2f0f48b9c --- /dev/null +++ b/tests/test_app_helpers.py @@ -0,0 +1,45 @@ +import pytest + +from falcon import HTTPNotFound +from falcon.app_helpers import default_serialize_error +from falcon.request import Request +from falcon.response import Response +from falcon.testing import create_environ + + +class TestDefaultSerializeError: + @pytest.mark.parametrize( + 'accept, content_type, data', + ( + ('application/json', 'application/json', b'{"title": "404 Not Found"}'), + ( + 'application/xml', + 'application/xml', + ( + b'' + b'404 Not Found' + ), + ), + ('custom/any+json', 'application/json', b'{"title": "404 Not Found"}'), + ( + 'custom/any+xml', + 'application/xml', + ( + b'' + b'404 Not Found' + ), + ), + ), + ) + def test_serializes_error_to_preffered_by_sender( + self, accept, content_type, data + ) -> None: + response = Response() + default_serialize_error( + Request(env=(create_environ(headers={'accept': accept}))), + response, + HTTPNotFound(), + ) + assert response.content_type == content_type + assert response.headers['vary'] == 'Accept' + assert response.data == data From f77518ff43a92d5bebf45c6e838999de5d7a2378 Mon Sep 17 00:00:00 2001 From: Piotr Kopalko Date: Tue, 21 Nov 2023 20:33:15 +0100 Subject: [PATCH 02/17] Extract options to variables for improved readability --- tests/test_app_helpers.py | 42 ++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/tests/test_app_helpers.py b/tests/test_app_helpers.py index 2f0f48b9c..e9c18ca11 100644 --- a/tests/test_app_helpers.py +++ b/tests/test_app_helpers.py @@ -6,29 +6,35 @@ from falcon.response import Response from falcon.testing import create_environ +JSON = ('application/json', 'application/json', b'{"title": "404 Not Found"}') +XML = ( + 'application/xml', + 'application/xml', + ( + b'' + b'404 Not Found' + ), +) +CUSTOM_JSON = ('custom/any+json', 'application/json', b'{"title": "404 Not Found"}') + +CUSTOM_XML = ( + 'custom/any+xml', + 'application/xml', + ( + b'' + b'404 Not Found' + ), +) + class TestDefaultSerializeError: @pytest.mark.parametrize( 'accept, content_type, data', ( - ('application/json', 'application/json', b'{"title": "404 Not Found"}'), - ( - 'application/xml', - 'application/xml', - ( - b'' - b'404 Not Found' - ), - ), - ('custom/any+json', 'application/json', b'{"title": "404 Not Found"}'), - ( - 'custom/any+xml', - 'application/xml', - ( - b'' - b'404 Not Found' - ), - ), + JSON, + XML, + CUSTOM_JSON, + CUSTOM_XML, ), ) def test_serializes_error_to_preffered_by_sender( From ff5be58d0c32eb50767fb91a49fdc72ac2cad6a4 Mon Sep 17 00:00:00 2001 From: Piotr Kopalko Date: Tue, 21 Nov 2023 20:34:03 +0100 Subject: [PATCH 03/17] Extract prefered response content type negotiation to function --- falcon/app_helpers.py | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/falcon/app_helpers.py b/falcon/app_helpers.py index 38b591914..255f73b71 100644 --- a/falcon/app_helpers.py +++ b/falcon/app_helpers.py @@ -226,8 +226,26 @@ 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)) + preferred = _negotiate_preffered_media_type(req) + + if preferred is not None: + if preferred == MEDIA_JSON: + handler, _, _ = resp.options.media_handlers._resolve( + MEDIA_JSON, MEDIA_JSON, raise_not_found=False + ) + resp.data = exception.to_json(handler) + else: + 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. + resp.content_type = preferred + + resp.append_header('Vary', 'Accept') + +def _negotiate_preffered_media_type(req: Request) -> str: + preferred = req.client_prefers((MEDIA_XML, 'text/xml', MEDIA_JSON)) if preferred is None: # NOTE(kgriffs): See if the client expects a custom media # type based on something Falcon supports. Returning something @@ -246,21 +264,7 @@ def default_serialize_error(req: Request, resp: Response, exception: HTTPError): preferred = MEDIA_JSON elif '+xml' in accept: preferred = MEDIA_XML - - if preferred is not None: - if preferred == MEDIA_JSON: - handler, _, _ = resp.options.media_handlers._resolve( - MEDIA_JSON, MEDIA_JSON, raise_not_found=False - ) - resp.data = exception.to_json(handler) - else: - 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. - resp.content_type = preferred - - resp.append_header('Vary', 'Accept') + return preferred class CloseableStreamIterator: From 0e4b6f86648cd310588976ccb709c6c4d565ea54 Mon Sep 17 00:00:00 2001 From: Piotr Kopalko Date: Wed, 22 Nov 2023 21:02:30 +0100 Subject: [PATCH 04/17] Use media handlers to serialize errors by accepted content type --- falcon/app_helpers.py | 20 ++++++++++-------- tests/test_app_helpers.py | 39 ++++++++++++++++++++++++++++++++---- tests/test_media_handlers.py | 14 +++++++++++++ 3 files changed, 61 insertions(+), 12 deletions(-) diff --git a/falcon/app_helpers.py b/falcon/app_helpers.py index 255f73b71..d22eccba6 100644 --- a/falcon/app_helpers.py +++ b/falcon/app_helpers.py @@ -14,8 +14,10 @@ """Utilities for the App class.""" +from collections import OrderedDict from inspect import iscoroutinefunction from typing import IO, Iterable, List, Tuple +from typing import Optional from falcon import util from falcon.constants import MEDIA_JSON @@ -226,14 +228,14 @@ def default_serialize_error(req: Request, resp: Response, exception: HTTPError): resp: Instance of ``falcon.Response`` exception: Instance of ``falcon.HTTPError`` """ - preferred = _negotiate_preffered_media_type(req) + preferred = _negotiate_preffered_media_type(req, resp) if preferred is not None: - if preferred == MEDIA_JSON: - handler, _, _ = resp.options.media_handlers._resolve( - MEDIA_JSON, MEDIA_JSON, raise_not_found=False - ) - resp.data = exception.to_json(handler) + handler, _, _ = resp.options.media_handlers._resolve( + preferred, MEDIA_JSON, raise_not_found=False + ) + if handler: + resp.data = handler.serialize(exception.to_dict(OrderedDict), preferred) else: resp.data = exception.to_xml() @@ -244,8 +246,10 @@ def default_serialize_error(req: Request, resp: Response, exception: HTTPError): resp.append_header('Vary', 'Accept') -def _negotiate_preffered_media_type(req: Request) -> str: - preferred = req.client_prefers((MEDIA_XML, 'text/xml', MEDIA_JSON)) +def _negotiate_preffered_media_type(req: Request, resp: Response) -> Optional[str]: + supported = {MEDIA_XML, 'text/xml', MEDIA_JSON} + supported.update(set(resp.options.media_handlers.keys())) + preferred = req.client_prefers(supported) if preferred is None: # NOTE(kgriffs): See if the client expects a custom media # type based on something Falcon supports. Returning something diff --git a/tests/test_app_helpers.py b/tests/test_app_helpers.py index e9c18ca11..31b8ad098 100644 --- a/tests/test_app_helpers.py +++ b/tests/test_app_helpers.py @@ -1,12 +1,16 @@ import pytest from falcon import HTTPNotFound +from falcon import ResponseOptions from falcon.app_helpers import default_serialize_error +from falcon.media import BaseHandler +from falcon.media import Handlers from falcon.request import Request from falcon.response import Response from falcon.testing import create_environ -JSON = ('application/json', 'application/json', b'{"title": "404 Not Found"}') +JSON_CONTENT = b'{"title": "404 Not Found"}' +JSON = ('application/json', 'application/json', JSON_CONTENT) XML = ( 'application/xml', 'application/xml', @@ -15,7 +19,7 @@ b'404 Not Found' ), ) -CUSTOM_JSON = ('custom/any+json', 'application/json', b'{"title": "404 Not Found"}') +CUSTOM_JSON = ('custom/any+json', 'application/json', JSON_CONTENT) CUSTOM_XML = ( 'custom/any+xml', @@ -26,8 +30,30 @@ ), ) +YAML = ( + 'application/yaml', + 'application/yaml', + (b'error:\n' b' title: 404 Not Found'), +) + + +class FakeYamlMediaHandler(BaseHandler): + def serialize(self, media: object, content_type: str) -> bytes: + return b'error:\n' b' title: 404 Not Found' + class TestDefaultSerializeError: + def test_if_no_content_type_and_accept_fall_back_to_json(self) -> None: + response = Response() + default_serialize_error( + Request(env=(create_environ())), + response, + HTTPNotFound(), + ) + assert response.content_type == 'application/json' + assert response.headers['vary'] == 'Accept' + assert response.data == JSON_CONTENT + @pytest.mark.parametrize( 'accept, content_type, data', ( @@ -35,12 +61,17 @@ class TestDefaultSerializeError: XML, CUSTOM_JSON, CUSTOM_XML, + YAML, ), ) - def test_serializes_error_to_preffered_by_sender( + def test_serializes_error_to_preferred_by_sender( self, accept, content_type, data ) -> None: - response = Response() + handlers = Handlers() + handlers['application/yaml'] = FakeYamlMediaHandler() + options = ResponseOptions() + options.media_handlers = handlers + response = Response(options=options) default_serialize_error( Request(env=(create_environ(headers={'accept': accept}))), response, diff --git a/tests/test_media_handlers.py b/tests/test_media_handlers.py index 8b95600b6..ef078e148 100644 --- a/tests/test_media_handlers.py +++ b/tests/test_media_handlers.py @@ -353,6 +353,20 @@ def on_get(self, req, resp): assert result.json == falcon.HTTPForbidden().to_dict() +def test_handlers_include_new_media_handlers_in_resolving() -> None: + class FakeHandler: + ... + + handlers = media.Handlers({falcon.MEDIA_URLENCODED: media.URLEncodedFormHandler()}) + handler = FakeHandler() + handlers['application/yaml'] = handler + resolved, _, _ = handlers._resolve( + 'application/yaml', 'application/json', raise_not_found=False + ) + assert resolved.__class__.__name__ == handler.__class__.__name__ + assert resolved == handler + + class TestBaseHandler: def test_defaultError(self): h = media.BaseHandler() From 22f2470270b51d925656fe4cf998c364b7d2eb21 Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Sun, 29 Sep 2024 11:40:53 +0200 Subject: [PATCH 05/17] refactor: remove use of ordered dict since it's no longer required --- falcon/app_helpers.py | 3 +-- falcon/http_error.py | 12 ++++++------ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/falcon/app_helpers.py b/falcon/app_helpers.py index 9cd991f57..a21672353 100644 --- a/falcon/app_helpers.py +++ b/falcon/app_helpers.py @@ -16,7 +16,6 @@ from __future__ import annotations -from collections import OrderedDict from inspect import iscoroutinefunction from typing import IO, Iterable, List, Literal, Optional, overload, Tuple, Union @@ -299,7 +298,7 @@ def default_serialize_error(req: Request, resp: Response, exception: HTTPError) preferred, MEDIA_JSON, raise_not_found=False ) if handler: - resp.data = handler.serialize(exception.to_dict(OrderedDict), preferred) + resp.data = handler.serialize(exception.to_dict(), preferred) else: resp.data = exception.to_xml() diff --git a/falcon/http_error.py b/falcon/http_error.py index b0aef8cc1..c18b49d4d 100644 --- a/falcon/http_error.py +++ b/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,7 +209,7 @@ 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 return handler.serialize(obj, MEDIA_JSON) From 234e76443a3b51744f0dd036123a3c8fd0d3805b Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Sun, 29 Sep 2024 11:46:16 +0200 Subject: [PATCH 06/17] feat: add content_type to testing result --- docs/_newsfragments/content_type.newandimproved.rst | 3 +++ falcon/testing/client.py | 7 ++++++- 2 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 docs/_newsfragments/content_type.newandimproved.rst diff --git a/docs/_newsfragments/content_type.newandimproved.rst b/docs/_newsfragments/content_type.newandimproved.rst new file mode 100644 index 000000000..7546a37eb --- /dev/null +++ b/docs/_newsfragments/content_type.newandimproved.rst @@ -0,0 +1,3 @@ +Added :attr:`falcon.testing.Result.content_type` and +:attr:`falcon.testing.StreamedResult.content_type` as an utility accessor +for the ``Content-Type`` header. diff --git a/falcon/testing/client.py b/falcon/testing/client.py index 47a55fef6..e1f14959f 100644 --- a/falcon/testing/client.py +++ b/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:] From ff7d82556fdc4fa44d4ea401b036921a6311c966 Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Sun, 29 Sep 2024 11:47:57 +0200 Subject: [PATCH 07/17] chore: improve implementation --- falcon/app_helpers.py | 48 +++++++++++---------- tests/test_app_helpers.py | 82 ------------------------------------ tests/test_httperror.py | 65 ++++++++++++++++++++++++++-- tests/test_media_handlers.py | 18 ++++---- 4 files changed, 95 insertions(+), 118 deletions(-) delete mode 100644 tests/test_app_helpers.py diff --git a/falcon/app_helpers.py b/falcon/app_helpers.py index a21672353..224e0b6c6 100644 --- a/falcon/app_helpers.py +++ b/falcon/app_helpers.py @@ -291,28 +291,7 @@ def default_serialize_error(req: Request, resp: Response, exception: HTTPError) resp: Instance of ``falcon.Response`` exception: Instance of ``falcon.HTTPError`` """ - preferred = _negotiate_preffered_media_type(req, resp) - - if preferred is not None: - handler, _, _ = resp.options.media_handlers._resolve( - preferred, MEDIA_JSON, raise_not_found=False - ) - if handler: - resp.data = handler.serialize(exception.to_dict(), preferred) - else: - 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. - resp.content_type = preferred - - resp.append_header('Vary', 'Accept') - - -def _negotiate_preffered_media_type(req: Request, resp: Response) -> Optional[str]: - supported = {MEDIA_XML, 'text/xml', MEDIA_JSON} - supported.update(set(resp.options.media_handlers.keys())) - preferred = req.client_prefers(supported) + preferred = req.client_prefers((MEDIA_XML, 'text/xml', MEDIA_JSON)) if preferred is None: # NOTE(kgriffs): See if the client expects a custom media # type based on something Falcon supports. Returning something @@ -331,7 +310,30 @@ def _negotiate_preffered_media_type(req: Request, resp: Response) -> Optional[st preferred = MEDIA_JSON elif '+xml' in accept: preferred = MEDIA_XML - return preferred + else: + # NOTE(caselit): if nothing else matchers try using the media handlers + # registered in the response + preferred = req.client_prefers(list(resp.options.media_handlers)) + + if preferred is not None: + handler, _, _ = resp.options.media_handlers._resolve( + preferred, MEDIA_JSON, raise_not_found=False + ) + if preferred == MEDIA_JSON: + # 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: + resp.data = handler.serialize(exception.to_dict(), preferred) + else: + 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. + resp.content_type = preferred + + resp.append_header('Vary', 'Accept') class CloseableStreamIterator: diff --git a/tests/test_app_helpers.py b/tests/test_app_helpers.py deleted file mode 100644 index 31b8ad098..000000000 --- a/tests/test_app_helpers.py +++ /dev/null @@ -1,82 +0,0 @@ -import pytest - -from falcon import HTTPNotFound -from falcon import ResponseOptions -from falcon.app_helpers import default_serialize_error -from falcon.media import BaseHandler -from falcon.media import Handlers -from falcon.request import Request -from falcon.response import Response -from falcon.testing import create_environ - -JSON_CONTENT = b'{"title": "404 Not Found"}' -JSON = ('application/json', 'application/json', JSON_CONTENT) -XML = ( - 'application/xml', - 'application/xml', - ( - b'' - b'404 Not Found' - ), -) -CUSTOM_JSON = ('custom/any+json', 'application/json', JSON_CONTENT) - -CUSTOM_XML = ( - 'custom/any+xml', - 'application/xml', - ( - b'' - b'404 Not Found' - ), -) - -YAML = ( - 'application/yaml', - 'application/yaml', - (b'error:\n' b' title: 404 Not Found'), -) - - -class FakeYamlMediaHandler(BaseHandler): - def serialize(self, media: object, content_type: str) -> bytes: - return b'error:\n' b' title: 404 Not Found' - - -class TestDefaultSerializeError: - def test_if_no_content_type_and_accept_fall_back_to_json(self) -> None: - response = Response() - default_serialize_error( - Request(env=(create_environ())), - response, - HTTPNotFound(), - ) - assert response.content_type == 'application/json' - assert response.headers['vary'] == 'Accept' - assert response.data == JSON_CONTENT - - @pytest.mark.parametrize( - 'accept, content_type, data', - ( - JSON, - XML, - CUSTOM_JSON, - CUSTOM_XML, - YAML, - ), - ) - def test_serializes_error_to_preferred_by_sender( - self, accept, content_type, data - ) -> None: - handlers = Handlers() - handlers['application/yaml'] = FakeYamlMediaHandler() - options = ResponseOptions() - options.media_handlers = handlers - response = Response(options=options) - default_serialize_error( - Request(env=(create_environ(headers={'accept': accept}))), - response, - HTTPNotFound(), - ) - assert response.content_type == content_type - assert response.headers['vary'] == 'Accept' - assert response.data == data diff --git a/tests/test_httperror.py b/tests/test_httperror.py index 0ae20dd8e..78be3e103 100644 --- a/tests/test_httperror.py +++ b/tests/test_httperror.py @@ -7,6 +7,8 @@ import pytest import falcon +from falcon.constants import MEDIA_JSON +from falcon.media import BaseHandler import falcon.testing as testing try: @@ -943,7 +945,64 @@ 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 = ('application/json', 'application/json', JSON_CONTENT) +CUSTOM_JSON = ('custom/any+json', 'application/json', JSON_CONTENT) + +XML_CONTENT = ( + b'410 Gone' +) +XML = ('application/xml', 'application/xml', XML_CONTENT) +CUSTOM_XML = ('custom/any+xml', 'application/xml', XML_CONTENT) + +YAML = ('application/yaml', 'application/yaml', (b'title: 410 Gone!')) + + +class FakeYamlMediaHandler(BaseHandler): + def serialize(self, media: object, content_type: str) -> bytes: + assert media == {'title': '410 Gone'} + return b'title: 410 Gone!' + + +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, + ), + ) + def test_serializes_error_to_preferred_by_sender( + self, accept, content_type, content, client + ): + client.app.resp_options.media_handlers['application/yaml'] = ( + FakeYamlMediaHandler() + ) + res = client.simulate_get(headers={'Accept': accept}) + assert res.content_type == content_type + assert res.headers['vary'] == 'Accept' + assert res.content == content diff --git a/tests/test_media_handlers.py b/tests/test_media_handlers.py index 0964bf4f2..9fd881b59 100644 --- a/tests/test_media_handlers.py +++ b/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 @@ -373,17 +375,13 @@ def on_get(self, req, resp): def test_handlers_include_new_media_handlers_in_resolving() -> None: - class FakeHandler: - ... - handlers = media.Handlers({falcon.MEDIA_URLENCODED: media.URLEncodedFormHandler()}) - handler = FakeHandler() - handlers['application/yaml'] = handler - resolved, _, _ = handlers._resolve( - 'application/yaml', 'application/json', raise_not_found=False - ) - assert resolved.__class__.__name__ == handler.__class__.__name__ - assert resolved == handler + 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: From e5059abf0ef302bf876f0110291cf1aad4a8ac74 Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Sun, 29 Sep 2024 11:50:16 +0200 Subject: [PATCH 08/17] docs: add news fragment --- docs/_newsfragments/2023.newandimproved.rst | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 docs/_newsfragments/2023.newandimproved.rst diff --git a/docs/_newsfragments/2023.newandimproved.rst b/docs/_newsfragments/2023.newandimproved.rst new file mode 100644 index 000000000..187480cb3 --- /dev/null +++ b/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 format with the client. +The implementation still default to JSON in case a client does not specify +any preference. From 8fe56a9081ea55bf73dbe4115b73bc45a8c8060e Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Sun, 29 Sep 2024 12:07:21 +0200 Subject: [PATCH 09/17] chore: add missing tests and improve docs --- docs/_newsfragments/2023.newandimproved.rst | 2 +- tests/test_testing.py | 27 +++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/docs/_newsfragments/2023.newandimproved.rst b/docs/_newsfragments/2023.newandimproved.rst index 187480cb3..db6c24ec2 100644 --- a/docs/_newsfragments/2023.newandimproved.rst +++ b/docs/_newsfragments/2023.newandimproved.rst @@ -1,4 +1,4 @@ The default error serializer will now use the response media handlers -to better negotiate the response format with the client. +to better negotiate the response serialization with the client. The implementation still default to JSON in case a client does not specify any preference. diff --git a/tests/test_testing.py b/tests/test_testing.py index dda7e1880..89a8c49e8 100644 --- a/tests/test_testing.py +++ b/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) From 2c5af63d0bd561af5652d39cea8b5c5a9dcd5348 Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Sun, 29 Sep 2024 13:51:33 +0200 Subject: [PATCH 10/17] fix: take into account async only handlers --- falcon/app_helpers.py | 9 ++++-- falcon/http_error.py | 1 + tests/test_httperror.py | 70 ++++++++++++++++++++++++++++++----------- 3 files changed, 60 insertions(+), 20 deletions(-) diff --git a/falcon/app_helpers.py b/falcon/app_helpers.py index 224e0b6c6..0f9893288 100644 --- a/falcon/app_helpers.py +++ b/falcon/app_helpers.py @@ -316,7 +316,7 @@ def default_serialize_error(req: Request, resp: Response, exception: HTTPError) preferred = req.client_prefers(list(resp.options.media_handlers)) if preferred is not None: - handler, _, _ = resp.options.media_handlers._resolve( + handler, serialize_sync, _ = resp.options.media_handlers._resolve( preferred, MEDIA_JSON, raise_not_found=False ) if preferred == MEDIA_JSON: @@ -325,7 +325,12 @@ def default_serialize_error(req: Request, resp: Response, exception: HTTPError) # media_handlers. resp.data = exception.to_json(handler) elif handler: - resp.data = handler.serialize(exception.to_dict(), preferred) + if serialize_sync: + resp.data = serialize_sync(exception.to_dict(), preferred) + else: + # NOTE(caselit): Let the app serialize the response if there is no sync + # serializer implemented in the handler. + resp.media = exception.to_dict() else: resp.data = exception.to_xml() diff --git a/falcon/http_error.py b/falcon/http_error.py index c18b49d4d..508bd2524 100644 --- a/falcon/http_error.py +++ b/falcon/http_error.py @@ -212,6 +212,7 @@ def to_json(self, handler: Optional[BaseHandler] = None) -> bytes: 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: diff --git a/tests/test_httperror.py b/tests/test_httperror.py index 78be3e103..68b09c45d 100644 --- a/tests/test_httperror.py +++ b/tests/test_httperror.py @@ -8,6 +8,8 @@ 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 @@ -951,16 +953,22 @@ def test_kw_only(self): JSON_CONTENT = b'{"title": "410 Gone"}' -JSON = ('application/json', 'application/json', JSON_CONTENT) -CUSTOM_JSON = ('custom/any+json', 'application/json', JSON_CONTENT) +JSON = (MEDIA_JSON, MEDIA_JSON, JSON_CONTENT) +CUSTOM_JSON = ('custom/any+json', MEDIA_JSON, JSON_CONTENT) XML_CONTENT = ( b'410 Gone' ) -XML = ('application/xml', 'application/xml', XML_CONTENT) -CUSTOM_XML = ('custom/any+xml', 'application/xml', XML_CONTENT) - -YAML = ('application/yaml', 'application/yaml', (b'title: 410 Gone!')) +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): @@ -969,6 +977,20 @@ def serialize(self, media: object, content_type: str) -> bytes: return b'title: 410 Gone!' +class AsyncOnlyMediaHandler(BaseHandler): + async def serialize_async(self, media: object, content_type: str) -> bytes: + assert media == {'title': '410 Gone'} + return b'this is async' + + +class SyncInterfaceMediaHandler(AsyncOnlyMediaHandler): + def serialize(self, media: object, content_type: str) -> bytes: + assert media == {'title': '410 Gone'} + return b'this is sync instead' + + _serialize_sync = serialize + + class TestDefaultSerializeError: @pytest.fixture def client(self, util, asgi): @@ -988,21 +1010,33 @@ def test_defaults_to_json(self, client, has_json_handler): @pytest.mark.parametrize( 'accept, content_type, content', - ( - JSON, - XML, - CUSTOM_JSON, - CUSTOM_XML, - YAML, - ), + (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 + self, accept, content_type, content, client, asgi ): - client.app.resp_options.media_handlers['application/yaml'] = ( - FakeYamlMediaHandler() + 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.content_type == content_type assert res.headers['vary'] == 'Accept' - assert res.content == content + 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() From 29413b89e06fc384e78df911cb3ab31ee0b403e2 Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Sun, 29 Sep 2024 14:07:02 +0200 Subject: [PATCH 11/17] chore: fix mypy errors --- tests/test_httperror.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/test_httperror.py b/tests/test_httperror.py index 68b09c45d..ba368f186 100644 --- a/tests/test_httperror.py +++ b/tests/test_httperror.py @@ -972,23 +972,23 @@ def test_kw_only(self): class FakeYamlMediaHandler(BaseHandler): - def serialize(self, media: object, content_type: str) -> bytes: + def serialize(self, media, content_type): assert media == {'title': '410 Gone'} return b'title: 410 Gone!' class AsyncOnlyMediaHandler(BaseHandler): - async def serialize_async(self, media: object, content_type: str) -> bytes: + async def serialize_async(self, media, content_type): assert media == {'title': '410 Gone'} return b'this is async' class SyncInterfaceMediaHandler(AsyncOnlyMediaHandler): - def serialize(self, media: object, content_type: str) -> bytes: + def serialize(self, media, content_type): assert media == {'title': '410 Gone'} return b'this is sync instead' - _serialize_sync = serialize + _serialize_sync = serialize # type: ignore[assignment] class TestDefaultSerializeError: From 64720753b7f8ce54156bb111aefe6298d1693a32 Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Sun, 29 Sep 2024 20:40:26 +0200 Subject: [PATCH 12/17] chore: simplify code a bit, avoiding unnecessary complexity --- falcon/app_helpers.py | 12 +++++------- falcon/asgi/app.py | 4 +++- tests/test_httperror.py | 6 +++--- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/falcon/app_helpers.py b/falcon/app_helpers.py index 0f9893288..5f34bbed5 100644 --- a/falcon/app_helpers.py +++ b/falcon/app_helpers.py @@ -316,7 +316,7 @@ def default_serialize_error(req: Request, resp: Response, exception: HTTPError) preferred = req.client_prefers(list(resp.options.media_handlers)) if preferred is not None: - handler, serialize_sync, _ = resp.options.media_handlers._resolve( + handler, _, _ = resp.options.media_handlers._resolve( preferred, MEDIA_JSON, raise_not_found=False ) if preferred == MEDIA_JSON: @@ -325,12 +325,10 @@ def default_serialize_error(req: Request, resp: Response, exception: HTTPError) # media_handlers. resp.data = exception.to_json(handler) elif handler: - if serialize_sync: - resp.data = serialize_sync(exception.to_dict(), preferred) - else: - # NOTE(caselit): Let the app serialize the response if there is no sync - # serializer implemented in the handler. - resp.media = exception.to_dict() + # 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() diff --git a/falcon/asgi/app.py b/falcon/asgi/app.py index d28c8c49a..dca7afd96 100644 --- a/falcon/asgi/app.py +++ b/falcon/asgi/app.py @@ -566,7 +566,9 @@ async def __call__( # type: ignore[override] # noqa: C901 ) if serialize_sync: - resp._media_rendered = serialize_sync(resp._media) + resp._media_rendered = serialize_sync( + resp._media, resp.content_type + ) else: resp._media_rendered = await handler.serialize_async( resp._media, resp.content_type diff --git a/tests/test_httperror.py b/tests/test_httperror.py index ba368f186..093df6c54 100644 --- a/tests/test_httperror.py +++ b/tests/test_httperror.py @@ -972,19 +972,19 @@ def test_kw_only(self): class FakeYamlMediaHandler(BaseHandler): - def serialize(self, media, content_type): + 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): + 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): + def serialize(self, media, content_type=None): assert media == {'title': '410 Gone'} return b'this is sync instead' From 2c4ab8fe85bab856e479270a19c71ebd3a8452d6 Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Sun, 29 Sep 2024 20:46:10 +0200 Subject: [PATCH 13/17] fix: avoid change to app code --- falcon/asgi/app.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/falcon/asgi/app.py b/falcon/asgi/app.py index dca7afd96..d28c8c49a 100644 --- a/falcon/asgi/app.py +++ b/falcon/asgi/app.py @@ -566,9 +566,7 @@ async def __call__( # type: ignore[override] # noqa: C901 ) if serialize_sync: - resp._media_rendered = serialize_sync( - resp._media, resp.content_type - ) + resp._media_rendered = serialize_sync(resp._media) else: resp._media_rendered = await handler.serialize_async( resp._media, resp.content_type From 55a9412451b524f99a166fcfd7fb46095460de9e Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Mon, 30 Sep 2024 23:05:00 +0200 Subject: [PATCH 14/17] chore: review feedback --- ...content_type.newandimproved.rst => 2349.newandimproved.rst} | 0 falcon/app_helpers.py | 3 ++- falcon/request.py | 3 ++- 3 files changed, 4 insertions(+), 2 deletions(-) rename docs/_newsfragments/{content_type.newandimproved.rst => 2349.newandimproved.rst} (100%) diff --git a/docs/_newsfragments/content_type.newandimproved.rst b/docs/_newsfragments/2349.newandimproved.rst similarity index 100% rename from docs/_newsfragments/content_type.newandimproved.rst rename to docs/_newsfragments/2349.newandimproved.rst diff --git a/falcon/app_helpers.py b/falcon/app_helpers.py index 5f34bbed5..3ceab989e 100644 --- a/falcon/app_helpers.py +++ b/falcon/app_helpers.py @@ -292,6 +292,7 @@ def default_serialize_error(req: Request, resp: Response, exception: HTTPError) exception: Instance of ``falcon.HTTPError`` """ preferred = req.client_prefers((MEDIA_XML, 'text/xml', MEDIA_JSON)) + if preferred is None: # NOTE(kgriffs): See if the client expects a custom media # type based on something Falcon supports. Returning something @@ -313,7 +314,7 @@ def default_serialize_error(req: Request, resp: Response, exception: HTTPError) else: # NOTE(caselit): if nothing else matchers try using the media handlers # registered in the response - preferred = req.client_prefers(list(resp.options.media_handlers)) + preferred = req.client_prefers(resp.options.media_handlers) if preferred is not None: handler, _, _ = resp.options.media_handlers._resolve( diff --git a/falcon/request.py b/falcon/request.py index 59ddddab3..3755abc6c 100644 --- a/falcon/request.py +++ b/falcon/request.py @@ -22,6 +22,7 @@ Callable, ClassVar, Dict, + Iterable, List, Literal, Mapping, @@ -1171,7 +1172,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: From 9414ee2ffc57327d3b76e6579a64a83761fce73c Mon Sep 17 00:00:00 2001 From: Federico Caselli Date: Mon, 30 Sep 2024 23:22:22 +0200 Subject: [PATCH 15/17] fix: properly account for q= when parting the accept header --- falcon/app_helpers.py | 11 ++++++----- falcon/request.py | 1 - tests/test_httperror.py | 33 ++++++++++++++++++++++++++++++++- 3 files changed, 38 insertions(+), 7 deletions(-) diff --git a/falcon/app_helpers.py b/falcon/app_helpers.py index 3ceab989e..86654aa1a 100644 --- a/falcon/app_helpers.py +++ b/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 @@ -311,10 +316,6 @@ def default_serialize_error(req: Request, resp: Response, exception: HTTPError) preferred = MEDIA_JSON elif '+xml' in accept: preferred = MEDIA_XML - else: - # NOTE(caselit): if nothing else matchers try using the media handlers - # registered in the response - preferred = req.client_prefers(resp.options.media_handlers) if preferred is not None: handler, _, _ = resp.options.media_handlers._resolve( diff --git a/falcon/request.py b/falcon/request.py index 3755abc6c..9bc38a59b 100644 --- a/falcon/request.py +++ b/falcon/request.py @@ -28,7 +28,6 @@ Mapping, Optional, overload, - Sequence, TextIO, Tuple, Type, diff --git a/tests/test_httperror.py b/tests/test_httperror.py index 093df6c54..fb59c0f85 100644 --- a/tests/test_httperror.py +++ b/tests/test_httperror.py @@ -962,7 +962,7 @@ def test_kw_only(self): 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!')) +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', @@ -1040,3 +1040,34 @@ def test_json_async_only_error(self, util): 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 From 96500fd2c77b10328013769bd45a198004bec9fd Mon Sep 17 00:00:00 2001 From: Vytautas Liuolia Date: Tue, 1 Oct 2024 07:05:46 +0200 Subject: [PATCH 16/17] docs(newsfragments): improve spelling/grammar --- docs/_newsfragments/2023.newandimproved.rst | 3 +-- docs/_newsfragments/2349.newandimproved.rst | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/docs/_newsfragments/2023.newandimproved.rst b/docs/_newsfragments/2023.newandimproved.rst index db6c24ec2..8bc2a197a 100644 --- a/docs/_newsfragments/2023.newandimproved.rst +++ b/docs/_newsfragments/2023.newandimproved.rst @@ -1,4 +1,3 @@ The default error serializer will now use the response media handlers to better negotiate the response serialization with the client. -The implementation still default to JSON in case a client does not specify -any preference. +The implementation still defaults to JSON if the client accepts it. diff --git a/docs/_newsfragments/2349.newandimproved.rst b/docs/_newsfragments/2349.newandimproved.rst index 7546a37eb..a5d143f7d 100644 --- a/docs/_newsfragments/2349.newandimproved.rst +++ b/docs/_newsfragments/2349.newandimproved.rst @@ -1,3 +1,3 @@ Added :attr:`falcon.testing.Result.content_type` and -:attr:`falcon.testing.StreamedResult.content_type` as an utility accessor +:attr:`falcon.testing.StreamedResult.content_type` as a utility accessor for the ``Content-Type`` header. From 3506deaa831037824b24621539f51cbda60cb873 Mon Sep 17 00:00:00 2001 From: Vytautas Liuolia Date: Tue, 1 Oct 2024 07:09:22 +0200 Subject: [PATCH 17/17] docs(newsfragments): revert changes to one newsfr since the impl was changed! --- docs/_newsfragments/2023.newandimproved.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/_newsfragments/2023.newandimproved.rst b/docs/_newsfragments/2023.newandimproved.rst index 8bc2a197a..b5001079c 100644 --- a/docs/_newsfragments/2023.newandimproved.rst +++ b/docs/_newsfragments/2023.newandimproved.rst @@ -1,3 +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 accepts it. +The implementation still defaults to JSON if the client does not indicate any +preference.