This project is archived and is in readonly mode.
Destroy should respect optimistic locking
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.31 KB
-
ronin-21181 (at lighthouseapp)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- Assigned user set to Pratik
- Title changed from [patch] Destroy should respect optimistic locking to Destroy should respect optimistic locking
-
Repository
- State changed from new to resolved
(from [0d922885fb54c19f04680482f024452859218910]) Ensure Model#destroy respects optimistic locking [#1966 Destroy should respect optimistic locking state:resolved]
Signed-off-by: Pratik Naik pratiknaik@gmail.com http://github.com/rails/rails/co...
-
Jeremy Kemper- 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.destroywill 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
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
- Assigned user changed from Pratik to Jeremy Kemper
-
-
Repository
(from [ed320cd8968bf67f4af981a5727ff0dce3ee1025]) Revert "Ensure Model#destroy respects optimistic locking"
Unresolved issues with :dependent => :destroy and counter caching.
[#1966 Destroy should respect optimistic locking state:open]
This reverts commit 0d922885fb54c19f04680482f024452859218910.
http://github.com/rails/rails/commit/ed320cd8968bf67f4af981a5727ff0... -
Repository
(from [fb61fbd35229154f4ced124568697878822336cb]) Revert "Ensure Model#destroy respects optimistic locking"
[#1966 Destroy should respect optimistic locking state:open]
This reverts commit 0d922885fb54c19f04680482f024452859218910.
Conflicts:
activerecord/lib/active_record/locking/optimistic.rbhttp://github.com/rails/rails/commit/fb61fbd35229154f4ced1245686978...
-
Curtis HawthorneHow 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.86 KB
-
-
Curtis Hawthorne
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
-
-
Curtis Hawthorne
Just realized that I hadn't correctly formatted the patch...
-
optimistic-destroy4.diff
6.86 KB
-
-
Paul Barry
+1, I tested Curtis's optimistic-destroy4.diff against the latest master, works great. This is also an important bug to fix, IMHO.
-
Repository
- 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 Destroy should respect optimistic locking state:committed]
Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/ce5af2fefe0e447669fa8b7031f075... -
Repository
(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 Destroy should respect optimistic locking state:committed]
Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/7e06494e32f944f8c99d7d21e17224... -