Merge branch '338730-rubocop-rule-to-prevent-subtransactions' into 'master'

Add rubocop to prevent use of subtransactions

See merge request gitlab-org/gitlab!68576

.rubocop.yml

@@ -712,3 +712,8 @@ QA/SelectorUsage:
     - 'ee/spec/**/*.rb'
   Exclude:
     - 'spec/rubocop/**/*_spec.rb'
+
+Performance/ActiveRecordSubtransactions:
+  Exclude:
+    - 'spec/**/*.rb'
+    - 'ee/spec/**/*.rb'

app/models/application_record.rb

@@ -31,7 +31,7 @@ class ApplicationRecord < ActiveRecord::Base
   end
 
   def self.safe_ensure_unique(retries: 0)
-    transaction(requires_new: true) do
+    transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
       yield
     end
   rescue ActiveRecord::RecordNotUnique
@@ -55,7 +55,7 @@ class ApplicationRecord < ActiveRecord::Base
   # currently one third of the default 15-second timeout
   def self.with_fast_read_statement_timeout(timeout_ms = 5000)
     ::Gitlab::Database::LoadBalancing::Session.current.fallback_to_replicas_for_ambiguous_queries do
-      transaction(requires_new: true) do
+      transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
         connection.exec_query("SET LOCAL statement_timeout = #{timeout_ms}")
 
         yield
@@ -80,7 +80,7 @@ class ApplicationRecord < ActiveRecord::Base
     #
     # When calling this method on an association, just calling `self.create` would call `ActiveRecord::Persistence.create`
     # and that skips some code that adds the newly created record to the association.
-    transaction(requires_new: true) { all.create(*args, &block) }
+    transaction(requires_new: true) { all.create(*args, &block) } # rubocop:disable Performance/ActiveRecordSubtransactions
   rescue ActiveRecord::RecordNotUnique
     find_by(*args)
   end

app/models/application_setting.rb

@@ -622,7 +622,7 @@ class ApplicationSetting < ApplicationRecord
   def self.create_from_defaults
     check_schema!
 
-    transaction(requires_new: true) do
+    transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
       super
     end
   rescue ActiveRecord::RecordNotUnique

app/models/internal_id.rb

@@ -239,7 +239,7 @@ class InternalId < ApplicationRecord
         lookup
       else
         begin
-          subject.transaction(requires_new: true) do
+          subject.transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
             InternalId.create!(
               **scope,
               usage: usage_value,
@@ -362,7 +362,7 @@ class InternalId < ApplicationRecord
         value
       else
         begin
-          subject.transaction(requires_new: true) do
+          subject.transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
             internal_id = InternalId.create!(**scope, usage: usage, last_value: value)
             internal_id.last_value
           end

app/services/projects/move_deploy_keys_projects_service.rb

@@ -5,7 +5,7 @@ module Projects
     def execute(source_project, remove_remaining_elements: true)
       return unless super
 
-      Project.transaction(requires_new: true) do
+      Project.transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
         move_deploy_keys_projects
         remove_remaining_deploy_keys_projects if remove_remaining_elements
 

app/services/projects/move_forks_service.rb

@@ -5,7 +5,7 @@ module Projects
     def execute(source_project, remove_remaining_elements: true)
       return unless super && source_project.fork_network
 
-      Project.transaction(requires_new: true) do
+      Project.transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
         move_fork_network_members
         update_root_project
         refresh_forks_count

app/services/projects/move_lfs_objects_projects_service.rb

@@ -5,7 +5,7 @@ module Projects
     def execute(source_project, remove_remaining_elements: true)
       return unless super
 
-      Project.transaction(requires_new: true) do
+      Project.transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
         move_lfs_objects_projects
         remove_remaining_lfs_objects_project if remove_remaining_elements
 

app/services/projects/move_notification_settings_service.rb

@@ -5,7 +5,7 @@ module Projects
     def execute(source_project, remove_remaining_elements: true)
       return unless super
 
-      Project.transaction(requires_new: true) do
+      Project.transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
         move_notification_settings
         remove_remaining_notification_settings if remove_remaining_elements
 

app/services/projects/move_project_authorizations_service.rb

@@ -9,7 +9,7 @@ module Projects
     def execute(source_project, remove_remaining_elements: true)
       return unless super
 
-      Project.transaction(requires_new: true) do
+      Project.transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
         move_project_authorizations
 
         remove_remaining_authorizations if remove_remaining_elements

app/services/projects/move_project_group_links_service.rb

@@ -9,7 +9,7 @@ module Projects
     def execute(source_project, remove_remaining_elements: true)
       return unless super
 
-      Project.transaction(requires_new: true) do
+      Project.transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
         move_group_links
         remove_remaining_project_group_links if remove_remaining_elements
 

app/services/projects/move_project_members_service.rb

@@ -9,7 +9,7 @@ module Projects
     def execute(source_project, remove_remaining_elements: true)
       return unless super
 
-      Project.transaction(requires_new: true) do
+      Project.transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
         move_project_members
         remove_remaining_members if remove_remaining_elements
 

app/services/projects/move_users_star_projects_service.rb

@@ -9,7 +9,7 @@ module Projects
 
       return unless user_stars.any?
 
-      Project.transaction(requires_new: true) do
+      Project.transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
         user_stars.update_all(project_id: @project.id)
 
         Project.reset_counters @project.id, :users_star_projects

app/services/users/migrate_to_ghost_user_service.rb

@@ -39,7 +39,7 @@ module Users
     private
 
     def migrate_records_in_transaction
-      user.transaction(requires_new: true) do
+      user.transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
         @ghost_user = User.ghost
 
         migrate_records

db/post_migrate/20201106134950_deduplicate_epic_iids.rb

@@ -85,7 +85,7 @@ class DeduplicateEpicIids < ActiveRecord::Migration[6.0]
 
       instance = subject.is_a?(::Class) ? nil : subject
 
-      subject.transaction(requires_new: true) do
+      subject.transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
         InternalId.create!(
           **scope,
           usage: usage_value,

ee/app/services/vulnerabilities/create_service.rb

@@ -16,7 +16,7 @@ module Vulnerabilities
 
       vulnerability = Vulnerability.new
 
-      Vulnerabilities::Finding.transaction(requires_new: true) do
+      Vulnerabilities::Finding.transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
         save_vulnerability(vulnerability, finding)
         Statistics::UpdateService.update_for(vulnerability)
         HistoricalStatistics::UpdateService.update_for(@project)

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

@@ -65,7 +65,7 @@ module EE
           end
 
           def self.safe_ensure_unique(retries: 0)
-            transaction(requires_new: true) do
+            transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
               yield
             end
           rescue ActiveRecord::RecordNotUnique

lib/gitlab/background_migration/backfill_design_internal_ids.rb

@@ -73,7 +73,7 @@ module Gitlab
         # violation. We can safely roll-back the nested transaction and perform
         # a lookup instead to retrieve the record.
         def create_record
-          subject.transaction(requires_new: true) do
+          subject.transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
             InternalId.create!(
               **scope,
               usage: usage_value,

lib/gitlab/background_migration/backfill_project_repositories.rb

@@ -21,7 +21,7 @@ module Gitlab
           shard_id = shards.fetch(name, nil)
           return shard_id if shard_id.present?
 
-          Shard.transaction(requires_new: true) do
+          Shard.transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
             create!(name)
           end
         rescue ActiveRecord::RecordNotUnique

lib/gitlab/database/with_lock_retries.rb

@@ -122,7 +122,7 @@ module Gitlab
       end
 
       def run_block_with_lock_timeout
-        ActiveRecord::Base.transaction(requires_new: true) do
+        ActiveRecord::Base.transaction(requires_new: true) do # rubocop:disable Performance/ActiveRecordSubtransactions
           execute("SET LOCAL lock_timeout TO '#{current_lock_timeout_in_ms}ms'")
 
           log(message: 'Lock timeout is set', current_iteration: current_iteration, lock_timeout_in_ms: current_lock_timeout_in_ms)

rubocop/cop/performance/active_record_subtransactions.rb

+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Performance
+      class ActiveRecordSubtransactions < RuboCop::Cop::Cop
+        MSG = 'Subtransactions should not be used. ' \
+          'For more information see: https://gitlab.com/gitlab-org/gitlab/-/issues/338346'
+
+        def_node_matcher :match_transaction_with_options, <<~PATTERN
+          (send _ :transaction (hash $...))
+        PATTERN
+
+        def_node_matcher :subtransaction_option?, <<~PATTERN
+          (pair (:sym :requires_new) (true))
+        PATTERN
+
+        def on_send(node)
+          match_transaction_with_options(node) do |option_nodes|
+            option_nodes.each do |option_node|
+              next unless subtransaction_option?(option_node)
+
+              add_offense(option_node)
+            end
+          end
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/performance/active_record_subtransactions_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require_relative '../../../../rubocop/cop/performance/active_record_subtransactions'
+
+RSpec.describe RuboCop::Cop::Performance::ActiveRecordSubtransactions do
+  subject(:cop) { described_class.new }
+
+  let(:message) { described_class::MSG }
+
+  context 'when calling #transaction with only requires_new: true' do
+    it 'registers an offense' do
+      expect_offense(<<~RUBY)
+        ApplicationRecord.transaction(requires_new: true) do
+                                      ^^^^^^^^^^^^^^^^^^ #{message}
+          Project.create!(name: 'MyProject')
+        end
+      RUBY
+    end
+  end
+
+  context 'when passing multiple arguments to #transaction, including requires_new: true' do
+    it 'registers an offense' do
+      expect_offense(<<~RUBY)
+        ApplicationRecord.transaction(isolation: :read_committed, requires_new: true) do
+                                                                  ^^^^^^^^^^^^^^^^^^ #{message}
+          Project.create!(name: 'MyProject')
+        end
+      RUBY
+    end
+  end
+
+  context 'when calling #transaction with requires_new: false' do
+    it 'does not register an offense' do
+      expect_no_offenses(<<~RUBY)
+        ApplicationRecord.transaction(requires_new: false) do
+          Project.create!(name: 'MyProject')
+        end
+      RUBY
+    end
+  end
+
+  context 'when calling #transaction with other options' do
+    it 'does not register an offense' do
+      expect_no_offenses(<<~RUBY)
+        ApplicationRecord.transaction(isolation: :read_committed) do
+          Project.create!(name: 'MyProject')
+        end
+      RUBY
+    end
+  end
+
+  context 'when calling #transaction with no arguments' do
+    it 'does not register an offense' do
+      expect_no_offenses(<<~RUBY)
+        ApplicationRecord.transaction do
+          Project.create!(name: 'MyProject')
+        end
+      RUBY
+    end
+  end
+end