Merge pull request #51525 from Shopify/aj-after-commit-return-value
Fix `ActiveJob::EnqueueAfterTransactionCommit` API
This commit is contained in:
commit
41d867d5b1
@ -16,9 +16,11 @@ module EnqueueAfterTransactionCommit # :nodoc:
|
||||
# - `:never` forces the job to be queued immediately.
|
||||
# - `:default` lets the queue adapter define the behavior (recommended).
|
||||
class_attribute :enqueue_after_transaction_commit, instance_accessor: false, instance_predicate: false, default: :never
|
||||
end
|
||||
|
||||
around_enqueue do |job, block|
|
||||
after_transaction = case job.class.enqueue_after_transaction_commit
|
||||
private
|
||||
def raw_enqueue
|
||||
after_transaction = case self.class.enqueue_after_transaction_commit
|
||||
when :always
|
||||
true
|
||||
when :never
|
||||
@ -28,11 +30,15 @@ module EnqueueAfterTransactionCommit # :nodoc:
|
||||
end
|
||||
|
||||
if after_transaction
|
||||
ActiveRecord.after_all_transactions_commit(&block)
|
||||
self.successfully_enqueued = true
|
||||
ActiveRecord.after_all_transactions_commit do
|
||||
self.successfully_enqueued = false
|
||||
super
|
||||
end
|
||||
self
|
||||
else
|
||||
block.call
|
||||
super
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
@ -50,14 +50,17 @@ module ClassMethods
|
||||
# custom serializers.
|
||||
#
|
||||
# Returns an instance of the job class queued with arguments available in
|
||||
# Job#arguments or false if the enqueue did not succeed.
|
||||
# Job#arguments or +false+ if the enqueue did not succeed.
|
||||
#
|
||||
# After the attempted enqueue, the job will be yielded to an optional block.
|
||||
#
|
||||
# If Active Job is used conjointly with Active Record, and #perform_later is called
|
||||
# inside an Active Record transaction, then the enqueue is implicitly deferred to after
|
||||
# the transaction is committed, or droped if it's rolled back. This behavior can
|
||||
# be changed on a per job basis:
|
||||
# the transaction is committed, or droped if it's rolled back. In such case #perform_later
|
||||
# will return the job instance like if it was successfully enqueued, but will still return
|
||||
# +false+ if a callback prevented the job from being enqueued.
|
||||
#
|
||||
# This behavior can be changed on a per job basis:
|
||||
#
|
||||
# class NotificationJob < ApplicationJob
|
||||
# self.enqueue_after_transaction_commit = false
|
||||
@ -98,6 +101,18 @@ def enqueue(options = {})
|
||||
self.successfully_enqueued = false
|
||||
|
||||
run_callbacks :enqueue do
|
||||
raw_enqueue
|
||||
end
|
||||
|
||||
if successfully_enqueued?
|
||||
self
|
||||
else
|
||||
false
|
||||
end
|
||||
end
|
||||
|
||||
private
|
||||
def raw_enqueue
|
||||
if scheduled_at
|
||||
queue_adapter.enqueue_at self, scheduled_at.to_f
|
||||
else
|
||||
@ -108,12 +123,5 @@ def enqueue(options = {})
|
||||
rescue EnqueueError => e
|
||||
self.enqueue_error = e
|
||||
end
|
||||
|
||||
if successfully_enqueued?
|
||||
self
|
||||
else
|
||||
false
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
105
activejob/test/cases/enqueue_after_transaction_commit_test.rb
Normal file
105
activejob/test/cases/enqueue_after_transaction_commit_test.rb
Normal file
@ -0,0 +1,105 @@
|
||||
# frozen_string_literal: true
|
||||
|
||||
require "helper"
|
||||
require "jobs/enqueue_error_job"
|
||||
|
||||
class EnqueueAfterTransactionCommitTest < ActiveSupport::TestCase
|
||||
class FakeActiveRecord
|
||||
attr_reader :calls
|
||||
|
||||
def initialize(should_yield = true)
|
||||
@calls = 0
|
||||
@yield = should_yield
|
||||
@callbacks = []
|
||||
end
|
||||
|
||||
def after_all_transactions_commit(&block)
|
||||
@calls += 1
|
||||
if @yield
|
||||
yield
|
||||
else
|
||||
@callbacks << block
|
||||
end
|
||||
end
|
||||
|
||||
def run_after_commit_callbacks
|
||||
callbacks, @callbacks = @callbacks, []
|
||||
callbacks.each(&:call)
|
||||
end
|
||||
end
|
||||
|
||||
class EnqueueAfterCommitJob < ActiveJob::Base
|
||||
self.enqueue_after_transaction_commit = :always
|
||||
|
||||
def perform
|
||||
# noop
|
||||
end
|
||||
end
|
||||
|
||||
class ErrorEnqueueAfterCommitJob < EnqueueErrorJob
|
||||
class EnqueueErrorAdapter
|
||||
def enqueue_after_transaction_commit?
|
||||
true
|
||||
end
|
||||
|
||||
def enqueue(...)
|
||||
raise ActiveJob::EnqueueError, "There was an error enqueuing the job"
|
||||
end
|
||||
|
||||
def enqueue_at(...)
|
||||
raise ActiveJob::EnqueueError, "There was an error enqueuing the job"
|
||||
end
|
||||
end
|
||||
|
||||
self.queue_adapter = EnqueueErrorAdapter.new
|
||||
self.enqueue_after_transaction_commit = :always
|
||||
|
||||
def perform
|
||||
# noop
|
||||
end
|
||||
end
|
||||
|
||||
test "#perform_later wait for transactions to complete before enqueuing the job" do
|
||||
fake_active_record = FakeActiveRecord.new
|
||||
stub_const(Object, :ActiveRecord, fake_active_record, exists: false) do
|
||||
assert_difference -> { fake_active_record.calls }, +1 do
|
||||
EnqueueAfterCommitJob.perform_later
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
test "#perform_later returns the Job instance even if it's delayed by `after_all_transactions_commit`" do
|
||||
fake_active_record = FakeActiveRecord.new(false)
|
||||
stub_const(Object, :ActiveRecord, fake_active_record, exists: false) do
|
||||
job = EnqueueAfterCommitJob.perform_later
|
||||
assert_instance_of EnqueueAfterCommitJob, job
|
||||
assert_predicate job, :successfully_enqueued?
|
||||
end
|
||||
end
|
||||
|
||||
test "#perform_later yields the enqueued Job instance even if it's delayed by `after_all_transactions_commit`" do
|
||||
fake_active_record = FakeActiveRecord.new(false)
|
||||
stub_const(Object, :ActiveRecord, fake_active_record, exists: false) do
|
||||
called = false
|
||||
job = EnqueueAfterCommitJob.perform_later do |yielded_job|
|
||||
called = true
|
||||
assert_instance_of EnqueueAfterCommitJob, yielded_job
|
||||
end
|
||||
assert called, "#perform_later yielded the job"
|
||||
assert_instance_of EnqueueAfterCommitJob, job
|
||||
assert_predicate job, :successfully_enqueued?
|
||||
end
|
||||
end
|
||||
|
||||
test "#perform_later assumes successful enqueue, but update status later" do
|
||||
fake_active_record = FakeActiveRecord.new(false)
|
||||
stub_const(Object, :ActiveRecord, fake_active_record, exists: false) do
|
||||
job = ErrorEnqueueAfterCommitJob.perform_later
|
||||
assert_instance_of ErrorEnqueueAfterCommitJob, job
|
||||
assert_predicate job, :successfully_enqueued?
|
||||
|
||||
fake_active_record.run_after_commit_callbacks
|
||||
assert_not_predicate job, :successfully_enqueued?
|
||||
end
|
||||
end
|
||||
end
|
@ -23,3 +23,5 @@ def adapter_is?(*adapter_class_symbols)
|
||||
end
|
||||
|
||||
require_relative "../../tools/test_common"
|
||||
|
||||
ActiveJob::Base.include(ActiveJob::EnqueueAfterTransactionCommit)
|
||||
|
@ -1,3 +1,7 @@
|
||||
* `stub_const` now accepts a `exists: false` parameter to allow stubbing missing constants.
|
||||
|
||||
*Jean Boussier*
|
||||
|
||||
* Make ActiveSupport::BacktraceCleaner copy filters and silencers on dup and clone
|
||||
|
||||
Previously the copy would still share the internal silencers and filters array,
|
||||
|
@ -15,17 +15,39 @@ module ConstantStubbing
|
||||
# Using this method rather than forcing <tt>World::List::Import::LARGE_IMPORT_THRESHOLD = 5000</tt> prevents
|
||||
# warnings from being thrown, and ensures that the old value is returned after the test has completed.
|
||||
#
|
||||
# If the constant doesn't already exists, but you need it set for the duration of the block
|
||||
# you can do so by passing `exists: false`.
|
||||
#
|
||||
# stub_const(object, :SOME_CONST, 1, exists: false) do
|
||||
# assert_equal 1, SOME_CONST
|
||||
# end
|
||||
#
|
||||
# Note: Stubbing a const will stub it across all threads. So if you have concurrent threads
|
||||
# (like separate test suites running in parallel) that all depend on the same constant, it's possible
|
||||
# divergent stubbing will trample on each other.
|
||||
def stub_const(mod, constant, new_value)
|
||||
old_value = mod.const_get(constant, false)
|
||||
mod.send(:remove_const, constant)
|
||||
mod.const_set(constant, new_value)
|
||||
yield
|
||||
ensure
|
||||
mod.send(:remove_const, constant)
|
||||
mod.const_set(constant, old_value)
|
||||
def stub_const(mod, constant, new_value, exists: true)
|
||||
if exists
|
||||
begin
|
||||
old_value = mod.const_get(constant, false)
|
||||
mod.send(:remove_const, constant)
|
||||
mod.const_set(constant, new_value)
|
||||
yield
|
||||
ensure
|
||||
mod.send(:remove_const, constant)
|
||||
mod.const_set(constant, old_value)
|
||||
end
|
||||
else
|
||||
if mod.const_defined?(constant)
|
||||
raise NameError, "already defined constant #{constant} in #{mod.name}"
|
||||
end
|
||||
|
||||
begin
|
||||
mod.const_set(constant, new_value)
|
||||
yield
|
||||
ensure
|
||||
mod.send(:remove_const, constant)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
@ -580,7 +580,7 @@ class TestConstStubbing < ActiveSupport::TestCase
|
||||
assert_equal 1, ConstStubbable::CONSTANT
|
||||
end
|
||||
|
||||
test "trying to stub a constant that does not exist in the receiver raises NameError" do
|
||||
test "stubbing a constant that does not exist in the receiver raises NameError" do
|
||||
assert_raises(NameError) do
|
||||
stub_const(ConstStubbable, :NOT_A_CONSTANT, 1) { }
|
||||
end
|
||||
@ -589,4 +589,18 @@ class TestConstStubbing < ActiveSupport::TestCase
|
||||
stub_const(SubclassOfConstStubbable, :CONSTANT, 1) { }
|
||||
end
|
||||
end
|
||||
|
||||
test "stubbing a constant that does not exist can be done with `exists: false`" do
|
||||
stub_const(ConstStubbable, :NOT_A_CONSTANT, 1, exists: false) do
|
||||
assert_equal 1, ConstStubbable::NOT_A_CONSTANT
|
||||
end
|
||||
|
||||
assert_raises(NameError) do
|
||||
ConstStubbable::NOT_A_CONSTANT
|
||||
end
|
||||
|
||||
assert_raises(NameError) do
|
||||
stub_const(Object, :ConstStubbable, 1, exists: false)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
Loading…
Reference in New Issue
Block a user