Skip to content

Commit

Permalink
Remove internal without_transaction_enrollment callbacks
Browse files Browse the repository at this point in the history
I've found the internal `without_transaction_enrollment` callbacks which
have not been newly used over five years, when I tried to work reverting
rails#9068 (rails#36049 (comment)).

I think that we will never make that callbacks public, since the
mechanism of `without_transaction_enrollment` is too implementation
specific, at least before rails#9068, records in a transaction had enrolled
all into the transaction.

That callbacks was introduced at rails#18936 to make `touch_later` rails#19324,
but I think that the internal callbacks is overkill to just make the
`touch_later` only, and invoking the extra callbacks also have a little
overhead even if we haven't used that.

So I think we can remove the internal callbacks for now, until we will
come up with a good use for that callbacks.
  • Loading branch information
kamipo committed May 1, 2020
1 parent 348e142 commit 75873d8
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 99 deletions.
15 changes: 6 additions & 9 deletions activerecord/lib/active_record/touch_later.rb
Expand Up @@ -3,10 +3,9 @@
module ActiveRecord
# = Active Record Touch Later
module TouchLater # :nodoc:
extend ActiveSupport::Concern

included do
before_commit_without_transaction_enrollment :touch_deferred_attributes
def before_committed!
touch_deferred_attributes if has_defer_touch_attrs? && persisted?
super
end

def touch_later(*names) # :nodoc:
Expand Down Expand Up @@ -42,11 +41,9 @@ def surreptitiously_touch(attrs)
end

def touch_deferred_attributes
if has_defer_touch_attrs? && persisted?
@_skip_dirty_tracking = true
touch(*@_defer_touch_attrs, time: @_touch_time)
@_defer_touch_attrs, @_touch_time = nil, nil
end
@_skip_dirty_tracking = true
touch(*@_defer_touch_attrs, time: @_touch_time)
@_defer_touch_attrs, @_touch_time = nil, nil
end

def has_defer_touch_attrs?
Expand Down
21 changes: 0 additions & 21 deletions activerecord/lib/active_record/transactions.rb
Expand Up @@ -10,9 +10,6 @@ module Transactions
included do
define_callbacks :commit, :rollback,
:before_commit,
:before_commit_without_transaction_enrollment,
:commit_without_transaction_enrollment,
:rollback_without_transaction_enrollment,
scope: [:kind, :name]
end

Expand Down Expand Up @@ -266,21 +263,6 @@ def after_rollback(*args, &block)
set_callback(:rollback, :after, *args, &block)
end

def before_commit_without_transaction_enrollment(*args, &block) # :nodoc:
set_options_for_callbacks!(args)
set_callback(:before_commit_without_transaction_enrollment, :before, *args, &block)
end

def after_commit_without_transaction_enrollment(*args, &block) # :nodoc:
set_options_for_callbacks!(args)
set_callback(:commit_without_transaction_enrollment, :after, *args, &block)
end

def after_rollback_without_transaction_enrollment(*args, &block) # :nodoc:
set_options_for_callbacks!(args)
set_callback(:rollback_without_transaction_enrollment, :after, *args, &block)
end

private
def set_options_for_callbacks!(args, enforced_options = {})
options = args.extract_options!.merge!(enforced_options)
Expand Down Expand Up @@ -323,7 +305,6 @@ def touch(*, **) #:nodoc:
end

def before_committed! # :nodoc:
_run_before_commit_without_transaction_enrollment_callbacks
_run_before_commit_callbacks
end

Expand All @@ -335,7 +316,6 @@ def committed!(should_run_callbacks: true) #:nodoc:
force_clear_transaction_record_state
if should_run_callbacks
@_committed_already_called = true
_run_commit_without_transaction_enrollment_callbacks
_run_commit_callbacks
end
ensure
Expand All @@ -347,7 +327,6 @@ def committed!(should_run_callbacks: true) #:nodoc:
def rolledback!(force_restore_state: false, should_run_callbacks: true) #:nodoc:
if should_run_callbacks
_run_rollback_callbacks
_run_rollback_without_transaction_enrollment_callbacks
end
ensure
restore_transaction_record_state(force_restore_state)
Expand Down
69 changes: 0 additions & 69 deletions activerecord/test/cases/transaction_callbacks_test.rb
Expand Up @@ -719,75 +719,6 @@ def test_trigger_on_update_where_row_was_deleted
end
end

class TransactionEnrollmentCallbacksTest < ActiveRecord::TestCase
class TopicWithoutTransactionalEnrollmentCallbacks < ActiveRecord::Base
self.table_name = :topics

before_commit_without_transaction_enrollment { |r| r.history << :before_commit }
after_commit_without_transaction_enrollment { |r| r.history << :after_commit }
after_rollback_without_transaction_enrollment { |r| r.history << :rollback }

def history
@history ||= []
end
end

def setup
@topic = TopicWithoutTransactionalEnrollmentCallbacks.create!
end

def test_commit_does_not_run_transactions_callbacks_without_enrollment
@topic.transaction do
@topic.content = "foo"
@topic.save!
end
assert_empty @topic.history
end

def test_commit_run_transactions_callbacks_with_explicit_enrollment
@topic.transaction do
2.times do
@topic.content = "foo"
@topic.save!
end
@topic.send(:add_to_transaction)
end
assert_equal [:before_commit, :after_commit], @topic.history
end

def test_commit_run_transactions_callbacks_with_nested_transactions
@topic.transaction do
@topic.transaction(requires_new: true) do
@topic.content = "foo"
@topic.save!
@topic.send(:add_to_transaction)
end
end
assert_equal [:before_commit, :after_commit], @topic.history
end

def test_rollback_does_not_run_transactions_callbacks_without_enrollment
@topic.transaction do
@topic.content = "foo"
@topic.save!
raise ActiveRecord::Rollback
end
assert_empty @topic.history
end

def test_rollback_run_transactions_callbacks_with_explicit_enrollment
@topic.transaction do
2.times do
@topic.content = "foo"
@topic.save!
end
@topic.send(:add_to_transaction)
raise ActiveRecord::Rollback
end
assert_equal [:rollback], @topic.history
end
end

class CallbacksOnActionAndConditionTest < ActiveRecord::TestCase
self.use_transactional_tests = false

Expand Down

0 comments on commit 75873d8

Please sign in to comment.