:dependent => :destroy may delete the wrong objects. #2300

Closed
b4mboo opened this Issue Jul 27, 2011 · 16 comments

Comments

Projects
None yet
9 participants
@b4mboo

b4mboo commented Jul 27, 2011

Whenever you define two classes like in the following example ...

class User < ActiveRecord::Base
  has_many :things, :dependent => :destroy
end

class Thing < ActiveRecord::Base
  belongs_to :user
end

... there is a possibility to get the wrong objects destroyed if you do something like this ...

# We need two different users.
User.all.size > 1 # => true
user_a = User.first
user_b = User.last

# Both users should have some things.
not (user_a.things.empty? or user_b.things.empty?) # => true

# Now for the actual problem:
problematic_thing = user_a.things.last
problematic_thing.update_attributes!(:user => user_b)
user_b.things.reload.include?(problematic_thing) # => true
user_a.destroy

# The problematic_thing will be destroyed.
user_b.things.reload.include?(problematic_thing) # => false

So by destroying user_a we destroyed one of user_b's things. That doesn't seem fair.

One possible solution to this problem would be to add an additional reload to activerecord/lib/active_record/transactions.rb:236 in 'destroy'. However I'm not sure this is the most elegant way to do this.

If I find the time, I'll write a test and a patch for the fix. I'd create a pull request of it.

@arunagw

This comment has been minimized.

Show comment
Hide comment
@arunagw

arunagw Dec 5, 2011

Member

@b4mboo Just wondering.. id this still a issue ??

Member

arunagw commented Dec 5, 2011

@b4mboo Just wondering.. id this still a issue ??

@b4mboo

This comment has been minimized.

Show comment
Hide comment
@b4mboo

b4mboo Dec 5, 2011

Yes it is. You still need a manual reload before destroying user_a to be sure you don't delete the problematic_thing.

b4mboo commented Dec 5, 2011

Yes it is. You still need a manual reload before destroying user_a to be sure you don't delete the problematic_thing.

@isaacsanders

This comment has been minimized.

Show comment
Hide comment
@isaacsanders

isaacsanders May 1, 2012

Contributor

The issue seems to be because problematic_thing is just a pointer to the Thing that is related to user_a. Would you all agree? If so, is this really a case where we need to take further action?

Contributor

isaacsanders commented May 1, 2012

The issue seems to be because problematic_thing is just a pointer to the Thing that is related to user_a. Would you all agree? If so, is this really a case where we need to take further action?

@b4mboo

This comment has been minimized.

Show comment
Hide comment
@b4mboo

b4mboo May 2, 2012

problematic_thing's attributes say it's no longer related to user_a. user_b has a valid (and persisted) association to problematic_thing. The only player with old references is user_a. One should think he no longer should be allowed to destroy problematic_thing, once user_a is destroyed.

Therefore I strongly disagree with @isaacsanders: It is not "just a pointer to the Thing that is related to user_a". user_a doesn't have a Thing anymore (poor guy g).

b4mboo commented May 2, 2012

problematic_thing's attributes say it's no longer related to user_a. user_b has a valid (and persisted) association to problematic_thing. The only player with old references is user_a. One should think he no longer should be allowed to destroy problematic_thing, once user_a is destroyed.

Therefore I strongly disagree with @isaacsanders: It is not "just a pointer to the Thing that is related to user_a". user_a doesn't have a Thing anymore (poor guy g).

@isaacsanders

This comment has been minimized.

Show comment
Hide comment
@isaacsanders

isaacsanders May 3, 2012

Contributor

I did not mean to offend you, if I did. The tone of your response seemed as such.

I am just looking to get issues resolved. You said that you can avoid the issue be reloading.

Also, you don't break the association that user_a has to problematic_thing by assigning problematic_thing to user_b. user_a still "believes" that it is associated, so destroy is working correctly for the object you called it on. Try removing problematic_thing from user_a before destroying it.

Contributor

isaacsanders commented May 3, 2012

I did not mean to offend you, if I did. The tone of your response seemed as such.

I am just looking to get issues resolved. You said that you can avoid the issue be reloading.

Also, you don't break the association that user_a has to problematic_thing by assigning problematic_thing to user_b. user_a still "believes" that it is associated, so destroy is working correctly for the object you called it on. Try removing problematic_thing from user_a before destroying it.

@b4mboo

This comment has been minimized.

Show comment
Hide comment
@b4mboo

b4mboo May 3, 2012

You did not. So don't worry. ;-)

The thing that worries me about this issue, is the fact that the associations are persisted. Since destroy is such a dangerous method, I think it would be nice to play it save and check whether the problematic_thing is still associated to that record and whether it really should be destroyed.

b4mboo commented May 3, 2012

You did not. So don't worry. ;-)

The thing that worries me about this issue, is the fact that the associations are persisted. Since destroy is such a dangerous method, I think it would be nice to play it save and check whether the problematic_thing is still associated to that record and whether it really should be destroyed.

@frodsan

This comment has been minimized.

Show comment
Hide comment
@frodsan

frodsan Sep 11, 2012

Contributor

This issue is still present in master:

>> Thing.count
   (0.7ms)  SELECT COUNT(*) FROM "things" 
=> 2
>> issue = user_a.things.last
  Thing Load (2.0ms)  SELECT "things".* FROM "things" WHERE "things"."user_id" = $1 ORDER BY "things"."id" DESC LIMIT 1  [["user_id", 7]]
=> #<Thing id: 3, user_id: 7, created_at: "2012-09-11 17:53:38", updated_at: "2012-09-11 17:53:38">
>> issue.update_attributes! user: user_b
   (0.4ms)  BEGIN
   (15.3ms)  UPDATE "things" SET "user_id" = 8, "updated_at" = '2012-09-11 17:54:43.596469' WHERE "things"."id" = 3
   (1.3ms)  COMMIT
=> true
>> user_a.destroy
   (0.3ms)  BEGIN
  Thing Load (0.9ms)  SELECT "things".* FROM "things" WHERE "things"."user_id" = $1  [["user_id", 7]]
  SQL (0.8ms)  DELETE FROM "things" WHERE "things"."id" = $1  [["id", 3]]
  SQL (0.4ms)  DELETE FROM "users" WHERE "users"."id" = $1  [["id", 7]]
   (2.4ms)  COMMIT
=> #<User id: 7, name: nil, created_at: "2012-09-11 17:53:35", updated_at: "2012-09-11 17:53:35">
>> user_b.things.reload.include? issue
  Thing Load (0.7ms)  SELECT "things".* FROM "things" WHERE "things"."user_id" = $1  [["user_id", 8]]
=> false
Contributor

frodsan commented Sep 11, 2012

This issue is still present in master:

>> Thing.count
   (0.7ms)  SELECT COUNT(*) FROM "things" 
=> 2
>> issue = user_a.things.last
  Thing Load (2.0ms)  SELECT "things".* FROM "things" WHERE "things"."user_id" = $1 ORDER BY "things"."id" DESC LIMIT 1  [["user_id", 7]]
=> #<Thing id: 3, user_id: 7, created_at: "2012-09-11 17:53:38", updated_at: "2012-09-11 17:53:38">
>> issue.update_attributes! user: user_b
   (0.4ms)  BEGIN
   (15.3ms)  UPDATE "things" SET "user_id" = 8, "updated_at" = '2012-09-11 17:54:43.596469' WHERE "things"."id" = 3
   (1.3ms)  COMMIT
=> true
>> user_a.destroy
   (0.3ms)  BEGIN
  Thing Load (0.9ms)  SELECT "things".* FROM "things" WHERE "things"."user_id" = $1  [["user_id", 7]]
  SQL (0.8ms)  DELETE FROM "things" WHERE "things"."id" = $1  [["id", 3]]
  SQL (0.4ms)  DELETE FROM "users" WHERE "users"."id" = $1  [["id", 7]]
   (2.4ms)  COMMIT
=> #<User id: 7, name: nil, created_at: "2012-09-11 17:53:35", updated_at: "2012-09-11 17:53:35">
>> user_b.things.reload.include? issue
  Thing Load (0.7ms)  SELECT "things".* FROM "things" WHERE "things"."user_id" = $1  [["user_id", 8]]
=> false
@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Feb 22, 2013

Member

This issue is now over 2 years old. One might think that the subject of destroying records is critical but following the discussion it seems this is seldom a problem in real applications. (Otherwise there would be people jumping into the discussion all the time).

The only discussed solution so far, adding a reload before effectively destroying the record, would mean a performance hit for everyone. Since adding reload before calling destroy is also the suggested workaround I would favor the workaround if you encounter situation where you might be affected by this problem over adding the performance hit for everyone. This is just my opinion tough.

@carlosantoniodasilva @rafaelfranca let's decide how to proceed so we can slay this ancient dragon.

Member

senny commented Feb 22, 2013

This issue is now over 2 years old. One might think that the subject of destroying records is critical but following the discussion it seems this is seldom a problem in real applications. (Otherwise there would be people jumping into the discussion all the time).

The only discussed solution so far, adding a reload before effectively destroying the record, would mean a performance hit for everyone. Since adding reload before calling destroy is also the suggested workaround I would favor the workaround if you encounter situation where you might be affected by this problem over adding the performance hit for everyone. This is just my opinion tough.

@carlosantoniodasilva @rafaelfranca let's decide how to proceed so we can slay this ancient dragon.

@neerajdotname

This comment has been minimized.

Show comment
Hide comment
@neerajdotname

neerajdotname Mar 12, 2013

Member

I would agree with @senny here .

It's not easy to fix this without taking some performance hit somewhere. Since the target is already loaded destroy will also delete the record which does not belong it anymore.

Member

neerajdotname commented Mar 12, 2013

I would agree with @senny here .

It's not easy to fix this without taking some performance hit somewhere. Since the target is already loaded destroy will also delete the record which does not belong it anymore.

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Mar 12, 2013

Member

@b4mboo we talked about the issue and adding a reload is not a sensible way to solve this issue. The current code works as expected and the issue described in this ticket is something, which is hard to prevent.

I'm closing the issue for now but if you have a good idea how to solve it without reload please open a pull request and ping me!

Member

senny commented Mar 12, 2013

@b4mboo we talked about the issue and adding a reload is not a sensible way to solve this issue. The current code works as expected and the issue described in this ticket is something, which is hard to prevent.

I'm closing the issue for now but if you have a good idea how to solve it without reload please open a pull request and ping me!

@senny senny closed this Mar 12, 2013

@primalr

This comment has been minimized.

Show comment
Hide comment
@primalr

primalr Apr 5, 2013

Hello guys! I just spent 5-6 hours battling weird issues and realized it was this one... Please don't close it - right now I'm making my code very un-dry just to work around it... and I shudder at the thought of how many weird bugs I have in my codebase that I don't know of...

I have no idea how this works under the hood, but seeing that it's some association cache or model cache that causes the problem -- it should be fixable by introducing another flag in the cache.

Here are my suggestions for fixes:
When problematic_thing gets it's association updated, maybe you could either:
a) flag the cache for problematic_thing to with a "reload yourself if you wanna :dependent => :destroy me!"-flag .. then if any other record tries to destroy it, it will reload itself when needed
b) flag the old association as needing reload before deletion (just in the cache), so it no longer

Or:
If you can't hook something up to the association change in a good way, perhaps we can use, or introduce, timestamps for the cache.
I.e, when destroying user_a, user_a is in the cache. Most probable, problematic_thing will also be in the cache in cases where this is a problem. If the problematic_thing cache is younger than user_a cache, it is very probable that it can be a problem to :dependent_destroy user a. (because the most probable thing you've done to problematic_thing is to change it's association away from user_a).

primalr commented Apr 5, 2013

Hello guys! I just spent 5-6 hours battling weird issues and realized it was this one... Please don't close it - right now I'm making my code very un-dry just to work around it... and I shudder at the thought of how many weird bugs I have in my codebase that I don't know of...

I have no idea how this works under the hood, but seeing that it's some association cache or model cache that causes the problem -- it should be fixable by introducing another flag in the cache.

Here are my suggestions for fixes:
When problematic_thing gets it's association updated, maybe you could either:
a) flag the cache for problematic_thing to with a "reload yourself if you wanna :dependent => :destroy me!"-flag .. then if any other record tries to destroy it, it will reload itself when needed
b) flag the old association as needing reload before deletion (just in the cache), so it no longer

Or:
If you can't hook something up to the association change in a good way, perhaps we can use, or introduce, timestamps for the cache.
I.e, when destroying user_a, user_a is in the cache. Most probable, problematic_thing will also be in the cache in cases where this is a problem. If the problematic_thing cache is younger than user_a cache, it is very probable that it can be a problem to :dependent_destroy user a. (because the most probable thing you've done to problematic_thing is to change it's association away from user_a).

@jerefrer

This comment has been minimized.

Show comment
Hide comment
@jerefrer

jerefrer Oct 5, 2016

I just ran into this too.

I definitely think that not auto-reloading before processing dependent: destroy leads to weird behavior.

My case:

event_guest.tickets.count # => 2
migrate_tickets_to_user(event_guest, user)
user.tickets # => 2
event_guest.tickets # => 2 # Should be 0. Already kind of problematic but hey ...
event_guest.destroy
Ticket.count # => 0. Should be 2.

You can see here that the tickets migrated from event_guest to user have been destroyed when calling event_guest.destroy even though they don't belong to event_guest any more. This doesn't happen when calling event_guest.reload.destroy.

I always favor readability and logic before performance, and in this case it did not feel natural to have to manually reload. Proof is I did not think I would have to, and had to find out the hard way.
Would that really be such a performance hit to call reload before dependent: destroy ?

jerefrer commented Oct 5, 2016

I just ran into this too.

I definitely think that not auto-reloading before processing dependent: destroy leads to weird behavior.

My case:

event_guest.tickets.count # => 2
migrate_tickets_to_user(event_guest, user)
user.tickets # => 2
event_guest.tickets # => 2 # Should be 0. Already kind of problematic but hey ...
event_guest.destroy
Ticket.count # => 0. Should be 2.

You can see here that the tickets migrated from event_guest to user have been destroyed when calling event_guest.destroy even though they don't belong to event_guest any more. This doesn't happen when calling event_guest.reload.destroy.

I always favor readability and logic before performance, and in this case it did not feel natural to have to manually reload. Proof is I did not think I would have to, and had to find out the hard way.
Would that really be such a performance hit to call reload before dependent: destroy ?

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Oct 20, 2016

Member

@rafaelfranca any thoughts on this one?

Member

senny commented Oct 20, 2016

@rafaelfranca any thoughts on this one?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Oct 20, 2016

Member

I think calling reload is job of the migrate_tickets_to_user method. It changed in place the arguments so it should ensure that the state left after its job is done is not a leaky state. I don't think Rails should handle this automatically.

Member

rafaelfranca commented Oct 20, 2016

I think calling reload is job of the migrate_tickets_to_user method. It changed in place the arguments so it should ensure that the state left after its job is done is not a leaky state. I don't think Rails should handle this automatically.

@jerefrer

This comment has been minimized.

Show comment
Hide comment
@jerefrer

jerefrer Oct 20, 2016

@rafaelfranca I think you've got a point, but I'm afraid my poorly designed code may have misled us out of the actual issue.

Also I re-read the whole thread and have to say that the bug doesn't seem as shocking as it was to me 16 days ago. But still, I think that getting rid of it would save some headache to future developers. I'm not sure how the fact not much people end up here would prove it not to be often encountered. Not everyone might end up finding this issue after all. Moreover, it may be causing damage to some databases without the people even noticing.

Have you given @primalr's ideas some thoughts ? They seem plausible solutions to me, but I don't know enough about the ActiveRecord code base to judge how hard they would be to implement.

@rafaelfranca I think you've got a point, but I'm afraid my poorly designed code may have misled us out of the actual issue.

Also I re-read the whole thread and have to say that the bug doesn't seem as shocking as it was to me 16 days ago. But still, I think that getting rid of it would save some headache to future developers. I'm not sure how the fact not much people end up here would prove it not to be often encountered. Not everyone might end up finding this issue after all. Moreover, it may be causing damage to some databases without the people even noticing.

Have you given @primalr's ideas some thoughts ? They seem plausible solutions to me, but I don't know enough about the ActiveRecord code base to judge how hard they would be to implement.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Oct 20, 2016

Member

Yes, I have. I don't see any way to detect this case.

Member

rafaelfranca commented Oct 20, 2016

Yes, I have. I don't see any way to detect this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment