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.

• after_commit is called after RELEASE SAVEPOINT rails/rails#39912 (comment) -
• after_create_callback is called before the transaction is commited started with joinable: false rails/rails#46182 (comment)

Passing joinable: false may cause unexpected behavior such as the after_commit callback not firing at the appropriate time.

See also kufu/activerecord-bitemporal#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:

• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
• 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.
• 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 rubocop/rails-style-guide#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]

#414 Related

[wata727]

documents joinable while not the recent. What happened?

See rails/rails#46186

[pirj]

Some more context rspec/rspec-rails#2598 (comment) - 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 #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]

A few minor fixes and good to go!
Thank you

[pirj]

Well, I bet transaction(joinable: true/false) would be a Rails transaction call with very high certainty.

I'd remove this clause and made the cop safe.

I would accept a single case of such a false positive from https://github.com/jeromedalbert/real-world-ruby-apps as a counter-argument.

[wata727]

Hmm, this is up for debate. Personally, I believe it should be marked as unsafe as long as there is theoretically a possibility of false positives.

I'd be interested to hear from other maintainers.

[koic]

It seems that the issue is not a false positive in the cop, but rather the change in behavior caused by the autocorrect. Therefore, shouldn't it be SafeAutoCorrect: false instead of Safe: false?

[pirj]

@koic @andyw8 @bbatsov WDYT?

[andyw8]

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

[andyw8]

Some reasoning could be added to the message here.

[wata727]

Fixed d8b64ad

[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?

[koic]

ping @wata727

[wata727]

Thank you for the reminder. How about Rails/UnjoinableTransaction?