From cbce188baf2cdc9372045ad2a56f2273799331d8 Mon Sep 17 00:00:00 2001 From: fatkodima Date: Fri, 26 Apr 2024 12:13:56 +0300 Subject: [PATCH] Add new `Rails/WhereRange` cop --- changelog/new_where_range_cop.md | 1 + config/default.yml | 6 + lib/rubocop/cop/rails/where_range.rb | 155 ++++++++++++++++++++ lib/rubocop/cop/rails_cops.rb | 1 + spec/rubocop/cop/rails/where_range_spec.rb | 160 +++++++++++++++++++++ 5 files changed, 323 insertions(+) create mode 100644 changelog/new_where_range_cop.md create mode 100644 lib/rubocop/cop/rails/where_range.rb create mode 100644 spec/rubocop/cop/rails/where_range_spec.rb 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..0352ccb504 --- /dev/null +++ b/lib/rubocop/cop/rails/where_range.rb @@ -0,0 +1,155 @@ +# 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 + + MSG = 'Use `%s` instead of manually constructing SQL.' + + RESTRICT_ON_SEND = %i[where not].freeze + + 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 + + # 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 + + 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}" + when RANGE_ANONYMOUS_RE + return if values_node.size < 2 + + range_operator = range_operator(Regexp.last_match(2)) + "#{values_node[0].source}#{range_operator}#{values_node[1].source}" + when GTEQ_NAMED_RE + value_node = values_node[0] + return unless value_node.hash_type? + + pair = find_pair(value_node, Regexp.last_match(2)) + return unless pair + + "#{pair.value.source}.." + when LTEQ_NAMED_RE + value_node = values_node[0] + return unless value_node.hash_type? + + pair = find_pair(value_node, Regexp.last_match(2)) + return unless pair + + range_operator = range_operator(Regexp.last_match(2)) + "#{range_operator}#{pair.value.source}" + when RANGE_NAMED_RE + value_node = values_node[0] + return unless 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)) + return unless pair1 && pair2 + + "#{pair1.value.source}#{range_operator}#{pair2.value.source}" + else + return + end + + [Regexp.last_match(1), 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..de34415aaf --- /dev/null +++ b/spec/rubocop/cop/rails/where_range_spec.rb @@ -0,0 +1,160 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Rails::WhereRange, :config 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 `<=`' 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 + + 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