From f2aacd51405724cdf7cfd36a439c9dbfce16973a Mon Sep 17 00:00:00 2001 From: Eloy Duran Date: Fri, 8 Jan 2010 20:47:49 +0100 Subject: Rollback the transaction when one of the autosave associations fails to save. [#3391 state:resolved] --- .../lib/active_record/autosave_association.rb | 14 +++++-- .../test/cases/autosave_association_test.rb | 45 +++++++++++++++++++++- activerecord/test/models/bird.rb | 6 +++ activerecord/test/models/parrot.rb | 6 +++ activerecord/test/models/pirate.rb | 6 +++ activerecord/test/models/ship.rb | 6 +++ 6 files changed, 78 insertions(+), 5 deletions(-) diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index 741fb2ef40..f7119a77cc 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -296,13 +296,15 @@ module ActiveRecord association.destroy(record) elsif autosave != false && (@new_record_before_save || record.new_record?) if autosave - association.send(:insert_record, record, false, false) + saved = association.send(:insert_record, record, false, false) else association.send(:insert_record, record) end elsif autosave - record.save(false) + saved = record.save(false) end + + raise ActiveRecord::Rollback if saved == false end end @@ -329,7 +331,9 @@ module ActiveRecord key = reflection.options[:primary_key] ? send(reflection.options[:primary_key]) : id if autosave != false && (new_record? || association.new_record? || association[reflection.primary_key_name] != key || autosave) association[reflection.primary_key_name] = key - association.save(!autosave) + saved = association.save(!autosave) + raise ActiveRecord::Rollback if !saved && autosave + saved end end end @@ -350,7 +354,7 @@ module ActiveRecord if autosave && association.marked_for_destruction? association.destroy elsif autosave != false - association.save(!autosave) if association.new_record? || autosave + saved = association.save(!autosave) if association.new_record? || autosave if association.updated? association_id = association.send(reflection.options[:primary_key] || :id) @@ -360,6 +364,8 @@ module ActiveRecord self[reflection.options[:foreign_type]] = association.class.base_class.name.to_s end end + + saved if autosave end end end diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index fd2b0d2d9f..7be605ed95 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -830,6 +830,18 @@ class TestAutosaveAssociationOnAHasOneAssociation < ActiveRecord::TestCase end end + def test_should_not_save_and_return_false_if_a_callback_cancelled_saving + pirate = Pirate.new(:catchphrase => 'Arr') + ship = pirate.build_ship(:name => 'The Vile Insanity') + ship.cancel_save_from_callback = true + + assert_no_difference 'Pirate.count' do + assert_no_difference 'Ship.count' do + assert !pirate.save + end + end + end + def test_should_rollback_any_changes_if_an_exception_occurred_while_saving before = [@pirate.catchphrase, @pirate.ship.name] @@ -913,6 +925,18 @@ class TestAutosaveAssociationOnABelongsToAssociation < ActiveRecord::TestCase end end + def test_should_not_save_and_return_false_if_a_callback_cancelled_saving + ship = Ship.new(:name => 'The Vile Insanity') + pirate = ship.build_pirate(:catchphrase => 'Arr') + pirate.cancel_save_from_callback = true + + assert_no_difference 'Ship.count' do + assert_no_difference 'Pirate.count' do + assert !ship.save + end + end + end + def test_should_rollback_any_changes_if_an_exception_occurred_while_saving before = [@ship.pirate.catchphrase, @ship.name] @@ -928,7 +952,6 @@ class TestAutosaveAssociationOnABelongsToAssociation < ActiveRecord::TestCase end assert_raise(RuntimeError) { assert !@ship.save } - # TODO: Why does using reload on @ship looses the associated pirate? assert_equal before, [@ship.pirate.reload.catchphrase, @ship.reload.name] end @@ -1029,6 +1052,26 @@ module AutosaveAssociationOnACollectionAssociationTests end end + def test_should_not_save_and_return_false_if_a_callback_cancelled_saving_in_either_create_or_update + @pirate.catchphrase = 'Changed' + @child_1.name = 'Changed' + @child_1.cancel_save_from_callback = true + + assert !@pirate.save + assert_equal "Don' botharrr talkin' like one, savvy?", @pirate.reload.catchphrase + assert_equal "Posideons Killer", @child_1.reload.name + + new_pirate = Pirate.new(:catchphrase => 'Arr') + new_child = new_pirate.send(@association_name).build(:name => 'Grace OMalley') + new_child.cancel_save_from_callback = true + + assert_no_difference 'Pirate.count' do + assert_no_difference "#{new_child.class.name}.count" do + assert !new_pirate.save + end + end + end + def test_should_rollback_any_changes_if_an_exception_occurred_while_saving before = [@pirate.catchphrase, *@pirate.send(@association_name).map(&:name)] new_names = ['Grace OMalley', 'Privateers Greed'] diff --git a/activerecord/test/models/bird.rb b/activerecord/test/models/bird.rb index 341d2eeffc..e61d48e6a5 100644 --- a/activerecord/test/models/bird.rb +++ b/activerecord/test/models/bird.rb @@ -1,3 +1,9 @@ class Bird < ActiveRecord::Base validates_presence_of :name + + attr_accessor :cancel_save_from_callback + before_save :cancel_save_callback_method, :if => :cancel_save_from_callback + def cancel_save_callback_method + false + end end \ No newline at end of file diff --git a/activerecord/test/models/parrot.rb b/activerecord/test/models/parrot.rb index 4a7ed52636..737ef9131b 100644 --- a/activerecord/test/models/parrot.rb +++ b/activerecord/test/models/parrot.rb @@ -6,6 +6,12 @@ class Parrot < ActiveRecord::Base alias_attribute :title, :name validates_presence_of :name + + attr_accessor :cancel_save_from_callback + before_save :cancel_save_callback_method, :if => :cancel_save_from_callback + def cancel_save_callback_method + false + end end class LiveParrot < Parrot diff --git a/activerecord/test/models/pirate.rb b/activerecord/test/models/pirate.rb index 88c1634717..f1dbe32c6e 100644 --- a/activerecord/test/models/pirate.rb +++ b/activerecord/test/models/pirate.rb @@ -51,6 +51,12 @@ class Pirate < ActiveRecord::Base attributes.delete('_reject_me_if_new').present? && new_record? end + attr_accessor :cancel_save_from_callback + before_save :cancel_save_callback_method, :if => :cancel_save_from_callback + def cancel_save_callback_method + false + end + private def log_before_add(record) log(record, "before_adding_method") diff --git a/activerecord/test/models/ship.rb b/activerecord/test/models/ship.rb index a96e38ab41..75c792d176 100644 --- a/activerecord/test/models/ship.rb +++ b/activerecord/test/models/ship.rb @@ -9,4 +9,10 @@ class Ship < ActiveRecord::Base accepts_nested_attributes_for :update_only_pirate, :update_only => true validates_presence_of :name + + attr_accessor :cancel_save_from_callback + before_save :cancel_save_callback_method, :if => :cancel_save_from_callback + def cancel_save_callback_method + false + end end -- cgit v1.2.3