Use 200 as default status for deletes that reply with content

[jthornec]

As per #1549, having deletes use 204 as the default status when a body is present causes some issues. Sending a 204 No Content with content will likely surprise some clients, and it causes the formatter to be skipped.

This PR adds a check for @body.present? so that we only default to 204 No Content when there's actually no content.

[JanStevens]

👍 On the MR!

I'm really thinking we should just use a helper that combines status + body false, like I suggested before in the discussion:

def return_no_content
  status 204
  body false
end

Why is this much better?

1. Explicit
2. No magic, maybe I want to return a 200 without any body, this will be converted to a 204 but I got a legacy client from 1989 that doesn't understand 204 (we all have our dragons :D)
3. We should have logical defaults and less magic, this really helps making Grape solid and consistent: This means we always return status 200/201(POST) unless set explicitly.

Also now we only set 204 on a delete when there is no body, this is just not consistent with other HTTP methods (why is delete so special?)

[dblock]

@jthornec Add a paragraph about this to UPGRADING please?

Consider the return_no_content improvement too, although we can take that as a subsequent PR from @JanStevens.

@LeFnord @namusyaka can you please pitch in here to make sure I don't merge something we don't want?

[dblock]

I like this change because it looks like it corrects 1 specific bug: if there's a body on DELETE, don't return 204, but 200. I can live with that.

Now, I do believe there're a lot of sensible arguments in #1549 around what to do "in general", notably the following does make sense to me too, but is maybe a bit more scary:

if request.request_method == Grape::Http::Headers::POST
  201
elsif @body.empty?
  204
else
  200
end

[JanStevens]

@dblock Again why the magic behaviour for a specific method + no body returned? Helper would be more than sufficient. I keep bitching about this since I strongly feel this is magical and quite unexpected behaviour (what if i want to delete return nothing but want status 200, legacy clients for example?)

I vote for helper only :D

[jthornec]

I've updated this PR to include the return_no_content helper and a section in UPGRADING.md explaining the changes.

[LeFnord]

thanks again … 👍

[dblock]

@JanStevens I hear you. If we hadn't shipped this change in a previous version I would probably just say "you're right" and leave things as is. @LeFnord @namusyaka I need some help deciding.

[namusyaka]

yes, l'll confirm this within a few hours.

[namusyaka]

First of all, we have to apologize that that changes in the case were insufficient.
Actually, the behavior that the formatter skipped, for example, wasn't what we meant.

Well, regarding this pull request, never this change is wrong.
Let's check rfc.

A successful response SHOULD be 200 (OK) if the response includes an entity describing the status, 202 (Accepted) if the action has not yet been enacted, or 204 (No Content) if the action has been enacted but the response does not include an entity.

https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.7

As written here, this changes is reasonable (but it's not complete..).

Also now we only set 204 on a delete when there is no body, this is just not consistent with other HTTP methods (why is delete so special?)

It's true that there is no consistency. However, we should commit considering changes at 0.19.0.
I can't say that this change certainly keeps consistent behavior in grape.
From developers who respected the last change, I think this change is the least damaging approach.
Also, The delete route can be said to be special because we made a mistake.

Therefore, I'd like to vote to both helper and this change.

[dblock]

Thanks @namusyaka, @LeFnord?

[LeFnord]

as I said here #1549 (comment), I see this as the best solution

Also now we only set 204 on a delete when there is no body, this is just not consistent with other HTTP methods (why is delete so special?)

good point, for that @dblock's suggestion for a general handling should be implemented

but this can also be done in a different PR IMHO

[dblock]

I am going to merge this. Lets let it sit for a bit on master. I'd like @JanStevens and @jthornec to at least confirm you're happy (enough) with this change.

[jthornec]

I'm happy with this change.

My issues were that the formatter wasn't running, and the 204 with content might confuse my client. This fixes both of those things, and all my app's tests pass now, so I'm good to move forward with the upgrade once this gets released.

[JanStevens]

Thanks @jthornec for the effort

[dblock]

I am going to cut a release today.

[LeFnord]

@dblock … 👍 cool, so I can run tests and cut new ones for grape-entity and grape-swagger

[dblock]

And done.