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 object adding default_content_type is perhaps bad magic #205

Closed
cdent opened this Issue Jun 18, 2015 · 7 comments

Comments

Projects
None yet
2 participants
@cdent
Copy link

cdent commented Jun 18, 2015

If a response is 304 or 204 or any of the other varieties with no body, then there should be no content-type header. WebOb doesn't respect this and instead will set the header to the value of Response.default_content_type.

Obviously this can be worked around in the code that uses Response (by setting a different default) however it seems that the default default behavior is incorrect.

Would it make sense to check status before choosing to use the default_content_type?

@cdent

This comment has been minimized.

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Jun 19, 2015

Yeah, this is a known issue. There are a couple of bugs reports in Pyramid that are related.

Here's a couple of related issues:

#204
#130
#112

openstack-gerrit added a commit to openstack/openstack that referenced this issue Mar 15, 2016

Updated openstack/openstack
Project: openstack/oslo.middleware  3988776dba4789360d3ea6bddf4eaeb33eb35d7c

cors: prevent WebOb setting a default Content-Type

This related to Pylons/webob#205 upstream.

Change-Id: I18eb78d4206c20efc934fe8709881c2bd6972983

openstack-gerrit pushed a commit to openstack/oslo.middleware that referenced this issue Mar 15, 2016

cors: prevent WebOb setting a default Content-Type
This related to Pylons/webob#205 upstream.

Change-Id: I18eb78d4206c20efc934fe8709881c2bd6972983

bertjwregeer added a commit that referenced this issue Jul 9, 2016

On HTTP status codes without body, don't set content type
When using Request.get_response() the response from the application may
contain a status code that does not allow for an message-body to be
returned, in this case adding the content type header is not valid, and
potentially changes the original response.

Closes #205
@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Jul 9, 2016

@cdent I have pushed a change that should solve this issue, please see #261, but specifically commit a35a117.

This should solve the problem, I would be interested in hearing your feedback. There are a lot of other changes going into 1.7 as well, so test runs on the OpenStack could would be appreciated :-).

@cdent

This comment has been minimized.

Copy link

cdent commented Jul 11, 2016

Cool. I'll give that branch a try against some stuff and report back. Looking at the specific commit those changes make sense. The only thing I'd wonder about is those situations where someone sets a response code that shouldn't have a body but also happens to set a body anyway. What happens then?

@cdent

This comment has been minimized.

Copy link

cdent commented Jul 11, 2016

I've tried the branch in a couple different projects that are using webob and thus far the only non-issue has been what I've commented on #261. So lgtm.

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Jul 11, 2016

If you have a body on a response that shouldn't have a body, WebOb will happily send it away. I am happy to let users shoot themselves in the foot, that flexibility should exist just in case they want to emulate some existing behaviour from an API, but I don't think creating that scenario in the constructor is entirely healthy.

I'm considering adding new internal only API that allows a Response() object to be created without doing any of the existing magic that happens for adding defaults iff it is being called from Request.get_response() so that we don't have to worry about this or any future side-effects when we really want the Response to look like what the user actually got returned from the application.

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Jul 30, 2016

I was going back over the original code before all my changes in #261, and I noticed that it was checking to see if the headerlist was already supplied, and if so it wouldn't add the default Content-Type.

The check was using: not headerlist though, and if the list is empty, that returns True. I've now fixed this in:

fb1a2ff

So that it checks that the headerlist was None, i.e. not provided by the caller, and only then will it set the Content-Type to the default. This fixes it in an even cleaner way and matches what Request.get_response() is much better.

Also means that after WebOb 1.7 OpenStack can pull some hacks whereby it sets default_content_type to None back out of the code base.

Tests to make sure this doesn't regress have been added in this commit:

160326a

@bertjwregeer bertjwregeer added this to the 1.7.0 milestone Jul 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment