Skip to content
This repository has been archived by the owner on Apr 17, 2018. It is now read-only.

Commit

Permalink
Ensure the correct ordering of links for more complex joins
Browse files Browse the repository at this point in the history
Consider a query like the following:

Person.all(Person.memberships.group.type => 'cool', Person.memberships.valid_from.gte => Date.today)

This can result in one of the following to link lists being set up (depending on non-deterministic
hash ordering).

[:memberships, :group, :memberships]
[:group, :memberships, :memberships]

Query#normalize_links walks through this list and creates the following structure:

[:memberships, :group] or [:group, :memberships]

If the first situation happens and the subsequent join statement is created, it will
create an invalid join statement. DataObjectsAdapter#join_statement walks through
the normalized list in revere order, so if that adds group to the query before
memberships in a join, the query will fail.

The fix is very simple. Because of how the initial list is constructed, a correctly
normalized list will be created by walking the initial 3 element list given above in the
reverse order. That will always result in the [:group, :memberships] links that
will work correctly if joined in reverse order
  • Loading branch information
dbussink committed Sep 27, 2009
1 parent 86406fe commit 5961aa9
Showing 1 changed file with 2 additions and 1 deletion.
3 changes: 2 additions & 1 deletion lib/dm-core/query.rb
Expand Up @@ -925,7 +925,7 @@ def normalize_links

@links.clear

while link = links.shift
while link = links.pop
relationship = case link
when Symbol, String then @relationships[link]
when Associations::Relationship then link
Expand Down Expand Up @@ -961,6 +961,7 @@ def normalize_links
@links << relationship
end
end
@links.reverse!
end

# Append conditions to this Query
Expand Down

1 comment on commit 5961aa9

@dkubb
Copy link
Member

@dkubb dkubb commented on 5961aa9 Sep 27, 2009

Choose a reason for hiding this comment

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

@dbussink: Great change. It totally makes sense to process @links like a stack. Can you add a spec that would fail with the old behavior? This is a pretty tricky issue, and I would hate to see a regression happen if this code was ever refactored into something different.

Please sign in to comment.