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

Why no 204? #15

Open
rikvanmechelen opened this issue Jun 2, 2014 · 12 comments · May be fixed by #41
Open

Why no 204? #15

rikvanmechelen opened this issue Jun 2, 2014 · 12 comments · May be fixed by #41

Comments

@rikvanmechelen
Copy link

I wondered why one should not return a 204: No content in response of a successful Patch or Delete request.

Since when you delete a resource, you eighter already have the full resource, or could easilly get it before deleting. The same goes for a patch request.

By doing this, the response can easily be a few kb lighter, which is always good, no?

@aronwoost
Copy link

👍

@rikvanmechelen
Copy link
Author

I could answer of why it might also not be good myself.

Sometimes you post a resource (e.g. an order) server side you calculate the vat etc..
you might want to return these calculated values to the client side, to ensure there are no errors on the client side calculations.

However, i still think that in a lot of the cases a 204 could be a good response.

@aronwoost
Copy link

Not sure if PATCH should return any data. DELETE certainly not.

@geemus
Copy link
Member

geemus commented Jun 5, 2014

@rikvanmechelen I think, as you indicated, that both options can certainly have pluses and minuses. We opted for the case that was more consistent overall, in the hopes that it makes it easier to learn and reason about if you can always assume you'll get the serialization back. It is extra data over the wire in some cases, but I think as a default it makes sense in order to make it more approachable. I could definitely see adding advanced options which could allow you to specify that you didn't care about the response as an optimization, but I think keeping it simple/consistent in this way is a good starting point.

@keithamus
Copy link
Contributor

I'm going to put my 👍 for 204 on a DELETE. An API should describe the resource you've given it at all times - one good reason for doing this is to help clients synchronise their models to the new dataset. A DELETE has no dataset, hence a 204 No Content is the correct status and description of the resource.

@geemus
Copy link
Member

geemus commented Jul 23, 2014

@keithamus I guess I would make a small distinction from your comment. I'd say that the API should describe the resource you are referring to (rather than what you've given). If you start from that, I think it seems reasonable that delete would also give you a copy of the state of things. I would agree that in many cases this info is not needed (ie the synchronize case should be pretty trivial on successful delete). That said, I suspect there are cases where getting the resource back would be useful (though I can't name one off hand) and this allows it to be more consistent with other actions. It is a bit of extra data over the wire, but in most systems I do not expect delete to be so frequent as to be the bottleneck. ie I can see how it might be useful and it is more consistent, with relatively little downside, so to me consistency/convenience wins the day. What do you think?

@keithamus
Copy link
Contributor

@geemus Maybe I was unclear. I believe an API should describe the current (aka new) state of the object which includes the operation. In other words the response is saying "That was fine, here's what the new representation of this looks like". I would expect that after a PATCH setting foo to true, the response back would have foo as true. Because of this, after a DELETE, I would expect to not get any data back, because no data exists - or "That was fine, here's what the new representation of this data looks like: nothing"

I'd be interested to see what upsides there are to including it back down the pipe. Anyone?

@geemus
Copy link
Member

geemus commented Jul 23, 2014

@keithamus thanks, that is clearer and (to me anyway) much more compelling. I hadn't really considered it exactly that way, for whatever reason. A lot of my motivation had been around trying to find a single rule that could be consistently applied (ie return the resource), but as you describe it I think there can still be a consistent rule, it just is "return the resulting resource". I had also thought there might be cases where you would want to know the last-state of something and this would avoid having to do GET+DELETE in succession, but having had that thought I was never successful in coming up with a concrete use case. So perhaps there is not one. What do you think? Seems like we are at least nearing the same page. How would you reword the recommendation?

@keithamus keithamus linked a pull request Jul 24, 2014 that will close this issue
@keithamus
Copy link
Contributor

@geemus I've got a PR over at #41 which is my attempt at rewording this.

@brandur
Copy link
Member

brandur commented Jul 24, 2014

Great discussion over here! Thanks for getting the ball rolling on it.

I'm -1 on this change. We added the 200 with a resource representation for a couple reasons:

  1. Consistency: There are no surprises with APIs built according to these guidelines. Every action on every resource returns the same thing (a representation of the resource), and clients can depend on that. I suppose the argument could be put forward that an empty body is technically a more accurate representation of a resource which has been deleted, but I think this might be the kind of philosophical point that's getting into subjective territory (is 204 better than an empty JSON object? what about one with all fields nulled?), and not strong enough to overcome the practical advantages of just returning the body.

  2. Practical utility: Having a resource representation here is useful for consumers. For example, if I'm designing a client that allows resources to be removed by an arbitrary ID, but where I want to present a consistent message back, having a body handy makes this easy and collapses the number of API calls that I want to make:

    heroku destroy -a 1234
    App "cryptic-brushlands-7772" has been destroyed.
    
    heroku destroy -a cryptic-brushlands-7772
    App "cryptic-brushlands-7772" has been destroyed.
    

These are the same principles that were used to guide the development of these guidelines overall: if a decision needs to be made between practical usefulness and academic RESTful correctness, take the former.

I'm also a fan of having a JSON API (especially one where you request it via the Accept header as you version) always return JSON. For example with the Heroku API, you can JSON parse any response, and it's always going to be safe to do so.

Lastly, I think there's value in preaching what we (Heroku) do here. It might be better not to tell other people to return 204's when we don't do it ourselves.

Once again, thanks for the contributions guys! Keep 'em coming.

@geemus
Copy link
Member

geemus commented Jul 24, 2014

@brandur - I also like consistency, but I think the argument for the resulting object (rather than the object when the action started) is pretty compelling. I guess this is one of the key points.

I agree with your messaging case, it may depend in part on how broadly applicable we think the friendly-id concept should be applied. We don't outline it in the guide (yet), and without that it makes this argument a bit harder to make. I think it could be pretty broadly applicable (uuids are not user friendly particularly). Maybe expanding on that would give us firmer footing here.

I also agree that I prefer usability to artificial measures of correctness.

JSON everywhere is commendable also. That said, we have a small selection of 204s in the Heroku API already which wouldn't be parseable (I think just around dyno actions maybe). Perhaps that is an exceptional enough case to deserve exceptions, but I'm wary of using sunk-cost as a point in favor or against (there are many things I'd love to change in our implementation).

Thoughts?

@calmdev
Copy link

calmdev commented Mar 11, 2015

With my projects I use deleted_at timestamps on all models. Permanently deleting a resource is a two step process. The initial delete actually updates a deleted_at timestamp on the model and returns the resource with it's deleted at time. Deleted models are then accessible in the collection's trash which returns all of models that have a deleted_at value. They are permanently deleted once a second delete request is performed on the trash.

From an end-user prospective it wouldn't make sense to send a PUT request to trigger the initial delete. This is a use-case of how returning JSON with delete can be useful, IMO.

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 a pull request may close this issue.

6 participants