diff --git a/changelog/new_where_range_cop.md b/changelog/new_where_range_cop.md new file mode 100644 index 0000000000..627552a0b3 --- /dev/null +++ b/changelog/new_where_range_cop.md @@ -0,0 +1 @@ +* [#1272](https://github.com/rubocop/rubocop-rails/pull/1272): Add new `Rails/WhereRange` cop. ([@fatkodima][]) diff --git a/config/default.yml b/config/default.yml index 7d1a7c1120..d8837f63c5 100644 --- a/config/default.yml +++ b/config/default.yml @@ -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: '<>' + # Accept `redirect_to(...) and return` and similar cases. Style/AndOr: EnforcedStyle: conditionals diff --git a/lib/rubocop/cop/rails/where_range.rb b/lib/rubocop/cop/rails/where_range.rb new file mode 100644 index 0000000000..18c73cfe3b --- /dev/null +++ b/lib/rubocop/cop/rails/where_range.rb @@ -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 `%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 diff --git a/lib/rubocop/cop/rails_cops.rb b/lib/rubocop/cop/rails_cops.rb index 0d92ff4ca5..e5173baeb0 100644 --- a/lib/rubocop/cop/rails_cops.rb +++ b/lib/rubocop/cop/rails_cops.rb @@ -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' diff --git a/spec/rubocop/cop/rails/where_range_spec.rb b/spec/rubocop/cop/rails/where_range_spec.rb new file mode 100644 index 0000000000..d118b76f83 --- /dev/null +++ b/spec/rubocop/cop/rails/where_range_spec.rb @@ -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..) + 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