From def2879d7d187df945d77c1028d4cef588cbc8a0 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Sun, 25 Jan 2015 15:27:43 -0700 Subject: [PATCH] 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` --- .../lib/active_record/relation/merger.rb | 41 +----------------- .../active_record/relation/where_clause.rb | 36 +++++++++++++++- .../test/cases/relation/where_clause_test.rb | 42 +++++++++++++++++++ 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 @@ def initialize(relation, other) 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 @@ def merge_joins 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 @@ def merge_single_values 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 @@ def +(other) ) 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 @@ def ==(other) 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 WhereClauseTest < ActiveRecord::TestCase 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 @@ def table def bind_param Arel::Nodes::BindParam.new end + + def column(name) + ActiveRecord::ConnectionAdapters::Column.new(name, nil, nil) + end end end -- GitLab