Json-api: Add response for DELETE on resource that does not exist

[lukasoppermann]

So as far as I understand when I run DELETE /resource/1 and this resource does not exists, the correct response is a 204 because the result is the expected one and the same as if the resource would have existed. Is this correct?

If so, I would suggest to add this to the specs to make it explicit.

[ethanresnick]

I'm pretty sure the result should be a simple 404 (or 410). 204 implies success and the operation here definitely didn't succeed, even if the end condition might be the same as if it had. Moreover, a 404 also communicates that the resource doesn't exist, so it's not like you lose communicating that aspect of the current state.

Then, if you go with a 404, the body of the response would be an error object filled out appropriately.

[lukasoppermann]

Okay, but maybe this should than be added to the specs.

[ethanresnick]

I think this is currently covered by the spec's "Other Responses" section under Deleting Resources, which says that "A server MAY respond with other HTTP status codes...in accordance with HTTP semantics."

That said, a PR that either documents this case explicitly or includes an example error object for this case on the examples page could be useful. We don't want to list every single status in the main spec, as that would get really long and duplicative of the HTTP spec, but I could maybe be convinced that this case is mentioning directly...

[lukasoppermann]

Hey, actually, maybe this is not the right idea. I just found the following in the specs:

http://jsonapi.org/format/#crud-updating-relationship-responses-204

Note: This is the appropriate response to a POST request sent to a URL from a to-many relationship link when that relationship already exists. It is also the appropriate response to a DELETE request sent to a URL from a to-many relationship link when that relationship does not exist.

[ethanresnick]

Reopening so we can figure out what to do about that note. Ping @dgeb. For reference, that note was originally added in https://github.com/json-api/json-api/commit/90d898a0cc141738bce3e4f20a868d4f21751dff and, at the time, there was a question about whether it was appropriate for DELETE requests. It looks like there was a determination that it was, but I'm not sure what the reasoning was. A 404 still seems more appropriate to me

[bintoro]

The note addresses a situation where the item intended for deletion is not part of the related resources to begin with. A 404 there would imply a nonexistent relationship URL. This doesn't have to do with the case (originally) described by OP, where 404 is indeed the correct response.

[lukasoppermann]

To me 404 sounds fine as well, I just think there is a need to specify this in the specs.

Covering more cases (and I would not say this is an edge case) makes it easier to work with the specs.

[kocsismate]

As far as I know, DELETE methods should (or at least could) be designed as idempotent, but I believe the current proposal would prevent these efforts.

According to the RESTful Web Services Cookbook:

The DELETE method is idempotent. This implies that the server must return response code 200 (OK) even if the server deleted the resource in a previous request. But in practice, implementing DELETE as an idempotent operation requires the server to keep track of all deleted resources. Otherwise, it can return a 404 (Not Found).

That's why I wouldn't make the 404 response code compulsory for a DELETE request if a resource does not exist, including the following sentence (modified from @lukasoppermann 's PR) either in the recommendations or in the specs would be more appropriate in my opinion:

A server MAY return a 404 Not Found status code if a deletion request fails due to the resource not existing.

[lukasoppermann]

@kocsismate sounds good to me too, as long as there is a recommended guide. I will happily update if we all can agree upon this wording. 👍

[lukasoppermann]

Hey @ethanresnick I updated the PR #1029 as suggested by @kocsismate. What do you think, can this be merged? cc: @dgeb

[ethanresnick]

Hey @lukasoppermann, thanks for following up on this.

I think the note in the spec that you pointed out earlier is still confusing and, while I understand @bintoro's reading of it, I'm not sure that reading is obvious from the current text. Regardless, I don't think the ambiguity in that note should hold up your PR.

I'll review that now.

[ethanresnick]

Closing in favor of #1029