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

response.content_type does not persist with render_to_response if optional request is omitted. #1534

Closed
jvanasco opened this issue Jan 21, 2015 · 11 comments

Comments

@jvanasco
Copy link
Contributor

This is the same underlying issue as described in #248, but with a different effect/gotcha.

I created a single file example here: https://gist.github.com/jvanasco/4a27eb95549de771b304

When returning the result of a render_to_response using one of the default renderers (i.e. json), the "content-type" header is set on the request.response by the renderer function -- so it is not available to the Response if there is no request.

This is weird. It's not doing what one would expect, and the render_to_response docs don't suggest that the request is required for this expected behavior (http://docs.pylonsproject.org/docs/pyramid/en/latest/api/renderers.html):

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.

It's not clear/expected from this text that one would need to submit a request attribute in order for the response configurations to actually be made. This is also misleading regarding the flow -- the expected change/configure of the "response" would not be made before calling the function, but within it.

I think it's acceptable & best to explain this within the docs. Perhaps something like "Important: If you do not supply a request parameter, the renderer will not be able to set the appropriate content headers on the response."

@mmerickel
Copy link
Member

The request object is "all but required". Unfortunately it isn't required but someday it might be. The rendering subsystem requires a threadlocal lookup for the registry if you don't supply the request.

Regardless, like @mcdonc said in #248, if you'd like to improve the docs you should feel free.

There are 2 bugs I found while researching your issue though.

  1. JSONP renderer currently requires a request object #1535 The JSONP renderer (not JSON) currently requires a request object.
  2. render_to_response should not affect request.response #1536 render_to_response should not be affecting request.response, it should be returning a different, unrelated response object.

@stevepiercy
Copy link
Member

Suggest this language:
"To set appropriate content headers on the response, you must supply a request parameter."

@mmerickel
Copy link
Member

I've opened #1536 as a direct complaint about this behavior. IMO the response object returned should be different from request.response and should have its properties mutated as expected, whether or not you supplied a request.

@jvanasco
Copy link
Contributor Author

@stevepiercy works for me

@mmerickel totally agree with you and #1536 . i looked into trying to patching that behavior, and saw some red flags so I recommended a docs change. the big issue is that this would likely create backwards incompatibilities with anyone who has build a custom render factory in accordance with the docs/tutorials.

@mmerickel
Copy link
Member

@jvanasco the only backward incompatibility would be if you expected the response returned from render_to_response to be the same as request.response. Is that the case in any code you are aware of, including any tutorials?

@jvanasco
Copy link
Contributor Author

Yeah. I'll collect them into the other ticket.

@mmerickel
Copy link
Member

@jvanasco Can you confirm that #1563 fixes this issue?

@mmerickel
Copy link
Member

poke @jvanasco

@jvanasco
Copy link
Contributor Author

jvanasco commented Mar 3, 2015

thanks for poking. totally forgot about this. i'll try to find the part I had an issue with tonight.

@jvanasco
Copy link
Contributor Author

jvanasco commented Mar 9, 2015

#1563 solves this.

Sorry this took so long. I had to find the code to test, then encountered #1600

@tseaver
Copy link
Member

tseaver commented Mar 9, 2015

#1563 is merged.

@tseaver tseaver closed this as completed Mar 9, 2015
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

No branches or pull requests

4 participants