Skip to content
Permalink
Browse files

Change transaction callbacks to not swallowing errors.

Before this change any error raised inside a transaction callback
are rescued and printed in the logs.

Now these errors are not rescue anymore and just bubble up,
as the other callbacks.
  • Loading branch information
rafaelfranca committed Jan 4, 2015
1 parent 3a59dd2 commit 07d3d402341e81ada0214f2cb2be1da69eadfe72
@@ -1,3 +1,16 @@
* Deprecate `ActiveRecord::Base.errors_in_transactional_callbacks=`.

*Rafael Mendonça França*

* Change transaction callbacks to not swallowing errors.

Before this change any error raised inside a transaction callback are
rescued and printed in the logs.

Now these errors are not rescue anymore and just bubble up, as the other callbacks.

*Rafael Mendonça França*

* Remove deprecated `sanitize_sql_hash_for_conditions`.

*Rafael Mendonça França*
@@ -69,12 +69,7 @@ def rollback
def rollback_records
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
record.rolledback! full_rollback?
end
ensure
ite.each do |i|
@@ -89,12 +84,7 @@ def commit
def commit_records
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
record.committed!
end
ensure
ite.each do |i|
@@ -4,23 +4,10 @@ module Transactions
extend ActiveSupport::Concern
#:nodoc:
ACTIONS = [:create, :destroy, :update]
#:nodoc:
CALLBACK_WARN_MESSAGE = "Currently, Active Record suppresses errors raised " \
"within `after_rollback`/`after_commit` callbacks and only print them to " \
"the logs. In the next version, these errors will no longer be suppressed. " \
"Instead, the errors will propagate normally just like in other Active " \
"Record callbacks.\n" \
"\n" \
"You can opt into the new behavior and remove this warning by setting:\n" \
"\n" \
" config.active_record.raise_in_transactional_callbacks = true\n\n"

included do
define_callbacks :commit, :rollback,
scope: [:kind, :name]

mattr_accessor :raise_in_transactional_callbacks, instance_writer: false
self.raise_in_transactional_callbacks = false
end

# = Active Record Transactions
@@ -236,9 +223,6 @@ 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.
@@ -247,9 +231,16 @@ 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

def raise_in_transactional_callbacks
ActiveSupport::Deprecation.warn('ActiveRecord::Base.raise_in_transactional_callbacks is deprecated and will be removed without replacement.')
true
end

def raise_in_transactional_callbacks=(value)
ActiveSupport::Deprecation.warn('ActiveRecord::Base.raise_in_transactional_callbacks= is deprecated, has no effect and will be removed without replacement.')
value
end

private
@@ -24,9 +24,6 @@
# 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

@@ -266,47 +266,6 @@ def @first.commits(i=0); @commits ||= 0; @commits += i if i; end
assert_equal 2, @first.rollbacks
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!";}
@first.after_rollback_block{|r| r.last_after_transaction_error = :rollback; raise "fail!";}

second = TopicWithCallbacks.find(3)
second.after_commit_block{|r| r.history << :after_commit}
second.after_rollback_block{|r| r.history << :after_rollback}

Topic.transaction do
@first.save!
second.save!
end
assert_equal :commit, @first.last_after_transaction_error
assert_equal [:after_commit], second.history

second.history.clear
Topic.transaction do
@first.save!
second.save!
raise ActiveRecord::Rollback
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
@@ -31,10 +31,5 @@ class Application < Rails::Application
# The default locale is :en and all translations from config/locales/*.rb,yml are auto loaded.
# config.i18n.load_path += Dir[Rails.root.join('my', 'locales', '*.{rb,yml}').to_s]
# config.i18n.default_locale = :de
<%- unless options.skip_active_record? -%>
# Do not swallow errors in after_commit/after_rollback callbacks.
config.active_record.raise_in_transactional_callbacks = true
<%- end -%>
end
end

0 comments on commit 07d3d40

Please sign in to comment.
You can’t perform that action at this time.