Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new Rails/PrivateTransactionOption cop #1236

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/new_add_rails_private_transaction_option_cop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#1236](https://github.com/rubocop/rubocop-rails/pull/1236): Add new `Rails/PrivateTransactionOption`. ([@wata727][])
6 changes: 6 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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: _)`.'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some reasoning could be added to the message here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed d8b64ad

Enabled: pending
Safe: false
VersionAdded: '<<next>>'

Rails/RakeEnvironment:
Description: 'Include `:environment` as a dependency for all Rake tasks.'
Enabled: true
Expand Down
44 changes: 44 additions & 0 deletions lib/rubocop/cop/rails/private_transaction_option.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Rails
# 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.
#
# @safety
# This Cop is unsafe because it cannot accurately identify
# the `ActiveRecord::Base.transaction` method call.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

#
# @example
# # bad
# ActiveRecord::Base.transaction(requires_new: true, joinable: false)
#
# # good
# ActiveRecord::Base.transaction(requires_new: true)
#
class PrivateTransactionOption < Base
MSG = 'Do not use `ActiveRecord::Base.transaction(joinable: _)`.'
wata727 marked this conversation as resolved.
Show resolved Hide resolved
RESTRICT_ON_SEND = %i[transaction].freeze

# @!method match_transaction_with_joinable(node)
def_node_matcher :match_transaction_with_joinable, <<~PATTERN
(send _ :transaction (hash <$(pair (sym :joinable) {true false}) ...>))
PATTERN

def on_send(node)
match_transaction_with_joinable(node) do |option_node|
add_offense(option_node)
end
end
end
end
end
end
1 change: 1 addition & 0 deletions lib/rubocop/cop/rails_cops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@
require_relative 'rails/pluralization_grammar'
require_relative 'rails/presence'
require_relative 'rails/present'
require_relative 'rails/private_transaction_option'
require_relative 'rails/rake_environment'
require_relative 'rails/read_write_attribute'
require_relative 'rails/redundant_active_record_all_method'
Expand Down
16 changes: 16 additions & 0 deletions spec/rubocop/cop/rails/private_transaction_option_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Rails::PrivateTransactionOption, :config do
it 'registers an offense when using `joinable: false`' do
expect_offense(<<~RUBY)
ActiveRecord::Base.transaction(requires_new: true, joinable: false)
^^^^^^^^^^^^^^^ Do not use `ActiveRecord::Base.transaction(joinable: _)`.
wata727 marked this conversation as resolved.
Show resolved Hide resolved
RUBY
end

wata727 marked this conversation as resolved.
Show resolved Hide resolved
it 'does not register an offense when using only `requires_new: true`' do
expect_no_offenses(<<~RUBY)
ActiveRecord::Base.transaction(requires_new: true)
wata727 marked this conversation as resolved.
Show resolved Hide resolved
RUBY
end
end