Merge pull request #274 from fatkodima/where-not-cop

Add new `Rails/WhereNot` cop

CHANGELOG.md

@@ -5,6 +5,7 @@
 ### New features
 
 * [#52](https://github.com/rubocop-hq/rubocop-rails/issues/52): Add new `Rails/AfterCommitOverride` cop. ([@fatkodima][])
+* [#274](https://github.com/rubocop-hq/rubocop-rails/pull/274): Add new `Rails/WhereNot` cop. ([@fatkodima][])
 
 ## 2.7.1 (2020-07-26)
 

config/default.yml

@@ -687,6 +687,12 @@ Rails/WhereExists:
  Enabled: 'pending'
  VersionAdded: '2.7'
 
+Rails/WhereNot:
+ Description: 'Use `where.not(...)` instead of manually constructing negated SQL in `where`.'
+ StyleGuide: 'https://rails.rubystyle.guide/#where-not'
+ Enabled: 'pending'
+ VersionAdded: '2.8'
+
 # Accept `redirect_to(...) and return` and similar cases.
 Style/AndOr:
  EnforcedStyle: conditionals

docs/modules/ROOT/pages/cops.adoc

@@ -80,5 +80,6 @@
 * xref:cops_rails.adoc#railsunknownenv[Rails/UnknownEnv]
 * xref:cops_rails.adoc#railsvalidation[Rails/Validation]
 * xref:cops_rails.adoc#railswhereexists[Rails/WhereExists]
+* xref:cops_rails.adoc#railswherenot[Rails/WhereNot]
 
 // END_COP_LIST

docs/modules/ROOT/pages/cops_rails.adoc

@@ -4134,3 +4134,39 @@ User.exists?(name: 'john')
 User.where('length(name) > 10').exists?
 user.posts.exists?(published: true)
 ----
+
+== Rails/WhereNot
+
+|===
+| Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged
+
+| Pending
+| Yes
+| Yes
+| 2.8
+| -
+|===
+
+This cop identifies places where manually constructed SQL
+in `where` can be replaced with `where.not(...)`.
+
+=== Examples
+
+[source,ruby]
+----
+# bad
+User.where('name != ?', 'Gabe')
+User.where('name != :name', name: 'Gabe')
+User.where('name IS NOT NULL')
+User.where('name NOT IN (?)', ['john', 'jane'])
+User.where('name NOT IN (:names)', names: ['john', 'jane'])
+
+# good
+User.where.not(name: 'Gabe')
+User.where.not(name: nil)
+User.where.not(name: ['john', 'jane'])
+----
+
+=== References
+
+* https://rails.rubystyle.guide/#where-not

lib/rubocop/cop/rails/where_not.rb

@@ -0,0 +1,106 @@
+# 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.not(...)`.
+ #
+ # @example
+ # # bad
+ # User.where('name != ?', 'Gabe')
+ # User.where('name != :name', name: 'Gabe')
+ # User.where('name IS NOT NULL')
+ # User.where('name NOT IN (?)', ['john', 'jane'])
+ # User.where('name NOT IN (:names)', names: ['john', 'jane'])
+ #
+ # # good
+ # User.where.not(name: 'Gabe')
+ # User.where.not(name: nil)
+ # User.where.not(name: ['john', 'jane'])
+ #
+ class WhereNot < Cop
+ include RangeHelp
+
+ MSG = 'Use `%<good_method>s` instead of manually constructing negated SQL in `where`.'
+
+ 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
+
+ NOT_EQ_ANONYMOUS_RE = /\A([\w.]+)\s+!=\s+\?\z/.freeze # column != ?
+ NOT_IN_ANONYMOUS_RE = /\A([\w.]+)\s+NOT\s+IN\s+\(\?\)\z/i.freeze # column NOT IN (?)
+ NOT_EQ_NAMED_RE = /\A([\w.]+)\s+!=\s+:(\w+)\z/.freeze # column != :column
+ NOT_IN_NAMED_RE = /\A([\w.]+)\s+NOT\s+IN\s+\(:(\w+)\)\z/i.freeze # column NOT IN (:column)
+ IS_NOT_NULL_RE = /\A([\w.]+)\s+IS\s+NOT\s+NULL\z/i.freeze # column IS NOT 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 NOT_EQ_ANONYMOUS_RE, NOT_IN_ANONYMOUS_RE
+ value_node.source
+ when NOT_EQ_NAMED_RE, NOT_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_NOT_NULL_RE
+ 'nil'
+ else
+ return
+ end
+
+ [Regexp.last_match(1), value]
+ end
+
+ def build_good_method(column, value)
+ if column.include?('.')
+ "where.not('#{column}' => #{value})"
+ else
+ "where.not(#{column}: #{value})"
+ end
+ end
+ end
+ end
+ end
+end

lib/rubocop/cop/rails_cops.rb

@@ -82,3 +82,4 @@
 require_relative 'rails/unknown_env'
 require_relative 'rails/validation'
 require_relative 'rails/where_exists'
+require_relative 'rails/where_not'

spec/rubocop/cop/rails/where_not_spec.rb

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