Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Merge pull request #10981 from neerajdotname/10958

remove bind values for where clauses that were removed
  • Loading branch information...
commit f81dd3640317b1ef9adedd79b4917499c784e94d 2 parents 42a3a37 + b074201
@rafaelfranca rafaelfranca authored
View
4 activerecord/CHANGELOG.md
@@ -86,6 +86,10 @@
*Kyle Stevens*
+* Remove not needed bind variables. Port of commit #5082345. Fixes #10958.
+
+ *Neeraj Singh*
+
* Confirm a record has not already been destroyed before decrementing counter cache.
*Ben Tucker*
View
53 activerecord/lib/active_record/relation/merger.rb
@@ -101,8 +101,15 @@ def merge_joins
end
def merge_multi_values
- relation.where_values = merged_wheres
- relation.bind_values = merged_binds
+ lhs_wheres = relation.where_values
+ rhs_wheres = values[:where] || []
+ lhs_binds = relation.bind_values
+ rhs_binds = values[:bind] || []
+
+ removed, kept = partition_overwrites(lhs_wheres, rhs_wheres)
+
+ relation.where_values = kept + rhs_wheres
+ relation.bind_values = filter_binds(lhs_binds, removed) + rhs_binds
if values[:reordering]
# override any order specified in the original relation
@@ -125,37 +132,29 @@ def merge_single_values
end
end
- def merged_binds
- if values[:bind]
- (relation.bind_values + values[:bind]).uniq(&:first)
- else
- relation.bind_values
- end
+ def filter_binds(lhs_binds, removed_wheres)
+ set = Set.new removed_wheres.map { |x| x.left.name }
+ lhs_binds.dup.delete_if { |col,_| set.include? col.name }
end
- def merged_wheres
- values[:where] ||= []
-
- if values[:where].empty? || relation.where_values.empty?
- relation.where_values + values[:where]
- else
- # Remove equalities from the existing relation with a LHS which is
- # present in the relation being merged in.
+ # 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
- seen = Set.new
- values[:where].each { |w|
- if w.respond_to?(:operator) && w.operator == :==
- seen << w.left
- end
- }
+ nodes = rhs_wheres.find_all do |w|
+ w.respond_to?(:operator) && w.operator == :==
+ end
+ seen = Set.new(nodes) { |node| node.left }
- relation.where_values.reject { |w|
- w.respond_to?(:operator) &&
- w.operator == :== &&
- seen.include?(w.left)
- } + values[:where]
+ lhs_wheres.partition do |w|
+ w.respond_to?(:operator) && w.operator == :== && seen.include?(w.left)
end
end
+
end
end
end
View
10 activerecord/test/cases/relations_test.rb
@@ -1546,4 +1546,14 @@ def __omg__
assert merged.to_sql.include?("wtf")
assert merged.to_sql.include?("bbq")
end
+
+ def test_merging_removes_lhs_bind_parameters
+ left = Post.where(id: Arel::Nodes::BindParam.new('?'))
+ column = Post.columns_hash['id']
+ left.bind_values += [[column, 20]]
+ right = Post.where(id: 10)
+
+ merged = left.merge(right)
+ assert_equal [], merged.bind_values
+ end
end
Please sign in to comment.
Something went wrong with that request. Please try again.