From bf36719a7b04a6695f286df3ae307ccb8f658405 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Wed, 22 Dec 2010 00:19:59 +0000 Subject: [PATCH] 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] --- .../associations/association_collection.rb | 4 ++-- .../lib/active_record/nested_attributes.rb | 13 ++++++++++++- .../has_many_through_associations_test.rb | 10 ++++++++++ 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 c513e8a..7964f4f 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 050b521..16023de 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 cf0eedb..39bc018 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 -- 1.7.1