Revert #1532 set default 204 for deletes

[JanStevens]

This MR: #1532 is plain wrong.

204 is only when there is NO CONTENT meaning I explicitly as a developer set body false. I now have to go through all my delete calls and explicitly set status 200, which makes no sense.

For example returning the deleted object is quite common, now we don't follow the spec because by default (even when there is body!) we return 204.

mind blown

[dblock]

@LeFnord @namusyaka ^^^ thoughts?

[dblock]

Also @urkle

[JanStevens]

Or at least we should evaluate if a body is returned and then return 200 else 204.

Less magic, more explicitness, happy devs 👍

[dblock]

@JanStevens you could help by PRing your suggestion on empty body, I would support that

[jthornec]

This just bit me too. We're using Grape with grape-roar something like this:

delete do
  model.destroy!
  present model, with: ModelPresenter
end

After updating Grape, our API DELETE endpoints started returning #<Model:0x00000000> instead of the expected { "model": "json" } because the new 204 status code is causing the formatter to be skipped.

This behaviour is a bit surprising, as I didn't immediately connect the new 204 No Content header to Grape skipping the formatter but still returning content.

@JanStevens if you don't have time to put a PR together in the next few days, I might be able to do it.

[JanStevens]

@jthornec That would be awesome pretty swamped with elixir atm

[dblock]

The formatter being skipped is a bigger deal than I thought, thanks for bringing it up @jthornec :(

[LeFnord]

sorry for the trouble :( … but @JanStevens, this change isn't wrong, it is only a modern interpretation of possibilities, means responding the deleted entity of a resource makes not much sense IMHO, cause the API consumer always has it … and we should reduce traffic 😉

I'm totally support your suggestion

Or at least we should evaluate if a body is returned and then return 200 else 204.
Less magic, more explicitness, happy devs 👍

and think this would be the best solution for all

btw, we should think about a re-ordering of the specs, and smaller, better specs, so such failures as the formatting stuff could be found much earlier :(

[JanStevens]

@LeFnord I still don't get why this change was needed, a body false at the end would result in exactly the same and the behaviour would be more explicit on your end. Everyone else wouldn't suddenly be surprised by the fact nothing is returned on delete (or even worse nothing is formatted).

this change isn't wrong, it is only a modern interpretation of possibilities, means responding the deleted entity of a resource makes not much sense IMHO, cause the API consumer always has it … and we should reduce traffic

Well on that point, if you have HATEOAS API endpoint then you will always return something. A 204 response is problematic because there are no links to follow. When hypermedia acts as the engine of application state, when there are no links, there's no state. In other words, a 204 response throws away all application state. Finally a good REST API would make it easy for the client to integrate, there are lot of valid reasons to have "modern" clients that still expect something back after a delete and use it to continue.

Anyways the specs should have catched the formatting issue, or at least the case of a delete + content returned should have been tested.

As for my suggestion, this is basically already implement, revert your changes and create a helper for return_no_content:

def return_no_content
  status 204
  body false
end

I really would shy away from the magic when nil is returns we return 204, might end in unsurprising results.

[jthornec]

Would it make more sense to choose the default status based on whether or not there's content, rather than the request type?

I don't think delete is special in this case, it's also possible to write a post endpoint that returns nothing. For example, the the HTTP/1.1 specification paragraph 9.5 says:

The action performed by the POST method might not result in a resource that can be identified by a URI. In this case, either 200 (OK) or 204 (No Content) is the appropriate response status, depending on whether or not the response includes an entity that describes the result.

[JanStevens]

@jthornec I was already commenting about that on your MR!

[jthornec]

Yeah... I was thinking about that right after I submitted it.

I think this is confusing because the HTTP codes themselves aren't super consistent: 200 vs 204 indicates content vs no content, but 200 vs 201 indicates existing entity vs new entity, which are (IMO) separate concerns. So we might choose 201 based on the request type (since a POST implies that we're creating something new), but I don't think we can use that logic for 204.

Maybe it should read something like:

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

But then, what order should those first two conditionals go in? If I complete a POST with no content, should that default to 201 or 204?

[JanStevens]

@jthornec hence my suggestion for a helper and leave out the black magic. It is really client and implementation specific, a post that returns nothing could as well be 204. Yet again I can think of reasons to keep it 201 (just for the sake of consistency).

[jthornec]

Hmm, yeah.

Googling around, it seems that using 204 for delete specifically is becoming a 'best practice', which (I suspect) is why it's implemented the way it is in #1532 and why it isn't applied to empty posts as well.

@dblock @LeFnord can you take a look at this? I have a PR above but I'm not sure how we should proceed.

[dblock]

Lets bring this conversation into #1550.

[dblock]

Closing via #1550.