diff --git a/.rubocop.yml b/.rubocop.yml index 6a460e30a9553bc81614b3e127bfebbb315b66fb..cc1b9258e1b64cb460dce0ce23b2efcc343ca788 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -717,3 +717,8 @@ Performance/ActiveRecordSubtransactions: Exclude: - 'spec/**/*.rb' - 'ee/spec/**/*.rb' + +Performance/ActiveRecordSubtransactionMethods: + Exclude: + - 'spec/**/*.rb' + - 'ee/spec/**/*.rb' diff --git a/app/controllers/clusters/clusters_controller.rb b/app/controllers/clusters/clusters_controller.rb index 32de9e69c852b400ce013153422932f7dd2f48e4..a24e3a3c4a9d5c10232bb5469195a21441e29c71 100644 --- a/app/controllers/clusters/clusters_controller.rb +++ b/app/controllers/clusters/clusters_controller.rb @@ -38,7 +38,7 @@ def index 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' diff --git a/app/models/application_record.rb b/app/models/application_record.rb index 2353a514097f0105d561318c70ccaa487723f427..d710abfdc9f7311ecda1fa536e697ff4ecc2b227 100644 --- a/app/models/application_record.rb +++ b/app/models/application_record.rb @@ -66,7 +66,7 @@ def self.with_fast_read_statement_timeout(timeout_ms = 5000) 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 diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb index 44d9beff27eb29fdbc873370909e5de1360cd93c..3753a14ad40be97abc4146fed0eddae343eb4873 100644 --- a/app/models/concerns/cache_markdown_field.rb +++ b/app/models/concerns/cache_markdown_field.rb @@ -171,7 +171,7 @@ def store_mentions! # 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 diff --git a/app/models/external_pull_request.rb b/app/models/external_pull_request.rb index 3fc166203e702bc758a193c53b037a8dee4221d3..fc45f5f8afc30858cedb789c269a93ca866f6093 100644 --- a/app/models/external_pull_request.rb +++ b/app/models/external_pull_request.rb @@ -89,7 +89,7 @@ def not_from_fork 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) diff --git a/app/services/merge_requests/approval_service.rb b/app/services/merge_requests/approval_service.rb index 62e599e3e271124da0db3677d1b5b42ff5741374..d04b816dcbffe31944de357eb32e6280497dbb20 100644 --- a/app/services/merge_requests/approval_service.rb +++ b/app/services/merge_requests/approval_service.rb @@ -35,7 +35,7 @@ def execute_approval_hooks(merge_request, current_user) end def save_approval(approval) - Approval.safe_ensure_unique do + Approval.safe_ensure_unique do # rubocop:disable Performance/ActiveRecordSubtransactionMethods approval.save end end diff --git a/app/services/terraform/remote_state_handler.rb b/app/services/terraform/remote_state_handler.rb index e9a13cee7645d1bc1e4653c2e71a2257c4d447b7..7543a79eddf34d4d3c1455d96bb29233351e7bb9 100644 --- a/app/services/terraform/remote_state_handler.rb +++ b/app/services/terraform/remote_state_handler.rb @@ -70,7 +70,7 @@ def create_or_find!(find_only:) 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 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 10c77813d99775faed4d63e900afefb561090ef4..25b7bbf8804e509336c2139b0389519faa7be333 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 @@ -59,7 +59,7 @@ def self.find_or_create_code_owner_rule(merge_request, entry) 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 diff --git a/rubocop/cop/performance/active_record_subtransaction_methods.rb b/rubocop/cop/performance/active_record_subtransaction_methods.rb new file mode 100644 index 0000000000000000000000000000000000000000..2769f8cab4240ea564ffde57208be715fe4bf825 --- /dev/null +++ b/rubocop/cop/performance/active_record_subtransaction_methods.rb @@ -0,0 +1,26 @@ +# 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 diff --git a/spec/rubocop/cop/performance/active_record_subtransaction_methods_spec.rb b/spec/rubocop/cop/performance/active_record_subtransaction_methods_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..382ea199e83ad195a04dacf610b7bae92d189629 --- /dev/null +++ b/spec/rubocop/cop/performance/active_record_subtransaction_methods_spec.rb @@ -0,0 +1,25 @@ +# 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