aboutsummaryrefslogtreecommitdiffstats
path: root/activerecord
diff options
context:
space:
mode:
Diffstat (limited to 'activerecord')
-rw-r--r--activerecord/CHANGELOG.md14
-rw-r--r--activerecord/lib/active_record/connection_adapters/abstract/transaction.rb30
-rw-r--r--activerecord/lib/active_record/transactions.rb26
-rw-r--r--activerecord/test/cases/helper.rb3
-rw-r--r--activerecord/test/cases/transaction_callbacks_test.rb76
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