#1966 Destroy should respect optimistic locking - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#1966 ✓committed

Destroy should respect optimistic locking

Reported by Curtis Hawthorne | February 14th, 2009 @ 03:37 AM | in 2.3.6

Optimistic locking doesn't currently affect #destroy calls. This is causing me problems because my after_destroy handlers can end up using stale data. I could also see it causing trouble if a model had some sort of 'do_not_delete' flag that was changed right after AR loaded the record.

So, it seems to me that #destroy should follow the same optimistic locking logic as updates do. That is, add the current lock version to the WHERE clause and raise a StaleObjectError if the delete fails.

I've written a patch (with a test and updated documentation) that accomplishes this.

optimistic-destroy.diff 3.3 KB

Comments and changes to this ticket

You flagged this item as spam.
ronin-21181 (at lighthouseapp) February 14th, 2009 @ 04:27 AM
I think it's a solid patch. I'm surprised it wasn't there before. The logic is straightforward, the tests and docs are clear. I recommend it :-)
Pratik March 9th, 2009 @ 02:48 PM
- Assigned user set to “Pratik”
- Title changed from “[patch] Destroy should respect optimistic locking” to “Destroy should respect optimistic locking”
Repository March 9th, 2009 @ 02:59 PM
- State changed from “new” to “resolved”
(from [0d922885fb54c19f04680482f024452859218910]) Ensure Model#destroy respects optimistic locking [#1966 state:resolved]

Signed-off-by: Pratik Naik pratiknaik@gmail.com http://github.com/rails/rails/co...
Jeremy Kemper August 17th, 2009 @ 07:44 PM
- State changed from “resolved” to “open”
- Milestone changed from “2.x” to “2.3.4”
This is giving me pain in conjunction with :dependent => :destroy and counter caching. parent.destroy will always fail if it has a counter cache any dependent objects.

When a dependent child object is destroyed, its parent's counter cache is decremented AND its lock version is updated. Then the parent destroy is attempted, but the lock version no longer matches.
Jeremy Kemper August 17th, 2009 @ 07:49 PM
What we really want here is to check the lock_version but with READ_COMMITTED transaction isolation. We don't care if the current transaction has bumped the lock, only if others have.
Jeremy Kemper August 17th, 2009 @ 11:17 PM
- Assigned user changed from “Pratik” to “Jeremy Kemper”
Jeremy Kemper September 11th, 2009 @ 11:04 PM
- Milestone changed from “2.3.4” to “2.3.6”
[milestone:id#50064 bulk edit command]
Repository November 17th, 2009 @ 11:31 PM
(from [ed320cd8968bf67f4af981a5727ff0dce3ee1025]) Revert "Ensure Model#destroy respects optimistic locking"

Unresolved issues with :dependent => :destroy and counter caching.

[#1966 state:open]

This reverts commit 0d922885fb54c19f04680482f024452859218910.
http://github.com/rails/rails/commit/ed320cd8968bf67f4af981a5727ff0...
Repository November 17th, 2009 @ 11:37 PM
(from [fb61fbd35229154f4ced124568697878822336cb]) Revert "Ensure Model#destroy respects optimistic locking"

[#1966 state:open]

This reverts commit 0d922885fb54c19f04680482f024452859218910.

Conflicts:
```
activerecord/lib/active_record/locking/optimistic.rb
```
http://github.com/rails/rails/commit/fb61fbd35229154f4ced1245686978...
You flagged this item as spam.
Curtis Hawthorne February 1st, 2010 @ 12:31 PM
How about just not updating the counters during a destroy? If the parent is going to be destroyed anyway, it doesn't seem like it does much good to the counter of its children as each individual one is destroyed. That should make the destroy go a bit faster and prevent the stale object exceptions.

I've updated my patch to have the parent redefine the before_destroy counter_cache callback on its children to just an empty method before destroying them (and it should apply cleanly against master now).

Let me know what you think.
- optimistic-destroy.diff 5.9 KB
You flagged this item as spam.
Curtis Hawthorne February 11th, 2010 @ 06:26 PM
Updated the patch to include better error messages as suggested by schorsch: http://github.com/rails/rails/commit/0d922885fb54c19f04680482f02445...

Also, I didn't mention it in the last post, but this patch does include unit tests to make sure that the parent.destroy scenario works without any problems.
- optimistic-destroy3.diff 6.2 KB
You flagged this item as spam.
Curtis Hawthorne February 22nd, 2010 @ 03:53 AM
Just realized that I hadn't correctly formatted the patch...
- optimistic-destroy4.diff 6.9 KB
You flagged this item as spam.
Paul Barry April 28th, 2010 @ 01:33 AM
+1, I tested Curtis's optimistic-destroy4.diff against the latest master, works great. This is also an important bug to fix, IMHO.
Repository April 28th, 2010 @ 05:56 AM
- State changed from “open” to “committed”
(from [ce5af2fefe0e447669fa8b7031f07558bfc84f4a]) Destroy respects optimistic locking.

Now works with :dependent => :destroy and includes unit tests for that
case. Also includes better error messages when updating/deleting stale
objects.

[#1966 state:committed]

Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/ce5af2fefe0e447669fa8b7031f075...
Repository April 28th, 2010 @ 05:56 AM
(from [7e06494e32f944f8c99d7d21e17224509332ee6b]) Destroy respects optimistic locking.

Now works with :dependent => :destroy and includes unit tests for that
case. Also includes better error messages when updating/deleting stale
objects.

[#1966 state:committed]

Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/7e06494e32f944f8c99d7d21e17224...
You flagged this item as spam.
bingbing March 26th, 2011 @ 01:30 AM
gucci watches