From 3885fcce786fc6df9bfb2a84d3b6e9399ab90c2b Mon Sep 17 00:00:00 2001 From: Earlopain <14981592+Earlopain@users.noreply.github.com> Date: Thu, 15 Aug 2024 12:53:24 +0200 Subject: [PATCH] [Fix #1199] Make `Rails/WhereEquals` aware of `where.not(...)` This copies a bit of logic from `WhereRange` but they need slighly different handling and it doesn't seem worth to extract yet. --- ...ix_make_rails_where_equals_aware_of_not.md | 1 + config/default.yml | 4 +-- lib/rubocop/cop/rails/where_equals.rb | 27 +++++++++++++------ spec/rubocop/cop/rails/where_equals_spec.rb | 17 ++++++++++++ 4 files changed, 39 insertions(+), 10 deletions(-) create mode 100644 changelog/fix_make_rails_where_equals_aware_of_not.md diff --git a/changelog/fix_make_rails_where_equals_aware_of_not.md b/changelog/fix_make_rails_where_equals_aware_of_not.md new file mode 100644 index 0000000000..1a61ab42f3 --- /dev/null +++ b/changelog/fix_make_rails_where_equals_aware_of_not.md @@ -0,0 +1 @@ +* [#1199](https://github.com/rubocop/rubocop-rails/issues/1199): Make `Rails/WhereEquals` aware of `where.not(...)`. ([@earlopain][]) diff --git a/config/default.yml b/config/default.yml index 561b36484a..1f3b5376dc 100644 --- a/config/default.yml +++ b/config/default.yml @@ -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: <> Rails/WhereExists: Description: 'Prefer `exists?(...)` over `where(...).exists?`.' diff --git a/lib/rubocop/cop/rails/where_equals.rb b/lib/rubocop/cop/rails/where_equals.rb index ce5b6d2a58..b1d56b7c24 100644 --- a/lib/rubocop/cop/rails/where_equals.rb +++ b/lib/rubocop/cop/rails/where_equals.rb @@ -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. @@ -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']) @@ -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' }) @@ -29,16 +32,18 @@ class WhereEquals < Base extend AutoCorrector MSG = 'Use `%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 @@ -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| @@ -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 diff --git a/spec/rubocop/cop/rails/where_equals_spec.rb b/spec/rubocop/cop/rails/where_equals_spec.rb index 11083179f9..1af170ebff 100644 --- a/spec/rubocop/cop/rails/where_equals_spec.rb +++ b/spec/rubocop/cop/rails/where_equals_spec.rb @@ -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') @@ -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