Fix AJ wrong deprecation message on `after_callbacks_if_terminated`:

- ### Problem

  In some cirumstances, the deprecation message to warn that AJ won't
  run `after_(enqueue/perform)` callbacks when the chain is halted
  by a `throw(:abort)` will be thrown even though no `throw(:abort)`
  was thrown.

  ```ruby
    run_callback(:foo) do
      ...
    end
  ```

  There is two possible way for the callback body to not be executed:

  1) `before` callback throw a `abort`
  2) `before` callback raises an error which is rescued by an
     around callback (See associated test in this commit for
     an example)

  When 2) happen we don't want to output a deprecation message,
  because what the message says isn't true and doesn't apply.

  ### Solution

  In order to differentiate between 1) and 2), I have added
  a `halted_callback_hook` which is called by ActiveSupport callback
  whenever the callback chain is halted.
This commit is contained in:
Edouard CHIN 2020-02-25 22:25:33 -04:00
parent 06dd162fb3
commit 0cdeee428e
4 changed files with 54 additions and 9 deletions

View File

@ -179,7 +179,10 @@ module ActiveJob
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`.
@ -187,6 +190,8 @@ module ActiveJob
in the new 6.1 framework defaults initializer.
EOM
end
super
end
end
end

View File

@ -65,8 +65,6 @@ module ActiveJob
if successfully_enqueued
self
else
warn_against_after_callbacks_execution_deprecation(_enqueue_callbacks)
if self.class.return_false_on_aborted_enqueue
false
else

View File

@ -43,14 +43,10 @@ module ActiveJob
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

View File

@ -116,6 +116,29 @@ class CallbacksTest < ActiveSupport::TestCase
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
@ -157,6 +180,29 @@ class CallbacksTest < ActiveSupport::TestCase
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