Merge pull request #648 from klaxit/exit-statement-in-transaction

[Fix #642] Add new `Rails/TransactionExitStatement` cop

Author: koic

CHANGELOG.md

@@ -540,3 +540,4 @@
 [@tachyons]: https://github.com/tachyons
 [@composerinteralia]: https://github.com/composerinteralia
 [@agrobbin]: https://github.com/agrobbin
+[@teckwan]: https://github.com/teckwan

changelog/new_add_transactionexitstatement_cop.md

@@ -0,0 +1 @@
+* [#642](https://github.com/rubocop/rubocop-rails/issues/642): New cop `Rails/TransactionExitStatement` to disallow `return`, `break` and `throw` in transactions. ([@teckwan][])

config/default.yml

@@ -903,6 +903,11 @@ Rails/TimeZoneAssignment:
     - spec/**/*.rb
     - test/**/*.rb
 
+Rails/TransactionExitStatement:
+  Description: 'Avoid the usage of `return`, `break` and `throw` in transaction blocks.'
+  Enabled: pending
+  VersionAdded: '<<next>>'
+
 Rails/UniqBeforePluck:
   Description: 'Prefer the use of uniq or distinct before pluck.'
   Enabled: true

lib/rubocop/cop/rails/transaction_exit_statement.rb

@@ -0,0 +1,77 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Rails
+      # This cop checks for the use of exit statements (namely `return`,
+      # `break` and `throw`) in transactions. This is due to the eventual
+      # unexpected behavior when using ActiveRecord >= 7, where transactions
+      # exitted using these statements are being rollbacked rather than
+      # committed (pre ActiveRecord 7 behavior).
+      #
+      # As alternatives, it would be more intuitive to explicitly raise an
+      # error when rollback is desired, and to use `next` when commit is
+      # desired.
+      #
+      # @example
+      #   # bad
+      #   ApplicationRecord.transaction do
+      #     return if user.active?
+      #   end
+      #
+      #   # bad
+      #   ApplicationRecord.transaction do
+      #     break if user.active?
+      #   end
+      #
+      #   # bad
+      #   ApplicationRecord.transaction do
+      #     throw if user.active?
+      #   end
+      #
+      #   # good
+      #   ApplicationRecord.transaction do
+      #     # Rollback
+      #     raise "User is active" if user.active?
+      #   end
+      #
+      #   # good
+      #   ApplicationRecord.transaction do
+      #     # Commit
+      #     next if user.active?
+      #   end
+      #
+      # @see https://github.com/rails/rails/commit/15aa4200e083
+      class TransactionExitStatement < Base
+        MSG = <<~MSG.chomp
+          Exit statement `%<statement>s` is not allowed. Use `raise` (rollback) or `next` (commit).
+        MSG
+
+        RESTRICT_ON_SEND = %i[transaction].freeze
+
+        def_node_search :exit_statements, <<~PATTERN
+          ({return | break | send nil? :throw} ...)
+        PATTERN
+
+        def on_send(node)
+          parent = node.parent
+
+          return unless parent&.block_type?
+
+          exit_statements(parent.body).each do |statement_node|
+            statement = if statement_node.return_type?
+                          'return'
+                        elsif statement_node.break_type?
+                          'break'
+                        else
+                          statement_node.method_name
+                        end
+            message = format(MSG, statement: statement)
+
+            add_offense(statement_node, message: message)
+          end
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/rails_cops.rb

@@ -107,6 +107,7 @@
 require_relative 'rails/table_name_assignment'
 require_relative 'rails/time_zone'
 require_relative 'rails/time_zone_assignment'
+require_relative 'rails/transaction_exit_statement'
 require_relative 'rails/uniq_before_pluck'
 require_relative 'rails/unique_validation_without_index'
 require_relative 'rails/unknown_env'

spec/rubocop/cop/rails/transaction_exit_statement_spec.rb

@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Rails::TransactionExitStatement, :config do
+  it 'registers an offense when `return` is used in transactions' do
+    expect_offense(<<~RUBY)
+      ApplicationRecord.transaction do
+        return if user.active?
+        ^^^^^^ Exit statement `return` is not allowed. Use `raise` (rollback) or `next` (commit).
+      end
+    RUBY
+  end
+
+  it 'registers an offense when `break` is used in transactions' do
+    expect_offense(<<~RUBY)
+      ApplicationRecord.transaction do
+        break if user.active?
+        ^^^^^ Exit statement `break` is not allowed. Use `raise` (rollback) or `next` (commit).
+      end
+    RUBY
+  end
+
+  it 'registers an offense when `throw` is used in transactions' do
+    expect_offense(<<~RUBY)
+      ApplicationRecord.transaction do
+        throw if user.active?
+        ^^^^^ Exit statement `throw` is not allowed. Use `raise` (rollback) or `next` (commit).
+      end
+    RUBY
+  end
+
+  it 'does not register an offense when `next` is used in transactions' do
+    expect_no_offenses(<<~RUBY)
+      ApplicationRecord.transaction do
+        next if user.active?
+      end
+    RUBY
+  end
+
+  it 'does not register an offense when `raise` is used in transactions' do
+    expect_no_offenses(<<~RUBY)
+      ApplicationRecord.transaction do
+        raise if user.active?
+      end
+    RUBY
+  end
+end