Add new Rails/PrivateTransactionOption cop
#1236
Conversation
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. rails/rails#39912 (comment) rails/rails#46182 (comment) Passing `joinable: false` may cause unexpected behavior such as the `after_commit` callback not firing at the appropriate time.
|
Can you first propose an addition to the 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. |
|
Opened rubocop/rails-style-guide#355.
A quick Google search shows the following articles:
Some articles mention unintended side effects, while others do not. |
|
https://api.rubyonrails.org/v6.0/classes/ActiveRecord/ConnectionAdapters/DatabaseStatements.html#method-i-transaction documents |
|
#414 Related |
|
|
Some more context rspec/rspec-rails#2598 (comment) - I withdraw my statement that adding |
|
Related cop suggestion #400 |
|
Agreed with your opinion. I don't believe it's always right to require In reality, there may be situations where you have to use |
|
Should it be possible to wrap a call to a gem that doesn’t It is worth mentioning that when reasoning about nested transactions and business logic, the error kernel design pattern comes handy. |
|
As far as I know, there's no way around it. You need to fix the gem. |
| # | ||
| # @safety | ||
| # This Cop is unsafe because it cannot accurately identify | ||
| # the `ActiveRecord::Base.transaction` method call. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
|
🤔 Should this be part of a more general cop for avoiding private ActiveRecord APIs? |
config/default.yml
Outdated
| @@ -782,6 +782,12 @@ Rails/Present: | |||
| # Convert usages of `unless blank?` to `if present?` | |||
| UnlessBlank: true | |||
|
|
|||
| Rails/PrivateTransactionOption: | |||
| Description: 'Avoid use of `ActiveRecord::Base.transaction(joinable: _)`.' | |||
There was a problem hiding this comment.
Some reasoning could be added to the message here.
Co-authored-by: Phil Pirozhkov <[email protected]>
wata727
force-pushed
the
add_new_rails_private_transaction_option
branch
from
7e71ea4
to
c22574f
Compare
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. |
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? |
This PR adds a new cop called
Rails/PrivateTransactionOption.This cop checks whether
ActiveRecord::Base.transaction(joinable: _)is used. Thejoinableoption is a private API and is not intended to be called from outside Active Record core.after_create_callbackis called before the transaction is commited started withjoinable: falserails/rails#46182 (comment)Passing
joinable: falsemay cause unexpected behavior such as theafter_commitcallback not firing at the appropriate time.See also kufu/activerecord-bitemporal#156
The motivation for wanting to add this is that even though the
joinableis 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 enablerequires_newimplicitly without realizing that it affects the behavior ofafter_commit.Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).bundle exec rake default. It executes all tests and runs RuboCop on its own code.{change_type}_{change_description}.mdif the new code introduces user-observable changes. See changelog entry format for details.