Merge pull request #39328 from kamipo/deprecate_inconsistent_merging_behavior

Deprecate inconsistent behavior that merging conditions on the same column

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 @@ module ActiveRecord
       end
 
       def self.empty
-        @empty ||= new([]).tap(&:referenced_columns).freeze
+        @empty ||= new([]).freeze
       end
 
       def contradiction?
@@ -113,9 +113,12 @@ module ActiveRecord
         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 @@ module ActiveRecord
         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 @@ class RelationMergingTest < ActiveRecord::TestCase
     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 @@ class RelationMergingTest < ActiveRecord::TestCase
     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 @@ class RelationMergingTest < ActiveRecord::TestCase
     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 @@ class RelationMergingTest < ActiveRecord::TestCase
     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