Add new `Rails/PrivateTransactionOption` cop

[wata727]

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.

See also https://github.com/kufu/activerecord-bitemporal/pull/156

The motivation for wanting to add this is that even though the joinable is undocumented, it was recommended in some articles, so it may be inadvertently widely used. It is possible that it is being used incorrectly to simply enable requires_new implicitly without realizing that it affects the behavior of after_commit.

---

Before submitting the PR make sure the following are checked:

• [x] The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• [x] Wrote good commit messages.
• [x] Commit message starts with [Fix #issue-number] (if the related issue exists).
• [x] Feature branch is up-to-date with master (if not - rebase it).
• [x] Squashed related commits together.
• [x] Added tests.
• [x] Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
• [x] Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
• [x] If this is a new cop, consider making a corresponding update to the Rails Style Guide.

[koic]

Can you first propose an addition to the Rails Style Guide? https://github.com/rubocop/rails-style-guide

I'm not sure which articles have made incorrect references, but including a description in the Style Guide that RuboCop Rails follows would serve as one countermeasure to them.

[wata727]

Opened https://github.com/rubocop/rails-style-guide/pull/355.

I'm not sure which articles have made incorrect references

A quick Google search shows the following articles:

• https://makandracards.com/makandra/42885-nested-activerecord-transaction-pitfalls
• https://dev.to/gerardosandoval/understanding-activerecord-nested-transactions-3hee
• https://zenn.dev/merda/articles/cbe2fdfd97ee75 (Japanese)
• https://thinkami.hatenablog.com/entry/2021/06/30/014210 (Japanese)

Some articles mention unintended side effects, while others do not.

[pirj]

https://api.rubyonrails.org/v6.0/classes/ActiveRecord/ConnectionAdapters/DatabaseStatements.html#method-i-transaction documents joinable while not the recent. What happened?

[pirj]

https://github.com/rubocop/rubocop-rails/pull/414 Related

[wata727]

documents joinable while not the recent. What happened?

See https://github.com/rails/rails/pull/46186

[pirj]

Some more context https://github.com/rspec/rspec-rails/issues/2598#issuecomment-1109014190 - I withdraw my statement that adding joinable: false is a good practice. With requires_new: true and all explicit transactions (apart from implicit update/create/etc transactions), joinable: false is redundant. And considering the side effects (due to joinable? checks?) it’s plain bad.

[pirj]

Related cop suggestion https://github.com/rubocop/rubocop-rails/issues/400

[wata727]

Agreed with your opinion. I don't believe it's always right to require requires_new: true in all transactions, but I do believe that using joinable: false is probably wrong most of the time.

In reality, there may be situations where you have to use joinable: false because the transactions used inside a third-party gem do not use requires_new: true, but considering the side effects, it should be a last resort. This cop helps you understand that it must be used with caution and its behavior is not guaranteed by Rails core team.

[pirj]

Should it be possible to wrap a call to a gem that doesn’t require_new in a transaction to isolate it instead of a wrapping joinable: false?

It is worth mentioning that when reasoning about nested transactions and business logic, the error kernel design pattern comes handy.

[wata727]

As far as I know, there's no way around it. You need to fix the gem. It might be useful to add some notes on how to fix such things, but I think that is outside the scope of this cop.

[pirj]

@koic @andyw8 @bbatsov WDYT?

[andyw8]

🤔 Should this be part of a more general cop for avoiding private ActiveRecord APIs?

[wata727]

🤔 Should this be part of a more general cop for avoiding private ActiveRecord APIs?

This might be one option, but at this point, I'm not sure which other APIs should be included. Also, if we want to provide other options for each private API, it may be useful to have a Cop for each one.

[koic]

Rails/PrivateTransactionOption cop

Can you consider naming the cop to suggest the risks associated with unexpected behavior in nested transactions, rather than focusing on the use of a private API?