Merge pull request #362 from eugeneius/where_equals

Add new Rails/WhereEquals cop

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## master (unreleased)
 
+### New features
+
+* [#362](https://github.com/rubocop-hq/rubocop-rails/pull/362): Add new `Rails/WhereEquals` cop. ([@eugeneius][])
+
 ## 2.8.1 (2020-09-16)
 
 ### Bug fixes

config/default.yml

@@ -705,6 +705,12 @@ Rails/Validation:
   Include:
     - app/models/**/*.rb
 
+Rails/WhereEquals:
+  Description: 'Pass conditions to `where` as a hash instead of manually constructing SQL.'
+  StyleGuide: 'https://rails.rubystyle.guide/#hash-conditions'
+  Enabled: 'pending'
+  VersionAdded: '2.9'
+
 Rails/WhereExists:
   Description: 'Prefer `exists?(...)` over `where(...).exists?`.'
   Enabled: 'pending'
@@ -717,7 +723,7 @@ Rails/WhereExists:
 
 Rails/WhereNot:
   Description: 'Use `where.not(...)` instead of manually constructing negated SQL in `where`.'
-  StyleGuide: 'https://rails.rubystyle.guide/#where-not'
+  StyleGuide: 'https://rails.rubystyle.guide/#hash-conditions'
   Enabled: 'pending'
   VersionAdded: '2.8'
 

docs/modules/ROOT/pages/cops.adoc

@@ -95,6 +95,7 @@ based on the https://rails.rubystyle.guide/[Rails Style Guide].
 * xref:cops_rails.adoc#railsuniquevalidationwithoutindex[Rails/UniqueValidationWithoutIndex]
 * xref:cops_rails.adoc#railsunknownenv[Rails/UnknownEnv]
 * xref:cops_rails.adoc#railsvalidation[Rails/Validation]
+* xref:cops_rails.adoc#railswhereequals[Rails/WhereEquals]
 * xref:cops_rails.adoc#railswhereexists[Rails/WhereExists]
 * xref:cops_rails.adoc#railswherenot[Rails/WhereNot]
 

docs/modules/ROOT/pages/cops_rails.adoc

@@ -4285,6 +4285,42 @@ validates :foo, uniqueness: true
 | Array
 |===
 
+== Rails/WhereEquals
+
+|===
+| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
+
+| Pending
+| Yes
+| Yes
+| 2.9
+| -
+|===
+
+This cop identifies places where manually constructed SQL
+in `where` can be replaced with `where(attribute: value)`.
+
+=== Examples
+
+[source,ruby]
+----
+# bad
+User.where('name = ?', 'Gabe')
+User.where('name = :name', name: 'Gabe')
+User.where('name IS NULL')
+User.where('name IN (?)', ['john', 'jane'])
+User.where('name IN (:names)', names: ['john', 'jane'])
+
+# good
+User.where(name: 'Gabe')
+User.where(name: nil)
+User.where(name: ['john', 'jane'])
+----
+
+=== References
+
+* https://rails.rubystyle.guide/#hash-conditions
+
 == Rails/WhereExists
 
 |===
@@ -4387,4 +4423,4 @@ User.where.not(name: ['john', 'jane'])
 
 === References
 
-* https://rails.rubystyle.guide/#where-not
+* https://rails.rubystyle.guide/#hash-conditions

lib/rubocop/cop/rails/where_equals.rb

@@ -0,0 +1,105 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Rails
+      # This cop identifies places where manually constructed SQL
+      # in `where` can be replaced with `where(attribute: value)`.
+      #
+      # @example
+      #   # bad
+      #   User.where('name = ?', 'Gabe')
+      #   User.where('name = :name', name: 'Gabe')
+      #   User.where('name IS NULL')
+      #   User.where('name IN (?)', ['john', 'jane'])
+      #   User.where('name IN (:names)', names: ['john', 'jane'])
+      #
+      #   # good
+      #   User.where(name: 'Gabe')
+      #   User.where(name: nil)
+      #   User.where(name: ['john', 'jane'])
+      class WhereEquals < Cop
+        include RangeHelp
+
+        MSG = 'Use `%<good_method>s` instead of manually constructing SQL.'
+
+        def_node_matcher :where_method_call?, <<~PATTERN
+          {
+            (send _ :where (array $str_type? $_ ?))
+            (send _ :where $str_type? $_ ?)
+          }
+        PATTERN
+
+        def on_send(node)
+          where_method_call?(node) do |template_node, value_node|
+            value_node = value_node.first
+
+            range = offense_range(node)
+
+            column_and_value = extract_column_and_value(template_node, value_node)
+            return unless column_and_value
+
+            good_method = build_good_method(*column_and_value)
+            message = format(MSG, good_method: good_method)
+
+            add_offense(node, location: range, message: message)
+          end
+        end
+
+        def autocorrect(node)
+          where_method_call?(node) do |template_node, value_node|
+            value_node = value_node.first
+
+            lambda do |corrector|
+              range = offense_range(node)
+
+              column, value = *extract_column_and_value(template_node, value_node)
+              replacement = build_good_method(column, value)
+
+              corrector.replace(range, replacement)
+            end
+          end
+        end
+
+        EQ_ANONYMOUS_RE = /\A([\w.]+)\s+=\s+\?\z/.freeze             # column = ?
+        IN_ANONYMOUS_RE = /\A([\w.]+)\s+IN\s+\(\?\)\z/i.freeze       # column IN (?)
+        EQ_NAMED_RE     = /\A([\w.]+)\s+=\s+:(\w+)\z/.freeze         # column = :column
+        IN_NAMED_RE     = /\A([\w.]+)\s+IN\s+\(:(\w+)\)\z/i.freeze   # column IN (:column)
+        IS_NULL_RE      = /\A([\w.]+)\s+IS\s+NULL\z/i.freeze         # column IS NULL
+
+        private
+
+        def offense_range(node)
+          range_between(node.loc.selector.begin_pos, node.loc.expression.end_pos)
+        end
+
+        def extract_column_and_value(template_node, value_node)
+          value =
+            case template_node.value
+            when EQ_ANONYMOUS_RE, IN_ANONYMOUS_RE
+              value_node.source
+            when EQ_NAMED_RE, IN_NAMED_RE
+              return unless value_node.hash_type?
+
+              pair = value_node.pairs.find { |p| p.key.value.to_sym == Regexp.last_match(2).to_sym }
+              pair.value.source
+            when IS_NULL_RE
+              'nil'
+            else
+              return
+            end
+
+          [Regexp.last_match(1), value]
+        end
+
+        def build_good_method(column, value)
+          if column.include?('.')
+            "where('#{column}' => #{value})"
+          else
+            "where(#{column}: #{value})"
+          end
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/rails_cops.rb

@@ -83,5 +83,6 @@
 require_relative 'rails/unique_validation_without_index'
 require_relative 'rails/unknown_env'
 require_relative 'rails/validation'
+require_relative 'rails/where_equals'
 require_relative 'rails/where_exists'
 require_relative 'rails/where_not'

spec/rubocop/cop/rails/where_equals_spec.rb

@@ -0,0 +1,163 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Rails::WhereEquals do
+  subject(:cop) { described_class.new }
+
+  it 'registers an offense and corrects when using `=` and anonymous placeholder' do
+    expect_offense(<<~RUBY)
+      User.where('name = ?', 'Gabe')
+           ^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(name: 'Gabe')` instead of manually constructing SQL.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      User.where(name: 'Gabe')
+    RUBY
+  end
+
+  it 'registers an offense and corrects when using `=` and named placeholder' do
+    expect_offense(<<~RUBY)
+      User.where('name = :name', name: 'Gabe')
+           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(name: 'Gabe')` instead of manually constructing SQL.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      User.where(name: 'Gabe')
+    RUBY
+  end
+
+  it 'registers an offense and corrects when using `IS NULL`' do
+    expect_offense(<<~RUBY)
+      User.where('name IS NULL')
+           ^^^^^^^^^^^^^^^^^^^^^ Use `where(name: nil)` instead of manually constructing SQL.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      User.where(name: nil)
+    RUBY
+  end
+
+  it 'registers an offense and corrects when using `IN` and anonymous placeholder' do
+    expect_offense(<<~RUBY)
+      User.where("name IN (?)", ['john', 'jane'])
+           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(name: ['john', 'jane'])` instead of manually constructing SQL.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      User.where(name: ['john', 'jane'])
+    RUBY
+  end
+
+  it 'registers an offense and corrects when using `IN` and named placeholder' do
+    expect_offense(<<~RUBY)
+      User.where("name IN (:names)", names: ['john', 'jane'])
+           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(name: ['john', 'jane'])` instead of manually constructing SQL.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      User.where(name: ['john', 'jane'])
+    RUBY
+  end
+
+  it 'registers an offense and corrects when using `=` and namespaced columns' do
+    expect_offense(<<~RUBY)
+      Course.where('enrollments.student_id = ?', student.id)
+             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where('enrollments.student_id' => student.id)` instead of manually constructing SQL.
+    RUBY
+
+    expect_correction(<<~RUBY)
+      Course.where('enrollments.student_id' => student.id)
+    RUBY
+  end
+
+  context 'with array arguments' do
+    it 'registers an offense and corrects when using `=` and anonymous placeholder' do
+      expect_offense(<<~RUBY)
+        User.where(['name = ?', 'Gabe'])
+             ^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(name: 'Gabe')` instead of manually constructing SQL.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        User.where(name: 'Gabe')
+      RUBY
+    end
+
+    it 'registers an offense and corrects when using `=` and named placeholder' do
+      expect_offense(<<~RUBY)
+        User.where(['name = :name', { name: 'Gabe' }])
+             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(name: 'Gabe')` instead of manually constructing SQL.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        User.where(name: 'Gabe')
+      RUBY
+    end
+
+    it 'registers an offense and corrects when using `IS NULL`' do
+      expect_offense(<<~RUBY)
+        User.where(['name IS NULL'])
+             ^^^^^^^^^^^^^^^^^^^^^^^ Use `where(name: nil)` instead of manually constructing SQL.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        User.where(name: nil)
+      RUBY
+    end
+
+    it 'registers an offense and corrects when using `IN` and anonymous placeholder' do
+      expect_offense(<<~RUBY)
+        User.where(["name IN (?)", ['john', 'jane']])
+             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(name: ['john', 'jane'])` instead of manually constructing SQL.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        User.where(name: ['john', 'jane'])
+      RUBY
+    end
+
+    it 'registers an offense and corrects when using `IN` and named placeholder' do
+      expect_offense(<<~RUBY)
+        User.where(["name IN (:names)", names: ['john', 'jane']])
+             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where(name: ['john', 'jane'])` instead of manually constructing SQL.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        User.where(name: ['john', 'jane'])
+      RUBY
+    end
+
+    it 'registers an offense and corrects when using `=` and namespaced columns' do
+      expect_offense(<<~RUBY)
+        Course.where(['enrollments.student_id = ?', student.id])
+               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use `where('enrollments.student_id' => student.id)` instead of manually constructing SQL.
+      RUBY
+
+      expect_correction(<<~RUBY)
+        Course.where('enrollments.student_id' => student.id)
+      RUBY
+    end
+  end
+
+  it 'does not register an offense when using `where`' do
+    expect_no_offenses(<<~RUBY)
+      User.where(name: nil)
+    RUBY
+  end
+
+  it 'does not register an offense when not using template string' do
+    expect_no_offenses(<<~RUBY)
+      User.where(name: 'john')
+    RUBY
+  end
+
+  it 'does not register an offense when template string does contains negation' do
+    expect_no_offenses(<<~RUBY)
+      User.where('name != ?', 'john')
+    RUBY
+  end
+
+  it 'does not register an offense when template string contains `=` and additional boolean logic' do
+    expect_no_offenses(<<~RUBY)
+      User.where('name = ? AND age = ?', 'john', 19)
+    RUBY
+  end
+end