Merge pull request #36029 from kamipo/deprecate_where_not

Deprecate `where.not` working as NOR and will be changed to NAND in Rails 6.1

5 files changed, 118 insertions, 15 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 485547f036..adc7e754a4 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,44 @@
+*   Deprecate `where.not` working as NOR and will be changed to NAND in Rails 6.1.
+
+    ```ruby
+    all = [treasures(:diamond), treasures(:sapphire), cars(:honda), treasures(:sapphire)]
+    assert_equal all, PriceEstimate.all.map(&:estimate_of)
+    ```
+
+    In Rails 6.0:
+
+    ```ruby
+    sapphire = treasures(:sapphire)
+
+    nor = all.reject { |e|
+      e.estimate_of_type == sapphire.class.polymorphic_name
+    }.reject { |e|
+      e.estimate_of_id == sapphire.id
+    }
+    assert_equal [cars(:honda)], nor
+
+    without_sapphire = PriceEstimate.where.not(
+      estimate_of_type: sapphire.class.polymorphic_name, estimate_of_id: sapphire.id
+    )
+    assert_equal nor, without_sapphire.map(&:estimate_of)
+    ```
+
+    In Rails 6.1:
+
+    ```ruby
+    sapphire = treasures(:sapphire)
+
+    nand = all - [sapphire]
+    assert_equal [treasures(:diamond), cars(:honda)], nand
+
+    without_sapphire = PriceEstimate.where.not(
+      estimate_of_type: sapphire.class.polymorphic_name, estimate_of_id: sapphire.id
+    )
+    assert_equal nand, without_sapphire.map(&:estimate_of)
+    ```
+
+    *Ryuta Kamizono*
+
 *   Fix dirty tracking after rollback.
 
     Fixes #15018, #30167, #33868.
diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb
index 5d3cea6741..f30428d0a5 100644
--- a/activerecord/lib/active_record/relation/query_methods.rb
+++ b/activerecord/lib/active_record/relation/query_methods.rb
@@ -41,18 +41,31 @@ module ActiveRecord
       #
       #    User.where.not(name: %w(Ko1 Nobu))
       #    # SELECT * FROM users WHERE name NOT IN ('Ko1', 'Nobu')
-      #
-      #    User.where.not(name: "Jon", role: "admin")
-      #    # SELECT * FROM users WHERE name != 'Jon' AND role != 'admin'
       def not(opts, *rest)
         opts = sanitize_forbidden_attributes(opts)
 
         where_clause = @scope.send(:where_clause_factory).build(opts, rest)
 
         @scope.references!(PredicateBuilder.references(opts)) if Hash === opts
-        @scope.where_clause += where_clause.invert
+
+        if not_behaves_as_nor?(opts)
+          ActiveSupport::Deprecation.warn(<<~MSG.squish)
+            NOT conditions will no longer behave as NOR in Rails 6.1.
+            To continue using NOR conditions, NOT each conditions manually
+            (`#{ opts.keys.map { |key| ".where.not(#{key.inspect} => ...)" }.join }`).
+          MSG
+          @scope.where_clause += where_clause.invert(:nor)
+        else
+          @scope.where_clause += where_clause.invert
+        end
+
         @scope
       end
+
+      private
+        def not_behaves_as_nor?(opts)
+          opts.is_a?(Hash) && opts.size > 1
+        end
     end
 
     FROZEN_EMPTY_ARRAY = [].freeze
diff --git a/activerecord/lib/active_record/relation/where_clause.rb b/activerecord/lib/active_record/relation/where_clause.rb
index 47728aac30..b91b135867 100644
--- a/activerecord/lib/active_record/relation/where_clause.rb
+++ b/activerecord/lib/active_record/relation/where_clause.rb
@@ -70,7 +70,15 @@ module ActiveRecord
           predicates == other.predicates
       end
 
-      def invert
+      def invert(as = :nand)
+        if predicates.size == 1
+          inverted_predicates = [ invert_predicate(predicates.first) ]
+        elsif as == :nor
+          inverted_predicates = predicates.map { |node| invert_predicate(node) }
+        else
+          inverted_predicates = [ Arel::Nodes::Not.new(ast) ]
+        end
+
         WhereClause.new(inverted_predicates)
       end
 
@@ -115,10 +123,6 @@ module ActiveRecord
           node.respond_to?(:operator) && node.operator == :==
         end
 
-        def inverted_predicates
-          predicates.map { |node| invert_predicate(node) }
-        end
-
         def invert_predicate(node)
           case node
           when NilClass
diff --git a/activerecord/test/cases/relation/where_clause_test.rb b/activerecord/test/cases/relation/where_clause_test.rb
index 0b06cec40b..b26a1a1d80 100644
--- a/activerecord/test/cases/relation/where_clause_test.rb
+++ b/activerecord/test/cases/relation/where_clause_test.rb
@@ -106,7 +106,7 @@ class ActiveRecord::Relation
         Arel::Nodes::Not.new(random_object)
       ])
 
-      assert_equal expected, original.invert
+      assert_equal expected, original.invert(:nor)
     end
 
     test "except removes binary predicates referencing a given column" do
diff --git a/activerecord/test/cases/relation/where_test.rb b/activerecord/test/cases/relation/where_test.rb
index b045184d7d..2ecccfe1f3 100644
--- a/activerecord/test/cases/relation/where_test.rb
+++ b/activerecord/test/cases/relation/where_test.rb
@@ -115,13 +115,58 @@ module ActiveRecord
       assert_equal expected.to_sql, actual.to_sql
     end
 
-    def test_polymorphic_shallow_where_not
-      treasure = treasures(:sapphire)
+    def test_where_not_polymorphic_association
+      sapphire = treasures(:sapphire)
 
-      expected = [price_estimates(:diamond), price_estimates(:honda)]
-      actual   = PriceEstimate.where.not(estimate_of: treasure)
+      all = [treasures(:diamond), sapphire, cars(:honda), sapphire]
+      assert_equal all, PriceEstimate.all.sort_by(&:id).map(&:estimate_of)
 
-      assert_equal expected.sort_by(&:id), actual.sort_by(&:id)
+      actual = PriceEstimate.where.not(estimate_of: sapphire)
+      only = PriceEstimate.where(estimate_of: sapphire)
+
+      expected = all - [sapphire]
+      assert_equal expected, actual.sort_by(&:id).map(&:estimate_of)
+      assert_equal all - expected, only.sort_by(&:id).map(&:estimate_of)
+    end
+
+    def test_where_not_polymorphic_id_and_type_as_nand
+      sapphire = treasures(:sapphire)
+
+      all = [treasures(:diamond), sapphire, cars(:honda), sapphire]
+      assert_equal all, PriceEstimate.all.sort_by(&:id).map(&:estimate_of)
+
+      actual = PriceEstimate.where.yield_self do |where_chain|
+        where_chain.stub(:not_behaves_as_nor?, false) do
+          where_chain.not(estimate_of_type: sapphire.class.polymorphic_name, estimate_of_id: sapphire.id)
+        end
+      end
+      only = PriceEstimate.where(estimate_of_type: sapphire.class.polymorphic_name, estimate_of_id: sapphire.id)
+
+      expected = all - [sapphire]
+      assert_equal expected, actual.sort_by(&:id).map(&:estimate_of)
+      assert_equal all - expected, only.sort_by(&:id).map(&:estimate_of)
+    end
+
+    def test_where_not_polymorphic_id_and_type_as_nor_is_deprecated
+      sapphire = treasures(:sapphire)
+
+      all = [treasures(:diamond), sapphire, cars(:honda), sapphire]
+      assert_equal all, PriceEstimate.all.sort_by(&:id).map(&:estimate_of)
+
+      message = <<~MSG.squish
+        NOT conditions will no longer behave as NOR in Rails 6.1.
+        To continue using NOR conditions, NOT each conditions manually
+        (`.where.not(:estimate_of_type => ...).where.not(:estimate_of_id => ...)`).
+      MSG
+      actual = assert_deprecated(message) do
+        PriceEstimate.where.not(estimate_of_type: sapphire.class.polymorphic_name, estimate_of_id: sapphire.id)
+      end
+      only = PriceEstimate.where(estimate_of_type: sapphire.class.polymorphic_name, estimate_of_id: sapphire.id)
+
+      expected = all - [sapphire]
+      # NOT (estimate_of_type = 'Treasure' OR estimate_of_id = sapphire.id) matches only `cars(:honda)` unfortunately.
+      assert_not_equal expected, actual.sort_by(&:id).map(&:estimate_of)
+      assert_equal all - expected, only.sort_by(&:id).map(&:estimate_of)
     end
 
     def test_polymorphic_nested_array_where