Add cop to guard again ActiveRecord::Base.transaction use

CHANGELOG.md

@@ -6,6 +6,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## 0.13.0 - 2024-01-11
+### Changed
+- Added rule for `ActiveRecord::Base.transaction` use
+
 ## 0.12.2 - 2024-01-03
 ### Changed
 - Removed Rubocop config for Rails

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    rubocop-vendor (0.12.2)
+    rubocop-vendor (0.13.0)
       rubocop
 
 GEM

config/default.yml

@@ -59,3 +59,8 @@ Vendor/WsSdkPathInjection:
   Description: 'Avoid using `ws_sdk` with path injection.'
   Enabled: true
   VersionAdded: '0.12.0'
+
+Vendor/ActiveRecordBaseTransactionUse:
+  Description: 'Avoid using ActiveRecord::Base.transaction.'
+  Enabled: true
+  VersionAdded: '0.13.0'

lib/rubocop/cop/vendor/active_record_base_transaction_use.rb

@@ -0,0 +1,82 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Vendor
+      # Flags uses of ActiveRecord::Base.transaction,
+      # as subclasses of ActiveRecord::Base may use a different
+      # database connection.
+      #
+      # This becomes relevant if, for instance, your application
+      # defines models or any subclass of ActiveRecord::Base
+      # specifying connection configurations, e.g. using `connects_to`.
+      #
+      # The guarantee that transaction connection matches the
+      # model connection is strongest when `MyModelClass.transaction`
+      # wraps database operations on instances of MyModelClass only.
+      #
+      # If multiple model classes are involved in a .transaction
+      # call, `.transaction` only needs to be called on one of them,
+      # or a common ancestor sharing the same connection
+      # if both models share the same underlying connection.
+      #
+      # If not, a workaround would be to open a transaction on both
+      # model classes.
+      #
+      # @example
+      #
+      #   # bad
+      #   ActiveRecord::Base.transaction do
+      #     ... database operations
+      #   end
+      #
+      #   # good
+      #   MyModelClass.transaction do
+      #     ... database operations on instances of MyModelClass
+      #   end
+      #
+      #   # also good
+      #   my_model_instance.with_lock do
+      #     ... database operations on my_model_instance
+      #   end
+      #
+      #   # good if and only if both models share a database connection
+      #   MyModelClass.transaction do
+      #     ... database operations on instances of MyModelClass
+      #     ... database operations on instances of MyOtherModelClass
+      #   end
+      #
+      #   # good if and only if ApplicationRecord shares a database
+      #   # connection with all models involved
+      #   ApplicationRecord.transaction do
+      #     ... database operations on instances of MyModelClass
+      #     ... database operations on instances of MyOtherModelClass
+      #   end
+      #
+      #   # good if the models do not share a database connection
+      #   MyModelClass.transaction do
+      #     MyOtherModelClass.transaction do
+      #       ... database operations on instances of MyModelClass
+      #       ... database operations on instances of MyOtherModelClass
+      #     end
+      #   end
+      #
+      class ActiveRecordBaseTransactionUse < Base
+        MSG = 'Avoid using `ActiveRecord::Base.transaction, as models inheriting a subclass of ActiveRecord::Base may use a different database connection from ActiveRecord::Base.connection.'
+
+        # @!method uses_active_record_base?(node)
+        def_node_matcher :uses_active_record_base?, <<-PATTERN
+          (const (const {nil? cbase} :ActiveRecord) :Base)
+        PATTERN
+
+        def on_send(node)
+          receiver_node, method_name = *node
+
+          return unless uses_active_record_base?(receiver_node) && method_name == :transaction
+
+          add_offense(node)
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/vendor_cops.rb

@@ -3,6 +3,7 @@
 module RuboCop
 end
 
+require_relative 'vendor/active_record_base_transaction_use'
 require_relative 'vendor/active_record_connection_execute'
 require_relative 'vendor/recursive_open_struct_gem'
 require_relative 'vendor/sidekiq_throttled_gem'

lib/rubocop/vendor/version.rb

@@ -2,6 +2,6 @@
 
 module RuboCop
   module Vendor
-    VERSION = '0.12.2'
+    VERSION = '0.13.0'
   end
 end

spec/rubocop/cop/vendor/active_record_base_transaction_use_spec.rb

@@ -0,0 +1,30 @@
+RSpec.describe RuboCop::Cop::Vendor::ActiveRecordBaseTransactionUse, :config do
+  subject(:cop) { described_class.new }
+
+  let(:msg) { 'Avoid using `ActiveRecord::Base.transaction, as models inheriting a subclass of ActiveRecord::Base may use a different database connection from ActiveRecord::Base.connection.' }
+
+  it 'registers an offense for usage of ActiveRecord::Base.transaction' do
+    expect_offense(<<~RUBY)
+      class MyModel < ApplicationRecord
+        def do_something
+          ActiveRecord::Base.transaction do
+          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Vendor/ActiveRecordBaseTransactionUse: #{msg}
+            nil
+          end
+        end
+      end
+    RUBY
+  end
+
+  it 'registers no offense for usage of MyModelClass.transaction' do
+    expect_no_offenses(<<~RUBY)
+      class MyModelClass < ApplicationRecord
+        def do_something
+          MyModelClass.transaction do
+            nil
+          end
+        end
+      end
+    RUBY
+  end
+end

spec/rubocop/cop/vendor/rollbar_inside_rescue_spec.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-RSpec.describe RuboCop::Cop::Vendor::RollbarInsideRescue, :config, :config do
+RSpec.describe RuboCop::Cop::Vendor::RollbarInsideRescue, :config do
   it 'registers an offense when using `Rollbar.error` without rescue' do
     expect_offense(<<~RUBY)
       Rollbar.error('Unable to perform division')

spec/rubocop/cop/vendor/rollbar_interpolation_spec.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-RSpec.describe RuboCop::Cop::Vendor::RollbarInterpolation, :config, :config do
+RSpec.describe RuboCop::Cop::Vendor::RollbarInterpolation, :config do
   it 'registers an offense when using `Rollbar.error` with interpolated string' do
     expect_offense(<<~'RUBY')
       Rollbar.error(e, "Unable to sync account #{account[:id]}")

spec/rubocop/cop/vendor/rollbar_logger_spec.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-RSpec.describe RuboCop::Cop::Vendor::RollbarLogger, :config, :config do
+RSpec.describe RuboCop::Cop::Vendor::RollbarLogger, :config do
   it 'registers an offense when using `Rollbar.debug`' do
     expect_offense(<<~RUBY)
       Rollbar.debug('Stale message')

spec/rubocop/cop/vendor/rollbar_with_exception_spec.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-RSpec.describe RuboCop::Cop::Vendor::RollbarWithException, :config, :config do
+RSpec.describe RuboCop::Cop::Vendor::RollbarWithException, :config do
   it 'registers an offense when using `Rollbar.error` without exception' do
     expect_offense(<<~RUBY)
       begin

spec/rubocop/cop/vendor/strict_dry_struct_spec.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-RSpec.describe RuboCop::Cop::Vendor::StrictDryStruct, :config, :config do
+RSpec.describe RuboCop::Cop::Vendor::StrictDryStruct, :config do
   it 'registers an offense when using Dry::Struct without strict' do
     expect_offense(<<~RUBY)
       class ExampleStruct < Dry::Struct