diff --git a/.rubocop.yml b/.rubocop.yml index 53ac2ca2eacf0a3b5e0c8b159179b873b2c8575b..16c528bc9845c6a059c2142c21d3aa223888f5e9 100644 --- a/.rubocop.yml +++ b/.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/**/* diff --git a/.rubocop_todo/gitlab/no_find_in_workers.yml b/.rubocop_todo/gitlab/no_find_in_workers.yml new file mode 100644 index 0000000000000000000000000000000000000000..41d3ff23fe9167a072a681f127b2f81ca2f9d60f --- /dev/null +++ b/.rubocop_todo/gitlab/no_find_in_workers.yml @@ -0,0 +1,100 @@ +--- +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' diff --git a/ee/app/workers/llm/completion_worker.rb b/ee/app/workers/llm/completion_worker.rb index 6626e245a7ed1dbf45f3aeaeb4c1a895b61183b0..d0f5ee99bbd1f801c5ea96390e010340deac1c2e 100644 --- a/ee/app/workers/llm/completion_worker.rb +++ b/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 diff --git a/gems/config/rubocop.yml b/gems/config/rubocop.yml index 63a859c2b3f03c1aab2f6986db5a05ef495d76cc..390c9e6b6b0270587b6e31fb52cd16ff327811e5 100644 --- a/gems/config/rubocop.yml +++ b/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 diff --git a/rubocop/cop/gitlab/no_find_in_workers.rb b/rubocop/cop/gitlab/no_find_in_workers.rb new file mode 100644 index 0000000000000000000000000000000000000000..1bb02dd31fc60166b1a84427cfbc03d342ddccfc --- /dev/null +++ b/rubocop/cop/gitlab/no_find_in_workers.rb @@ -0,0 +1,43 @@ +# 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 diff --git a/spec/rubocop/cop/gitlab/no_find_in_workers_spec.rb b/spec/rubocop/cop/gitlab/no_find_in_workers_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..237bb623e23cb529fd9f2954b6657718aaeb2308 --- /dev/null +++ b/spec/rubocop/cop/gitlab/no_find_in_workers_spec.rb @@ -0,0 +1,36 @@ +# 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