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

activerecord/CHANGELOG

@@ -1,5 +1,10 @@
 *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]

activerecord/lib/active_record/associations/has_many_through_association.rb

@@ -79,12 +79,6 @@ def save_through_record(record)
           @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 @@ def delete_records(records, method)
           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)

activerecord/test/cases/associations/has_many_through_associations_test.rb

@@ -77,6 +77,21 @@ def test_associate_existing_record_twice_should_add_records_twice
     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