Add rubocops to ensure Sidekiq workers include ApplicationWorker and don't manually set their queue

7 files changed, 117 insertions, 6 deletions

diff --git a/app/workers/concerns/application_worker.rb b/app/workers/concerns/application_worker.rb
index 285b5bada7d..37586e161c9 100644
--- a/app/workers/concerns/application_worker.rb
+++ b/app/workers/concerns/application_worker.rb
@@ -3,7 +3,7 @@ Sidekiq::Worker.extend ActiveSupport::Concern
 module ApplicationWorker
   extend ActiveSupport::Concern
 
-  include Sidekiq::Worker
+  include Sidekiq::Worker # rubocop:disable Cop/IncludeSidekiqWorker
 
   included do
     set_queue
@@ -17,7 +17,7 @@ module ApplicationWorker
     def set_queue
       queue_name = [queue_namespace, base_queue_name].compact.join(':')
 
-      sidekiq_options queue: queue_name
+      sidekiq_options queue: queue_name # rubocop:disable Cop/SidekiqOptionsQueue
     end
 
     def base_queue_name
diff --git a/rubocop/cop/include_sidekiq_worker.rb b/rubocop/cop/include_sidekiq_worker.rb
new file mode 100644
index 00000000000..4a6332286a2
--- /dev/null
+++ b/rubocop/cop/include_sidekiq_worker.rb
@@ -0,0 +1,29 @@
+require_relative '../spec_helpers'
+
+module RuboCop
+  module Cop
+    # Cop that makes sure workers include `ApplicationWorker`, not `Sidekiq::Worker`.
+    class IncludeSidekiqWorker < RuboCop::Cop::Cop
+      include SpecHelpers
+
+      MSG = 'Include `ApplicationWorker`, not `Sidekiq::Worker`.'.freeze
+
+      def_node_matcher :includes_sidekiq_worker?, <<~PATTERN
+        (send nil :include (const (const nil :Sidekiq) :Worker))
+      PATTERN
+
+      def on_send(node)
+        return if in_spec?(node)
+        return unless includes_sidekiq_worker?(node)
+
+        add_offense(node.arguments.first, :expression)
+      end
+
+      def autocorrect(node)
+        lambda do |corrector|
+          corrector.replace(node.source_range, 'ApplicationWorker')
+        end
+      end
+    end
+  end
+end
diff --git a/rubocop/cop/sidekiq_options_queue.rb b/rubocop/cop/sidekiq_options_queue.rb
new file mode 100644
index 00000000000..43b35ba0214
--- /dev/null
+++ b/rubocop/cop/sidekiq_options_queue.rb
@@ -0,0 +1,27 @@
+require_relative '../spec_helpers'
+
+module RuboCop
+  module Cop
+    # Cop that prevents manually setting a queue in Sidekiq workers.
+    class SidekiqOptionsQueue < RuboCop::Cop::Cop
+      include SpecHelpers
+
+      MSG = 'Do not manually set a queue; `ApplicationWorker` sets one automatically.'.freeze
+
+      def_node_matcher :sidekiq_options?, <<~PATTERN
+        (send nil :sidekiq_options $...)
+      PATTERN
+
+      def on_send(node)
+        return if in_spec?(node)
+        return unless sidekiq_options?(node)
+
+        node.arguments.first.each_node(:pair) do |pair|
+          key_name = pair.key.children[0]
+
+          add_offense(pair, :expression) if key_name == :queue
+        end
+      end
+    end
+  end
+end
diff --git a/rubocop/rubocop.rb b/rubocop/rubocop.rb
index eb52be3d731..3e3b4c8349a 100644
--- a/rubocop/rubocop.rb
+++ b/rubocop/rubocop.rb
@@ -3,10 +3,12 @@ require_relative 'cop/active_record_serialize'
 require_relative 'cop/custom_error_class'
 require_relative 'cop/gem_fetcher'
 require_relative 'cop/in_batches'
+require_relative 'cop/include_sidekiq_worker'
 require_relative 'cop/line_break_after_guard_clauses'
 require_relative 'cop/polymorphic_associations'
 require_relative 'cop/project_path_helper'
 require_relative 'cop/redirect_with_status'
+require_relative 'cop/sidekiq_options_queue'
 require_relative 'cop/migration/add_column'
 require_relative 'cop/migration/add_concurrent_foreign_key'
 require_relative 'cop/migration/add_concurrent_index'
diff --git a/spec/rubocop/cop/include_sidekiq_worker_spec.rb b/spec/rubocop/cop/include_sidekiq_worker_spec.rb
new file mode 100644
index 00000000000..7f406535dda
--- /dev/null
+++ b/spec/rubocop/cop/include_sidekiq_worker_spec.rb
@@ -0,0 +1,31 @@
+require 'spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../rubocop/cop/include_sidekiq_worker'
+
+describe RuboCop::Cop::IncludeSidekiqWorker do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  context 'when `Sidekiq::Worker` is included' do
+    let(:source) { 'include Sidekiq::Worker' }
+    let(:correct_source) { 'include ApplicationWorker' }
+
+    it 'registers an offense ' do
+      inspect_source(cop, source)
+
+      aggregate_failures do
+        expect(cop.offenses.size).to eq(1)
+        expect(cop.offenses.map(&:line)).to eq([1])
+        expect(cop.highlights).to eq(['Sidekiq::Worker'])
+      end
+    end
+
+    it 'autocorrects to the right version' do
+      autocorrected = autocorrect_source(cop, source)
+
+      expect(autocorrected).to eq(correct_source)
+    end
+  end
+end
diff --git a/spec/rubocop/cop/sidekiq_options_queue_spec.rb b/spec/rubocop/cop/sidekiq_options_queue_spec.rb
new file mode 100644
index 00000000000..a31de381631
--- /dev/null
+++ b/spec/rubocop/cop/sidekiq_options_queue_spec.rb
@@ -0,0 +1,26 @@
+require 'spec_helper'
+require 'rubocop'
+require 'rubocop/rspec/support'
+require_relative '../../../rubocop/cop/sidekiq_options_queue'
+
+describe RuboCop::Cop::SidekiqOptionsQueue do
+  include CopHelper
+
+  subject(:cop) { described_class.new }
+
+  it 'registers an offense when `sidekiq_options` is used with the `queue` option' do
+    inspect_source(cop, 'sidekiq_options queue: "some_queue"')
+
+    aggregate_failures do
+      expect(cop.offenses.size).to eq(1)
+      expect(cop.offenses.map(&:line)).to eq([1])
+      expect(cop.highlights).to eq(['queue: "some_queue"'])
+    end
+  end
+
+  it 'does not register an offense when `sidekiq_options` is used with another option' do
+    inspect_source(cop, 'sidekiq_options retry: false')
+
+    expect(cop.offenses).to be_empty
+  end
+end
diff --git a/spec/workers/every_sidekiq_worker_spec.rb b/spec/workers/every_sidekiq_worker_spec.rb
index bf99c24e73c..9e3b99b3502 100644
--- a/spec/workers/every_sidekiq_worker_spec.rb
+++ b/spec/workers/every_sidekiq_worker_spec.rb
@@ -1,10 +1,6 @@
 require 'spec_helper'
 
 describe 'Every Sidekiq worker' do
-  it 'includes ApplicationWorker' do
-    expect(Gitlab::SidekiqConfig.workers).to all(include(ApplicationWorker))
-  end
-
   it 'does not use the default queue' do
     expect(Gitlab::SidekiqConfig.workers.map(&:queue)).not_to include('default')
   end