From e861a14ace320d9dc6baaad97eaa69a32bf63e14 Mon Sep 17 00:00:00 2001 From: Kazuma Watanabe Date: Sat, 10 Feb 2024 16:17:02 +0000 Subject: [PATCH 1/3] Add new `Rails/PrivateTransactionOption` cop This PR adds a new cop called `Rails/PrivateTransactionOption`. This cop checks whether `ActiveRecord::Base.transaction(joinable: _)` is used. The `joinable` option is a private API and is not intended to be called from outside Active Record core. https://github.com/rails/rails/issues/39912#issuecomment-665483779 https://github.com/rails/rails/issues/46182#issuecomment-1265966330 Passing `joinable: false` may cause unexpected behavior such as the `after_commit` callback not firing at the appropriate time. --- ...dd_rails_private_transaction_option_cop.md | 1 + config/default.yml | 6 +++ .../cop/rails/private_transaction_option.rb | 44 +++++++++++++++++++ lib/rubocop/cop/rails_cops.rb | 1 + .../rails/private_transaction_option_spec.rb | 16 +++++++ 5 files changed, 68 insertions(+) create mode 100644 changelog/new_add_rails_private_transaction_option_cop.md create mode 100644 lib/rubocop/cop/rails/private_transaction_option.rb create mode 100644 spec/rubocop/cop/rails/private_transaction_option_spec.rb diff --git a/changelog/new_add_rails_private_transaction_option_cop.md b/changelog/new_add_rails_private_transaction_option_cop.md new file mode 100644 index 0000000000..50d5a5f0af --- /dev/null +++ b/changelog/new_add_rails_private_transaction_option_cop.md @@ -0,0 +1 @@ +* [#1236](https://github.com/rubocop/rubocop-rails/pull/1236): Add new `Rails/PrivateTransactionOption`. ([@wata727][]) diff --git a/config/default.yml b/config/default.yml index 4ad7a0d053..a674f6a0ce 100644 --- a/config/default.yml +++ b/config/default.yml @@ -782,6 +782,12 @@ Rails/Present: # Convert usages of `unless blank?` to `if present?` UnlessBlank: true +Rails/PrivateTransactionOption: + Description: 'Avoid use of `ActiveRecord::Base.transaction(joinable: _)`.' + Enabled: pending + Safe: false + VersionAdded: '<>' + Rails/RakeEnvironment: Description: 'Include `:environment` as a dependency for all Rake tasks.' Enabled: true diff --git a/lib/rubocop/cop/rails/private_transaction_option.rb b/lib/rubocop/cop/rails/private_transaction_option.rb new file mode 100644 index 0000000000..f6e36bd4a0 --- /dev/null +++ b/lib/rubocop/cop/rails/private_transaction_option.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Rails + # Checks whether `ActiveRecord::Base.transaction(joinable: _)` is used. + # + # The `joinable` option is a private API and is not intended to be called + # from outside Active Record core. + # https://github.com/rails/rails/issues/39912#issuecomment-665483779 + # https://github.com/rails/rails/issues/46182#issuecomment-1265966330 + # + # Passing `joinable: false` may cause unexpected behavior such as the + # `after_commit` callback not firing at the appropriate time. + # + # @safety + # This Cop is unsafe because it cannot accurately identify + # the `ActiveRecord::Base.transaction` method call. + # + # @example + # # bad + # ActiveRecord::Base.transaction(requires_new: true, joinable: false) + # + # # good + # ActiveRecord::Base.transaction(requires_new: true) + # + class PrivateTransactionOption < Base + MSG = 'Do not use `ActiveRecord::Base.transaction(joinable: _)`.' + RESTRICT_ON_SEND = %i[transaction].freeze + + # @!method match_transaction_with_joinable(node) + def_node_matcher :match_transaction_with_joinable, <<~PATTERN + (send _ :transaction (hash <$(pair (sym :joinable) {true false}) ...>)) + PATTERN + + def on_send(node) + match_transaction_with_joinable(node) do |option_node| + add_offense(option_node) + end + end + end + end + end +end diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 0d92ff4ca5..e04ae1994a 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -88,6 +88,7 @@ require_relative 'rails/pluralization_grammar' require_relative 'rails/presence' require_relative 'rails/present' +require_relative 'rails/private_transaction_option' require_relative 'rails/rake_environment' require_relative 'rails/read_write_attribute' require_relative 'rails/redundant_active_record_all_method' diff --git a/spec/rubocop/cop/rails/private_transaction_option_spec.rb b/spec/rubocop/cop/rails/private_transaction_option_spec.rb new file mode 100644 index 0000000000..eb4ef78ad5 --- /dev/null +++ b/spec/rubocop/cop/rails/private_transaction_option_spec.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::PrivateTransactionOption, :config do + it 'registers an offense when using `joinable: false`' do + expect_offense(<<~RUBY) + ActiveRecord::Base.transaction(requires_new: true, joinable: false) + ^^^^^^^^^^^^^^^ Do not use `ActiveRecord::Base.transaction(joinable: _)`. + RUBY + end + + it 'does not register an offense when using only `requires_new: true`' do + expect_no_offenses(<<~RUBY) + ActiveRecord::Base.transaction(requires_new: true) + RUBY + end +end From c22574f8ec21b19e85fbdee81814cc1ce3dccb30 Mon Sep 17 00:00:00 2001 From: Kazuma Watanabe Date: Sat, 27 Jul 2024 16:38:48 +0900 Subject: [PATCH 2/3] Apply suggestions from code review Co-authored-by: Phil Pirozhkov --- .../cop/rails/private_transaction_option.rb | 2 +- .../rails/private_transaction_option_spec.rb | 21 +++++++++++++++---- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/lib/rubocop/cop/rails/private_transaction_option.rb b/lib/rubocop/cop/rails/private_transaction_option.rb index f6e36bd4a0..a86c4ed084 100644 --- a/lib/rubocop/cop/rails/private_transaction_option.rb +++ b/lib/rubocop/cop/rails/private_transaction_option.rb @@ -25,7 +25,7 @@ module Rails # ActiveRecord::Base.transaction(requires_new: true) # class PrivateTransactionOption < Base - MSG = 'Do not use `ActiveRecord::Base.transaction(joinable: _)`.' + MSG = 'Use a negated `requires_new` option instead of the internal `joinable`.' RESTRICT_ON_SEND = %i[transaction].freeze # @!method match_transaction_with_joinable(node) diff --git a/spec/rubocop/cop/rails/private_transaction_option_spec.rb b/spec/rubocop/cop/rails/private_transaction_option_spec.rb index eb4ef78ad5..03be2f2039 100644 --- a/spec/rubocop/cop/rails/private_transaction_option_spec.rb +++ b/spec/rubocop/cop/rails/private_transaction_option_spec.rb @@ -1,16 +1,29 @@ # frozen_string_literal: true RSpec.describe RuboCop::Cop::Rails::PrivateTransactionOption, :config do - it 'registers an offense when using `joinable: false`' do + it 'registers an offense when using `ActiveRecord::Base.joinable: false`' do expect_offense(<<~RUBY) - ActiveRecord::Base.transaction(requires_new: true, joinable: false) - ^^^^^^^^^^^^^^^ Do not use `ActiveRecord::Base.transaction(joinable: _)`. + ActiveRecord::Base.transaction(requires_new: true, joinable: false) do + ^^^^^^^^^^^^^^^ Use a negated `requires_new` option instead of the internal `joinable`. + # ... + end + RUBY + end + + it 'registers an offense when using `Account.transaction(joinable: false)`' do + expect_offense(<<~RUBY) + Account.transaction(requires_new: true, joinable: false) do + ^^^^^^^^^^^^^^^ Use a negated `requires_new` option instead of the internal `joinable`. + # ... + end RUBY end it 'does not register an offense when using only `requires_new: true`' do expect_no_offenses(<<~RUBY) - ActiveRecord::Base.transaction(requires_new: true) + ActiveRecord::Base.transaction(requires_new: true) do + # ... + end RUBY end end From d8b64ad47e9b4bd4337ead872d1b8695c3601414 Mon Sep 17 00:00:00 2001 From: Kazuma Watanabe Date: Sat, 27 Jul 2024 08:05:49 +0000 Subject: [PATCH 3/3] fixup! Add new `Rails/PrivateTransactionOption` cop --- config/default.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/default.yml b/config/default.yml index a674f6a0ce..100a009728 100644 --- a/config/default.yml +++ b/config/default.yml @@ -783,7 +783,7 @@ Rails/Present: UnlessBlank: true Rails/PrivateTransactionOption: - Description: 'Avoid use of `ActiveRecord::Base.transaction(joinable: _)`.' + Description: 'Avoid use of the private `ActiveRecord::Base.transaction(joinable: _)` option.' Enabled: pending Safe: false VersionAdded: '<>'