Repository: falconry/falcon

Issue:
Unify behavior between asgi and wsgi
Currently the implementation of the response class for the wsgi and the asgi differ when handling the `data` property. Namely the asgi version does not override data with the media value, while the wsgi version overrides the data with the media serialization data.

In the code on master there are currently these todos
https://github.com/falconry/falcon/blob/c3e91875f1030c68d17efa27cf8df3d18f337761/falcon/response.py#L192-L195
https://github.com/falconry/falcon/blob/c3e91875f1030c68d17efa27cf8df3d18f337761/falcon/response.py#L192-L195

Should the wsgi response class behave like the asgi, and use the method `render_body` to get the body in the response?

Discussion:
kgriffs: > I'm not sure where and how the documentation should be updated. Do you have suggestions?

No problem, I can just add a patch to include the doc updates. Everything else looks great, thanks!
kgriffs: Docs updated. This should be ready for final review. @vytas7 
CaselIT: The change in docs looks good!

Patch:
diff --git a/docs/_newsfragments/1679.breakingchange.rst b/docs/_newsfragments/1679.breakingchange.rst
new file mode 100644
index 0000000..ad00992
--- /dev/null
+++ b/docs/_newsfragments/1679.breakingchange.rst
@@ -0,0 +1,7 @@
+The :attr:`falcon.Response.data` property now just simply returns the same data
+object that it was set to, if any, rather than also checking and serializing
+the value of the :attr:`falcon.Response.media` property. Instead, a new
+:meth:`~falcon.Response.render_body` method has been implemented, which can be
+used to obtain the HTTP response body for the request, taking into account
+the :attr:`~falcon.Response.body`, :attr:`~falcon.Response.data`, and
+:attr:`~falcon.Response.media` attributes.
diff --git a/docs/_newsfragments/1679.misc.rst b/docs/_newsfragments/1679.misc.rst
new file mode 100644
index 0000000..3aec289
--- /dev/null
+++ b/docs/_newsfragments/1679.misc.rst
@@ -0,0 +1,5 @@
+A new method, :meth:`~falcon.Request.get_media`, was added that can now be used
+instead of the :attr:`falcon.Request.media` property to make it more clear to
+app maintainers that getting the media object for a request involves a
+side-effect of consuming and deserializing the body stream. The original
+property remains available to ensure backwards-compatibility with existing apps.
diff --git a/docs/api/request_and_response_asgi.rst b/docs/api/request_and_response_asgi.rst
index b2cabb3..1d12ec6 100644
--- a/docs/api/request_and_response_asgi.rst
+++ b/docs/api/request_and_response_asgi.rst
@@ -34,6 +34,7 @@ Request
 .. autoclass:: falcon.asgi.Request
     :members:
     :inherited-members:
+    :exclude-members: media
 
 .. autoclass:: falcon.asgi.BoundedStream
     :members:
diff --git a/docs/api/request_and_response_wsgi.rst b/docs/api/request_and_response_wsgi.rst
index 4d15322..82fe431 100644
--- a/docs/api/request_and_response_wsgi.rst
+++ b/docs/api/request_and_response_wsgi.rst
@@ -34,6 +34,8 @@ Request
 
 .. autoclass:: falcon.Request
     :members:
+    :exclude-members: media
+
 
 .. autoclass:: falcon.Forwarded
     :members:
diff --git a/falcon/__init__.py b/falcon/__init__.py
index 8034e59..c979777 100644
--- a/falcon/__init__.py
+++ b/falcon/__init__.py
@@ -53,11 +53,6 @@ ASGI_SUPPORTED = _sys.version_info.minor > 5
 """Set to ``True`` when ASGI is supported for the current Python version."""
 
 
-# NOTE(kgriffs): Special singleton to be used internally whenever using
-#   None would be ambiguous.
-_UNSET = object()
-
-
 # NOTE(kgriffs): Only to be used internally on the rare occasion that we
 #   need to log something that we can't communicate any other way.
 _logger = _logging.getLogger('falcon')
diff --git a/falcon/app.py b/falcon/app.py
index cdd111c..1f30d76 100644
--- a/falcon/app.py
+++ b/falcon/app.py
@@ -969,24 +969,16 @@ class App:
             The length is returned as ``None`` when unknown. The
             iterable is determined as follows:
 
-                * If resp.body is not ``None``, returns
-                  ([resp.body], len(resp.body)),
-                  encoded as UTF-8 if it is a Unicode string.
-                  Bytestrings are returned as-is.
-                * If resp.data is not ``None``, returns ([resp.data], len(resp.data))
+                * If the result of render_body() is not ``None``, returns
+                  ([data], len(data))
                 * If resp.stream is not ``None``, returns resp.stream
                   iterable using wsgi.file_wrapper, if necessary:
                   (closeable_iterator, None)
                 * Otherwise, returns ([], 0)
 
         """
-        body = resp.body
-        if body is not None:
-            if not isinstance(body, bytes):
-                body = body.encode('utf-8')
-            return [body], len(body)
 
-        data = resp.data
+        data = resp.render_body()
         if data is not None:
             return [data], len(data)
 
diff --git a/falcon/asgi/request.py b/falcon/asgi/request.py
index ddf7787..975c3a7 100644
--- a/falcon/asgi/request.py
+++ b/falcon/asgi/request.py
@@ -263,7 +263,7 @@ class Request(falcon.request.Request):
 
             See also: :class:`falcon.asgi.BoundedStream`
         media (object): An awaitable property that acts as an alias for
-            :meth:`~.get_media`. This can be used to ease the porting of a
+            :meth:`~.get_media`. This can be used to ease the porting of
             a WSGI app to ASGI, although the ``await`` keyword must still be
             added when referencing the property::
 
@@ -566,17 +566,6 @@ class Request(falcon.request.Request):
 
         return scheme
 
-    @property
-    def prefix(self):
-        if self._cached_prefix is None:
-            self._cached_prefix = (
-                self.scheme + '://' +
-                self.netloc +
-                self.app
-            )
-
-        return self._cached_prefix
-
     @property
     def host(self):
         try:
@@ -691,9 +680,9 @@ class Request(falcon.request.Request):
     async def get_media(self):
         """Returns a deserialized form of the request stream.
 
-        When called the first time, the request stream will be deserialized
-        using the Content-Type header as well as the media-type handlers
-        configured via :class:`falcon.RequestOptions`. The result will
+        The first time this method is called, the request stream will be
+        deserialized using the Content-Type header as well as the media-type
+        handlers configured via :class:`falcon.RequestOptions`. The result will
         be cached and returned in subsequent calls::
 
             deserialized_media = await req.get_media()
@@ -702,7 +691,7 @@ class Request(falcon.request.Request):
         deserialize the request body, the exception will propagate up
         to the caller.
 
-        See :ref:`media` for more information regarding media handling.
+        See also :ref:`media` for more information regarding media handling.
 
         Warning:
             This operation will consume the request stream the first time
diff --git a/falcon/asgi/response.py b/falcon/asgi/response.py
index f0acc8e..561fcc2 100644
--- a/falcon/asgi/response.py
+++ b/falcon/asgi/response.py
@@ -17,7 +17,7 @@
 from asyncio.coroutines import CoroWrapper  # type: ignore
 from inspect import iscoroutine, iscoroutinefunction
 
-from falcon import _UNSET
+from falcon.constants import _UNSET
 import falcon.response
 from falcon.util.misc import is_python_func
 
@@ -68,14 +68,6 @@ class Response(falcon.response.Response):
                 ensure Unicode characters are properly encoded in the
                 HTTP response.
 
-            Note:
-                Unlike the WSGI Response class, the ASGI Response class
-                does not implement the side-effect of serializing
-                the media object (if one is set) when the `data`
-                attribute is read. Instead,
-                :py:meth:`falcon.asgi.Response.render_body` should
-                be used to get the correct content for the response.
-
         stream: An async iterator or generator that yields a series of
             byte strings that will be streamed to the ASGI server as a
             series of "http.response.body" events. Falcon will assume the
@@ -199,7 +191,6 @@ class Response(falcon.response.Response):
     #   an additional function call.
     _sse = None
     _registered_callbacks = None
-    _media_rendered = _UNSET
 
     @property
     def sse(self):
@@ -209,23 +200,6 @@ class Response(falcon.response.Response):
     def sse(self, value):
         self._sse = value
 
-    @property
-    def media(self):
-        return self._media
-
-    @media.setter
-    def media(self, value):
-        self._media = value
-        self._media_rendered = _UNSET
-
-    @property
-    def data(self):
-        return self._data
-
-    @data.setter
-    def data(self, value):
-        self._data = value
-
     def set_stream(self, stream, content_length):
         """Convenience method for setting both `stream` and `content_length`.
 
@@ -258,16 +232,21 @@ class Response(falcon.response.Response):
         self._headers['content-length'] = str(content_length)
 
     async def render_body(self):
-        """Get the raw content for the response body.
+        """Get the raw bytestring content for the response body.
 
-        This coroutine can be awaited to get the raw body data that should
-        be returned in the HTTP response.
+        This coroutine can be awaited to get the raw data for the
+        HTTP response body, taking into account the :attr:`~.body`,
+        :attr:`~.data`, and :attr:`~.media` attributes.
+
+        Note:
+            This method ignores :attr:`~.stream`; the caller must check
+            and handle that attribute directly.
 
         Returns:
             bytes: The UTF-8 encoded value of the `body` attribute, if
-                set. Otherwise, the value of the `data` attribute if set, or
-                finally the serialized value of the `media` attribute. If
-                none of these attributes are set, ``None`` is returned.
+            set. Otherwise, the value of the `data` attribute if set, or
+            finally the serialized value of the `media` attribute. If
+            none of these attributes are set, ``None`` is returned.
         """
 
         body = self.body
diff --git a/falcon/constants.py b/falcon/constants.py
index 40e39da..77c7902 100644
--- a/falcon/constants.py
+++ b/falcon/constants.py
@@ -106,3 +106,7 @@ SINGLETON_HEADERS = frozenset([
     'referer',
     'user-agent',
 ])
+
+# NOTE(kgriffs): Special singleton to be used internally whenever using
+#   None would be ambiguous.
+_UNSET = object()
diff --git a/falcon/request.py b/falcon/request.py
index 76c5da3..605ce90 100644
--- a/falcon/request.py
+++ b/falcon/request.py
@@ -332,17 +332,13 @@ class Request:
 
                 doc = json.load(req.bounded_stream)
 
-        media (object): Returns a deserialized form of the request stream.
-            When called, it will attempt to deserialize the request stream
-            using the Content-Type header as well as the media-type handlers
-            configured via :class:`falcon.RequestOptions`.
+        media (object): Property that acts as an alias for
+            :meth:`~.get_media`. This alias provides backwards-compatibility
+            for apps that were built for versions of the framework prior to
+            3.0::
 
-            See :ref:`media` for more information regarding media handling.
-
-            Warning:
-                This operation will consume the request stream the first time
-                it's called and cache the results. Follow-up calls will just
-                retrieve a cached version of the object.
+                # Equivalent to: deserialized_media = req.get_media()
+                deserialized_media = req.media
 
         expect (str): Value of the Expect header, or ``None`` if the
             header is missing.
@@ -793,7 +789,7 @@ class Request:
     def prefix(self):
         if self._cached_prefix is None:
             self._cached_prefix = (
-                self.env['wsgi.url_scheme'] + '://' +
+                self.scheme + '://' +
                 self.netloc +
                 self.app
             )
@@ -975,8 +971,27 @@ class Request:
 
         return netloc_value
 
-    @property
-    def media(self):
+    def get_media(self):
+        """Returns a deserialized form of the request stream.
+
+        The first time this method is called, the request stream will be
+        deserialized using the Content-Type header as well as the media-type
+        handlers configured via :class:`falcon.RequestOptions`. The result will
+        be cached and returned in subsequent calls::
+
+            deserialized_media = req.get_media()
+
+        If the matched media handler raises an error while attempting to
+        deserialize the request body, the exception will propagate up
+        to the caller.
+
+        See also :ref:`media` for more information regarding media handling.
+
+        Warning:
+            This operation will consume the request stream the first time
+            it's called and cache the results. Follow-up calls will just
+            retrieve a cached version of the object.
+        """
         if self._media is not None or self.bounded_stream.eof:
             return self._media
 
@@ -997,6 +1012,8 @@ class Request:
 
         return self._media
 
+    media = property(get_media)
+
     # ------------------------------------------------------------------------
     # Methods
     # ------------------------------------------------------------------------
diff --git a/falcon/response.py b/falcon/response.py
index febd702..61b9dfc 100644
--- a/falcon/response.py
+++ b/falcon/response.py
@@ -17,6 +17,7 @@
 import mimetypes
 
 from falcon import DEFAULT_MEDIA_TYPE
+from falcon.constants import _UNSET
 from falcon.errors import HeaderNotSupported
 from falcon.media import Handlers
 from falcon.response_helpers import (
@@ -154,6 +155,7 @@ class Response:
         '_extra_headers',
         '_headers',
         '_media',
+        '_media_rendered',
         '__dict__',
     )
 
@@ -184,41 +186,12 @@ class Response:
         self.stream = None
         self._data = None
         self._media = None
+        self._media_rendered = _UNSET
 
         self.context = self.context_type()
 
     @property
     def data(self):
-        # TODO(kgriffs): Remove the side-effect that accessing this
-        #   property causes (do something similar to what we did on the
-        #   ASGI side). This will be a breaking change, so caution is
-        #   advised.
-
-        # NOTE(kgriffs): Test explicitly against None since the
-        # app may have set it to an empty binary string.
-        if self._data is not None:
-            return self._data
-
-        # NOTE(kgriffs): Test explicitly against None since the
-        # app may have set it to an empty string that should still
-        # be serialized.
-        if self._media is None:
-            return None
-
-        if not self.content_type:
-            self.content_type = self.options.default_media_type
-
-        handler = self.options.media_handlers.find_by_media_type(
-            self.content_type,
-            self.options.default_media_type
-        )
-
-        # NOTE(kgriffs): Set _data to avoid re-serializing if the
-        # data() property is called multiple times.
-        self._data = handler.serialize(
-            self._media,
-            self.content_type
-        )
         return self._data
 
     @data.setter
@@ -234,19 +207,9 @@ class Response:
         return self._media
 
     @media.setter
-    def media(self, obj):
-        self._media = obj
-
-        # NOTE(kgriffs): This will be set just-in-time by the data() property,
-        # rather than serializing immediately. That way, if media() is called
-        # multiple times we don't waste time serializing objects that will
-        # just be thrown away.
-        #
-        # TODO(kgriffs): This makes precedence harder to reason about, since
-        #   it is no longer about what attributes have and have not been set,
-        #   but also what order they were set in. On the ASGI side this has
-        #   already been addressed.
-        self._data = None
+    def media(self, value):
+        self._media = value
+        self._media_rendered = _UNSET
 
     @property
     def stream_len(self):
@@ -260,6 +223,56 @@ class Response:
         #   so that apps relying on it do not fail silently.
         raise AttributeError(_STREAM_LEN_REMOVED_MSG)
 
+    def render_body(self):
+        """Get the raw bytestring content for the response body.
+
+        This method returns the raw data for the HTTP response body, taking
+        into account the :attr:`~.body`, :attr:`~.data`, and :attr:`~.media`
+        attributes.
+
+        Note:
+            This method ignores :attr:`~.stream`; the caller must check
+            and handle that attribute directly.
+
+        Returns:
+            bytes: The UTF-8 encoded value of the `body` attribute, if
+            set. Otherwise, the value of the `data` attribute if set, or
+            finally the serialized value of the `media` attribute. If
+            none of these attributes are set, ``None`` is returned.
+        """
+
+        body = self.body
+        if body is None:
+            data = self._data
+
+            if data is None and self._media is not None:
+                # NOTE(kgriffs): We use a special _UNSET singleton since
+                #   None is ambiguous (the media handler might return None).
+                if self._media_rendered is _UNSET:
+                    if not self.content_type:
+                        self.content_type = self.options.default_media_type
+
+                    handler = self.options.media_handlers.find_by_media_type(
+                        self.content_type,
+                        self.options.default_media_type
+                    )
+
+                    self._media_rendered = handler.serialize(
+                        self._media,
+                        self.content_type
+                    )
+
+                data = self._media_rendered
+        else:
+            try:
+                # NOTE(kgriffs): Normally we expect body to be a string
+                data = body.encode()
+            except AttributeError:
+                # NOTE(kgriffs): Assume it was a bytes object already
+                data = body
+
+        return data
+
     def __repr__(self):
         return '<%s: %s>' % (self.__class__.__name__, self.status)
 


Test patch:
diff --git a/tests/asgi/test_response_media_asgi.py b/tests/asgi/test_response_media_asgi.py
index fdaddc2..5eae5b4 100644
--- a/tests/asgi/test_response_media_asgi.py
+++ b/tests/asgi/test_response_media_asgi.py
@@ -162,3 +162,62 @@ def test_mimeparse_edgecases():
     client = create_client(TestResource())
     result = client.simulate_get('/')
     assert result.json == doc
+
+
+def runTest(test_fn):
+    doc = {'something': True}
+
+    class TestResource:
+        async def on_get(self, req, resp):
+
+            await test_fn(resp)
+
+            resp.body = None
+            resp.data = None
+            resp.media = doc
+
+    client = create_client(TestResource())
+    result = client.simulate_get('/')
+    assert result.json == doc
+
+
+class TestRenderBodyPrecedence:
+    def test_body(self):
+        async def test(resp):
+            resp.body = 'body'
+            resp.data = b'data'
+            resp.media = ['media']
+
+            assert await resp.render_body() == b'body'
+
+        runTest(test)
+
+    def test_data(self):
+        async def test(resp):
+            resp.data = b'data'
+            resp.media = ['media']
+
+            assert await resp.render_body() == b'data'
+
+        runTest(test)
+
+    def test_media(self):
+        async def test(resp):
+            resp.media = ['media']
+            assert json.loads((await resp.render_body()).decode('utf-8')) == ['media']
+
+        runTest(test)
+
+
+def test_media_rendered_cached():
+    async def test(resp):
+        resp.media = {'foo': 'bar'}
+
+        first = await resp.render_body()
+        assert first is await resp.render_body()
+        assert first is resp._media_rendered
+
+        resp.media = 123
+        assert first is not await resp.render_body()
+
+    runTest(test)
diff --git a/tests/test_request_media.py b/tests/test_request_media.py
index 27ba1f7..56d2e9f 100644
--- a/tests/test_request_media.py
+++ b/tests/test_request_media.py
@@ -31,7 +31,7 @@ class ResourceCachedMedia:
         self.captured_req_media = req.media
 
         # NOTE(kgriffs): Ensure that the media object is cached
-        assert self.captured_req_media is req.media
+        assert self.captured_req_media is req.get_media()
 
 
 class ResourceCachedMediaAsync:
diff --git a/tests/test_response_media.py b/tests/test_response_media.py
index 700e8a6..d43fc45 100644
--- a/tests/test_response_media.py
+++ b/tests/test_response_media.py
@@ -50,7 +50,7 @@ def test_json(client, media_type):
     resp.content_type = media_type
     resp.media = {'something': True}
 
-    assert json.loads(resp.data.decode('utf-8')) == {'something': True}
+    assert json.loads(resp.render_body().decode('utf-8')) == {'something': True}
 
 
 @pytest.mark.parametrize('document', [
@@ -96,11 +96,11 @@ def test_msgpack(media_type):
 
     # Bytes
     resp.media = {b'something': True}
-    assert resp.data == b'\x81\xc4\tsomething\xc3'
+    assert resp.render_body() == b'\x81\xc4\tsomething\xc3'
 
     # Unicode
     resp.media = {'something': True}
-    assert resp.data == b'\x81\xa9something\xc3'
+    assert resp.render_body() == b'\x81\xa9something\xc3'
 
 
 def test_unknown_media_type(client):
@@ -110,7 +110,7 @@ def test_unknown_media_type(client):
     with pytest.raises(errors.HTTPUnsupportedMediaType) as err:
         resp.content_type = 'nope/json'
         resp.media = {'something': True}
-        __ = resp.data  # NOQA
+        resp.render_body()
 
     assert err.value.description == 'nope/json is an unsupported media type.'
 
@@ -132,7 +132,7 @@ def test_default_media_type(client):
     resp.content_type = ''
     resp.media = {'something': True}
 
-    assert json.loads(resp.data.decode('utf-8')) == {'something': True}
+    assert json.loads(resp.render_body().decode('utf-8')) == {'something': True}
     assert resp.content_type == 'application/json'
 
 
@@ -144,13 +144,59 @@ def test_mimeparse_edgecases(client):
     resp.content_type = 'application/vnd.something'
     with pytest.raises(errors.HTTPUnsupportedMediaType):
         resp.media = {'something': True}
-        __ = resp.data  # NOQA
+        resp.render_body()
 
     resp.content_type = 'invalid'
     with pytest.raises(errors.HTTPUnsupportedMediaType):
         resp.media = {'something': True}
-        __ = resp.data  # NOQA
+        resp.render_body()
 
     # Clear the content type, shouldn't raise this time
     resp.content_type = None
     resp.media = {'something': True}
+
+
+class TestRenderBodyPrecedence:
+    def test_body(self, client):
+        client.simulate_get('/')
+
+        resp = client.resource.captured_resp
+
+        resp.body = 'body'
+        resp.data = b'data'
+        resp.media = ['media']
+
+        assert resp.render_body() == b'body'
+
+    def test_data(self, client):
+        client.simulate_get('/')
+
+        resp = client.resource.captured_resp
+
+        resp.data = b'data'
+        resp.media = ['media']
+
+        assert resp.render_body() == b'data'
+
+    def test_media(self, client):
+        client.simulate_get('/')
+
+        resp = client.resource.captured_resp
+        resp.media = ['media']
+
+        assert json.loads(resp.render_body().decode('utf-8')) == ['media']
+
+
+def test_media_rendered_cached(client):
+    client.simulate_get('/')
+
+    resp = client.resource.captured_resp
+
+    resp.media = {'foo': 'bar'}
+
+    first = resp.render_body()
+    assert first is resp.render_body()
+    assert first is resp._media_rendered
+
+    resp.media = 123
+    assert first is not resp.render_body()