Merge branch '338730-cop-to-prevent-methods-using-subtransactions' into 'master'

Cop to disallow methods that use subtransactions

See merge request gitlab-org/gitlab!68594

.rubocop.yml

@@ -717,3 +717,8 @@ Performance/ActiveRecordSubtransactions:
   Exclude:
     - 'spec/**/*.rb'
     - 'ee/spec/**/*.rb'
+
+Performance/ActiveRecordSubtransactionMethods:
+  Exclude:
+    - 'spec/**/*.rb'
+    - 'ee/spec/**/*.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)
+      @aws_role = Aws::Role.create_or_find_by!(user: current_user) # rubocop:disable Performance/ActiveRecordSubtransactionMethods
       @instance_types = load_instance_types.to_json
 
     elsif params[:provider] == 'gcp'

app/models/application_record.rb

@@ -66,7 +66,7 @@ class ApplicationRecord < ActiveRecord::Base
   def self.safe_find_or_create_by(*args, &block)
     return optimized_safe_find_or_create_by(*args, &block) if Feature.enabled?(:optimize_safe_find_or_create_by, default_enabled: :yaml)
 
-    safe_ensure_unique(retries: 1) do
+    safe_ensure_unique(retries: 1) do # rubocop:disable Performance/ActiveRecordSubtransactionMethods
       find_or_create_by(*args, &block)
     end
   end

app/models/concerns/cache_markdown_field.rb

@@ -171,7 +171,7 @@ module CacheMarkdownField
 
     # One retry is enough as next time `model_user_mention` should return the existing mention record,
     # that threw the `ActiveRecord::RecordNotUnique` exception in first place.
-    self.class.safe_ensure_unique(retries: 1) do
+    self.class.safe_ensure_unique(retries: 1) do # rubocop:disable Performance/ActiveRecordSubtransactionMethods
       user_mention = model_user_mention
 
       # this may happen due to notes polymorphism, so noteable_id may point to a record

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
+    safe_ensure_unique(retries: 1) do # rubocop:disable Performance/ActiveRecordSubtransactionMethods
       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
+      Approval.safe_ensure_unique do # rubocop:disable Performance/ActiveRecordSubtransactionMethods
         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)
+      state = Terraform::State.create_or_find_by(find_params) # rubocop:disable Performance/ActiveRecordSubtransactionMethods
 
       # 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
+            safe_ensure_unique(retries: 1) do # rubocop:disable Performance/ActiveRecordSubtransactionMethods
               find_or_create_by(*args)
             end
           end

rubocop/cop/performance/active_record_subtransaction_methods.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Performance
+      # Cop that disallows certain methods that rely on subtransactions in their implementation.
+      # Companion to Performance/ActiveRecordSubtransactions, which bans direct usage of subtransactions.
+      class ActiveRecordSubtransactionMethods < RuboCop::Cop::Cop
+        MSG = 'Methods that rely on subtransactions should not be used. ' \
+          'For more information see: https://gitlab.com/gitlab-org/gitlab/-/issues/338346'
+
+        DISALLOWED_METHODS = %i[
+          safe_ensure_unique
+          create_or_find_by
+          create_or_find_by!
+        ].freeze
+
+        def on_send(node)
+          return unless DISALLOWED_METHODS.include?(node.method_name)
+
+          add_offense(node, location: :selector)
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/performance/active_record_subtransaction_methods_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require_relative '../../../../rubocop/cop/performance/active_record_subtransaction_methods'
+
+RSpec.describe RuboCop::Cop::Performance::ActiveRecordSubtransactionMethods do
+  subject(:cop) { described_class.new }
+
+  let(:message) { described_class::MSG }
+
+  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}
+      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
+    end
+  end
+end