From b8013dd47ee9be3a8b318bb8aa55a91664312133 Mon Sep 17 00:00:00 2001
From: pbair <pbair@gitlab.com>
Date: Thu, 19 Aug 2021 12:58:38 -0400
Subject: [PATCH] Add rubocop to prevent use of subtransactions
Add a new rubocop rule that prevents direct use of subtransactions,
meaning any call to #transaction with the options `requires_new: true`.
---
.rubocop.yml | 5 ++
app/models/application_record.rb | 6 +-
app/models/application_setting.rb | 2 +-
app/models/internal_id.rb | 4 +-
.../move_deploy_keys_projects_service.rb | 2 +-
app/services/projects/move_forks_service.rb | 2 +-
.../move_lfs_objects_projects_service.rb | 2 +-
.../move_notification_settings_service.rb | 2 +-
.../move_project_authorizations_service.rb | 2 +-
.../move_project_group_links_service.rb | 2 +-
.../projects/move_project_members_service.rb | 2 +-
.../move_users_star_projects_service.rb | 2 +-
.../users/migrate_to_ghost_user_service.rb | 2 +-
.../20201106134950_deduplicate_epic_iids.rb | 2 +-
.../vulnerabilities/create_service.rb | 2 +-
.../migrate_approver_to_approval_rules.rb | 2 +-
.../backfill_design_internal_ids.rb | 2 +-
.../backfill_project_repositories.rb | 2 +-
lib/gitlab/database/with_lock_retries.rb | 2 +-
.../active_record_subtransactions.rb | 30 +++++++++
.../active_record_subtransactions_spec.rb | 62 +++++++++++++++++++
21 files changed, 118 insertions(+), 21 deletions(-)
create mode 100644 rubocop/cop/performance/active_record_subtransactions.rb
create mode 100644 spec/rubocop/cop/performance/active_record_subtransactions_spec.rb
diff --git a/.rubocop.yml b/.rubocop.yml
index a82910d89fbc1..6a460e30a9553 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 29ef76547e066..2353a514097f0 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 c4b6bcb9395f0..8ed408d2c23d0 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 107b1914af434..9cbb5f92bbdd6 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 51d84af249e25..17513f0ba11f8 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 33f0bab12c95c..f61552f661f95 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 57a8d3d69c6fb..ca8b4bfde6878 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 efe06f158ccd2..fc268f5762b88 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 c95ad60ab5e22..30f3b892131c1 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 349953ff973d4..8344b0b404b95 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 9a1b7c6d1b623..11629c3fd7e8e 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 20121d429e2f5..b8564a02301cf 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 a471f55e644a2..09fd74e9818f6 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 bc7daf9329d65..8fddc81057b6b 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 e9b27bee2c75a..02aad1506e459 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 62b0219e62fa0..10c77813d9977 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 6d1df95c66d92..236c6b6eb9a72 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 f5c8796bd18ec..a9eaeb0562dc5 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 bbf8f133f0fc7..70acbeac9e151 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 0000000000000..a550b558e52a1
--- /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 0000000000000..0da2e30062a2f
--- /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
--
GitLab