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

It should be possible to set Content-Type to text/html when using renderers #1344

Open
lieryan opened this issue May 15, 2014 · 12 comments
Open
Labels

Comments

@lieryan
Copy link

lieryan commented May 15, 2014

Currently, this code will be served to the browser as text/plain thus the HTML are not rendered by the browser:

from wsgiref.simple_server import make_server
from pyramid.config import Configurator

def hello_world(request):
    request.response.content_type = "text/html"
    return "<p>Hello World</p>"

config = Configurator()
config.add_route('hello', '/')
config.add_view(hello_world, route_name='hello', renderer='string')
app = config.make_wsgi_app()
make_server('', 8000, app).serve_forever()

A little bit of investigative work shows that the issue is here:

File: /pyramid/renderers.py
155 def string_renderer_factory(info):
156     def _render(value, system):
157         if not isinstance(value, string_types):
158             value = str(value)
159         request = system.get('request')
160         if request is not None:
161             response = request.response
162             ct = response.content_type
163             if ct == response.default_content_type:
164                 response.content_type = 'text/plain'
165         return value
166     return _render

Here response.default_content_type == 'text/html' and the string renderer replaces the specified content_type with its default of text/plain. I think this is unintuitive/unexpected behavior, instead when request.response.content_type is explicitly set to 'text/html', the renderer should be not change it.

I didn't test the json renderer, but since there is a similar code over there, I'd assume it has the same bug as well.

@mmerickel mmerickel added the bugs label May 21, 2014
@mmerickel
Copy link
Member

We should try to fix this if we can. My initial idea is to modify pyramid.response.Response to override default_content_type to be some sort of dummy object that can be tested for via response.content_type is response.default_content_type (using is).

@ledmonster
Copy link

response.content_type is retrieved from headers, so it seems to be difficult to judge whether content_type is overrided or not...

@jaseemabid
Copy link

Similar bug with json renderer.

Somehow my default_content_type is None and json renderer won't attach the correct 'Content-Type: application/json' header to response.

        request = system.get('request')
        if request is not None:
            response = request.response
            ct = response.content_type
            if ct == response.default_content_type:
                response.content_type = 'application/json'
        default = self._make_default(request)
        return self.serializer(value, default=default, **self.kw)

@mmerickel
Copy link
Member

I think to preserve bw-compat the easiest way is to make a custom DEFAULT_CONTENT_TYPE object inheriting from str that we can do an is comparison on.

@marplatense
Copy link

I believe I am experiencing a related issue: I am coding my own conditional response (based on etags) and whenever I do:

raise exception_response(result)

my webob test suite complains:

  File "/home/myproject/env/local/lib/python2.7/site-packages/webtest/lint.py", line 483, in check_content_type
    "which must not return content.") % code)
AssertionError: Content-Type header found in a 304 response, which must not return content.

I am still debugging it though.

@mmerickel
Copy link
Member

This issue only pertains to how renderers affect responses. Your example has nothing to do with that unless you describe further the problem. A 304 repsonse should not be going through a renderer at all.

@mmerickel mmerickel modified the milestone: 1.6 Feb 6, 2015
@mmerickel mmerickel removed the 1.6 label Feb 6, 2015
@mmerickel
Copy link
Member

I attempted a fix here, but webob does its own mangling of the string enough that it's not possible to keep the same object throughout. We need a different approach.

https://github.com/Pylons/pyramid/compare/fix.default-content-type

@digitalresistor
Copy link
Member

My fix is here: #1565

It works, but I just feel that there has to be a better way.

@mmerickel
Copy link
Member

Some discussion with @mcdonc came up with a possible fix for this. It basically involves adding a response.content_type_changed property to the response object. Upon access of the property it would compare to some initial value in the response.__init__ to determine if the value had changed. I haven't thought it through further than this but adding the property allows us to internalize the logic in the response class making things simpler.

This is obviously what you've started with implicit_content_type but we should be able to fix it to do the computation later on the getter to account for someone doing response.headers['content-type'] = ....

@digitalresistor
Copy link
Member

What if someone uses response.headers['content-type'] = 'text/html', now your check in the getter can't know that it was changed and response.content_type_changed will remain false.

@digitalresistor
Copy link
Member

This should get punted to WebOb instead.

@mcdonc
Copy link
Member

mcdonc commented Apr 13, 2015

Agree with @bertjwregeer (although once we change WebOb we'll probably need to change Pyramid to depend on that new version and might need to change Pyramid too).

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

Successfully merging a pull request may close this issue.

7 participants