Deprecate inconsistent behavior that merging conditions on the same c…

…olumn

Related to #39250 and #39236.

The purpose of the change here is to unify inconsistent behavior on the
merging.

For now, mergee side condition is replaced by merger side condition only
if both arel nodes are Equality or In clauses.

In other words, if mergee side condition is not Equality or In clauses
(e.g. `between`, `or`, `gt`, `lt`, etc), those conditions will be kept
even on the same column.

This behavior is harder to predict unless people are familiar with the
merging behavior.

Originally I suppose this behavior is just an implementation issue
rather than an intended one, since `unscope` and `rewhere`, which were
introduced later than `merge`, works more consistently.

Since most of the conditions are usually Equality and In clauses, I
don't suppose most people have encountered this merging issue, but I'd
like to deprecate the inconsistent behavior and will completely unify
that to improve a future UX.

```ruby
# Rails 6.1 (IN clause is replaced by merger side equality condition)
Author.where(id: [david.id, mary.id]).merge(Author.where(id: bob)) # => [bob]

# Rails 6.1 (both conflict conditions exists, deprecated)
Author.where(id: david.id..mary.id).merge(Author.where(id: bob)) # => []

# Rails 6.1 with rewhere to migrate to Rails 6.2's behavior
Author.where(id: david.id..mary.id).merge(Author.where(id: bob), rewhere: true) # => [bob]

# Rails 6.2 (same behavior with IN clause, mergee side condition is consistently replaced)
Author.where(id: [david.id, mary.id]).merge(Author.where(id: bob)) # => [bob]
Author.where(id: david.id..mary.id).merge(Author.where(id: bob)) # => [bob]
```

activerecord/CHANGELOG.md

@@ -1,3 +1,24 @@
+*   Merging conditions on the same column no longer maintain both conditions,
+    and will be consistently replaced by the latter condition in Rails 6.2.
+    To migrate to Rails 6.2's behavior, use `relation.merge(other, rewhere: true)`.
+
+    ```ruby
+    # Rails 6.1 (IN clause is replaced by merger side equality condition)
+    Author.where(id: [david.id, mary.id]).merge(Author.where(id: bob)) # => [bob]
+
+    # Rails 6.1 (both conflict conditions exists, deprecated)
+    Author.where(id: david.id..mary.id).merge(Author.where(id: bob)) # => []
+
+    # Rails 6.1 with rewhere to migrate to Rails 6.2's behavior
+    Author.where(id: david.id..mary.id).merge(Author.where(id: bob), rewhere: true) # => [bob]
+
+    # Rails 6.2 (same behavior with IN clause, mergee side condition is consistently replaced)
+    Author.where(id: [david.id, mary.id]).merge(Author.where(id: bob)) # => [bob]
+    Author.where(id: david.id..mary.id).merge(Author.where(id: bob)) # => [bob]
+    ```
+
+    *Ryuta Kamizono*
+
 *   Do not mark Postgresql MAC address and UUID attributes as changed when the assigned value only varies by case.
 
     *Peter Fry*

activerecord/lib/active_record/relation/where_clause.rb

@@ -86,7 +86,7 @@ def invert(as = :nand)
       end
 
       def self.empty
-        @empty ||= new([]).tap(&:referenced_columns).freeze
+        @empty ||= new([]).freeze
       end
 
       def contradiction?
@@ -113,9 +113,12 @@ def extract_attributes
         attr_reader :predicates
 
         def referenced_columns
-          @referenced_columns ||= begin
-            equality_nodes = predicates.select { |n| equality_node?(n) }
-            Set.new(equality_nodes, &:left)
+          predicates.each_with_object({}) do |node, hash|
+            attr = extract_attribute(node) || begin
+              node.left if equality_node?(node) && node.left.is_a?(Arel::Predications)
+            end
+
+            hash[attr] = node if attr
           end
         end
 
@@ -144,8 +147,27 @@ def equalities(predicates)
         end
 
         def predicates_unreferenced_by(other)
-          predicates.reject do |n|
-            equality_node?(n) && other.referenced_columns.include?(n.left)
+          referenced_columns = other.referenced_columns
+
+          predicates.reject do |node|
+            attr = extract_attribute(node) || begin
+              node.left if equality_node?(node) && node.left.is_a?(Arel::Predications)
+            end
+            next false unless attr
+
+            ref = referenced_columns[attr]
+            next false unless ref
+
+            if equality_node?(node) && equality_node?(ref) || node == ref
+              true
+            else
+              ActiveSupport::Deprecation.warn(<<-MSG.squish)
+                Merging (#{node.to_sql}) and (#{ref.to_sql}) no longer maintain
+                both conditions, and will be replaced by the latter in Rails 6.2.
+                To migrate to Rails 6.2's behavior, use `relation.merge(other, rewhere: true)`.
+              MSG
+              false
+            end
           end
         end
 

activerecord/test/cases/relation/merging_test.rb

@@ -23,18 +23,20 @@ def test_merge_in_clause
     assert_equal [mary, bob],   mary_and_bob
 
     assert_equal [mary], david_and_mary.merge(Author.where(id: mary))
-    assert_equal [bob],  david_and_mary.merge(Author.where(id: bob))
-
     assert_equal [mary], david_and_mary.merge(Author.rewhere(id: mary))
-    assert_equal [bob],  david_and_mary.merge(Author.rewhere(id: bob))
-
     assert_equal [mary], david_and_mary.merge(Author.where(id: mary), rewhere: true)
+
+    assert_equal [bob],  david_and_mary.merge(Author.where(id: bob))
+    assert_equal [bob],  david_and_mary.merge(Author.rewhere(id: bob))
     assert_equal [bob],  david_and_mary.merge(Author.where(id: bob), rewhere: true)
 
-    assert_equal [mary, bob], david_and_mary.merge(mary_and_bob)
-    assert_equal [david, mary], mary_and_bob.merge(david_and_mary)
+    assert_equal [david, bob], mary_and_bob.merge(Author.where(id: [david, bob]))
+    assert_equal [david, bob], mary_and_bob.merge(Author.where(id: [david, bob]), rewhere: true)
 
+    assert_equal [mary, bob], david_and_mary.merge(mary_and_bob)
     assert_equal [mary, bob], david_and_mary.merge(mary_and_bob, rewhere: true)
+
+    assert_equal [david, mary], mary_and_bob.merge(david_and_mary)
     assert_equal [david, mary], mary_and_bob.merge(david_and_mary, rewhere: true)
 
     david_and_bob = Author.where(id: david).or(Author.where(name: "Bob"))
@@ -52,19 +54,36 @@ def test_merge_between_clause
     assert_equal [david, mary], david_and_mary
     assert_equal [mary, bob],   mary_and_bob
 
-    assert_equal [mary], david_and_mary.merge(Author.where(id: mary))
-    assert_equal [],     david_and_mary.merge(Author.where(id: bob))
+    author_id = Regexp.escape(Author.connection.quote_table_name("authors.id"))
+    message = %r/Merging \(#{author_id} BETWEEN (\?|\W?\w?\d) AND \g<1>\) and \(#{author_id} (?:= \g<1>|IN \(\g<1>, \g<1>\))\) no longer maintain both conditions, and will be replaced by the latter in Rails 6\.2\./
 
+    assert_deprecated(message) do
+      assert_equal [mary], david_and_mary.merge(Author.where(id: mary))
+    end
     assert_equal [mary], david_and_mary.merge(Author.rewhere(id: mary))
-    assert_equal [bob],  david_and_mary.merge(Author.rewhere(id: bob))
-
     assert_equal [mary], david_and_mary.merge(Author.where(id: mary), rewhere: true)
+
+    assert_deprecated(message) do
+      assert_equal [], david_and_mary.merge(Author.where(id: bob))
+    end
+    assert_equal [bob],  david_and_mary.merge(Author.rewhere(id: bob))
     assert_equal [bob],  david_and_mary.merge(Author.where(id: bob), rewhere: true)
 
-    assert_equal [mary], david_and_mary.merge(mary_and_bob)
-    assert_equal [mary], mary_and_bob.merge(david_and_mary)
+    assert_deprecated(message) do
+      assert_equal [bob], mary_and_bob.merge(Author.where(id: [david, bob]))
+    end
+    assert_equal [david, bob], mary_and_bob.merge(Author.where(id: [david, bob]), rewhere: true)
+
+    message = %r/Merging \(#{author_id} BETWEEN (\?|\W?\w?\d) AND \g<1>\) and \(#{author_id} BETWEEN \g<1> AND \g<1>\) no longer maintain both conditions, and will be replaced by the latter in Rails 6\.2\./
 
+    assert_deprecated(message) do
+      assert_equal [mary], david_and_mary.merge(mary_and_bob)
+    end
     assert_equal [mary, bob], david_and_mary.merge(mary_and_bob, rewhere: true)
+
+    assert_deprecated(message) do
+      assert_equal [mary], mary_and_bob.merge(david_and_mary)
+    end
     assert_equal [david, mary], mary_and_bob.merge(david_and_mary, rewhere: true)
 
     david_and_bob = Author.where(id: david).or(Author.where(name: "Bob"))
@@ -82,19 +101,36 @@ def test_merge_or_clause
     assert_equal [david, mary], david_and_mary
     assert_equal [mary, bob],   mary_and_bob
 
-    assert_equal [mary], david_and_mary.merge(Author.where(id: mary))
-    assert_equal [],     david_and_mary.merge(Author.where(id: bob))
+    author_id = Regexp.escape(Author.connection.quote_table_name("authors.id"))
+    message = %r/Merging \(\(#{author_id} = (\?|\W?\w?\d) OR #{author_id} = \g<1>\)\) and \(#{author_id} (?:= \g<1>|IN \(\g<1>, \g<1>\))\) no longer maintain both conditions, and will be replaced by the latter in Rails 6\.2\./
 
+    assert_deprecated(message) do
+      assert_equal [mary], david_and_mary.merge(Author.where(id: mary))
+    end
     assert_equal [mary], david_and_mary.merge(Author.rewhere(id: mary))
-    assert_equal [bob],  david_and_mary.merge(Author.rewhere(id: bob))
-
     assert_equal [mary], david_and_mary.merge(Author.where(id: mary), rewhere: true)
+
+    assert_deprecated(message) do
+      assert_equal [], david_and_mary.merge(Author.where(id: bob))
+    end
+    assert_equal [bob],  david_and_mary.merge(Author.rewhere(id: bob))
     assert_equal [bob],  david_and_mary.merge(Author.where(id: bob), rewhere: true)
 
-    assert_equal [mary], david_and_mary.merge(mary_and_bob)
-    assert_equal [mary], mary_and_bob.merge(david_and_mary)
+    assert_deprecated(message) do
+      assert_equal [bob], mary_and_bob.merge(Author.where(id: [david, bob]))
+    end
+    assert_equal [david, bob], mary_and_bob.merge(Author.where(id: [david, bob]), rewhere: true)
+
+    message = %r/Merging \(\(#{author_id} = (\?|\W?\w?\d) OR #{author_id} = \g<1>\)\) and \(\(#{author_id} = \g<1> OR #{author_id} = \g<1>\)\) no longer maintain both conditions, and will be replaced by the latter in Rails 6\.2\./
 
+    assert_deprecated(message) do
+      assert_equal [mary], david_and_mary.merge(mary_and_bob)
+    end
     assert_equal [mary, bob], david_and_mary.merge(mary_and_bob, rewhere: true)
+
+    assert_deprecated(message) do
+      assert_equal [mary], mary_and_bob.merge(david_and_mary)
+    end
     assert_equal [david, mary], mary_and_bob.merge(david_and_mary, rewhere: true)
 
     david_and_bob = Author.where(id: david).or(Author.where(name: "Bob"))
@@ -109,8 +145,16 @@ def test_merge_not_in_clause
     non_mary_and_bob = Author.where.not(id: [mary, bob])
 
     assert_equal [david], non_mary_and_bob
-    assert_equal [david], Author.where(id: david).merge(non_mary_and_bob)
-    assert_equal [], Author.where(id: mary).merge(non_mary_and_bob)
+
+    assert_deprecated do
+      assert_equal [david], Author.where(id: david).merge(non_mary_and_bob)
+    end
+    assert_equal [david], Author.where(id: david).merge(non_mary_and_bob, rewhere: true)
+
+    assert_deprecated do
+      assert_equal [], Author.where(id: mary).merge(non_mary_and_bob)
+    end
+    assert_equal [david], Author.where(id: mary).merge(non_mary_and_bob, rewhere: true)
   end
 
   def test_merge_doesnt_duplicate_same_clauses