New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to stop swallowing errors on callbacks. #16537

Merged
merged 1 commit into from Aug 18, 2014
Jump to file or symbol
Failed to load files and symbols.
+138 −11
Diff settings

Always

Just for now

Copy path View file
@@ -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 @@ def rolledback?
@state == :rolledback
end
def completed?
committed? || rolledback?
end
def set_state(state)
if !VALID_STATES.include?(state)
raise ArgumentError, "Invalid transaction state: #{state}"
@@ -63,27 +67,39 @@ def rollback
end
def rollback_records
records.uniq.each do |record|
ite = records.uniq

This comment has been minimized.

@jonleighton

jonleighton Aug 19, 2014

Member

Is ite an acronym for something?

This comment has been minimized.

@arthurnn

arthurnn Aug 20, 2014

Member

iterator ...

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

This comment has been minimized.

@jonleighton

jonleighton Aug 19, 2014

Member

I found the logic here hard to follow. I think the issue is that the rollback!/committed! methods are doing two things which we wish to be able to separate:

  1. run callbacks
  2. change internal state of the object

Perhaps we could split out these things into separate methods? Then we could do the state-changing on each record before running any callbacks (perhaps). Or at least avoid the boolean (which makes it non-obvious what is going on).

This comment has been minimized.

@chancancode

chancancode Aug 19, 2014

Member

The plan was to switch the boolean arg into kwarg, which could happen once we branched 4-2-stable. But it seems like splitting that up is a better plan 👍

This comment has been minimized.

@arthurnn

arthurnn Aug 20, 2014

Member

I will split that up!

end
def commit
@state.set_state(:committed)
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 @@ def initialize(connection, savepoint_name, options)
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 @@ def initialize(connection, options)
end
def rollback
super
connection.rollback_db_transaction
super
rollback_records
end
def commit
super
connection.commit_db_transaction
super

This comment has been minimized.

@arthurnn

arthurnn Aug 18, 2014

Member

only should call super(which changes the state of the transaction), after the actual DB call had happened. (same applies to the rollback method)

commit_records
end
end
@@ -177,7 +193,7 @@ def within_new_transaction(options = {})
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 @@ def transaction(options = {}, &block)
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 @@ def after_commit(*args, &block)
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 @@ def rollback_active_record_state!
#
# 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?

This comment has been minimized.

@jonleighton

jonleighton Aug 19, 2014

Member

The precedence here is (a && b) || c, but I think you intended a && (b || c)?

This comment has been minimized.

@arthurnn

arthurnn Aug 20, 2014

Member

you are right.. will fix it.

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 @@
# 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 @@ def @first.commits(i=0); @commits ||= 0; @commits += i if i; end
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 @@ def @first.last_after_transaction_error; @last_transaction_error; end
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
ProTip! Use n and p to navigate between commits in a pull request.