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

'json' renderer forces response body to be non-empty #72

Closed
alfredodeza opened this Issue Jul 11, 2016 · 8 comments

Comments

Projects
None yet
3 participants
@alfredodeza
Copy link
Contributor

alfredodeza commented Jul 11, 2016

The following test case fails:

class TestJsonRendering(PecanTestCase):

    def test_no_body_json(self):

        class RootController(object):
            @expose('json')
            def index(self):
                pass

        app = TestApp(Pecan(RootController()))

        r = app.get('/')
        assert r.status_int == 204
        assert r.body == ''

The actual r.status_int is 200, and the body is null.

There is no way to force a response to have an empty body when using the json renderer because it treats both the implicit (with no return) and explicit (returning an actual None) as valid input to render a json response.

In pecan.core it sees that there is an actual body in the response so it doesn't force the 204.

I don't know in what situations one would want to render a None to null, but I did had a valid use case where the controller might return json or no body at all, in which I was expecting a 204.

This might be a documentation fix and maybe some debug messages to alert of this.

@ryanpetrello

This comment has been minimized.

Copy link
Member

ryanpetrello commented Jul 11, 2016

My opinion is that this is actually a bug, and that a controller that returns None with an @expose('json') should be an HTTP 204 (as your test case suggests).

@alfredodeza

This comment has been minimized.

Copy link
Contributor

alfredodeza commented Jul 11, 2016

@ryanpetrello but you can't know for sure if the controller actually returned a None or if it just didn't return anything (which also, implicitly will get a None).

This for example is just plain impossible with the other renderers (e.g. with mako) because returning None (or nothing at all) would create a templating error even if the template has no variables to use.

@ryanpetrello

This comment has been minimized.

Copy link
Member

ryanpetrello commented Jul 12, 2016

This change seems to break a handful of OpenStack project tests that rely on the prior behavior (they're asserting that the response status is 200 OK).

@ryanpetrello

This comment has been minimized.

Copy link
Member

ryanpetrello commented Jul 12, 2016

@alfredodeza I don't think it matters if the None is explicitly returned. Currently if you implicitly return None, the JSON version just renders null (which I'd say is incorrect), and any template decorated controller throws a cryptic type-related error

@ryanpetrello

This comment has been minimized.

Copy link
Member

ryanpetrello commented Jul 12, 2016

I guess what I'm arguing for is this: if you have a controller that returns None, pecan should make the response an 204 No Content.

@alfredodeza

This comment has been minimized.

Copy link
Contributor

alfredodeza commented Jul 12, 2016

I am OK with that. It doesn't help the render to null which makes it valid. But if no other renderer allows for this, I vote +1 on normalization of the behavior.

@cleverdevil

This comment has been minimized.

Copy link
Member

cleverdevil commented Jul 12, 2016

204 No Content is pretty clearly intended for this purpose, so I agree with @ryanpetrello.

https://httpstatuses.com/204 states the reason for this status code is:

The server has successfully fulfilled the request and that there is no additional content to send in the response payload body.

The request is successful, and the body contains no content. Any project that is checking explicitly for a 200 OK here is relying on a bug in our implementation, and should instead be checking that the response is, in fact, successful (e.g. the response status is in the 2XX-series).

@alfredodeza

This comment has been minimized.

Copy link
Contributor

alfredodeza commented Jul 12, 2016

I agree with you both :)

However it is entirely possible that a controller method implementation wants to return a None and wants to have it rendered as null in the body.

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