Merge pull request #16537 from arthurnn/stop_swallowing_errors_2
Add option to stop swallowing errors on callbacks.
This commit is contained in:
commit
879dde9b1a
|
@ -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.
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
||||
|
|
|
@ -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
|
||||
|
|
Loading…
Reference in New Issue