Skip to content
Browse files

Ensure not to load the entire association when bulk updating existing…

… records using nested attributes
  • Loading branch information...
1 parent 5208cc3 commit 2ff5f38abb4a44ed5356c34b40d30d446fb63408 @lifo lifo committed Apr 14, 2010
View
20 activerecord/lib/active_record/associations/association_collection.rb
@@ -451,6 +451,16 @@ def find_target
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 @@ def build_record(attrs)
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) }
View
14 activerecord/lib/active_record/nested_attributes.rb
@@ -348,14 +348,24 @@ def assign_nested_attributes_for_collection_association(association_name, attrib
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}) : []
@mat813
mat813 added a note Apr 14, 2010

That seems wrong, when the primary key of the association is not "id" it's failing, it should do :

association.find(attribute_ids)

or if it's not possible, it should get the primary key for the association.

@lifo
Ruby on Rails member
lifo added a note Apr 14, 2010

Could you please submit a failing test and assign the ticket to me ?

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ 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'])
View
12 activerecord/test/cases/nested_attributes_test.rb
@@ -453,6 +453,16 @@ def test_should_take_a_hash_and_assign_the_attributes_to_the_associated_models
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 @@ def test_should_ignore_new_associated_records_with_truthy_destroy_attribute
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

0 comments on commit 2ff5f38

Please sign in to comment.
Something went wrong with that request. Please try again.