Skip to content
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

Fix AJ wrong deprecation message on after_callbacks_if_terminated: #38576

Merged
merged 2 commits into from
Mar 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion actionpack/lib/action_controller/metal/instrumentation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ def redirect_to(*)

private
# A hook invoked every time a before callback is halted.
def halted_callback_hook(filter)
def halted_callback_hook(filter, _)
ActiveSupport::Notifications.instrument("halted_callback.action_controller", filter: filter)
end

Expand Down
7 changes: 6 additions & 1 deletion activejob/lib/active_job/callbacks.rb
Original file line number Diff line number Diff line change
Expand Up @@ -179,14 +179,19 @@ def around_enqueue(*filters, &blk)
end

private
def warn_against_after_callbacks_execution_deprecation(callbacks) # :nodoc:
def halted_callback_hook(_filter, name) # :nodoc:
return super unless %i(enqueue perform).include?(name.to_sym)
callbacks = public_send("_#{name}_callbacks")

if !self.class.skip_after_callbacks_if_terminated && callbacks.any? { |c| c.kind == :after }
ActiveSupport::Deprecation.warn(<<~EOM)
In Rails 6.2, `after_enqueue`/`after_perform` callbacks no longer run if `before_enqueue`/`before_perform` respectively halts with `throw :abort`.
To enable this behavior, uncomment the `config.active_job.skip_after_callbacks_if_terminated` config
in the new 6.1 framework defaults initializer.
EOM
end

super
end
end
end
2 changes: 0 additions & 2 deletions activejob/lib/active_job/enqueuing.rb
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,6 @@ def enqueue(options = {})
if successfully_enqueued
self
else
warn_against_after_callbacks_execution_deprecation(_enqueue_callbacks)

if self.class.return_false_on_aborted_enqueue
false
else
Expand Down
8 changes: 2 additions & 6 deletions activejob/lib/active_job/execution.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,10 @@ def perform_now
self.executions = (executions || 0) + 1

deserialize_arguments_if_needed
successfully_performed = false

job = run_callbacks :perform do
perform(*arguments).tap { successfully_performed = true }
run_callbacks :perform do
perform(*arguments)
end

warn_against_after_callbacks_execution_deprecation(_perform_callbacks) unless successfully_performed
job
rescue => exception
rescue_with_handler(exception) || raise
end
Expand Down
2 changes: 1 addition & 1 deletion activejob/lib/active_job/instrumentation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def instrument(operation, payload = {}, &block)
"#{operation}.active_job", payload.merge(adapter: queue_adapter, job: self), &enhanced_block
end

def halted_callback_hook(_)
def halted_callback_hook(*)
super
@_halted_callback_hook_called = true
end
Expand Down
46 changes: 46 additions & 0 deletions activejob/test/cases/callbacks_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,29 @@ def perform
ActiveJob::Base.skip_after_callbacks_if_terminated = prev
end

test "#enqueue does not throw a deprecation warning when skip_after_callbacks_if_terminated_is false and job did not throw an abort" do
prev = ActiveJob::Base.skip_after_callbacks_if_terminated
ActiveJob::Base.skip_after_callbacks_if_terminated = false

job = Class.new(ActiveJob::Base) do
after_enqueue { nil }

around_enqueue do |_, block|
block.call
rescue ArgumentError
nil
end

before_enqueue { raise ArgumentError }
end

assert_not_deprecated do
job.perform_later
end
ensure
ActiveJob::Base.skip_after_callbacks_if_terminated = prev
end

test "#perform does not run after_perform callbacks when skip_after_callbacks_if_terminated is true" do
prev = ActiveJob::Base.skip_after_callbacks_if_terminated
ActiveJob::Base.skip_after_callbacks_if_terminated = true
Expand Down Expand Up @@ -157,6 +180,29 @@ def perform
ActiveJob::Base.skip_after_callbacks_if_terminated = prev
end

test "#perform does not throw a deprecation warning when skip_after_callbacks_if_terminated_is false and job did not throw an abort" do
prev = ActiveJob::Base.skip_after_callbacks_if_terminated
ActiveJob::Base.skip_after_callbacks_if_terminated = false

job = Class.new(ActiveJob::Base) do
after_perform { nil }

around_perform do |_, block|
block.call
rescue ArgumentError
nil
end

before_perform { raise ArgumentError }
end

assert_not_deprecated do
job.perform_now
end
ensure
ActiveJob::Base.skip_after_callbacks_if_terminated = prev
end

test "#enqueue returns self when the job was enqueued" do
job = CallbackJob.new
assert_equal job, job.enqueue
Expand Down
25 changes: 24 additions & 1 deletion activesupport/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,26 @@
* [Breaking change] `ActiveSupport::Callbacks#halted_callback_hook` now receive a 2nd argument:

`ActiveSupport::Callbacks#halted_callback_hook` now receive the name of the callback
being halted as second argument.
This change will allow you to differentiate which callbacks halted the chain
and act accordingly.

```ruby
class Book < ApplicationRecord
before_save { throw(:abort) }
before_create { throw(:abort) }

def halted_callback_hook(filter, callback_name)
Rails.logger.info("Book couldn't be #{callback_name}d")
end

Book.create # => "Book couldn't be created"
book.save # => "Book couldn't be saved"
end
```

*Edouard Chin*

rafaelfranca marked this conversation as resolved.
Show resolved Hide resolved
* Support `prepend` with `ActiveSupport::Concern`.

Allows a module with `extend ActiveSupport::Concern` to be prepended.
Expand All @@ -14,7 +37,7 @@
prepend Imposter
end

Class methods are prepended to the base class, concerning is also
Class methods are prepended to the base class, concerning is also
updated: `concerning :Imposter, prepend: true do`.

*Jason Karns, Elia Schito*
Expand Down
19 changes: 9 additions & 10 deletions activesupport/lib/active_support/callbacks.rb
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ def run_callbacks(kind)
# A hook invoked every time a before callback is halted.
# This can be overridden in ActiveSupport::Callbacks implementors in order
# to provide better debugging/logging.
def halted_callback_hook(filter)
def halted_callback_hook(filter, name)
end

module Conditionals # :nodoc:
Expand All @@ -159,17 +159,17 @@ module Filters
Environment = Struct.new(:target, :halted, :value)

class Before
def self.build(callback_sequence, user_callback, user_conditions, chain_config, filter)
def self.build(callback_sequence, user_callback, user_conditions, chain_config, filter, name)
halted_lambda = chain_config[:terminator]

if user_conditions.any?
halting_and_conditional(callback_sequence, user_callback, user_conditions, halted_lambda, filter)
halting_and_conditional(callback_sequence, user_callback, user_conditions, halted_lambda, filter, name)
else
halting(callback_sequence, user_callback, halted_lambda, filter)
halting(callback_sequence, user_callback, halted_lambda, filter, name)
end
end

def self.halting_and_conditional(callback_sequence, user_callback, user_conditions, halted_lambda, filter)
def self.halting_and_conditional(callback_sequence, user_callback, user_conditions, halted_lambda, filter, name)
callback_sequence.before do |env|
target = env.target
value = env.value
Expand All @@ -179,7 +179,7 @@ def self.halting_and_conditional(callback_sequence, user_callback, user_conditio
result_lambda = -> { user_callback.call target, value }
env.halted = halted_lambda.call(target, result_lambda)
if env.halted
target.send :halted_callback_hook, filter
target.send :halted_callback_hook, filter, name
end
end

Expand All @@ -188,7 +188,7 @@ def self.halting_and_conditional(callback_sequence, user_callback, user_conditio
end
private_class_method :halting_and_conditional

def self.halting(callback_sequence, user_callback, halted_lambda, filter)
def self.halting(callback_sequence, user_callback, halted_lambda, filter, name)
callback_sequence.before do |env|
target = env.target
value = env.value
Expand All @@ -197,9 +197,8 @@ def self.halting(callback_sequence, user_callback, halted_lambda, filter)
unless halted
result_lambda = -> { user_callback.call target, value }
env.halted = halted_lambda.call(target, result_lambda)

if env.halted
target.send :halted_callback_hook, filter
target.send :halted_callback_hook, filter, name
end
end

Expand Down Expand Up @@ -337,7 +336,7 @@ def apply(callback_sequence)

case kind
when :before
Filters::Before.build(callback_sequence, user_callback.make_lambda, user_conditions, chain_config, @filter)
Filters::Before.build(callback_sequence, user_callback.make_lambda, user_conditions, chain_config, @filter, name)
when :after
Filters::After.build(callback_sequence, user_callback.make_lambda, user_conditions, chain_config)
when :around
Expand Down
6 changes: 4 additions & 2 deletions activesupport/test/callbacks_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,7 @@ def self.set_save_callbacks
set_callback :save, :after, :third
end

attr_reader :history, :saved, :halted
attr_reader :history, :saved, :halted, :callback_name
def initialize
@history = []
end
Expand Down Expand Up @@ -639,8 +639,9 @@ def save
end
end

def halted_callback_hook(filter)
def halted_callback_hook(filter, name)
@halted = filter
@callback_name = name
end
end

Expand Down Expand Up @@ -823,6 +824,7 @@ def test_termination_invokes_hook
terminator = CallbackTerminator.new
terminator.save
assert_equal :second, terminator.halted
assert_equal :save, terminator.callback_name
end

def test_block_never_called_if_terminated
Expand Down
19 changes: 19 additions & 0 deletions guides/source/upgrading_ruby_on_rails.md
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,25 @@ which formats your action accepts, i.e.
format.any(:xml, :json) { render request.format.to_sym => @people }
```

### `ActiveSupport::Callbacks#halted_callback_hook` now receive a second argument

Active Support allows you to override the `halted_callback_hook` whenever a callback
halts the chain. This method now receive a second argument which is the name of the callback being halted.
If you have classes that override this method, make sure it accepts two arguments. Note that this is a breaking
change without a prior deprecation cycle (for performance reasons).

Example:

```ruby
class Book < ApplicationRecord
before_save { throw(:abort) }
before_create { throw(:abort) }

def halted_callback_hook(filter, callback_name) # => This method now accepts 2 arguments instead of 1
Rails.logger.info("Book couldn't be #{callback_name}d")
end
end
```

Upgrading from Rails 5.2 to Rails 6.0
-------------------------------------
Expand Down