Cop Idea: Disallow return / break / throw in transaction

[ybiquitous]

Is your feature request related to a problem? Please describe.

The use of return, break, or throw in an ActiveRecord::Base.transaction block was deprecated since Rails 6.1.0. (rails/rails@31be40d)

And with Rails 7.0.0, the behavior has changed to rollback. (rails/rails@15aa420)

For example, the following code seems unclear to me:

def do_something(user)
  ApplicationRecord do
    user.save!

    unless user.active?
      return true # rollback!
    end

    return false
  end
end

Describe the solution you'd like

So, I suggest a new cop detect the use of return, break, or throw in a transaction block:

Here is a PoC:

class RuboCop::Cop::Rails::NoReturnInTransaction < RuboCop::Cop::Base
  MSG = "Disallow `return`, `break`, or `throw` in the transaction block".freeze

  def on_return(node)
    check(node)
  end

  def on_break(node)
    check(node)
  end

  def on_send(node)
    check(node) if node.method_name == :throw
  end

  private

  def check(node)
    node.each_ancestor do |ancestor|
      if ancestor.type == :block && ancestor.send_node.method_name == :transaction
        add_offense(node)
        next
      end
    end
  end
end

This new cop suggests to:

• avoid using return, break, or throw
• use raise ActiveRecord::Rollback if hoping rollback

EDIT: I've added the bad/good examples to clarify this Cop's advantage as below:

ApplicationRecord.transaction do
  user.save!

  # bad
  return if user.active?
  break if user.acitve?
  throw :some if user.active?

  # good
  raise ActiveRecord::Rollback if user.active?
end

Describe alternatives you've considered

No alternatives.

Additional context

No additional context.

[koic]

Can you write bad and good examples? The specification will be clearer.

[ybiquitous]

@koic Thank you. I've updated the issue description. 👍🏼

[teckwan]

Hi guys! My organisation was talking about implementing a cop just for this, so I thought it would be cool to open a PR for this.

use raise ActiveRecord::Rollback if hoping rollback

As a side note, it's not a good idea to raise ActiveRecord::Rollback in general as it can have unwanted effects (see https://api.rubyonrails.org/classes/ActiveRecord/Transactions/ClassMethods.html, in "Nested Transactions"). What would be better is to just raise an error in my opinion. @ybiquitous

[ybiquitous]

@teckwan Thank you so much for implementing the new cop and sharing the caution about ActiveRecord::Rollback. 😊