ActiveRecord::Base.transaction with requires_new: true is recommended

[ipepe]

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

Not all databases support nested transactions. Therefore, Rails will sometimes silently ignore a nested transaction and simply reuse the other transaction. However, a ActiveRecord::Rollback within the nested transaction will be caught by the block of the nested transaction. Therefore it will be ignored by the outer transaction, and not cause a roll back!
To avoid this unexpected behaviour, you have to explicitly tell rails for each transaction to indeed use proper nesting:

ActiveRecord::Base.transaction(joinable: false, requires_new: true) do
  # inner code
end

This is a safer default for working with custom transactions.

Source: https://makandracards.com/makandra/42885-nested-activerecord-transaction-pitfalls

Describe the solution you'd like

A rubocop Cop that would highlight .transaction without requires_new: true argument. I think it should be doable to implement it with autocorrect.

Describe alternatives you've considered

I didn't consider any alternatives to implementing this as rubocop-rails cop.

Additional context

This may be hard because Cop would need to check class hierarchy to see if for example Email.transaction is ActiveRecord transaction or is it SMTP or other type of transaction.

[tsrivishnu]

I don't really know how hard it is to implement this but I like the idea of adding this cop.

[tsrivishnu]

A potential for bugs when using after_commit callbacks:

If a nested transaction is created with requires_new: true and joinable: false, the after_commit callbacks are triggered for records created inside this nested transaction even before the parent transaction is committed.

In our case, we send record changes using after_commit to Kafka logs so that we have a log of changes. In nested transactions, we noticed that there are change messages in Kafka that don't reflect the database state. This leads to inconsistencies. I haven't testing using after_rollback to send another message to discard the previous one.

For demonstration:

class User < ApplicationRecord
  after_commit :emit_change_message

  def emit_change_mess
    puts "Changing the user's name: #{first_name} #{last_name}"
    # Log changes to kafka
    ...
  end
end

ActiveRecord::Base.transaction(joinable: false, requires_new: true) do
  User.last.update(first_name: "Transaction 1")
  ActiveRecord::Base.transaction(joinable: false, requires_new: true) do
    User.last.update(last_name: "Transaction 2")
    raise ActiveRecord::Rollback
  end
end
# => Changing user's name: first_name: Transaction 1, last_name: Doe
# => Changing user's name: first_name: Transaction 1, last_name: Transaction 2

User.last.first_name
# => "John"
User.last.last_name
# => "Doe"

In the above example, the changes are rollbacked as expected but after_commit callbacks are triggered. If joinable: true is used, the after_commit callbacks are not triggered but the other transaction is not rollbacked.

After noticing this, I think it best's to not recommend using requires_new: true for transactions as a default. The implications are not very evident/obvious and developers might find it very hard to debug issues that are caused with after_commit being triggered from nested transactions.

Instead of recommending transaction(requires_new: true), I would recommend not raising ActiveRecord::Rollback in transactions.

[adamrdavid]

@tsrivishnu I have experienced the behavior you describe queuing sidekiq jobs in after_commit hooks. From this rails code, I believe the issue is with joinable: false and does not affect requires_new: true.

https://github.com/rails/rails/blob/v5.2.6/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb#L196-L205

It does seem however the joinable: false cannot be avoided for cases of dependent: :destroy as mentioned here

Note that all exceptions other than ActiveRecord::Rollback will also trigger rollbacks, but not suffer from this problem, so it might be a better choice to simply avoid ActiveRecord::Rollback. However the issue does not go away, since some ActiveRecord features (e.g. autosave: true, dependent: destroy ) will themselves trigger ActiveRecord::Rollback if things go wrong.