Merge branch 'tchu-add-worker-find_by_id-cop' into 'master'

Add cop to avoid find in Sidekiq workers

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



Merged-by: Peter Leitzen <pleitzen@gitlab.com>
Approved-by: Serena Fang <sfang@gitlab.com>
Approved-by: Peter Leitzen <pleitzen@gitlab.com>
Reviewed-by: Peter Leitzen <pleitzen@gitlab.com>
Reviewed-by: Terri Chu <tchu@gitlab.com>
Co-authored-by: Terri Chu <tchu@gitlab.com>

.rubocop.yml

@@ -1214,3 +1214,9 @@ Rails/StrongParams:
 QA/FabricateUsage:
   Include:
     - 'qa/qa/specs/**/*_spec.rb'
+
+Gitlab/NoFindInWorkers:
+  Enabled: true
+  Include:
+    - app/workers/**/*
+    - ee/app/workers/**/*

.rubocop_todo/gitlab/no_find_in_workers.yml

+---
+Gitlab/NoFindInWorkers:
+  Details: grace period
+  Exclude:
+    - 'app/workers/approve_blocked_pending_approval_users_worker.rb'
+    - 'app/workers/auto_devops/disable_worker.rb'
+    - 'app/workers/bulk_imports/entity_worker.rb'
+    - 'app/workers/bulk_imports/export_request_worker.rb'
+    - 'app/workers/bulk_imports/finish_batched_pipeline_worker.rb'
+    - 'app/workers/bulk_imports/pipeline_batch_worker.rb'
+    - 'app/workers/bulk_imports/pipeline_worker.rb'
+    - 'app/workers/bulk_imports/relation_batch_export_worker.rb'
+    - 'app/workers/bulk_imports/relation_export_worker.rb'
+    - 'app/workers/bulk_imports/user_contributions_export_worker.rb'
+    - 'app/workers/concerns/cluster_applications.rb'
+    - 'app/workers/concerns/project_import_options.rb'
+    - 'app/workers/create_pipeline_worker.rb'
+    - 'app/workers/delete_diff_files_worker.rb'
+    - 'app/workers/delete_merged_branches_worker.rb'
+    - 'app/workers/design_management/copy_design_collection_worker.rb'
+    - 'app/workers/design_management/new_version_worker.rb'
+    - 'app/workers/disallow_two_factor_for_group_worker.rb'
+    - 'app/workers/disallow_two_factor_for_subgroups_worker.rb'
+    - 'app/workers/emails_on_push_worker.rb'
+    - 'app/workers/error_tracking_issue_link_worker.rb'
+    - 'app/workers/export_csv_worker.rb'
+    - 'app/workers/gitlab/bitbucket_import/advance_stage_worker.rb'
+    - 'app/workers/gitlab/bitbucket_server_import/advance_stage_worker.rb'
+    - 'app/workers/gitlab/github_gists_import/import_gist_worker.rb'
+    - 'app/workers/gitlab/github_gists_import/start_import_worker.rb'
+    - 'app/workers/gitlab/github_import/advance_stage_worker.rb'
+    - 'app/workers/gitlab/jira_import/advance_stage_worker.rb'
+    - 'app/workers/google_cloud/create_cloudsql_instance_worker.rb'
+    - 'app/workers/group_destroy_worker.rb'
+    - 'app/workers/group_export_worker.rb'
+    - 'app/workers/group_import_worker.rb'
+    - 'app/workers/import_issues_csv_worker.rb'
+    - 'app/workers/integrations/irker_worker.rb'
+    - 'app/workers/issuable/create_reminder_worker.rb'
+    - 'app/workers/issuable_export_csv_worker.rb'
+    - 'app/workers/issues/placement_worker.rb'
+    - 'app/workers/members_destroyer/unassign_issuables_worker.rb'
+    - 'app/workers/merge_requests/delete_source_branch_worker.rb'
+    - 'app/workers/merge_requests/handle_assignees_change_worker.rb'
+    - 'app/workers/merge_requests/resolve_todos_worker.rb'
+    - 'app/workers/merge_worker.rb'
+    - 'app/workers/namespaces/root_statistics_worker.rb'
+    - 'app/workers/namespaces/schedule_aggregation_worker.rb'
+    - 'app/workers/onboarding/user_added_worker.rb'
+    - 'app/workers/packages/go/sync_packages_worker.rb'
+    - 'app/workers/packages/npm/deprecate_package_worker.rb'
+    - 'app/workers/project_destroy_worker.rb'
+    - 'app/workers/project_export_worker.rb'
+    - 'app/workers/projects/after_import_worker.rb'
+    - 'app/workers/projects/git_garbage_collect_worker.rb'
+    - 'app/workers/projects/import_export/parallel_project_export_worker.rb'
+    - 'app/workers/projects/import_export/relation_export_worker.rb'
+    - 'app/workers/projects/import_export/relation_import_worker.rb'
+    - 'app/workers/projects/import_export/wait_relation_exports_worker.rb'
+    - 'app/workers/projects/inactive_projects_deletion_notification_worker.rb'
+    - 'app/workers/projects/update_repository_storage_worker.rb'
+    - 'app/workers/rebase_worker.rb'
+    - 'app/workers/repository_check/single_repository_worker.rb'
+    - 'app/workers/repository_cleanup_worker.rb'
+    - 'app/workers/repository_fork_worker.rb'
+    - 'app/workers/snippets/update_repository_storage_worker.rb'
+    - 'app/workers/upload_checksum_worker.rb'
+    - 'app/workers/wikis/git_garbage_collect_worker.rb'
+    - 'app/workers/work_items/import_work_items_csv_worker.rb'
+    - 'ee/app/workers/abuse/new_abuse_report_worker.rb'
+    - 'ee/app/workers/adjourned_project_deletion_worker.rb'
+    - 'ee/app/workers/admin_emails_worker.rb'
+    - 'ee/app/workers/analytics/devops_adoption/create_snapshot_worker.rb'
+    - 'ee/app/workers/approval_rules/external_approval_rule_payload_worker.rb'
+    - 'ee/app/workers/audit_events/audit_event_streaming_worker.rb'
+    - 'ee/app/workers/ci/runners/export_usage_csv_worker.rb'
+    - 'ee/app/workers/compliance_management/chain_of_custody_report_worker.rb'
+    - 'ee/app/workers/compliance_management/framework_export_mailer_worker.rb'
+    - 'ee/app/workers/compliance_management/pending_status_check_worker.rb'
+    - 'ee/app/workers/compliance_management/project_framework_export_mailer_worker.rb'
+    - 'ee/app/workers/compliance_management/standards_adherence_export_mailer_worker.rb'
+    - 'ee/app/workers/compliance_management/update_default_framework_worker.rb'
+    - 'ee/app/workers/compliance_management/violation_export_mailer_worker.rb'
+    - 'ee/app/workers/create_github_webhook_worker.rb'
+    - 'ee/app/workers/dependencies/export_worker.rb'
+    - 'ee/app/workers/elastic/namespace_update_worker.rb'
+    - 'ee/app/workers/elastic/project_transfer_worker.rb'
+    - 'ee/app/workers/elastic_association_indexer_worker.rb'
+    - 'ee/app/workers/elastic_namespace_indexer_worker.rb'
+    - 'ee/app/workers/gitlab/export/segmented_export_finalisation_worker.rb'
+    - 'ee/app/workers/gitlab/export/segmented_export_worker.rb'
+    - 'ee/app/workers/group_wikis/git_garbage_collect_worker.rb'
+    - 'ee/app/workers/groups/update_repository_storage_worker.rb'
+    - 'ee/app/workers/namespaces/cascade_duo_features_enabled_worker.rb'
+    - 'ee/app/workers/namespaces/storage_usage_export_worker.rb'
+    - 'ee/app/workers/repository_update_mirror_worker.rb'
+    - 'ee/app/workers/requirements_management/import_requirements_csv_worker.rb'
+    - 'ee/app/workers/search/zoekt/namespace_indexer_worker.rb'
+    - 'ee/app/workers/work_items/rolledup_dates/bulk_update_handler.rb'
+    - 'ee/app/workers/work_items/rolledup_dates/update_rolledup_dates_worker.rb'

ee/app/workers/llm/completion_worker.rb

@@ -21,11 +21,13 @@ def serialize_message(message)
       end
 
       def deserialize_message(message_hash, options)
+        # rubocop: disable Gitlab/NoFindInWorkers -- not ActiveRecordFind
         message_hash['user'] &&= GitlabSchema.parse_gid(message_hash['user']).find
         message_hash['context'] = begin
           message_hash['context']['resource'] &&= GitlabSchema.parse_gid(message_hash['context']['resource']).find
           ::Gitlab::Llm::AiMessageContext.new(message_hash['context'])
         end
+        # rubocop: enable Gitlab/NoFindInWorkers
 
         ::Gitlab::Llm::AiMessage.for(action: message_hash['ai_action']).new(options.merge(message_hash))
       end

gems/config/rubocop.yml

@@ -139,3 +139,7 @@ Rails/StrongParams:
   Enabled: true
   Include:
     - '**/controllers/**/*'
+
+# This cop doesn't make sense in the context of gems
+Gitlab/NoFindInWorkers:
+  Enabled: false

rubocop/cop/gitlab/no_find_in_workers.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Gitlab
+      # Checks for use of ActiveRecord find in Sidekiq workers.
+      #
+      # @example
+      #   # bad
+      #   class ExampleWorker
+      #     def perform
+      #       record = Klass.find(id)
+      #     end
+      #   end
+      #
+      #   # good
+      #   class ExampleWorker
+      #     def perform
+      #       record = Klass.find_by_id(id)
+      #       return unless record
+      #     end
+      #   end
+      #
+      class NoFindInWorkers < RuboCop::Cop::Base
+        DOC_LINK = 'https://docs.gitlab.com/ee/development/sidekiq/#retries'
+
+        RESTRICT_ON_SEND = [:find].freeze
+
+        MSG = <<~MSG.freeze
+          Refrain from using `find`, use `find_by` instead. See #{DOC_LINK}.
+        MSG
+
+        def_node_matcher :find_method?, <<~PATTERN
+          (send _ :find ...)
+        PATTERN
+
+        def on_send(node)
+          add_offense(node, message: MSG)
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/gitlab/no_find_in_workers_spec.rb

+# frozen_string_literal: true
+
+require 'rubocop_spec_helper'
+require_relative '../../../../rubocop/cop/gitlab/no_find_in_workers'
+
+RSpec.describe RuboCop::Cop::Gitlab::NoFindInWorkers, feature_category: :shared do
+  context 'when find is used' do
+    it 'adds an offense' do
+      expect_offense(<<~CODE)
+        class SomeWorker
+          include ApplicationWorker
+
+          def perform
+            namespace = Namespace.find(namespace_id)
+                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Refrain from using `find`, use `find_by` instead. See https://docs.gitlab.com/ee/development/sidekiq/#retries.[...]
+          end
+        end
+      CODE
+    end
+  end
+
+  context 'when find is not used' do
+    it 'adds no offense' do
+      expect_no_offenses(<<~CODE)
+        class SomeWorker
+          include ApplicationWorker
+
+          def perform
+            namespace = Namespace.find_by(namespace_id)
+            return unless namespace
+          end
+        end
+      CODE
+    end
+  end
+end