Ayende @ Rahien

It's a girl

Thou shall not delete

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);
}
Tags:

Posted By: Ayende Rahien

Published at

Originally posted at

Comments

Koen Verheyen
05/02/2012 09:19 AM by
Koen Verheyen

Happens in the best of families. Unit testing will save your day...

Daniel Lang
05/02/2012 09:23 AM by
Daniel Lang

Actually a good way to teach blogger about good habits and not deleting comments.

Guy
05/02/2012 09:29 AM by
Guy

Snobbish controller...

Moti
05/02/2012 09:35 AM by
Moti

Shouldn't you return status code 404 if the comment was not found?

Betty
05/02/2012 09:41 AM by
Betty

@Moti why? the end result is the same - the comment doesn't exist.

Mark
05/02/2012 09:52 AM by
Mark

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 :)

Moti
05/02/2012 10:04 AM by
Moti

@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.

Timothy Walters
05/02/2012 10:32 AM by
Timothy Walters

Delete the user? Ouch!

Remind me not to piss you off :P

Damien
05/02/2012 10:52 AM by
Damien

@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.

Khalid Abuhakmeh
05/02/2012 11:19 AM by
Khalid Abuhakmeh

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.

Moti
05/02/2012 11:25 AM by
Moti

@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.

Bjorn Coltof
05/02/2012 11:41 AM by
Bjorn Coltof

With one swift strike you were able to remove a potential bad commentor from your system, like RavenDB this is a self-optimizing system...

Charlie
05/02/2012 12:18 PM by
Charlie

@Bjorn -- Unless of course an admin is the one doing the deleting.

Rasmus Schultz
05/02/2012 12:33 PM by
Rasmus Schultz

I was expecting a post about why it's an entirely bad idea to delete things at all - oh well, maybe some other day ;-)

Bjorn Coltof
05/02/2012 12:43 PM by
Bjorn Coltof

@Charlie I agree it might be a bit aggressive...

Jesse
05/02/2012 01:00 PM by
Jesse

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.

Gene Hughson
05/02/2012 01:12 PM by
Gene Hughson

I'm surprised no one jumped on the three "return"s in one function.

Jokie
05/02/2012 01:13 PM by
Jokie

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.

Richard Dingwall
05/02/2012 01:21 PM by
Richard Dingwall

+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.

Jason Hanford-Smith
05/02/2012 01:59 PM by
Jason Hanford-Smith

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,

Chanan Braunstein
05/02/2012 02:08 PM by
Chanan Braunstein

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?

Jokie
05/02/2012 02:08 PM by
Jokie

@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.

Moti
05/02/2012 02:11 PM by
Moti

@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.

Jokie
05/02/2012 02:15 PM by
Jokie

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.

Jonty
05/02/2012 02:17 PM by
Jonty

@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

njy
05/02/2012 03:37 PM by
njy

RESTafarians wars, here they come :)

Torvin
05/02/2012 04:47 PM by
Torvin

@Moti I wonder, have you ever tried DELETE FROM on a relational DB?

njy
05/02/2012 05:20 PM by
njy

@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'

Moti
05/02/2012 06:18 PM by
Moti

@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

João P. Bragança
05/02/2012 06:50 PM by
João P. Bragança

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.

abm
05/02/2012 09:58 PM by
abm

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.

Alex Vilela
05/04/2012 08:31 AM by
Alex Vilela

Now include a user.Comments OnDeleteCascade and you got yourself an easy way of saving DB space...

Rajeesh C V
05/04/2012 10:42 AM by
Rajeesh C V

OMG!!!

luckily he didn't delete the blog itself.

Dathan
05/08/2012 08:28 PM by
Dathan

@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.

Comments have been closed on this topic.