Thou shall not delete
time to read 1 min | 145 words
Ouch!
public ActionResult DeleteComment(int id) { var userComment = RavenSession.Load<UserComment>(id); if (userComment == null) return new HttpStatusCodeResult(204); var user = RavenSession.GetUser(User.Identity.Name); if(user == null || (user.Role != UserRole.Moderator && user.Role != UserRole.Admin)) return new HttpStatusCodeResult(403, "You must be logged in as moderator or admin to be able to delete comments"); RavenSession.Delete(user); return new HttpStatusCodeResult(204); }
Comments
Happens in the best of families. Unit testing will save your day...
Actually a good way to teach blogger about good habits and not deleting comments.
Snobbish controller...
Shouldn't you return status code 404 if the comment was not found?
@Moti why? the end result is the same - the comment doesn't exist.
Took me a while to see what the problem was. A classic example of where a unit test is more effective than a code review.
I'd blame VS autocomplete though :)
@Betty,
Just to be inline with the http spec
204: The server has fulfilled the request but does not need to return an entity-body, and might want to return updated metainformation. (http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html)
IMHO, the server did not fulfill my request. I request a delete, the server could not delete since the content was not found, therefore, 404 is the correct status code to return.
Delete the user? Ouch!
Remind me not to piss you off :P
@Moti - you requested that item 123456 no longer exist. Item 123456 does not exist. The server has done sufficient work (none) to satisfy your request.
Interesting problem. You have GetUser hanging off of the session. Which returns a strongly typed User. If there was an equivalent DeleteUser or DeleteComment then you might have avoided the issue all together.
I guess the API is enough rope to hand yourself with. Risks of a generalized API I guess.
@Damian if you would tell me that in this case it's not important (or as oren puts it "YAGNI"), i might agree. But in terms of correctness, well, you are wrong.
As the client, when i sent a delete request to the server - If the server returns me the same result weather it actually did delete or not, it is behaving incorrectly.
For example, lets say i want to keep the top-10 published comments in-memory. Also, let's assume the moderator has a textbox where he types the comment number. Now, for every time the moderator types in a number and submits, i have to go and referesh the list of 10 last comments, since it may deleted a top-10 comment. Its true even if he just typed a random number.
Now ofcurse this is bad design and i wouldn't do that, but i'm just saying that it is an example where server not spitting out the real response for my request, causing my application to do more work.
With one swift strike you were able to remove a potential bad commentor from your system, like RavenDB this is a self-optimizing system...
@Bjorn -- Unless of course an admin is the one doing the deleting.
I was expecting a post about why it's an entirely bad idea to delete things at all - oh well, maybe some other day ;-)
@Charlie I agree it might be a bit aggressive...
Gotta disagree with Moti on this one. It's all the same to the caller at the end of the day. Whether it doesn't exist now, or whether it doesn't exist after the next few lines of code execute, the result is the same. Now, if we were requesting to view comment 12345 and it didn't exist, that's a different story.
I'm surprised no one jumped on the three "return"s in one function.
I agree with Moti, If I give a server a command I expect it to execute the command, and not only that, but any info the server communicates to me should be within the context of said command. Asking the server to delete, something is not the same as asking the server to ensure the said thing does not exist, one is more like update while the other is more like upsert, two different commands that should respond appropriately.
+1 Jesse. The user would have got a 404 if they tried to delete a non-existing comment, at the point when they tried to view it.
People seem to have missed that this is not going to remove a commenter but and Admin or Moderator. This would be a great way to lock out a site by spamming a thread with comments and then have all the admins deleted,
Out of curiosity,
I know that if I wanted to make a bundle that had a command called SafeDelete, for example, I can do that. But, is it possible to override the bulit-in Delete command so that any call to Delete would execute my bundle code and do whatever it is that a hypothetical SafeDelete would do instead of the normal Delete operation?
@Richard you are assuming that between viewing and the delete comand being issued, no other process may have deleted the said asset, that is not always the case.
@Richard
Did you read my last comment? I gave you a valid example where the user does not need to view something before deleting it.
Assuming to know (guess) every path the user get to your resources is a very bad assumption.
Another good example would be if this was an api. If the we have a client that is designed to log who does what and when, then our log would end up with multiple entries of different users having deleted the same asset at different times, which would be wrong.
@Moti, @Jesse, @Richard
HTTP DELETE should be idempotent. Therefore 204 is correct (assuming the response does not contain an entity).
http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.1.2
RESTafarians wars, here they come :)
@Moti I wonder, have you ever tried DELETE FROM on a relational DB?
@Torvin: using that parallel doesn't seems right to me. If you do a SELECT * FROM TABLE WHERE ID = 'meh' you don't get back an error, you get an empty result set. In http you get a 404.
Http and SQL are different worlds, just sayin'
@Jonty Take another look at the link you posted . Get is also idempotent. Does it mean i get the same status code every time i hit a resource? what if i delete the resource, should i still get 200? Idempotency is about the state of your system and about the side effects (it clearly says so on the link you provided), not about the status codes.
@Torvin Sure, care to link me the W3 specifications that conforms with this behavior? Also with SQL i can (for most parts) know the number of rows affected and work with that. And of curse, see @njy answer
As with all things REST you have a lot of options here. The concern here would be how do you want the client to react? If you want the client to display an error, 40x. otherwise, 20x. Either one is acceptable.
João has hit on the crucial issue. It really depends on how you want clients to respond to conditions. Is it an error or not? That depends on the business logic.
One could use a 200 to signal a deletion of an existing item and a successful message in its body content and a 204 to signal that the item didn't exist but the server processed the request normally and doesn't need to return any content.
But if one wants to be symmetrical and parsimonious, 204 will be perfectly adequate.
In the case of setting up sensible logging, one can easily arrange things so the first time the item is deleted it will be noted accordingly and any subsequent deletions of the same item is also noted as attempts on a nonexistent item.
Now include a user.Comments OnDeleteCascade and you got yourself an easy way of saving DB space...
OMG!!!
luckily he didn't delete the blog itself.
@Moti you should take another look at his link, as well. You're describing a heterogeneous sequence of otherwise idempotent operations that is not, in aggregate, idempotent. That's a situation that's specifically called out in the spec as being acceptable, but is NOT equivalent to a homogeneous sequence of operations (e.g., a bunch of GETs in series). Without any interleaved DELETE operations, you should get the same resource every time you issue a GET for it.
However, I agree with you that 404 is the correct reply if it doesn't exist. Consider the case where I issue a GET and receive a 200, then issue another GET and receive a 304. This is clearly an idempotent operation, since the definition of idempotence is that the "side effects" of a sequence of N calls is the same as the "side effects" from a single call, which is the case here. Similarly, issuing a DELETE and receiving a 204, then issuing the DELETE again and receiving a 404 is basically the same situation. The side effects of two calls (from the server's point of view) are the same -- the resource doesn't exist, but the semantics from the client perspective are richer. Though, 410 would arguably be the more appropriate response instead of 404.
Comment preview