When a has_many association is not :uniq, appending the same record multiple times should append it to the @target multiple times [#5964 state:resolved]

3 files changed, 24 insertions, 3 deletions

diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb
index c513e8ab08..7964f4fa2b 100644
--- a/activerecord/lib/active_record/associations/association_collection.rb
+++ b/activerecord/lib/active_record/associations/association_collection.rb
@@ -462,10 +462,10 @@ module ActiveRecord
           callback(:before_add, record)
           yield(record) if block_given?
           @target ||= [] unless loaded?
-          if index = @target.index(record)
+          if @reflection.options[:uniq] && index = @target.index(record)
             @target[index] = record
           else
-             @target << record
+            @target << record
           end
           callback(:after_add, record)
           set_inverse_instance(record, @owner)
diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb
index 050b521b6a..16023defe3 100644
--- a/activerecord/lib/active_record/nested_attributes.rb
+++ b/activerecord/lib/active_record/nested_attributes.rb
@@ -405,7 +405,18 @@ module ActiveRecord
           end
 
         elsif existing_record = existing_records.detect { |record| record.id.to_s == attributes['id'].to_s }
-          association.send(:add_record_to_target_with_callbacks, existing_record) if !association.loaded? && !call_reject_if(association_name, attributes)
+          unless association.loaded? || call_reject_if(association_name, attributes)
+            # Make sure we are operating on the actual object which is in the association's
+            # proxy_target array (either by finding it, or adding it if not found)
+            target_record = association.proxy_target.detect { |record| record == existing_record }
+
+            if target_record
+              existing_record = target_record
+            else
+              association.send(:add_record_to_target_with_callbacks, existing_record)
+            end
+          end
+
           assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy])
 
         else
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 6b71e73718..dfb3292502 100644
--- a/activerecord/test/cases/associations/has_many_through_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -45,6 +45,16 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
     assert posts(:thinking).reload.people(true).include?(people(:david))
   end
 
+  def test_associate_existing_record_twice_should_add_to_target_twice
+    post   = posts(:thinking)
+    person = people(:david)
+
+    assert_difference 'post.people.to_a.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