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

Content-Type: application/json shouldn't have a charset parameter. #2860

Closed
gabisurita opened this issue Dec 8, 2016 · 12 comments
Closed

Content-Type: application/json shouldn't have a charset parameter. #2860

gabisurita opened this issue Dec 8, 2016 · 12 comments

Comments

@gabisurita
Copy link

Pyramid JSON renderer returns Content-Type: application/json; charset=utf-8, but according to RFC 4627, the application/json type doesn't have a charset parameter. It also describes why it's not needed.

See http://stackoverflow.com/questions/13096259/can-charset-parameter-be-used-with-application-json-content-type-in-http-1-1

@mmerickel
Copy link
Member

What version of pyramid and webob are you using? This has been fixed but I can't remember if it's released yet (but I think it is).

@gabisurita
Copy link
Author

I'm using pyramid-1.7.3 with WebOb-1.6.3. I think it's still unreleased. This was solved on #2706, right?

Thank you and sorry for the repost.

@mmerickel
Copy link
Member

mmerickel commented Dec 9, 2016

That PR you linked was reverted in favor of another solution as it was backward incompatible. I'm thinking of Pylons/webob#197 which was released in webob 1.6.0.

Here is a test case that passes. Can you update it to reproduce your issue?

from pyramid.config import Configurator
from webtest import TestApp

def json_view(request):
    return {}

config = Configurator()
config.add_view(json_view, renderer='json')
app = config.make_wsgi_app()

testapp = TestApp(app)
r = testapp.get('/')
assert r.content_type == 'application/json'

@gabisurita
Copy link
Author

gabisurita commented Dec 9, 2016

If we assert response.headers['Content-Type'] instead of response.content_type, the following test doesn't pass:

from pyramid.config import Configurator
from webtest import TestApp

def json_view(request):
    return {}

config = Configurator()
config.add_view(json_view, renderer='json')
app = config.make_wsgi_app()

testapp = TestApp(app)
r = testapp.get('/')
assert r.headers['Content-Type'] == 'application/json'

@mmerickel
Copy link
Member

Upgrading to webob 1.7.0rc1 fixes this, however it also emits a RuntimeWarning so pyramid is probably doing something a little bit wrong here.

@mmerickel mmerickel added the bugs label Dec 9, 2016
@digitalresistor
Copy link
Member

Pyramid changes from text/html; charset=utf-8 to setting content_type to application/json the warning you will be getting here is that WebOb has explicitly removed the charset.

In the future WebOb will remove all content_type_parameters when setting the content_type.

Pyramid isn't doing anything wrong, WebOb is warning about changed behaviour that a user may not be expecting.

@mmerickel
Copy link
Member

mmerickel commented Dec 9, 2016

Is there a way for pyramid to avoid this warning in the update? Obviously people are going to open more bugs in our tracker if every renderer='json' view they have starts emitting a RuntimeWarning. Could we delete the charset if it's utf8 or something?

@digitalresistor
Copy link
Member

res.charset = None
res.content_type = 'application/json'

Although it sounds like I should modify it instead to be a DeprecationWarning and that won't be shown by default...

@digitalresistor
Copy link
Member

digitalresistor commented Dec 9, 2016

res.headers['Content-Type'] = 'application/json'

Bypasses all of it, and does the right thing, and no warning will be emitted. It's because of the "magic" that res.content_type does (to keep the content-type parameters which is going away) that I wanted to warn users.

@digitalresistor
Copy link
Member

This is now fixed in WebOb 1.7.0rc2

@mmerickel
Copy link
Member

I'm trying to decide if it's worth just setting the header directly. Thoughts?

@digitalresistor
Copy link
Member

With the pinning of 1.7.0 in the has_body ticket there is no need for it.

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

No branches or pull requests

3 participants