diff options
Diffstat (limited to 'activerecord')
| -rw-r--r-- | activerecord/CHANGELOG.md | 14 | ||||
| -rw-r--r-- | activerecord/lib/active_record/connection_adapters/abstract/transaction.rb | 30 | ||||
| -rw-r--r-- | activerecord/lib/active_record/transactions.rb | 26 | ||||
| -rw-r--r-- | activerecord/test/cases/helper.rb | 3 | ||||
| -rw-r--r-- | activerecord/test/cases/transaction_callbacks_test.rb | 76 |
5 files changed, 138 insertions, 11 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index b09d9e336b..0fbe5bfaed 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,17 @@ +* Currently, Active Record will rescue any errors raised within + after_rollback/after_create callbacks and print them to the logs. Next versions of rails + will not rescue those errors anymore, and just bubble them up, as the other callbacks. + + This adds a opt-in flag to enable that behaviour, of not rescuing the errors. + Example: + + # For not swallow errors in after_commit/after_rollback callbacks. + config.active_record.errors_in_transactional_callbacks = true + + Fixes #13460. + + *arthurnn* + * Fixed an issue where custom accessor methods (such as those generated by `enum`) with the same name as a global method are incorrectly overridden when subclassing. diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb index 8f06cf3a1f..90be835d8a 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb @@ -22,6 +22,10 @@ module ActiveRecord @state == :rolledback end + def completed? + committed? || rolledback? + end + def set_state(state) if !VALID_STATES.include?(state) raise ArgumentError, "Invalid transaction state: #{state}" @@ -63,13 +67,19 @@ module ActiveRecord end def rollback_records - records.uniq.each do |record| + ite = records.uniq + while record = ite.shift begin record.rolledback! full_rollback? rescue => e + raise if ActiveRecord::Base.raise_in_transactional_callbacks record.logger.error(e) if record.respond_to?(:logger) && record.logger end end + ensure + ite.each do |i| + i.rolledback!(full_rollback?, false) + end end def commit @@ -77,13 +87,19 @@ module ActiveRecord end def commit_records - records.uniq.each do |record| + ite = records.uniq + while record = ite.shift begin record.committed! rescue => e + raise if ActiveRecord::Base.raise_in_transactional_callbacks record.logger.error(e) if record.respond_to?(:logger) && record.logger end end + ensure + ite.each do |i| + i.committed!(false) + end end def full_rollback?; true; end @@ -103,14 +119,14 @@ module ActiveRecord end def rollback - super connection.rollback_to_savepoint(savepoint_name) + super rollback_records end def commit - super connection.release_savepoint(savepoint_name) + super parent = connection.transaction_manager.current_transaction records.each { |r| parent.add_record(r) } end @@ -130,14 +146,14 @@ module ActiveRecord end def rollback - super connection.rollback_db_transaction + super rollback_records end def commit - super connection.commit_db_transaction + super commit_records end end @@ -177,7 +193,7 @@ module ActiveRecord begin commit_transaction unless error rescue Exception - transaction.rollback + transaction.rollback unless transaction.state.completed? raise end end diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb index 7e4dc4c895..9a56291b35 100644 --- a/activerecord/lib/active_record/transactions.rb +++ b/activerecord/lib/active_record/transactions.rb @@ -3,11 +3,23 @@ module ActiveRecord module Transactions extend ActiveSupport::Concern ACTIONS = [:create, :destroy, :update] + CALLBACK_WARN_MESSAGE = <<-EOF +Currently, Active Record will rescue any errors raised within +after_rollback/after_create callbacks and print them to the logs. In the next +version, these errors will no longer be rescued. Instead, they will simply +bubble just like other Active Record callbacks. + +You can opt into the new behavior and remove this warning by setting +config.active_record.raise_in_transactional_callbacks to true. +EOF included do define_callbacks :commit, :rollback, terminator: ->(_, result) { result == false }, scope: [:kind, :name] + + mattr_accessor :raise_in_transactional_callbacks, instance_writer: false + self.raise_in_transactional_callbacks = false end # = Active Record Transactions @@ -223,6 +235,9 @@ module ActiveRecord def after_commit(*args, &block) set_options_for_callbacks!(args) set_callback(:commit, :after, *args, &block) + unless ActiveRecord::Base.raise_in_transactional_callbacks + ActiveSupport::Deprecation.warn(CALLBACK_WARN_MESSAGE) + end end # This callback is called after a create, update, or destroy are rolled back. @@ -231,6 +246,9 @@ module ActiveRecord def after_rollback(*args, &block) set_options_for_callbacks!(args) set_callback(:rollback, :after, *args, &block) + unless ActiveRecord::Base.raise_in_transactional_callbacks + ActiveSupport::Deprecation.warn(CALLBACK_WARN_MESSAGE) + end end private @@ -290,16 +308,16 @@ module ActiveRecord # # Ensure that it is not called if the object was never persisted (failed create), # but call it after the commit of a destroyed object. - def committed! #:nodoc: - run_callbacks :commit if destroyed? || persisted? + def committed!(should_run_callbacks = true) #:nodoc: + run_callbacks :commit if should_run_callbacks && destroyed? || persisted? ensure force_clear_transaction_record_state end # Call the +after_rollback+ callbacks. The +force_restore_state+ argument indicates if the record # state should be rolled back to the beginning or just to the last savepoint. - def rolledback!(force_restore_state = false) #:nodoc: - run_callbacks :rollback + def rolledback!(force_restore_state = false, should_run_callbacks = true) #:nodoc: + run_callbacks :rollback if should_run_callbacks ensure restore_transaction_record_state(force_restore_state) clear_transaction_record_state diff --git a/activerecord/test/cases/helper.rb b/activerecord/test/cases/helper.rb index 6a8aff4b69..e43b796237 100644 --- a/activerecord/test/cases/helper.rb +++ b/activerecord/test/cases/helper.rb @@ -24,6 +24,9 @@ ActiveSupport::Deprecation.debug = true # Disable available locale checks to avoid warnings running the test suite. I18n.enforce_available_locales = false +# Enable raise errors in after_commit and after_rollback. +ActiveRecord::Base.raise_in_transactional_callbacks = true + # Connect to the database ARTest.connect diff --git a/activerecord/test/cases/transaction_callbacks_test.rb b/activerecord/test/cases/transaction_callbacks_test.rb index a3f39804b7..b5ac1bdaf9 100644 --- a/activerecord/test/cases/transaction_callbacks_test.rb +++ b/activerecord/test/cases/transaction_callbacks_test.rb @@ -267,6 +267,9 @@ class TransactionCallbacksTest < ActiveRecord::TestCase end def test_after_transaction_callbacks_should_prevent_callbacks_from_being_called + old_transaction_config = ActiveRecord::Base.raise_in_transactional_callbacks + ActiveRecord::Base.raise_in_transactional_callbacks = false + def @first.last_after_transaction_error=(e); @last_transaction_error = e; end def @first.last_after_transaction_error; @last_transaction_error; end @first.after_commit_block{|r| r.last_after_transaction_error = :commit; raise "fail!";} @@ -291,6 +294,79 @@ class TransactionCallbacksTest < ActiveRecord::TestCase end assert_equal :rollback, @first.last_after_transaction_error assert_equal [:after_rollback], second.history + ensure + ActiveRecord::Base.raise_in_transactional_callbacks = old_transaction_config + end + + def test_after_commit_should_not_raise_when_raise_in_transactional_callbacks_false + old_transaction_config = ActiveRecord::Base.raise_in_transactional_callbacks + ActiveRecord::Base.raise_in_transactional_callbacks = false + @first.after_commit_block{ fail "boom" } + Topic.transaction { @first.save! } + ensure + ActiveRecord::Base.raise_in_transactional_callbacks = old_transaction_config + end + + def test_after_commit_callback_should_not_swallow_errors + @first.after_commit_block{ fail "boom" } + assert_raises(RuntimeError) do + Topic.transaction do + @first.save! + end + end + end + + def test_after_commit_callback_when_raise_should_not_restore_state + first = TopicWithCallbacks.new + second = TopicWithCallbacks.new + first.after_commit_block{ fail "boom" } + second.after_commit_block{ fail "boom" } + + begin + Topic.transaction do + first.save! + assert_not_nil first.id + second.save! + assert_not_nil second.id + end + rescue + end + assert_not_nil first.id + assert_not_nil second.id + assert first.reload + end + + def test_after_rollback_callback_should_not_swallow_errors_when_set_to_raise + error_class = Class.new(StandardError) + @first.after_rollback_block{ raise error_class } + assert_raises(error_class) do + Topic.transaction do + @first.save! + raise ActiveRecord::Rollback + end + end + end + + def test_after_rollback_callback_when_raise_should_restore_state + error_class = Class.new(StandardError) + + first = TopicWithCallbacks.new + second = TopicWithCallbacks.new + first.after_rollback_block{ raise error_class } + second.after_rollback_block{ raise error_class } + + begin + Topic.transaction do + first.save! + assert_not_nil first.id + second.save! + assert_not_nil second.id + raise ActiveRecord::Rollback + end + rescue error_class + end + assert_nil first.id + assert_nil second.id end def test_after_rollback_callbacks_should_validate_on_condition |