Merge pull request rubocop#639 from niklas-hasselmeyer/where-not-with-multiple-arguments

[Fix rubocop#565] Add cop Rails/WhereNotWithMultipleConditions

Author: koic

changelog/new_add_cop_where_not_with_multiple_conditions.md

@@ -0,0 +1 @@
+* [#565](https://github.com/rubocop/rubocop-rails/issues/565): Add cop Rails/WhereNotWithMultipleConditions. ([@niklas-hasselmeyer][])

config/default.yml

@@ -1090,6 +1090,11 @@ Rails/WhereNot:
   Enabled: 'pending'
   VersionAdded: '2.8'
 
+Rails/WhereNotWithMultipleConditions:
+  Description: 'Do not use `where.not(...)` with multiple conditions.'
+  Enabled: 'pending'
+  VersionAdded: '<<next>>'
+
 # Accept `redirect_to(...) and return` and similar cases.
 Style/AndOr:
   EnforcedStyle: conditionals

lib/rubocop/cop/rails/where_not_with_multiple_conditions.rb

@@ -0,0 +1,55 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Rails
+      # Identifies calls to `where.not` with multiple hash arguments.
+      #
+      # The behavior of `where.not` changed in Rails 6.1. Prior to the change,
+      # `.where.not(trashed: true, role: 'admin')` evaluated to
+      # `WHERE trashed != TRUE AND role != 'admin'`.
+      # From Rails 6.1 onwards, this executes the query
+      # `WHERE NOT (trashed == TRUE AND roles == 'admin')`.
+      #
+      # @example
+      #   # bad
+      #   User.where.not(trashed: true, role: 'admin')
+      #   User.where.not(trashed: true, role: ['moderator', 'admin'])
+      #   User.joins(:posts).where.not(posts: { trashed: true, title: 'Rails' })
+      #
+      #   # good
+      #   User.where.not(trashed: true)
+      #   User.where.not(role: ['moderator', 'admin'])
+      #   User.where.not(trashed: true).where.not(role: ['moderator', 'admin'])
+      #   User.where.not('trashed = ? OR role = ?', true, 'admin')
+      class WhereNotWithMultipleConditions < Base
+        MSG = 'Use a SQL statement instead of `where.not` with multiple conditions.'
+        RESTRICT_ON_SEND = %i[not].freeze
+
+        def_node_matcher :where_not_call?, <<~PATTERN
+          (send (send _ :where) :not $...)
+        PATTERN
+
+        def on_send(node)
+          where_not_call?(node) do |args|
+            next unless args[0].hash_type?
+            next unless multiple_arguments_hash? args[0]
+
+            range = node.receiver.loc.selector.with(end_pos: node.loc.expression.end_pos)
+
+            add_offense(range)
+          end
+        end
+
+        private
+
+        def multiple_arguments_hash?(hash)
+          return true if hash.pairs.size >= 2
+          return false unless hash.values[0].hash_type?
+
+          multiple_arguments_hash?(hash.values[0])
+        end
+      end
+    end
+  end
+end

lib/rubocop/cop/rails_cops.rb

@@ -127,3 +127,4 @@
 require_relative 'rails/where_exists'
 require_relative 'rails/where_missing'
 require_relative 'rails/where_not'
+require_relative 'rails/where_not_with_multiple_conditions'

spec/rubocop/cop/rails/where_not_with_multiple_conditions_spec.rb

@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+RSpec.describe RuboCop::Cop::Rails::WhereNotWithMultipleConditions, :config do
+  it 'does not register an offense for where.not with one condition' do
+    expect_no_offenses(<<~RUBY)
+      User.where.not(trashed: true)
+    RUBY
+  end
+
+  it 'registers an offense for where.not with multiple conditions' do
+    expect_offense(<<~RUBY)
+      User.where.not(trashed: true, role: 'admin')
+           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use a SQL statement instead of `where.not` with multiple conditions.
+    RUBY
+  end
+
+  it 'registers an offense for where.not with nested multiple conditions' do
+    expect_offense(<<~RUBY)
+      User.joins(:posts).where.not({ posts: { trashed: true, title: 'Rails' } })
+                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Use a SQL statement instead of `where.not` with multiple conditions.
+    RUBY
+  end
+
+  it 'does not register an offense for where with multiple conditions' do
+    expect_no_offenses(<<~RUBY)
+      User.where(trashed: false, role: 'admin')
+    RUBY
+  end
+
+  it 'does not register an offense for where.not with a SQL string' do
+    expect_no_offenses(<<~RUBY)
+      User.where.not('trashed = ? OR role = ?', true, 'admin')
+    RUBY
+  end
+
+  it 'does not register an offense for where.not with one array condition' do
+    expect_no_offenses(<<~RUBY)
+      User.where.not(role: ['moderator', 'admin'])
+    RUBY
+  end
+
+  it 'does not register an offense for chained where.not' do
+    expect_no_offenses(<<~RUBY)
+      User.where.not(trashed: true).where.not(role: 'admin')
+    RUBY
+  end
+end