From b1e509ad7a8c8264544f10f4666705cd806b5408 Mon Sep 17 00:00:00 2001 From: Francesco Rodriguez Date: Mon, 2 Jul 2012 11:57:10 -0500 Subject: [PATCH] Backport #3329 to 3-2-stable Fix bug with autosave collection association on new record with a marked for destroy record in autosave collection. Fixes #6918. --- .../lib/active_record/autosave_association.rb | 36 +++++++++++-------- .../test/cases/autosave_association_test.rb | 10 ++++++ 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index c86eaba498303..e1499fc3b02f7 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -332,25 +332,31 @@ def save_collection_association(reflection) if records = associated_records_to_validate_or_save(association, @new_record_before_save, autosave) begin - records.each do |record| - next if record.destroyed? + records_to_destroy = [] + + records.each do |record| + next if record.destroyed? + + saved = true + + if autosave && record.marked_for_destruction? + records_to_destroy << record + elsif autosave != false && (@new_record_before_save || record.new_record?) + if autosave + saved = association.insert_record(record, false) + else + association.insert_record(record) unless reflection.nested? + end + elsif autosave + saved = record.save(:validate => false) + end - saved = true + raise ActiveRecord::Rollback unless saved + end - if autosave && record.marked_for_destruction? + records_to_destroy.each do |record| association.proxy.destroy(record) - elsif autosave != false && (@new_record_before_save || record.new_record?) - if autosave - saved = association.insert_record(record, false) - else - association.insert_record(record) unless reflection.nested? - end - elsif autosave - saved = record.save(:validate => false) end - - raise ActiveRecord::Rollback unless saved - end rescue records.each {|x| IdentityMap.remove(x) } if IdentityMap.enabled? raise diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 315a9b001b389..e6b881389bcc6 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -770,6 +770,16 @@ def destroy(*args) assert_equal before, @pirate.reload.birds end + def test_when_new_record_a_child_marked_for_destruction_should_not_affect_other_records_from_saving + @pirate = @ship.build_pirate(:catchphrase => "Arr' now I shall keep me eye on you matey!") # new record + + 3.times { |i| @pirate.birds.build(:name => "birds_#{i}") } + @pirate.birds[1].mark_for_destruction + @pirate.save! + + assert_equal 2, @pirate.birds.reload.length + end + # Add and remove callbacks tests for association collections. %w{ method proc }.each do |callback_type| define_method("test_should_run_add_callback_#{callback_type}s_for_has_many") do