Merge pull request #1563 from Pylons/fix.idempotent-render-to-response

update render_to_response to prevent renderers from mutating request.response

3 files changed, 107 insertions, 22 deletions

diff --git a/CHANGES.txt b/CHANGES.txt
index ca2020cdb..3084bcfe6 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -102,6 +102,20 @@ Features
 - Support keyword-only arguments and function annotations in views in
   Python 3. See https://github.com/Pylons/pyramid/pull/1556
 
+- ``request.response`` will no longer be mutated when using the
+  ``pyramid.renderers.render_to_response()`` API.  It is now necessary to
+  pass in a ``response=`` argument to ``render_to_response`` if you wish to
+  supply the renderer with a custom response object for it to use. If you
+  do not pass one then a response object will be created using the
+  application's ``IResponseFactory``. Almost all renderers
+  mutate the ``request.response`` response object (for example, the JSON
+  renderer sets ``request.response.content_type`` to ``application/json``).
+  However, when invoking ``render_to_response`` it is not expected that the
+  response object being returned would be the same one used later in the
+  request. The response object returned from ``render_to_response`` is now
+  explicitly different from ``request.response``. This does not change the
+  API of a renderer. See https://github.com/Pylons/pyramid/pull/1563
+
 Bug Fixes
 ---------
 
diff --git a/pyramid/renderers.py b/pyramid/renderers.py
index 3c35551ea..088d451bb 100644
--- a/pyramid/renderers.py
+++ b/pyramid/renderers.py
@@ -1,3 +1,4 @@
+import contextlib
 import json
 import os
 
@@ -73,24 +74,16 @@ def render(renderer_name, value, request=None, package=None):
     helper = RendererHelper(name=renderer_name, package=package,
                             registry=registry)
 
-    saved_response = None
-    # save the current response, preventing the renderer from affecting it
-    attrs = request.__dict__ if request is not None else {}
-    if 'response' in attrs:
-        saved_response = attrs['response']
-        del attrs['response']
-
-    result = helper.render(value, None, request=request)
-
-    # restore the original response, overwriting any changes
-    if saved_response is not None:
-        attrs['response'] = saved_response
-    elif 'response' in attrs:
-        del attrs['response']
+    with temporary_response(request):
+        result = helper.render(value, None, request=request)
 
     return result
 
-def render_to_response(renderer_name, value, request=None, package=None):
+def render_to_response(renderer_name,
+                       value,
+                       request=None,
+                       package=None,
+                       response=None):
     """ Using the renderer ``renderer_name`` (a template
     or a static renderer), render the value (or set of values) using
     the result of the renderer's ``__call__`` method (usually a string
@@ -121,9 +114,16 @@ def render_to_response(renderer_name, value, request=None, package=None):
 
     Supply a ``request`` parameter in order to provide the renderer
     with the most correct 'system' values (``request`` and ``context``
-    in particular). Keep in mind that if the ``request`` parameter is
-    not passed in, any changes to ``request.response`` attributes made
-    before calling this function will be ignored.
+    in particular). Keep in mind that any changes made to ``request.response``
+    prior to calling this function will not be reflected in the resulting
+    response object. A new response object will be created for each call
+    unless one is passed as the ``response`` argument.
+
+    .. versionchanged:: 1.6
+       In previous versions, any changes made to ``request.response`` outside
+       of this function call would affect the returned response. This is no
+       longer the case. If you wish to send in a pre-initialized response
+       then you may pass one in the ``response`` argument.
 
     """
     try:
@@ -134,7 +134,33 @@ def render_to_response(renderer_name, value, request=None, package=None):
         package = caller_package()
     helper = RendererHelper(name=renderer_name, package=package,
                             registry=registry)
-    return helper.render_to_response(value, None, request=request)
+
+    with temporary_response(request):
+        if response is not None:
+            request.response = response
+        result = helper.render_to_response(value, None, request=request)
+
+    return result
+
+@contextlib.contextmanager
+def temporary_response(request):
+    """
+    Temporarily delete request.response and restore it afterward.
+    """
+    saved_response = None
+    # save the current response, preventing the renderer from affecting it
+    attrs = request.__dict__ if request is not None else {}
+    if 'response' in attrs:
+        saved_response = attrs['response']
+        del attrs['response']
+
+    yield
+
+    # restore the original response, overwriting any changes
+    if saved_response is not None:
+        attrs['response'] = saved_response
+    elif 'response' in attrs:
+        del attrs['response']
 
 def get_renderer(renderer_name, package=None):
     """ Return the renderer object for the renderer ``renderer_name``.
diff --git a/pyramid/tests/test_renderers.py b/pyramid/tests/test_renderers.py
index 6d79cc291..ed6344a40 100644
--- a/pyramid/tests/test_renderers.py
+++ b/pyramid/tests/test_renderers.py
@@ -517,10 +517,11 @@ class Test_render_to_response(unittest.TestCase):
     def tearDown(self):
         testing.tearDown()
 
-    def _callFUT(self, renderer_name, value, request=None, package=None):
+    def _callFUT(self, renderer_name, value, request=None, package=None,
+                 response=None):
         from pyramid.renderers import render_to_response
         return render_to_response(renderer_name, value, request=request,
-                                  package=package)
+                                  package=package, response=response)
 
     def test_it_no_request(self):
         renderer = self.config.testing_add_renderer(
@@ -554,6 +555,43 @@ class Test_render_to_response(unittest.TestCase):
         renderer.assert_(a=1)
         renderer.assert_(request=request)
 
+    def test_response_preserved(self):
+        request = testing.DummyRequest()
+        response = object() # should error if mutated
+        request.response = response
+        # use a json renderer, which will mutate the response
+        result = self._callFUT('json', dict(a=1), request=request)
+        self.assertEqual(result.body, b'{"a": 1}')
+        self.assertNotEqual(request.response, result)
+        self.assertEqual(request.response, response)
+
+    def test_no_response_to_preserve(self):
+        from pyramid.decorator import reify
+        class DummyRequestWithClassResponse(object):
+            _response = DummyResponse()
+            _response.content_type = None
+            _response.default_content_type = None
+            @reify
+            def response(self):
+                return self._response
+        request = DummyRequestWithClassResponse()
+        # use a json renderer, which will mutate the response
+        result = self._callFUT('json', dict(a=1), request=request)
+        self.assertEqual(result.body, b'{"a": 1}')
+        self.assertFalse('response' in request.__dict__)
+
+    def test_custom_response_object(self):
+        class DummyRequestWithClassResponse(object):
+            pass
+        request = DummyRequestWithClassResponse()
+        response = DummyResponse()
+        # use a json renderer, which will mutate the response
+        result = self._callFUT('json', dict(a=1), request=request,
+                               response=response)
+        self.assertTrue(result is response)
+        self.assertEqual(result.body, b'{"a": 1}')
+        self.assertFalse('response' in request.__dict__)
+
 class Test_get_renderer(unittest.TestCase):
     def setUp(self):
         self.config = testing.setUp()
@@ -614,7 +652,14 @@ class Dummy:
 
 class DummyResponse:
     status = '200 OK'
+    default_content_type = 'text/html'
+    content_type = default_content_type
     headerlist = ()
     app_iter = ()
-    body = ''
+    body = b''
+
+    # compat for renderer that will set unicode on py3
+    def _set_text(self, val): # pragma: no cover
+        self.body = val.encode('utf8')
+    text = property(fset=_set_text)