Merge branch 'sc1-update-sidekiqapi-cop' into 'master'

Update SidekiqApiUsage cop to caution about shard-awareness

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/148609



Merged-by: Tianwen Chen <tchen@gitlab.com>
Approved-by: Tianwen Chen <tchen@gitlab.com>
Co-authored-by: Sylvester Chin <schin@gitlab.com>

doc/development/sidekiq/index.md

@@ -29,6 +29,19 @@ All workers should include `ApplicationWorker` instead of `Sidekiq::Worker`,
 which adds some convenience methods and automatically sets the queue based on
 the [routing rules](../../administration/sidekiq/processing_specific_job_classes.md#routing-rules).
 
+## Sharding
+
+All calls to Sidekiq APIs must account for sharding. To achieve this,
+utilize the Sidekiq API within the `Sidekiq::Client.via` block to guarantee the correct `Sidekiq.redis` pool is utilized.
+Obtain the suitable Redis pool by invoking the `Gitlab::SidekiqSharding::Router.get_shard_instance` method.
+
+```ruby
+pool_name, pool = Gitlab::SidekiqSharding::Router.get_shard_instance(worker_class.sidekiq_options['store'])
+Sidekiq::Client.via(pool) do
+  ...
+end
+```
+
 ## Retries
 
 Sidekiq defaults to using [25 retries](https://github.com/mperham/sidekiq/wiki/Error-Handling#automatic-job-retry),

rubocop/cop/sidekiq_api_usage.rb

@@ -3,8 +3,11 @@
 module RuboCop
   module Cop
     class SidekiqApiUsage < RuboCop::Cop::Base
-      MSG = 'Refrain from directly using Sidekiq APIs.' \
-          'Only permitted in migrations, administrations and Sidekiq middlewares.'
+      MSG = 'Refrain from directly using Sidekiq APIs. ' \
+          'Only permitted in migrations, administrations and Sidekiq middlewares. ' \
+          'When disabling the cop, ensure that Sidekiq APIs are wrapped with ' \
+          'Sidekiq::Client.via(..) { ... } block to remain shard aware. ' \
+          'See doc/development/sidekiq/index.md#sharding for more information.'
 
       ALLOWED_WORKER_METHODS = [
         :skipping_transaction_check,
@@ -12,6 +15,8 @@ class SidekiqApiUsage < RuboCop::Cop::Base
         :raise_exception_for_being_inside_a_transaction?
       ].freeze
 
+      ALLOWED_CLIENT_METHODS = [:via].freeze
+
       def_node_matcher :using_sidekiq_api?, <<~PATTERN
         (send (const (const nil? :Sidekiq) $_  ) $... )
       PATTERN
@@ -23,6 +28,9 @@ def on_send(node)
           # allow methods defined in config/initializers/forbid_sidekiq_in_transactions.rb
           next if klass == :Worker && ALLOWED_WORKER_METHODS.include?(methods_called[0])
 
+          # allow Sidekiq::Client.via calls
+          next if klass == :Client && ALLOWED_CLIENT_METHODS.include?(methods_called[0])
+
           add_offense(node, message: MSG)
         end
       end

spec/rubocop/cop/sidekiq_api_usage_spec.rb

@@ -27,6 +27,19 @@
       PATTERN
     end
 
+    it 'registers no offences for calling .via' do
+      expect_no_offenses(<<~PATTERN)
+        Sidekiq::Client.via { "testing" }
+      PATTERN
+    end
+
+    it 'registers offence for calling other Sidekiq::Client methods' do
+      expect_offense(<<~PATTERN)
+        Sidekiq::Client.push('test')
+        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ #{msg}
+      PATTERN
+    end
+
     it 'registers offence for calling other Sidekiq::Worker methods' do
       expect_offense(<<~PATTERN)
         Sidekiq::Worker.drain_all