Add rule to prevent bad `rescue` usage inside a transaction

[deanylev]

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

A common mistake made in our Rails codebase is:

    ActiveRecord::Base.transaction do
      # create a few records
    rescue StandardError => e
      # we've rescued too early, meaning no rollback will occur!
    end

Instead of

    begin
      ActiveRecord::Base.transaction do
        # create a few records
      end
    rescue StandardError => e
      # the rollback has already occurred by this point, so we're good to go
    end

Describe the solution you'd like

A RuboCop rule to prevent this would be brilliant, as the effects to data integrity in production can be devastating

Describe alternatives you've considered

I have tried writing a rule myself, to no avail

Additional context

Add any other context or screenshots about the feature request here.

[Earlopain]

I'm not sure if a generic rule for this can be added. The user may want to supress the rollback in which case this seems fine to do. There could also be a raise ActiveRecord::Rollback in the rescue in which case it too would be fine (although a cop could handle that case).

What if a more specific exception like ActiveRecord::RecordNotUnique is rescued. Would that be ok?

That said, a cop for this would not be too complex. Here is what I have:

module RuboCop
  module Cop
    module Rails
      class TransactionRescue < Base
        MSG = 'Do not use beginless rescue inside of transactions.'

        # @!method transaction_rescue?(node)
        def_node_matcher :transaction_rescue?, <<~PATTERN
          (block
            (send _ :transaction)
            _
            (rescue
              {(...) nil?}
              (resbody (array ...) nil? ...)
            nil?)
          )
        PATTERN

        def on_block(node)
          return unless transaction_rescue?(node)

          add_offense(node)
        end
        alias on_numblock on_block
      end
    end
  end
end

It will only detect beginless rescue (not when it was started with an explicit begin) but overall should be about what you want. I didn't test it much though.

[skhrshin]

I would like to have this cop. In my recent work, I have observed that our project members (including me) often forget to add raise ActiveRecord::Rollback.

To make transaction behavior more readable, I suggest writing cases where we intentionally suppress rollbacks for specific errors using a discrete block, like the example below. This makes it clear that the error is intentionally ignored within an active transaction.

ApplicationRecord.transaction do
  begin
    ...
  rescue ActiveRecord::RecordNotUnique
  end
end