Skip to content

Commit

Permalink
fix: prevent sidekiq_options from overriding ActiveJob queue settings
Browse files Browse the repository at this point in the history
  • Loading branch information
RickCSong authored and marcelolx committed Mar 30, 2022
1 parent 041d2b5 commit e3d4f7a
Show file tree
Hide file tree
Showing 5 changed files with 48 additions and 16 deletions.
4 changes: 3 additions & 1 deletion lib/sidekiq-scheduler/schedule.rb
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,9 @@ def prepare_schedule(schedule_hash)
def infer_queue(klass)
klass = try_to_constantize(klass)

if klass.respond_to?(:sidekiq_options)
# ActiveJob uses queue_as when the job is created
# to determine the queue
if klass.respond_to?(:sidekiq_options) && !SidekiqScheduler::Utils.active_job_enqueue?(klass)
klass.sidekiq_options['queue']
end
end
Expand Down
13 changes: 1 addition & 12 deletions lib/sidekiq-scheduler/scheduler.rb
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ def enqueue_job(job_config, time = Time.now)
config['args'] = arguments_with_metadata(config['args'], scheduled_at: time.to_f)
end

if active_job_enqueue?(config['class'])
if SidekiqScheduler::Utils.active_job_enqueue?(config['class'])
SidekiqScheduler::Utils.enqueue_with_active_job(config)
else
SidekiqScheduler::Utils.enqueue_with_sidekiq(config)
Expand Down Expand Up @@ -307,17 +307,6 @@ def enabled_queue?(job_queue, queues)
queues.empty? || queues.include?(job_queue)
end

# Returns true if the enqueuing needs to be done for an ActiveJob
# class false otherwise.
#
# @param [Class] klass the class to check is decendant from ActiveJob
#
# @return [Boolean]
def active_job_enqueue?(klass)
klass.is_a?(Class) && defined?(ActiveJob::Enqueuing) &&
klass.included_modules.include?(ActiveJob::Enqueuing)
end

# Convert the given arguments in the format expected to be enqueued.
#
# @param [Hash] config the options to be converted
Expand Down
11 changes: 11 additions & 0 deletions lib/sidekiq-scheduler/utils.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,17 @@ def self.initialize_active_job(klass, args)
end
end

# Returns true if the enqueuing needs to be done for an ActiveJob
# class false otherwise.
#
# @param [Class] klass the class to check is decendant from ActiveJob
#
# @return [Boolean]
def self.active_job_enqueue?(klass)
klass.is_a?(Class) && defined?(ActiveJob::Enqueuing) &&
klass.included_modules.include?(ActiveJob::Enqueuing)
end

# Enqueues the job using the Sidekiq client.
#
# @param [Hash] config The job configuration
Expand Down
11 changes: 11 additions & 0 deletions spec/sidekiq-scheduler/schedule_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,17 @@
expect(SidekiqScheduler::Store.job_from_redis(job_id)['queue']).to eq('system')
end
end

context 'when job is an ActiveJob job' do
let(:job_id) { 'email_sender' }
let(:schedule) { { 'class' => 'EmailSender' } }

it 'does not set the queue name' do
subject

expect(SidekiqScheduler::Store.job_from_redis(job_id)['queue']).to eq(nil)
end
end
end

describe '.set_schedule' do
Expand Down
25 changes: 22 additions & 3 deletions spec/sidekiq-scheduler/utils_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -237,23 +237,42 @@
end

describe '.enqueue_with_active_job' do
subject { described_class.enqueue_with_active_job(job) }
subject { described_class.enqueue_with_active_job(config) }

let(:job) { { 'class' => EmailSender, 'args' => [] } }
let(:config) { { 'class' => job, 'args' => args, 'queue' => queue } }
let(:job) { EmailSender }
let(:args) { [] }
let(:queue) { nil }

it 'with no args' do
expect(EmailSender).to receive(:new).with(no_args).twice.and_call_original

subject

expect(subject).to have_attributes(arguments: args, queue_name: 'email')
end

context 'with args' do
let(:job) { { 'class' => AddressUpdater, 'args' => [100] } }
let(:job) { AddressUpdater }
let(:args) { [100] }

it 'should be correctly enqueued' do
expect(AddressUpdater).to receive(:new).with(100).and_call_original
expect(AddressUpdater).to receive(:new).with(no_args).and_call_original

expect(subject).to have_attributes(arguments: args, queue_name: 'default')
end
end

context 'with queue name set by config' do
let(:queue) { 'critical' }

it 'should be correctly enqueued' do
expect(EmailSender).to receive(:new).with(no_args).twice.and_call_original

subject

expect(subject).to have_attributes(arguments: args, queue_name: queue)
end
end
end
Expand Down

0 comments on commit e3d4f7a

Please sign in to comment.