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/WhereRange cop #1272

Merged
merged 1 commit into from
May 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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_where_range_cop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* [#1272](https://github.com/rubocop/rubocop-rails/pull/1272): Add new `Rails/WhereRange` cop. ([@fatkodima][])
6 changes: 6 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1222,6 +1222,12 @@ Rails/WhereNotWithMultipleConditions:
VersionAdded: '2.17'
VersionChanged: '2.18'

Rails/WhereRange:
Description: 'Use ranges in `where` instead of manually constructing SQL.'
StyleGuide: 'https://rails.rubystyle.guide/#where-ranges'
Enabled: pending
VersionAdded: '<<next>>'

# Accept `redirect_to(...) and return` and similar cases.
Style/AndOr:
EnforcedStyle: conditionals
Expand Down
157 changes: 157 additions & 0 deletions lib/rubocop/cop/rails/where_range.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
# frozen_string_literal: true

module RuboCop
module Cop
module Rails
# Identifies places where manually constructed SQL
# in `where` can be replaced with ranges.
#
# @example
# # bad
# User.where('age >= ?', 18)
# User.where.not('age >= ?', 18)
# User.where('age < ?', 18)
# User.where('age >= ? AND age < ?', 18, 21)
# User.where('age >= :start', start: 18)
# User.where('users.age >= ?', 18)
#
# # good
# User.where(age: 18..)
# User.where.not(age: 18..)
# User.where(age: ...18)
# User.where(age: 18...21)
# User.where(users: { age: 18.. })
#
# # good
# # There are no beginless ranges in ruby.
# User.where('age > ?', 18)
#
class WhereRange < Base
include RangeHelp
extend AutoCorrector
extend TargetRubyVersion
extend TargetRailsVersion

MSG = 'Use `%<good_method>s` instead of manually constructing SQL.'

RESTRICT_ON_SEND = %i[where not].freeze

# column >= ?
GTEQ_ANONYMOUS_RE = /\A([\w.]+)\s+>=\s+\?\z/.freeze
# column <[=] ?
LTEQ_ANONYMOUS_RE = /\A([\w.]+)\s+(<=?)\s+\?\z/.freeze
# column >= ? AND column <[=] ?
RANGE_ANONYMOUS_RE = /\A([\w.]+)\s+>=\s+\?\s+AND\s+\1\s+(<=?)\s+\?\z/i.freeze
# column >= :value
GTEQ_NAMED_RE = /\A([\w.]+)\s+>=\s+:(\w+)\z/.freeze
# column <[=] :value
LTEQ_NAMED_RE = /\A([\w.]+)\s+(<=?)\s+:(\w+)\z/.freeze
# column >= :value1 AND column <[=] :value2
RANGE_NAMED_RE = /\A([\w.]+)\s+>=\s+:(\w+)\s+AND\s+\1\s+(<=?)\s+:(\w+)\z/i.freeze

minimum_target_ruby_version 2.6
minimum_target_rails_version 6.0

def_node_matcher :where_range_call?, <<~PATTERN
{
(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_range_call?(node) do |template_node, values_node|
column, value = extract_column_and_value(template_node, values_node)

return unless column

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

add_offense(range, message: message) do |corrector|
corrector.replace(range, good_method)
end
end
end

private

def where_not?(node)
receiver = node.receiver
receiver&.send_type? && receiver&.method?(:where)
end

# rubocop:disable Metrics
def extract_column_and_value(template_node, values_node)
value =
case template_node.value
when GTEQ_ANONYMOUS_RE
"#{values_node[0].source}.."
when LTEQ_ANONYMOUS_RE
range_operator = range_operator(Regexp.last_match(2))
"#{range_operator}#{values_node[0].source}" if target_ruby_version >= 2.7
when RANGE_ANONYMOUS_RE
if values_node.size >= 2
range_operator = range_operator(Regexp.last_match(2))
"#{values_node[0].source}#{range_operator}#{values_node[1].source}"
end
when GTEQ_NAMED_RE
value_node = values_node[0]

if value_node.hash_type?
pair = find_pair(value_node, Regexp.last_match(2))
"#{pair.value.source}.." if pair
end
when LTEQ_NAMED_RE
value_node = values_node[0]

if value_node.hash_type?
pair = find_pair(value_node, Regexp.last_match(2))
if pair && target_ruby_version >= 2.7
range_operator = range_operator(Regexp.last_match(2))
"#{range_operator}#{pair.value.source}"
end
end
when RANGE_NAMED_RE
value_node = values_node[0]

if value_node.hash_type?
range_operator = range_operator(Regexp.last_match(3))
pair1 = find_pair(value_node, Regexp.last_match(2))
pair2 = find_pair(value_node, Regexp.last_match(4))
"#{pair1.value.source}#{range_operator}#{pair2.value.source}" if pair1 && pair2
end
end

[Regexp.last_match(1), value] if value
end
# rubocop:enable Metrics

def range_operator(comparison_operator)
comparison_operator == '<' ? '...' : '..'
end

def find_pair(hash_node, value)
hash_node.pairs.find { |pair| pair.key.value.to_sym == value.to_sym }
end

def offense_range(node)
range_between(node.loc.selector.begin_pos, node.source_range.end_pos)
end

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

"#{method_name}(#{table}: { #{column}: #{value} })"
else
"#{method_name}(#{column}: #{value})"
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 @@ -138,3 +138,4 @@
require_relative 'rails/where_missing'
require_relative 'rails/where_not'
require_relative 'rails/where_not_with_multiple_conditions'
require_relative 'rails/where_range'
202 changes: 202 additions & 0 deletions spec/rubocop/cop/rails/where_range_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,202 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Rails::WhereRange, :config do
context 'Ruby <= 2.5', :ruby25 do
it 'does not register an offense when using anonymous `>=`' do
expect_no_offenses(<<~RUBY)
Model.where('column >= ?', value)
RUBY
end

it 'does not register an offense when using anonymous `<=`' do
expect_no_offenses(<<~RUBY)
Model.where('column <= ?', value)
RUBY
end
end

context 'Rails 5.1', :rails51 do
it 'does not register an offense when using anonymous `>=`' do
expect_no_offenses(<<~RUBY)
Model.where('column >= ?', value)
RUBY
end
end

context 'Rails 6.0', :rails60 do
context 'Ruby >= 2.6', :ruby26 do
it 'registers an offense and corrects when using anonymous `>=`' do
expect_offense(<<~RUBY)
Model.where('column >= ?', value)
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(column: value..)` instead of manually constructing SQL.
RUBY

expect_correction(<<~RUBY)
Model.where(column: value..)
Copy link
Member

Choose a reason for hiding this comment

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

Endless ranges (start..) are supported since Ruby 2.6, while beginless ranges (..end) are supported since Ruby 2.7. Each should be considered accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ruby 2.7 is EOL for a long time. Not to mention 2.6.
Instead, wdyt about dropping the support for it altogether from the gem?

Copy link
Member

Choose a reason for hiding this comment

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

The runtime Ruby version (2.7+) and parsing Ruby version (2.0+) are different. This is the parsing Ruby version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean by the original comment, that I should update the test cases only or the code of the cop too?

Copy link
Member

Choose a reason for hiding this comment

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

RSpec supports Ruby 1.8.7
RuboCop “Targets Ruby 2.0+ code analysis”.
RuboCop Rails:

Rails cops support the following versions:
Rails 4.2+

Please don’t get me wrong, I always advocate to drop support for EOL versions, even in minor/patch versions. But while those versions are supported, we need to be compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added minimum_target_ruby_version 2.7. Do you mean to lower it to 2.6 and handle both cases of endless ranges support differently?

Copy link
Member

Choose a reason for hiding this comment

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

I think it’s just perfect. My apologies for my ambiguous language, I meant not to break it for earlier ruby/rails versions.

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean to lower it to 2.6 and handle both cases of endless ranges support differently?

Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Please, take a look.

RUBY
end

it 'registers an offense and corrects when using named `>=`' do
expect_offense(<<~RUBY)
Model.where('column >= :min', min: value)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(column: value..)` instead of manually constructing SQL.
RUBY

expect_correction(<<~RUBY)
Model.where(column: value..)
RUBY
end

it 'registers an offense and corrects when using anonymous `>=` with an explicit table' do
expect_offense(<<~RUBY)
Model.where('table.column >= ?', value)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(table: { column: value.. })` instead of manually constructing SQL.
RUBY

expect_correction(<<~RUBY)
Model.where(table: { column: value.. })
RUBY
end

it 'does not register an offense when using anonymous `>`' do
expect_no_offenses(<<~RUBY)
Model.where('column > ?', value)
RUBY
end

it 'registers an offense and corrects when using anonymous `>= AND <`' do
expect_offense(<<~RUBY)
Model.where('column >= ? AND column < ?', value1, value2)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(column: value1...value2)` instead of manually constructing SQL.
RUBY

expect_correction(<<~RUBY)
Model.where(column: value1...value2)
RUBY
end

it 'registers an offense and corrects when using anonymous `>= AND <=`' do
expect_offense(<<~RUBY)
Model.where('column >= ? AND column <= ?', value1, value2)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(column: value1..value2)` instead of manually constructing SQL.
RUBY

expect_correction(<<~RUBY)
Model.where(column: value1..value2)
RUBY
end

it 'registers an offense and corrects when using named `>= AND <`' do
expect_offense(<<~RUBY)
Model.where('column >= :min AND column < :max', min: value1, max: value2)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(column: value1...value2)` instead of manually constructing SQL.
RUBY

expect_correction(<<~RUBY)
Model.where(column: value1...value2)
RUBY
end

it 'does not register an offense when using different columns' do
expect_no_offenses(<<~RUBY)
Model.where('column1 >= ? AND column2 < ?', value1, value2)
RUBY
end

it 'does not register an offense when using named `>= AND <` and placeholders do not exist' do
expect_no_offenses(<<~RUBY)
Model.where('column >= :min AND column < :max', min: value)
RUBY
end

it 'registers an offense and corrects when using `>=` with an array' do
expect_offense(<<~RUBY)
Model.where(['column >= ?', value])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(column: value..)` instead of manually constructing SQL.
RUBY

expect_correction(<<~RUBY)
Model.where(column: value..)
RUBY
end

it 'registers an offense and corrects when using `>= AND <=` with an array' do
expect_offense(<<~RUBY)
Model.where(['column >= ? AND column <= ?', value1, value2])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(column: value1..value2)` instead of manually constructing SQL.
RUBY

expect_correction(<<~RUBY)
Model.where(column: value1..value2)
RUBY
end

it 'registers an offense and corrects when using `where.not`' do
expect_offense(<<~RUBY)
Model.where.not('column >= ?', value)
^^^^^^^^^^^^^^^^^^^^^^^^^ Use `not(column: value..)` instead of manually constructing SQL.
RUBY

expect_correction(<<~RUBY)
Model.where.not(column: value..)
RUBY
end

it 'does not register an offense when using ranges' do
expect_no_offenses(<<~RUBY)
Model.where(column: value..)
RUBY
end

it 'does not register an offense when using `not` with ranges' do
expect_no_offenses(<<~RUBY)
Model.where.not(column: value..)
RUBY
end

it 'does not register an offense when using `not` not preceding by `where`' do
expect_no_offenses(<<~RUBY)
foo.not('column >= ?', value)
RUBY
end
end

context 'Ruby >= 2.6', :ruby26, unsupported_on: :prism do
it 'does not register an offense when using anonymous `<=`' do
expect_no_offenses(<<~RUBY)
Model.where('column <= ?', value)
RUBY
end

it 'does not register an offense when using anonymous `<`' do
expect_no_offenses(<<~RUBY)
Model.where('column < ?', value)
RUBY
end
end

context 'Ruby >= 2.7', :ruby27 do
it 'registers an offense and corrects when using anonymous `<=`' do
expect_offense(<<~RUBY)
Model.where('column <= ?', value)
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(column: ..value)` instead of manually constructing SQL.
RUBY

expect_correction(<<~RUBY)
Model.where(column: ..value)
RUBY
end

it 'registers an offense and corrects when using anonymous `<`' do
expect_offense(<<~RUBY)
Model.where('column < ?', value)
^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(column: ...value)` instead of manually constructing SQL.
RUBY

expect_correction(<<~RUBY)
Model.where(column: ...value)
RUBY
end
end
end
end