diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c0d1e5..eba3e71 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/Gemfile.lock b/Gemfile.lock index a936e86..3354604 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - rubocop-vendor (0.12.2) + rubocop-vendor (0.13.0) rubocop GEM diff --git a/config/default.yml b/config/default.yml index a395b6b..5f530da 100644 --- a/config/default.yml +++ b/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' diff --git a/lib/rubocop/cop/vendor/active_record_base_transaction_use.rb b/lib/rubocop/cop/vendor/active_record_base_transaction_use.rb new file mode 100644 index 0000000..c5c1a0b --- /dev/null +++ b/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 diff --git a/lib/rubocop/cop/vendor_cops.rb b/lib/rubocop/cop/vendor_cops.rb index 50df498..d63a65e 100644 --- a/lib/rubocop/cop/vendor_cops.rb +++ b/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' diff --git a/lib/rubocop/vendor/version.rb b/lib/rubocop/vendor/version.rb index 88e5aa7..6a9cf0e 100644 --- a/lib/rubocop/vendor/version.rb +++ b/lib/rubocop/vendor/version.rb @@ -2,6 +2,6 @@ module RuboCop module Vendor - VERSION = '0.12.2' + VERSION = '0.13.0' end end diff --git a/spec/rubocop/cop/vendor/active_record_base_transaction_use_spec.rb b/spec/rubocop/cop/vendor/active_record_base_transaction_use_spec.rb new file mode 100644 index 0000000..93726b9 --- /dev/null +++ b/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 diff --git a/spec/rubocop/cop/vendor/rollbar_inside_rescue_spec.rb b/spec/rubocop/cop/vendor/rollbar_inside_rescue_spec.rb index 05f8053..3738aa6 100644 --- a/spec/rubocop/cop/vendor/rollbar_inside_rescue_spec.rb +++ b/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') diff --git a/spec/rubocop/cop/vendor/rollbar_interpolation_spec.rb b/spec/rubocop/cop/vendor/rollbar_interpolation_spec.rb index 674e514..e7db49e 100644 --- a/spec/rubocop/cop/vendor/rollbar_interpolation_spec.rb +++ b/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]}") diff --git a/spec/rubocop/cop/vendor/rollbar_logger_spec.rb b/spec/rubocop/cop/vendor/rollbar_logger_spec.rb index 4ac329a..a481d1e 100644 --- a/spec/rubocop/cop/vendor/rollbar_logger_spec.rb +++ b/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') diff --git a/spec/rubocop/cop/vendor/rollbar_with_exception_spec.rb b/spec/rubocop/cop/vendor/rollbar_with_exception_spec.rb index 998be57..e9511da 100644 --- a/spec/rubocop/cop/vendor/rollbar_with_exception_spec.rb +++ b/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 diff --git a/spec/rubocop/cop/vendor/strict_dry_struct_spec.rb b/spec/rubocop/cop/vendor/strict_dry_struct_spec.rb index a4e74da..1c7ff84 100644 --- a/spec/rubocop/cop/vendor/strict_dry_struct_spec.rb +++ b/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