Merge branch 'pl-rubocop-subtransaction-methods' into 'master'

RuboCop: Ban more methods which might cause subtransactions

See merge request gitlab-org/gitlab!69315

.rubocop_manual_todo.yml

@@ -2591,3 +2591,66 @@ Database/MultipleDatabases:
     - 'spec/support/helpers/usage_data_helpers.rb'
     - 'spec/tasks/gitlab/backup_rake_spec.rb'
     - 'spec/tasks/gitlab/db_rake_spec.rb'
+
+# WIP: https://gitlab.com/gitlab-org/gitlab/-/issues/339787
+Performance/ActiveRecordSubtransactionMethods:
+  Exclude:
+    - 'app/controllers/clusters/clusters_controller.rb'
+    - 'app/controllers/repositories/lfs_storage_controller.rb'
+    - 'app/controllers/search_controller.rb'
+    - 'app/models/application_record.rb'
+    - 'app/models/ci/ref.rb'
+    - 'app/models/container_repository.rb'
+    - 'app/models/design_management/design_collection.rb'
+    - 'app/models/error_tracking/error.rb'
+    - 'app/models/external_pull_request.rb'
+    - 'app/models/gpg_signature.rb'
+    - 'app/models/merge_request.rb'
+    - 'app/models/plan.rb'
+    - 'app/models/project.rb'
+    - 'app/models/shard.rb'
+    - 'app/models/x509_certificate.rb'
+    - 'app/models/x509_commit_signature.rb'
+    - 'app/models/x509_issuer.rb'
+    - 'app/services/bulk_imports/relation_export_service.rb'
+    - 'app/services/ci/update_build_state_service.rb'
+    - 'app/services/event_create_service.rb'
+    - 'app/services/groups/import_export/import_service.rb'
+    - 'app/services/lfs/file_transformer.rb'
+    - 'app/services/merge_requests/approval_service.rb'
+    - 'app/services/namespaces/statistics_refresher_service.rb'
+    - 'app/services/packages/rubygems/create_dependencies_service.rb'
+    - 'app/services/packages/rubygems/metadata_extraction_service.rb'
+    - 'app/services/projects/create_service.rb'
+    - 'app/services/projects/lfs_pointers/lfs_download_service.rb'
+    - 'app/services/service_desk_settings/update_service.rb'
+    - 'app/services/service_ping/submit_service.rb'
+    - 'app/services/terraform/remote_state_handler.rb'
+    - 'app/workers/namespaces/schedule_aggregation_worker.rb'
+    - 'app/workers/project_export_worker.rb'
+    - 'db/migrate/20200212014653_rename_security_dashboard_feature_flag_to_instance_security_dashboard.rb'
+    - 'db/post_migrate/20200214034836_remove_security_dashboard_feature_flag.rb'
+    - 'db/post_migrate/20210824174615_prepare_ci_builds_metadata_and_ci_build_async_indexes.rb'
+    - 'ee/app/models/ci/minutes/namespace_monthly_usage.rb'
+    - 'ee/app/models/ci/minutes/project_monthly_usage.rb'
+    - 'ee/app/models/concerns/deprecated_approvals_before_merge.rb'
+    - 'ee/app/models/ee/iteration.rb'
+    - 'ee/app/models/ee/plan.rb'
+    - 'ee/app/models/elastic/index_setting.rb'
+    - 'ee/app/models/gitlab_subscription.rb'
+    - 'ee/app/models/software_license.rb'
+    - 'ee/app/services/boards/user_preferences/update_service.rb'
+    - 'ee/app/services/ci/minutes/update_project_and_namespace_usage_service.rb'
+    - 'ee/app/services/ee/analytics/cycle_analytics/stages/base_service.rb'
+    - 'ee/app/services/security/store_report_service.rb'
+    - 'ee/app/services/security/store_scan_service.rb'
+    - 'ee/app/workers/import_software_licenses_worker.rb'
+    - 'ee/db/fixtures/production/027_plans.rb'
+    - 'ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb'
+    - 'ee/lib/gitlab/elastic/indexer.rb'
+    - 'lib/gitlab/ci/pipeline/seed/environment.rb'
+    - 'lib/gitlab/ci/pipeline/seed/processable/resource_group.rb'
+    - 'lib/gitlab/ci/trace/chunked_io.rb'
+    - 'lib/gitlab/composer/cache.rb'
+    - 'lib/gitlab/database/async_indexes/migration_helpers.rb'
+    - 'lib/gitlab/issuables_count_for_state.rb'

app/controllers/clusters/clusters_controller.rb

@@ -38,7 +38,7 @@ class Clusters::ClustersController < Clusters::BaseController
 
   def new
     if params[:provider] == 'aws'
-      @aws_role = Aws::Role.create_or_find_by!(user: current_user) # rubocop:disable Performance/ActiveRecordSubtransactionMethods
+      @aws_role = Aws::Role.create_or_find_by!(user: current_user)
       @instance_types = load_instance_types.to_json
 
     elsif params[:provider] == 'gcp'

app/models/external_pull_request.rb

@@ -89,7 +89,7 @@ class ExternalPullRequest < ApplicationRecord
   end
 
   def self.safe_find_or_initialize_and_update(find:, update:)
-    safe_ensure_unique(retries: 1) do # rubocop:disable Performance/ActiveRecordSubtransactionMethods
+    safe_ensure_unique(retries: 1) do
       model = find_or_initialize_by(find)
 
       if model.update(update)

app/services/merge_requests/approval_service.rb

@@ -35,7 +35,7 @@ module MergeRequests
     end
 
     def save_approval(approval)
-      Approval.safe_ensure_unique do # rubocop:disable Performance/ActiveRecordSubtransactionMethods
+      Approval.safe_ensure_unique do
         approval.save
       end
     end

app/services/terraform/remote_state_handler.rb

@@ -70,7 +70,7 @@ module Terraform
 
       return find_state!(find_params) if find_only
 
-      state = Terraform::State.create_or_find_by(find_params) # rubocop:disable Performance/ActiveRecordSubtransactionMethods
+      state = Terraform::State.create_or_find_by(find_params)
 
       # https://github.com/rails/rails/issues/36027
       return state unless state.errors.of_kind? :name, :taken

ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb

@@ -59,7 +59,7 @@ module EE
           end
 
           def self.safe_find_or_create_by(*args)
-            safe_ensure_unique(retries: 1) do # rubocop:disable Performance/ActiveRecordSubtransactionMethods
+            safe_ensure_unique(retries: 1) do
               find_or_create_by(*args)
             end
           end

rubocop/cop/performance/active_record_subtransaction_methods.rb

@@ -11,9 +11,12 @@ module RuboCop
 
         DISALLOWED_METHODS = %i[
           safe_ensure_unique
+          safe_find_or_create_by
+          safe_find_or_create_by!
+          with_fast_read_statement_timeout
           create_or_find_by
           create_or_find_by!
-        ].freeze
+        ].to_set.freeze
 
         def on_send(node)
           return unless DISALLOWED_METHODS.include?(node.method_name)

spec/rubocop/cop/performance/active_record_subtransaction_methods_spec.rb

 # frozen_string_literal: true
 
 require 'fast_spec_helper'
+require 'rspec-parameterized'
+
 require_relative '../../../../rubocop/cop/performance/active_record_subtransaction_methods'
 
 RSpec.describe RuboCop::Cop::Performance::ActiveRecordSubtransactionMethods do
@@ -10,16 +12,18 @@ RSpec.describe RuboCop::Cop::Performance::ActiveRecordSubtransactionMethods do
 
   shared_examples 'a method that uses a subtransaction' do |method_name|
     it 'registers an offense' do
-      expect_offense(<<~RUBY)
-        Project.#{method_name}
-                #{'^' * method_name.length} #{message}
+      expect_offense(<<~RUBY, method_name: method_name, message: message)
+        Project.%{method_name}
+                ^{method_name} %{message}
       RUBY
     end
   end
 
   context 'when the method uses a subtransaction' do
-    described_class::DISALLOWED_METHODS.each do |method|
-      it_behaves_like 'a method that uses a subtransaction', method
+    where(:method) { described_class::DISALLOWED_METHODS.to_a }
+
+    with_them do
+      include_examples 'a method that uses a subtransaction', params[:method]
     end
   end
 end