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.

delete_all doesn't work when chained with joins

#6555

Given a case when Pet.has_many :toys, the following delete_all fails:

Pet.joins(:toys).where(:toys => {:name => 'Bone'}).delete_all

bacause current Arel implementation ignores joins parameter and tries to execute the following SQL.

DELETE FROM "pets" WHERE "toys"."name" = 'Bone'

Attached is a patch that avoids this error by constructing a subquery only when delete_all is chained with joins.

Reported by Akira Matsuda · March 10th, 2011 @ 10:10 AM

State: open
Milestone: 3.1
Assigned to: Jeff Kreeftmeijer Jeff Kreeftmeijer
Importance: Low

Activity

  1. Santiago Pastorino
    Santiago Pastorino
    • State changed from new to open
    • Milestone set to 3.1
    • Importance changed from to Low

    March 10th, 2011 @ 06:14 PM

  2. Jeff Kreeftmeijer
    Jeff Kreeftmeijer
    • Assigned user set to Jeff Kreeftmeijer

    Akira's patch fixes the issue for me, but the attached test fails because Pet.joins(:toys).where(:toys => {:name => 'Bone'}).count returns 0.

    Also, I'm not a great fan of the formatting:

    def test_delete_all_with_joins
      assert (count = Pet.joins(:toys).where(:toys => {:name => 'Bone'}).count) > 0
    
      assert_equal count, Pet.joins(:toys).where(:toys => {:name => 'Bone'}).delete_all
    end
    

    It would probably be more readable (and understandable) like this:

    def test_delete_all_with_joins
      pets = Pet.joins(:toys).where(:toys => {:name => 'Bone'})
      count = pets.count
      
      assert count > 0
      assert_equal count, pets.delete_all
    end
    

    Akira, could you check out if the test fails for you too? What do you think about my code formatting suggestion? Please note I haven't tested the last code snippet, it's just to show how I'd like it to look :)

    March 12th, 2011 @ 12:02 PM

  3. Jeff Kreeftmeijer
    Jeff Kreeftmeijer
    • Tag changed from activerecord rails3, bug, delete_all, edge, patch, tested to activerecord, delete_all, patch

    March 12th, 2011 @ 12:03 PM