Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update render_to_response to prevent renderers from mutating request.response #1563

Merged
merged 5 commits into from
Feb 22, 2015

Conversation

mmerickel
Copy link
Member

fixes #1536

I think this should be fairly non-controversial. When using render_to_response you'd probably expect that to not have any side effects on request.response which you may also end up using during the request. This patch just keeps them separate.

@mmerickel
Copy link
Member Author

In 72bf6bb I added the ability to specify a custom response object as an argument. So if you need to modify some params prior to the renderer touching the object then you can just create the object and pass it in.

response = Response()
result = render_to_response('json', {}, request=request, response=response)
assert result is response # --> True

This PR very slightly backward incompatible since it was apparently documented to mutate request.response in the last sentence of the docstring, but I really think this is an improvement in usability. Previously you could really only call render_to_response once and you'd get subtle side-effects when calling it twice while sharing request.response. Not that many people would've called it twice in one request.

@digitalresistor
Copy link
Member

It was documented that if you mutated request.response before passing request to render_to_reponse then those mutations would be visible to render_to_response. Nowhere did it state in that docstring (from the way I am reading it) that render_to_response would mutate request.response itself.

The one thing you could add to be backwards compatible to what was documented was to make a copy of the response object on request if no response is explicitly passed in.

@mmerickel
Copy link
Member Author

What approach would you take? I'm still leaning toward my solution atm because I think it makes more sense. Compatibility has not yet dissuaded me.

@digitalresistor
Copy link
Member

Honestly the original way was very unintuitive, I would go with your changes and call it good. I simply pointed out that the language used in the original docstring gave you some leeway to make a b/w compat fix.

@tseaver
Copy link
Member

tseaver commented Feb 19, 2015

LGTM

mmerickel added a commit that referenced this pull request Feb 22, 2015
update render_to_response to prevent renderers from mutating request.response
@mmerickel mmerickel merged commit 71bb8cc into master Feb 22, 2015
@mmerickel mmerickel deleted the fix.idempotent-render-to-response branch February 22, 2015 18:58
dairiki added a commit to dairiki/pyramid that referenced this pull request Apr 15, 2015
These tests test for, among other things, the nits described in comments
on Pylons#1563, namely:
- ``Request.response`` should be restored even in the renderer raises an
  exception
- If ``request.response`` is initially set to ``None``, it should be
  restored to ``None`` (rather than deleted).

(Some of these tests currently fail.)
dairiki added a commit to dairiki/pyramid that referenced this pull request Apr 15, 2015
This fixes two bugs in the ``temporary_response`` context manager:
- ``Request.response`` should be restored even in the renderer raises an
  exception
- If ``request.response`` is initially set to ``None``, it should be
  restored to ``None`` (rather than deleted).

References: Pylons#1563
dairiki added a commit to dairiki/pyramid that referenced this pull request Apr 15, 2015
These tests test for, among other things, the nits described in comments
on Pylons#1563, namely:
- ``Request.response`` should be restored even if the renderer raises an
  exception
- If ``request.response`` is initially set to ``None``, it should be
  restored to ``None`` (rather than deleted).

(Some of these tests currently fail.)
dairiki added a commit to dairiki/pyramid that referenced this pull request Apr 15, 2015
This fixes two bugs in the ``temporary_response`` context manager:
- ``Request.response`` should be restored even if the renderer raises an
  exception
- If ``request.response`` is initially set to ``None``, it should be
  restored to ``None`` (rather than deleted).

References: Pylons#1563
mmerickel pushed a commit that referenced this pull request Apr 15, 2015
These tests test for, among other things, the nits described in comments
on #1563, namely:
- ``Request.response`` should be restored even if the renderer raises an
  exception
- If ``request.response`` is initially set to ``None``, it should be
  restored to ``None`` (rather than deleted).

(Some of these tests currently fail.)
mmerickel pushed a commit that referenced this pull request Apr 15, 2015
This fixes two bugs in the ``temporary_response`` context manager:
- ``Request.response`` should be restored even if the renderer raises an
  exception
- If ``request.response`` is initially set to ``None``, it should be
  restored to ``None`` (rather than deleted).

References: #1563
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

render_to_response should not affect request.response
4 participants