Move where merging logic over to `WhereClause`

This object being a black box, it knows the details of how to merge
itself with another where clause. This removes all references to where
values or bind values in `Relation::Merger`

3 files changed, 79 insertions, 40 deletions

diff --git a/activerecord/lib/active_record/relation/merger.rb b/activerecord/lib/active_record/relation/merger.rb
index 4ee37be6be..2f3ca539a7 100644
--- a/activerecord/lib/active_record/relation/merger.rb
+++ b/activerecord/lib/active_record/relation/merger.rb
@@ -50,7 +50,7 @@ module ActiveRecord
       end
 
       NORMAL_VALUES = Relation::VALUE_METHODS -
-                      [:joins, :where, :order, :bind, :reverse_order, :lock, :create_with, :reordering, :from] # :nodoc:
+                      [:joins, :where, :order, :reverse_order, :lock, :create_with, :reordering, :from] # :nodoc:
 
       def normal_values
         NORMAL_VALUES
@@ -106,19 +106,7 @@ module ActiveRecord
       end
 
       def merge_multi_values
-        lhs_wheres = relation.where_values
-        rhs_wheres = other.where_values
-
-        lhs_binds  = relation.bind_values
-        rhs_binds  = other.bind_values
-
-        removed, kept = partition_overwrites(lhs_wheres, rhs_wheres)
-
-        where_values = kept + rhs_wheres
-        bind_values  = filter_binds(lhs_binds, removed) + rhs_binds
-
-        relation.where_values = where_values
-        relation.bind_values  = bind_values
+        relation.where_clause = relation.where_clause.merge(other.where_clause)
 
         if other.reordering_value
           # override any order specified in the original relation
@@ -139,31 +127,6 @@ module ActiveRecord
           relation.create_with_value = (relation.create_with_value || {}).merge(other.create_with_value)
         end
       end
-
-      def filter_binds(lhs_binds, removed_wheres)
-        return lhs_binds if removed_wheres.empty?
-
-        set = Set.new removed_wheres.map { |x| x.left.name.to_s }
-        lhs_binds.dup.delete_if { |col,_| set.include? col.name }
-      end
-
-      # Remove equalities from the existing relation with a LHS which is
-      # present in the relation being merged in.
-      # returns [things_to_remove, things_to_keep]
-      def partition_overwrites(lhs_wheres, rhs_wheres)
-        if lhs_wheres.empty? || rhs_wheres.empty?
-          return [[], lhs_wheres]
-        end
-
-        nodes = rhs_wheres.find_all do |w|
-          w.respond_to?(:operator) && w.operator == :==
-        end
-        seen = Set.new(nodes) { |node| node.left }
-
-        lhs_wheres.partition do |w|
-          w.respond_to?(:operator) && w.operator == :== && seen.include?(w.left)
-        end
-      end
     end
   end
 end
diff --git a/activerecord/lib/active_record/relation/where_clause.rb b/activerecord/lib/active_record/relation/where_clause.rb
index b39fc1fed1..cd6da052a9 100644
--- a/activerecord/lib/active_record/relation/where_clause.rb
+++ b/activerecord/lib/active_record/relation/where_clause.rb
@@ -1,6 +1,6 @@
 module ActiveRecord
   class Relation
-    class WhereClause
+    class WhereClause # :nodoc:
       attr_reader :parts, :binds
 
       def initialize(parts, binds)
@@ -15,6 +15,13 @@ module ActiveRecord
         )
       end
 
+      def merge(other)
+        WhereClause.new(
+          parts_unreferenced_by(other) + other.parts,
+          non_conflicting_binds(other) + other.binds,
+        )
+      end
+
       def ==(other)
         other.is_a?(WhereClause) &&
           parts == other.parts &&
@@ -24,6 +31,33 @@ module ActiveRecord
       def self.empty
         new([], [])
       end
+
+      protected
+
+      def referenced_columns
+        @referenced_columns ||= begin
+          equality_nodes = parts.select { |n| equality_node?(n) }
+          Set.new(equality_nodes, &:left)
+        end
+      end
+
+      private
+
+      def parts_unreferenced_by(other)
+        parts.reject do |n|
+          equality_node?(n) && other.referenced_columns.include?(n.left)
+        end
+      end
+
+      def equality_node?(node)
+        node.respond_to?(:operator) && node.operator == :==
+      end
+
+      def non_conflicting_binds(other)
+        conflicts = referenced_columns & other.referenced_columns
+        conflicts.map! { |node| node.name.to_s }
+        binds.reject { |col, _| conflicts.include?(col.name) }
+      end
     end
   end
 end
diff --git a/activerecord/test/cases/relation/where_clause_test.rb b/activerecord/test/cases/relation/where_clause_test.rb
index 66e2f3ab48..ec334240cc 100644
--- a/activerecord/test/cases/relation/where_clause_test.rb
+++ b/activerecord/test/cases/relation/where_clause_test.rb
@@ -28,6 +28,44 @@ class ActiveRecord::Relation
       assert_equal clause, clause + WhereClause.empty
     end
 
+    test "merge combines two where clauses" do
+      a = WhereClause.new([table["id"].eq(1)], [])
+      b = WhereClause.new([table["name"].eq("Sean")], [])
+      expected = WhereClause.new([table["id"].eq(1), table["name"].eq("Sean")], [])
+
+      assert_equal expected, a.merge(b)
+    end
+
+    test "merge keeps the right side, when two equality clauses reference the same column" do
+      a = WhereClause.new([table["id"].eq(1), table["name"].eq("Sean")], [])
+      b = WhereClause.new([table["name"].eq("Jim")], [])
+      expected = WhereClause.new([table["id"].eq(1), table["name"].eq("Jim")], [])
+
+      assert_equal expected, a.merge(b)
+    end
+
+    test "merge removes bind parameters matching overlapping equality clauses" do
+      a = WhereClause.new(
+        [table["id"].eq(bind_param), table["name"].eq(bind_param)],
+        [[column("id"), 1], [column("name"), "Sean"]],
+      )
+      b = WhereClause.new(
+        [table["name"].eq(bind_param)],
+        [[column("name"), "Jim"]]
+      )
+      expected = WhereClause.new(
+        [table["id"].eq(bind_param), table["name"].eq(bind_param)],
+        [[column("id"), 1], [column("name"), "Jim"]],
+      )
+
+      assert_equal expected, a.merge(b)
+    end
+
+    test "merge allows for columns with the same name from different tables" do
+      skip "This is not possible as of 4.2, and the binds do not yet contain sufficient information for this to happen"
+      # We might be able to change the implementation to remove conflicts by index, rather than column name
+    end
+
     private
 
     def table
@@ -37,5 +75,9 @@ class ActiveRecord::Relation
     def bind_param
       Arel::Nodes::BindParam.new
     end
+
+    def column(name)
+      ActiveRecord::ConnectionAdapters::Column.new(name, nil, nil)
+    end
   end
 end