Ensure not to load the entire association when bulk updating existing records using nested attributes

3 files changed, 33 insertions, 13 deletions

diff --git a/activerecord/lib/active_record/associations/association_collection.rb b/activerecord/lib/active_record/associations/association_collection.rb
index 6a4cef0d50..0dfd966466 100644
--- a/activerecord/lib/active_record/associations/association_collection.rb
+++ b/activerecord/lib/active_record/associations/association_collection.rb
@@ -451,6 +451,16 @@ module ActiveRecord
           records
         end
 
+        def add_record_to_target_with_callbacks(record)
+          callback(:before_add, record)
+          yield(record) if block_given?
+          @target ||= [] unless loaded?
+          @target << record unless @reflection.options[:uniq] && @target.include?(record)
+          callback(:after_add, record)
+          set_inverse_instance(record, @owner)
+          record
+        end
+
       private
         def create_record(attrs)
           attrs.update(@reflection.options[:conditions]) if @reflection.options[:conditions].is_a?(Hash)
@@ -475,16 +485,6 @@ module ActiveRecord
           end
         end
 
-        def add_record_to_target_with_callbacks(record)
-          callback(:before_add, record)
-          yield(record) if block_given?
-          @target ||= [] unless loaded?
-          @target << record unless @reflection.options[:uniq] && @target.include?(record)
-          callback(:after_add, record)
-          set_inverse_instance(record, @owner)
-          record
-        end
-
         def remove_records(*records)
           records = flatten_deeper(records)
           records.each { |record| raise_on_type_mismatch(record) }
diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb
index ee2d420194..70a460d41d 100644
--- a/activerecord/lib/active_record/nested_attributes.rb
+++ b/activerecord/lib/active_record/nested_attributes.rb
@@ -348,14 +348,24 @@ module ActiveRecord
         attributes_collection = attributes_collection.sort_by { |index, _| index.to_i }.map { |_, attributes| attributes }
       end
 
+      association = send(association_name)
+
+      existing_records = if association.loaded?
+        association.to_a
+      else
+        attribute_ids = attributes_collection.map {|a| a['id'] || a[:id] }.compact
+        attribute_ids.present? ? association.all(:conditions => {:id => attribute_ids}) : []
+      end
+
       attributes_collection.each do |attributes|
         attributes = attributes.with_indifferent_access
 
         if attributes['id'].blank?
           unless reject_new_record?(association_name, attributes)
-            send(association_name).build(attributes.except(*UNASSIGNABLE_KEYS))
+            association.build(attributes.except(*UNASSIGNABLE_KEYS))
           end
-        elsif existing_record = send(association_name).detect { |record| record.id.to_s == attributes['id'].to_s }
+        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) unless association.loaded?
           assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy])
         else
           raise_nested_attributes_record_not_found(association_name, attributes['id'])
diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb
index 7ca9c416cb..eae8ae7e39 100644
--- a/activerecord/test/cases/nested_attributes_test.rb
+++ b/activerecord/test/cases/nested_attributes_test.rb
@@ -453,6 +453,16 @@ module NestedAttributesOnACollectionAssociationTests
     assert_equal 'Privateers Greed', @pirate.send(@association_name).last.name
   end
 
+  def test_should_not_load_association_when_updating_existing_records
+    @pirate.reload
+    @pirate.send(association_setter, [{ :id => @child_1.id, :name => 'Grace OMalley' }])
+    assert ! @pirate.send(@association_name).loaded?
+
+    @pirate.save
+    assert ! @pirate.send(@association_name).loaded?
+    assert_equal 'Grace OMalley', @child_1.reload.name
+  end
+
   def test_should_take_a_hash_with_composite_id_keys_and_assign_the_attributes_to_the_associated_models
     @child_1.stubs(:id).returns('ABC1X')
     @child_2.stubs(:id).returns('ABC2X')
@@ -507,7 +517,7 @@ module NestedAttributesOnACollectionAssociationTests
 
   def test_should_ignore_new_associated_records_if_a_reject_if_proc_returns_false
     @alternate_params[association_getter]['baz'] = {}
-    assert_no_difference("@pirate.send(@association_name).length") do
+    assert_no_difference("@pirate.send(@association_name).count") do
       @pirate.attributes = @alternate_params
     end
   end