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

Allow JSON formatter to return a blank body #618

Closed
wants to merge 3 commits into from

Conversation

mbleigh
Copy link
Contributor

@mbleigh mbleigh commented Apr 6, 2014

I think most users expect that body '' or return nil will end up with a blank response, but the current JSON formatter put it as "" instead. This fixes that.

Made it a pull because I want to make sure no one is depending on it returning a blank string. Technically JSON is only valid if it's an Object or Array, but there may be a use case I'm not thinking of.

@dm1try
Copy link
Member

dm1try commented Apr 6, 2014

Seems like a blank body technically also is not a JSON :) Nevertheless it's more preferable for situation wherebody '' or return nil.

@mbleigh
Copy link
Contributor Author

mbleigh commented Apr 6, 2014

Yeah, I considered also making the Content-Type: text/plain when this is the case automatically, but it seems like overkill.

@dblock
Copy link
Member

dblock commented Apr 6, 2014

I think this semantically incorrect, whatever we think people expect. Today Grape will serialize and deserialize the values correctly. If you change it above you won't be able to anymore.

2.0.0-p353 :021 > MultiJson.load('null')
 => nil 
2.0.0-p353 :023 > MultiJson.load('""')
 => ""

Wit this change you would be returning an empty result, which isn't valid JSON, and you're not able to load it back.

2.0.0-p353 :024 > MultiJson.load('')
MultiJson::ParseError: JSON::ParserError

Changing content-type on the fly would be an even further stretch I think :)

@dblock
Copy link
Member

dblock commented Apr 6, 2014

To clarify what I am saying: today Grape is capable of returning both an empty string and a null. I am not saying the client should rely on that, but it's definitely by design and would be an API change.

@@ -10,6 +10,7 @@ Next Release

* [#614](https://github.com/intridea/grape/pull/614): Params with `nil` value are now refused by `RegexpValidator` - [@dm1try](https://github.com/dm1try).
* [#494](https://github.com/intridea/grape/issues/494): Fixed performance issue with requests carrying a large payload - [@dblock](https://github.com/dblock).
* [#618](https://github.com/intridea/grape/pull/618): When body is `nil` or `''`, make the response a blank body even for JSON - [@mbleigh](https://github.com/mbleigh)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing a period at the end :)

@mbleigh
Copy link
Contributor Author

mbleigh commented Apr 7, 2014

@dblock do you think it would be better to have an endpoint shortcut for a blank response?

I considered using body false as a way to specify that you don't want to return body content at all. Maybe a special method like blank_response or blank_body?

The major times this comes up for me are with 304 responses and DELETE requests.

@dblock
Copy link
Member

dblock commented Apr 7, 2014

Did you check whether raising an error! with a 304 is actually returning a body?

My 0.02c is that you shouldn't be returning an empty body for a DELETE, you should be returning the original object that has been deleted, but that's a preference.

I would definitely vote for something that lets you return an empty body very explicitly. Maybe just return an instance of Grape::Response::EmptyBody or something like that?

@mbleigh
Copy link
Contributor Author

mbleigh commented Apr 14, 2014

What do you think of a macro e.g. empty_body! that would set Content-Type to text/plain and body to ''

@dblock
Copy link
Member

dblock commented Apr 14, 2014

I think that would work.

@dblock
Copy link
Member

dblock commented Jul 6, 2014

Bump.

@rzane
Copy link

rzane commented Nov 24, 2014

Any progress on this? It would be really nice to have a Grape equivalent for Rails' head method.

@dblock
Copy link
Member

dblock commented Dec 16, 2014

I went with body false, which is just an alias for:

@body = nil
status 204

#850

@dblock dblock closed this Dec 16, 2014
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

Successfully merging this pull request may close these issues.

4 participants