From ff7bde62c857ec94f45a5be3bc76468deb8b0b3a Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Wed, 22 Dec 2010 00:19:59 +0000 Subject: 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] --- .../active_record/associations/association_collection.rb | 4 ++-- activerecord/lib/active_record/nested_attributes.rb | 13 ++++++++++++- .../associations/has_many_through_associations_test.rb | 10 ++++++++++ 3 files changed, 24 insertions(+), 3 deletions(-) (limited to 'activerecord') 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 -- cgit v1.2.3