Fix adding multiple instances of the same record to a has_many :through.

Fixes #3425.

3 files changed, 57 insertions, 21 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index 698a337dcc..fac9ad1188 100644
--- a/activerecord/CHANGELOG
+++ b/activerecord/CHANGELOG
@@ -39,6 +39,10 @@
 
 *Rails 3.1.2 (unreleased)*
 
+* Fix adding multiple instances of the same record to a has_many :through. [GH #3425]
+
+  [Jon Leighton]
+
 * Fix creating records in a through association with a polymorphic source type. [GH #3247]
 
   [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 2e818dca5d..d82d041b69 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -6,6 +6,11 @@ module ActiveRecord
     class HasManyThroughAssociation < HasManyAssociation #:nodoc:
       include ThroughAssociation
 
+      def initialize(owner, reflection)
+        super
+        @through_records = {}
+      end
+
       # Returns the size of the collection by executing a SELECT COUNT(*) query if the collection hasn't been
       # loaded and calling collection.size if it has. If it's more likely than not that the collection does
       # have a size larger than zero, and you need to fetch that collection afterwards, it'll take one fewer
@@ -42,27 +47,42 @@ module ActiveRecord
           end
         end
 
-        through_record(record).save!
+        save_through_record(record)
         update_counter(1)
         record
       end
 
       private
 
-        def through_record(record)
-          through_association = owner.association(through_reflection.name)
-          attributes = construct_join_attributes(record)
-
-          through_record = Array.wrap(through_association.target).find { |candidate|
-            candidate.attributes.slice(*attributes.keys) == attributes
-          }
+        def through_association
+          owner.association(through_reflection.name)
+        end
 
-          unless through_record
-            through_record = through_association.build(attributes)
+        # We temporarily cache through record that has been build, because if we build a
+        # through record in build_record and then subsequently call insert_record, then we
+        # want to use the exact same object.
+        #
+        # However, after insert_record has been called, we clear the cache entry because
+        # we want it to be possible to have multiple instances of the same record in an
+        # association
+        def build_through_record(record)
+          @through_records[record.object_id] ||= begin
+            through_record = through_association.build(construct_join_attributes(record))
             through_record.send("#{source_reflection.name}=", record)
+            through_record
           end
+        end
 
-          through_record
+        def save_through_record(record)
+          build_through_record(record).save!
+        ensure
+          @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 = {})
@@ -73,9 +93,9 @@ module ActiveRecord
           inverse = source_reflection.inverse_of
           if inverse
             if inverse.macro == :has_many
-              record.send(inverse.name) << through_record(record)
+              record.send(inverse.name) << build_through_record(record)
             elsif inverse.macro == :has_one
-              record.send("#{inverse.name}=", through_record(record))
+              record.send("#{inverse.name}=", build_through_record(record))
             end
           end
 
@@ -104,7 +124,7 @@ module ActiveRecord
         def delete_records(records, method)
           ensure_not_nested
 
-          through = owner.association(through_reflection.name)
+          through = through_association
           scope   = through.scoped.where(construct_join_attributes(*records))
 
           case method
@@ -126,14 +146,16 @@ module ActiveRecord
         end
 
         def delete_through_records(through, records)
-          if through_reflection.macro == :has_many
-            records.each do |record|
-              through.target.delete(through_record(record))
-            end
-          else
-            records.each do |record|
-              through.target = nil if through.target == through_record(record)
+          records.each do |record|
+            through_record = through_record(record)
+
+            if through_reflection.macro == :has_many
+              through.target.delete(through_record)
+            else
+              through.target = nil if through.target == through_record
             end
+
+            @through_records.delete(record.object_id)
           end
         end
 
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 2f4dd9e55c..115e06144e 100644
--- a/activerecord/test/cases/associations/has_many_through_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -67,6 +67,16 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
     end
   end
 
+  def test_associate_existing_record_twice_should_add_records_twice
+    post   = posts(:thinking)
+    person = people(:david)
+
+    assert_difference 'post.people.count', 2 do
+      post.people << person
+      post.people << person
+    end
+  end
+
   def test_associating_new
     assert_queries(1) { posts(:thinking) }
     new_person = nil # so block binding catches it