Skip to content

Commit

Permalink
Fix "last equality wins" logic in relation merge
Browse files Browse the repository at this point in the history
This is a real fix (as compared to the band-aid in b127d86), which uses
the recently-added equality methods for ARel nodes. It has the side
benefit of simplifying the merge code a bit.
  • Loading branch information
ernie committed Aug 19, 2012
1 parent f9fc26e commit bf80522
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 14 deletions.
15 changes: 5 additions & 10 deletions activerecord/lib/active_record/relation/merger.rb
Expand Up @@ -97,18 +97,13 @@ def merged_wheres
merged_wheres = relation.where_values + values[:where] merged_wheres = relation.where_values + values[:where]


unless relation.where_values.empty? unless relation.where_values.empty?
# Remove duplicate ARel attributes. Last one wins. # Remove equalities with duplicated left-hand. Last one wins.
seen = Hash.new { |h,table| h[table] = {} } seen = {}
merged_wheres = merged_wheres.reverse.reject { |w| merged_wheres = merged_wheres.reverse.reject { |w|
nuke = false nuke = false
# We might have non-attributes on the left side of equality nodes, if w.respond_to?(:operator) && w.operator == :==
# so we need to make sure they quack like an attribute. nuke = seen[w.left]
if w.respond_to?(:operator) && w.operator == :== && seen[w.left] = true
w.left.respond_to?(:relation)
name = w.left.name
table = w.left.relation.name
nuke = seen[table][name]
seen[table][name] = true
end end
nuke nuke
}.reverse }.reverse
Expand Down
11 changes: 7 additions & 4 deletions activerecord/test/cases/relations_test.rb
Expand Up @@ -675,11 +675,14 @@ def test_relation_merging_with_arel_equalities_keeps_last_equality
assert_equal [developers(:poor_jamis)], devs.to_a assert_equal [developers(:poor_jamis)], devs.to_a
end end


def test_relation_merging_with_arel_equalities_with_a_non_attribute_left_hand_ignores_non_attributes_when_discarding_equalities def test_relation_merging_with_arel_equalities_keeps_last_equality_with_non_attribute_left_hand
salary_attr = Developer.arel_table[:salary] salary_attr = Developer.arel_table[:salary]
devs = Developer.where(salary_attr.eq(80000)).merge( devs = Developer.where(
Developer.where(salary_attr.eq(9000)). Arel::Nodes::NamedFunction.new('abs', [salary_attr]).eq(80000)
where(Arel::Nodes::NamedFunction.new('abs', [salary_attr]).eq(9000)) ).merge(
Developer.where(
Arel::Nodes::NamedFunction.new('abs', [salary_attr]).eq(9000)
)
) )
assert_equal [developers(:poor_jamis)], devs.to_a assert_equal [developers(:poor_jamis)], devs.to_a
end end
Expand Down

2 comments on commit bf80522

@tjwallace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ernie why is there "last equality wins" logic anyways? If a users generates a query with WHERE column = 1 AND column = 2, why not let it return no records? This is related to my question/bug #6596.

@ernie
Copy link
Contributor Author

@ernie ernie commented on bf80522 Aug 22, 2012 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.