Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

Association :dependent => delete_all + :foreign_key can result in data corruption

#3086

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

Reported by Chris Hapgood · August 21st, 2009 @ 04:34 PM

State: resolved
Milestone: 2.3.6
Assigned to: nobody
Importance: none

Activity

  1. CancelProfileIsBroken
    CancelProfileIsBroken
    • Tag changed from activerecord, association to activerecord, association, bugmash

    September 25th, 2009 @ 12:48 PM

  2. Chris Hapgood
    Chris Hapgood

    Commit a070873771ebede9097735f07fc40720dce89c46 seems relevant...

    November 3rd, 2009 @ 11:52 PM

  3. Matt Jones
    Matt Jones
    • 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).

    November 4th, 2009 @ 02:00 AM

  4. Chris Hapgood
    Chris Hapgood

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

    November 4th, 2009 @ 02:33 AM

  5. Matt Jones
    Matt Jones

    Identical (apart from the counts in reflection_test) patch for master attached.

    November 4th, 2009 @ 04:03 AM

  6. Matt Jones
    Matt Jones
    • 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.

    November 7th, 2009 @ 07:04 PM

  7. Michael Koziarski
    Michael Koziarski
    • State changed from new to resolved

    applied locally, will arrive in master / 2-3-stable momentarily.

    November 10th, 2009 @ 05:09 AM