diff --git a/.rubocop.yml b/.rubocop.yml
index a82910d89fbc1a7195a5a002d8e95a70381739f9..6a460e30a9553bc81614b3e127bfebbb315b66fb 100644
--- a/.rubocop.yml
+++ b/.rubocop.yml
@@ -712,3 +712,8 @@ QA/SelectorUsage:
- 'ee/spec/**/*.rb'
Exclude:
- 'spec/rubocop/**/*_spec.rb'
+
+Performance/ActiveRecordSubtransactions:
+ Exclude:
+ - 'spec/**/*.rb'
+ - 'ee/spec/**/*.rb'
diff --git a/app/models/application_record.rb b/app/models/application_record.rb
index 29ef76547e0667e39c1b0b14360e6ccb13a09a63..2353a514097f0105d561318c70ccaa487723f427 100644
--- a/app/models/application_record.rb
+++ b/app/models/application_record.rb
@@ -31,7 +31,7 @@ def self.pluck_primary_key
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 @@ def self.safe_find_or_create_by!(*args, &block)
# 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 @@ def self.optimized_safe_find_or_create_by(*args, &block)
#
# 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
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index c4b6bcb9395f000eb19deaac37d19d1fad3460a5..8ed408d2c23d0b064a06c742d8733a0031d781e1 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -622,7 +622,7 @@ def instance_review_permitted?
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
diff --git a/app/models/internal_id.rb b/app/models/internal_id.rb
index 107b1914af434214db93524304c30a19cb1959ef..9cbb5f92bbdd62b328ae3d4da7fe802f714f0ce9 100644
--- a/app/models/internal_id.rb
+++ b/app/models/internal_id.rb
@@ -239,7 +239,7 @@ def create_record
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 @@ def create_record!(subject, scope, usage, value)
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
diff --git a/app/services/projects/move_deploy_keys_projects_service.rb b/app/services/projects/move_deploy_keys_projects_service.rb
index 51d84af249e25eab0317ca272564af7ae75fb979..17513f0ba11f86439755666b6500334c594c2969 100644
--- a/app/services/projects/move_deploy_keys_projects_service.rb
+++ b/app/services/projects/move_deploy_keys_projects_service.rb
@@ -5,7 +5,7 @@ class MoveDeployKeysProjectsService < BaseMoveRelationsService
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
diff --git a/app/services/projects/move_forks_service.rb b/app/services/projects/move_forks_service.rb
index 33f0bab12c95cecd1f23a8b85a9b4d0b50e2c5e6..f61552f661f95c6c98435e46cbb8c2f1b3df0797 100644
--- a/app/services/projects/move_forks_service.rb
+++ b/app/services/projects/move_forks_service.rb
@@ -5,7 +5,7 @@ class MoveForksService < BaseMoveRelationsService
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
diff --git a/app/services/projects/move_lfs_objects_projects_service.rb b/app/services/projects/move_lfs_objects_projects_service.rb
index 57a8d3d69c6fb83507ebb25f01fa646ef17b51ff..ca8b4bfde68784e0e1967e14302f2f8fff21415c 100644
--- a/app/services/projects/move_lfs_objects_projects_service.rb
+++ b/app/services/projects/move_lfs_objects_projects_service.rb
@@ -5,7 +5,7 @@ class MoveLfsObjectsProjectsService < BaseMoveRelationsService
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
diff --git a/app/services/projects/move_notification_settings_service.rb b/app/services/projects/move_notification_settings_service.rb
index efe06f158ccd22ceb8ba949b2b71245062262ea7..fc268f5762b883ee35f72f42d69d66810e225401 100644
--- a/app/services/projects/move_notification_settings_service.rb
+++ b/app/services/projects/move_notification_settings_service.rb
@@ -5,7 +5,7 @@ class MoveNotificationSettingsService < BaseMoveRelationsService
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
diff --git a/app/services/projects/move_project_authorizations_service.rb b/app/services/projects/move_project_authorizations_service.rb
index c95ad60ab5e229dabd6dcd97806b840701e08016..30f3b892131c1352c4d18c1404a6ee7112b160ee 100644
--- a/app/services/projects/move_project_authorizations_service.rb
+++ b/app/services/projects/move_project_authorizations_service.rb
@@ -9,7 +9,7 @@ class MoveProjectAuthorizationsService < BaseMoveRelationsService
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
diff --git a/app/services/projects/move_project_group_links_service.rb b/app/services/projects/move_project_group_links_service.rb
index 349953ff973d4de385f9d3e2370d74fd3e6e3166..8344b0b404b955197ebbcef1fe27333d8da7dc37 100644
--- a/app/services/projects/move_project_group_links_service.rb
+++ b/app/services/projects/move_project_group_links_service.rb
@@ -9,7 +9,7 @@ class MoveProjectGroupLinksService < BaseMoveRelationsService
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
diff --git a/app/services/projects/move_project_members_service.rb b/app/services/projects/move_project_members_service.rb
index 9a1b7c6d1b6239dc09c0d38b91339b6fff5d9ece..11629c3fd7e8eed34f40b987cf91c38e7fcc13d8 100644
--- a/app/services/projects/move_project_members_service.rb
+++ b/app/services/projects/move_project_members_service.rb
@@ -9,7 +9,7 @@ class MoveProjectMembersService < BaseMoveRelationsService
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
diff --git a/app/services/projects/move_users_star_projects_service.rb b/app/services/projects/move_users_star_projects_service.rb
index 20121d429e2f5711e9bdcf62d7cc97ea9f248ace..b8564a02301cf53892d948992a1e13cf9f5d04d3 100644
--- a/app/services/projects/move_users_star_projects_service.rb
+++ b/app/services/projects/move_users_star_projects_service.rb
@@ -9,7 +9,7 @@ def execute(source_project, remove_remaining_elements: true)
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
diff --git a/app/services/users/migrate_to_ghost_user_service.rb b/app/services/users/migrate_to_ghost_user_service.rb
index a471f55e644a267ac4be53992dea703d52007071..09fd74e9818f674b67df55a137cc91e61563d0d0 100644
--- a/app/services/users/migrate_to_ghost_user_service.rb
+++ b/app/services/users/migrate_to_ghost_user_service.rb
@@ -39,7 +39,7 @@ def execute
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
diff --git a/db/post_migrate/20201106134950_deduplicate_epic_iids.rb b/db/post_migrate/20201106134950_deduplicate_epic_iids.rb
index bc7daf9329d65b004588ceb6d2bb64deb35dfc2c..8fddc81057b6b929fffb0718136d6d7c31a98dff 100644
--- a/db/post_migrate/20201106134950_deduplicate_epic_iids.rb
+++ b/db/post_migrate/20201106134950_deduplicate_epic_iids.rb
@@ -85,7 +85,7 @@ def create_record
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,
diff --git a/ee/app/services/vulnerabilities/create_service.rb b/ee/app/services/vulnerabilities/create_service.rb
index e9b27bee2c75a2db2203a7739e246dfb666f3918..02aad1506e459a0b00523649e7cfe0d7d9a30d49 100644
--- a/ee/app/services/vulnerabilities/create_service.rb
+++ b/ee/app/services/vulnerabilities/create_service.rb
@@ -16,7 +16,7 @@ def execute
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)
diff --git a/ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb b/ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb
index 62b0219e62fa0d3bea28d3d94d9f3f6fa4a8c199..10c77813d99775faed4d63e900afefb561090ef4 100644
--- a/ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb
+++ b/ee/lib/ee/gitlab/background_migration/migrate_approver_to_approval_rules.rb
@@ -65,7 +65,7 @@ def self.safe_find_or_create_by(*args)
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
diff --git a/lib/gitlab/background_migration/backfill_design_internal_ids.rb b/lib/gitlab/background_migration/backfill_design_internal_ids.rb
index 6d1df95c66d920472000498d0fee219005770baa..236c6b6eb9a72990af3aa3f53e3274b44fac4a61 100644
--- a/lib/gitlab/background_migration/backfill_design_internal_ids.rb
+++ b/lib/gitlab/background_migration/backfill_design_internal_ids.rb
@@ -73,7 +73,7 @@ def usage_value
# 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,
diff --git a/lib/gitlab/background_migration/backfill_project_repositories.rb b/lib/gitlab/background_migration/backfill_project_repositories.rb
index f5c8796bd18ecb886da48a07c017d7395c795428..a9eaeb0562dc5df9bde7cb2d434287a60b6a1336 100644
--- a/lib/gitlab/background_migration/backfill_project_repositories.rb
+++ b/lib/gitlab/background_migration/backfill_project_repositories.rb
@@ -21,7 +21,7 @@ def find_shard_id(name)
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
diff --git a/lib/gitlab/database/with_lock_retries.rb b/lib/gitlab/database/with_lock_retries.rb
index bbf8f133f0fc722ebfbd75ae2729ecccccb63dea..70acbeac9e151f24bbd5c6216b90f2c1da55dd95 100644
--- a/lib/gitlab/database/with_lock_retries.rb
+++ b/lib/gitlab/database/with_lock_retries.rb
@@ -122,7 +122,7 @@ def run_block
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)
diff --git a/rubocop/cop/performance/active_record_subtransactions.rb b/rubocop/cop/performance/active_record_subtransactions.rb
new file mode 100644
index 0000000000000000000000000000000000000000..a550b558e52a1cbb43b495d85fb860a5bc4a4ce5
--- /dev/null
+++ b/rubocop/cop/performance/active_record_subtransactions.rb
@@ -0,0 +1,30 @@
+# 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
diff --git a/spec/rubocop/cop/performance/active_record_subtransactions_spec.rb b/spec/rubocop/cop/performance/active_record_subtransactions_spec.rb
new file mode 100644
index 0000000000000000000000000000000000000000..0da2e30062a2fb88abdc6cf102e8cf939bcd2dd4
--- /dev/null
+++ b/spec/rubocop/cop/performance/active_record_subtransactions_spec.rb
@@ -0,0 +1,62 @@
+# 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