Skip to content

Commit

Permalink
[Fix #1199] Make Rails/WhereEquals aware of where.not(...)
Browse files Browse the repository at this point in the history
This copies a bit of logic from `WhereRange` but they need slighly
different handling and it doesn't seem worth to extract yet.
  • Loading branch information
Earlopain committed Aug 15, 2024
1 parent b8c4126 commit 3885fcc
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 10 deletions.
1 change: 1 addition & 0 deletions changelog/fix_make_rails_where_equals_aware_of_not.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#1199](https://github.com/rubocop/rubocop-rails/issues/1199): Make `Rails/WhereEquals` aware of `where.not(...)`. ([@earlopain][])
4 changes: 2 additions & 2 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1185,12 +1185,12 @@ Rails/Validation:
- app/models/**/*.rb

Rails/WhereEquals:
Description: 'Pass conditions to `where` as a hash instead of manually constructing SQL.'
Description: 'Pass conditions to `where` and `where.not` as a hash instead of manually constructing SQL.'
StyleGuide: 'https://rails.rubystyle.guide/#hash-conditions'
Enabled: 'pending'
SafeAutoCorrect: false
VersionAdded: '2.9'
VersionChanged: '2.10'
VersionChanged: <<next>>

Rails/WhereExists:
Description: 'Prefer `exists?(...)` over `where(...).exists?`.'
Expand Down
27 changes: 19 additions & 8 deletions lib/rubocop/cop/rails/where_equals.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ module RuboCop
module Cop
module Rails
# Identifies places where manually constructed SQL
# in `where` can be replaced with `where(attribute: value)`.
# in `where` and `where.not` can be replaced with
# `where(attribute: value)` and `where.not(attribute: value)`.
#
# @safety
# This cop's autocorrection is unsafe because is may change SQL.
Expand All @@ -13,6 +14,7 @@ module Rails
# @example
# # bad
# User.where('name = ?', 'Gabe')
# User.where.not('name = ?', 'Gabe')
# User.where('name = :name', name: 'Gabe')
# User.where('name IS NULL')
# User.where('name IN (?)', ['john', 'jane'])
Expand All @@ -21,6 +23,7 @@ module Rails
#
# # good
# User.where(name: 'Gabe')
# User.where.not(name: 'Gabe')
# User.where(name: nil)
# User.where(name: ['john', 'jane'])
# User.where(users: { name: 'Gabe' })
Expand All @@ -29,16 +32,18 @@ class WhereEquals < Base
extend AutoCorrector

MSG = 'Use `%<good_method>s` instead of manually constructing SQL.'
RESTRICT_ON_SEND = %i[where].freeze
RESTRICT_ON_SEND = %i[where not].freeze

def_node_matcher :where_method_call?, <<~PATTERN
{
(call _ :where (array $str_type? $_ ?))
(call _ :where $str_type? $_ ?)
(call _ {:where :not} (array $str_type? $_ ?))
(call _ {:where :not} $str_type? $_ ?)
}
PATTERN

def on_send(node)
return if node.method?(:not) && !where_not?(node)

where_method_call?(node) do |template_node, value_node|
value_node = value_node.first

Expand All @@ -47,7 +52,7 @@ def on_send(node)
column, value = extract_column_and_value(template_node, value_node)
return unless value

good_method = build_good_method(column, value)
good_method = build_good_method(node.method_name, column, value)
message = format(MSG, good_method: good_method)

add_offense(range, message: message) do |corrector|
Expand Down Expand Up @@ -88,15 +93,21 @@ def extract_column_and_value(template_node, value_node)
[Regexp.last_match(1), value]
end

def build_good_method(column, value)
def build_good_method(method_name, column, value)
if column.include?('.')
table, column = column.split('.')

"where(#{table}: { #{column}: #{value} })"
"#{method_name}(#{table}: { #{column}: #{value} })"
else
"where(#{column}: #{value})"
"#{method_name}(#{column}: #{value})"
end
end

def where_not?(node)
return false unless (receiver = node.receiver)

receiver.send_type? && receiver.method?(:where)
end
end
end
end
Expand Down
17 changes: 17 additions & 0 deletions spec/rubocop/cop/rails/where_equals_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,17 @@
RUBY
end

it 'registers an offense and corrects when using `=` and anonymous placeholder with not' do
expect_offense(<<~RUBY)
User.where.not('name = ?', 'Gabe')
^^^^^^^^^^^^^^^^^^^^^^^ Use `not(name: 'Gabe')` instead of manually constructing SQL.
RUBY

expect_correction(<<~RUBY)
User.where.not(name: 'Gabe')
RUBY
end

it 'registers an offense and corrects when using `=` and anonymous placeholder with safe navigation' do
expect_offense(<<~RUBY)
User&.where('name = ?', 'Gabe')
Expand Down Expand Up @@ -200,4 +211,10 @@
User.where("name IN (:names)", )
RUBY
end

it 'does not register an offense when using `not` not preceded by `where`' do
expect_no_offenses(<<~RUBY)
users.not('name = ?', 'Gabe')
RUBY
end
end

0 comments on commit 3885fcc

Please sign in to comment.