Remove all revelant through records.

If a record is removed from a has_many :through, all of the join records
relating to that record should also be removed from the through
association's target.

(Previously the records were removed in the database, but only one was
removed from the in-memory target array.)

3 files changed, 29 insertions, 9 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index fac9ad1188..203b110e98 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -39,6 +39,11 @@
 
 *Rails 3.1.2 (unreleased)*
 
+* If a record is removed from a has_many :through, all of the join records relating to that
+  record should also be removed from the through association's target.
+
+  [Jon Leighton]
+
 * Fix adding multiple instances of the same record to a has_many :through. [GH #3425]
 
   [Jon Leighton]
diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb
index d82d041b69..6ba405ba4c 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -79,12 +79,6 @@ module ActiveRecord
           @through_records.delete(record.object_id)
         end
 
-        def through_record(record)
-          attributes = construct_join_attributes(record)
-          candidates = Array.wrap(through_association.target)
-          candidates.find { |c| c.attributes.slice(*attributes.keys) == attributes }
-        end
-
         def build_record(attributes, options = {})
           ensure_not_nested
 
@@ -145,14 +139,20 @@ module ActiveRecord
           update_counter(-count)
         end
 
+        def through_records_for(record)
+          attributes = construct_join_attributes(record)
+          candidates = Array.wrap(through_association.target)
+          candidates.find_all { |c| c.attributes.slice(*attributes.keys) == attributes }
+        end
+
         def delete_through_records(through, records)
           records.each do |record|
-            through_record = through_record(record)
+            through_records = through_records_for(record)
 
             if through_reflection.macro == :has_many
-              through.target.delete(through_record)
+              through_records.each { |r| through.target.delete(r) }
             else
-              through.target = nil if through.target == through_record
+              through.target = nil if through_records.include?(through.target)
             end
 
             @through_records.delete(record.object_id)
diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb
index 115e06144e..7a6aba6a6b 100644
--- a/activerecord/test/cases/associations/has_many_through_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -77,6 +77,21 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
     end
   end
 
+  def test_add_two_instance_and_then_deleting
+    post   = posts(:thinking)
+    person = people(:david)
+
+    post.people << person
+    post.people << person
+
+    counts = ['post.people.count', 'post.people.to_a.count', 'post.readers.count', 'post.readers.to_a.count']
+    assert_difference counts, -2 do
+      post.people.delete(person)
+    end
+
+    assert !post.people.reload.include?(person)
+  end
+
   def test_associating_new
     assert_queries(1) { posts(:thinking) }
     new_person = nil # so block binding catches it