This project is archived and is in readonly mode.
Association :dependent => delete_all + :foreign_key can result in data corruption
Reported by Chris Hapgood | August 21st, 2009 @ 04:34 PM | in 2.3.6
"Code Red, Mission Critical, The World is Coming to an End"
Well, maybe the world isn't coming to an end, but zombies are walking around in my database. If I define a has_many association with both the :foreign_key option and :dependent => :delete_all, deleting the object does not, in fact, delete any dependent records.
Given an association defined as follows:
class User
has_many :permissions, :primary_key => 'auth_token', :foreign_key => 'token', :dependent => :delete_all
end
Expected Behavior When I destroy an instance of
User
Then Permission instances are deleted where user.auth_token ==
permission.token
*Observed Behavior* When I destroy an instance of User
Then Permission instances are deleted where user.id ==
permissions.token
In addition to NOT deleting the expected records, this bug runs the real risk of deleting innocent unrelated records.
The bug has been observed in 2.3.1 and 2.3.3.
Comments and changes to this ticket
-
CancelProfileIsBroken September 25th, 2009 @ 12:48 PM
- Tag changed from “activerecord, association” to “activerecord, association, bugmash”
-
Chris Hapgood November 3rd, 2009 @ 11:52 PM
Commit a070873771ebede9097735f07fc40720dce89c46 seems relevant...
-
Matt Jones November 4th, 2009 @ 02:00 AM
- Tag changed from “activerecord, association, bugmash” to “activerecord, association, bugmash, patch”
Commit a07087377 actually unrelated; this appears to be caused by one line in configure_dependency_for_has_many that was calling quoted_id rather than the correct code to generate conditions for deleting records.
The attached patch (against 2-3-stable) includes a test for this behavior, and should also fix an identical issue with :dependent => :nullify (which uses the same condition code).
-
Chris Hapgood November 4th, 2009 @ 02:33 AM
+1 I reviewed and tested the patch. It tests the failure I describe. It applies cleanly to 2-3-stable (). The included test fails prior to the code change and passes afterward. The code change itself looks reasonable.
-
Matt Jones November 4th, 2009 @ 04:03 AM
Identical (apart from the counts in reflection_test) patch for master attached.
-
Matt Jones November 7th, 2009 @ 07:04 PM
- Milestone set to “2.3.6”
I knew there was a cleaner way to get a quoted owner id; it's stashed away in HasManyAssociation#owner_quoted_id. I've updated the 2.3 patch to reflect this; the intent is much clearer now.
Patches attached for master and 2-3-stable. Setting to 2.3.5, as there's no reason that a one-line patch that prevents blowing up random data shouldn't be applied.
-
Michael Koziarski November 10th, 2009 @ 05:09 AM
- State changed from “new” to “resolved”
applied locally, will arrive in master / 2-3-stable momentarily.
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>
Chris Hapgood
Guy Leaver
Michael Koziarski