Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve user supplied JOIN order #15461

Closed
wants to merge 2 commits into from

Conversation

thedarkone
Copy link
Contributor

JOIN clauses order is important, previous implementation has always put string or arel joins at the end (after auto-generated association joins).

Fixes #12953, #15488, #16635.

/cc @tenderlove

@tenderlove
Copy link
Member

Can we do this without adding a new possibly initialized instance variable? I don't want to keep new state in the JoinDependency object.

@thedarkone
Copy link
Contributor Author

Can we do this without adding a new possibly initialized instance variable? I don't want to keep new state in the JoinDependency object.

@tenderlove thanks, done, re-pushed. The new version now also contains fix for #15488 (AR::Relation::Merger#merge_joins had the same problem as AR::Relation::QueryMethods#build_joins). Please review ❤️.

@SebastianZaha
Copy link

Anything that can be done to help get this merged? I can confirm this fixes #15488

thedarkone and others added 2 commits September 12, 2014 18:18
JOIN clauses order is important, previous implementation always
put string or arel joins at then end (after auto-generated
association joins).

Fixes rails#12953, rails#15488, rails#16635.
@thedarkone
Copy link
Contributor Author

Did some code tweaks and added Takamichi Yoshikawa's test case.

@SebastianZaha @adorechic: sorry, that this taking so long to get merged in.. 😣

join_dependency_param = {associations_name => subtree}
end
end
join_dependency_param
Copy link
Member

Choose a reason for hiding this comment

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

Having a hard time following these assignments. Can be nil, a hash, or a string… tough to trace its behavior among its collaborating methods.

thedarkone referenced this pull request Jan 28, 2015
Attempting to grok this code by refactoring it as I go through it.
@mrpinsky
Copy link
Contributor

@thedarkone Is this PR still relevant, or can it be closed?

@thedarkone
Copy link
Contributor Author

The issue of maintaining user supplied JOIN order is still relevant, but the PR is quite stale (AFAIK the code that this targets got changed considerably).

I can't work on this right now, so if needs be the PR can be closed by the maintainers.

@rafaelfranca
Copy link
Member

It is indeed outdated. I'll close the PR and link to the open issues in case someone wants to pick it up. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strange ActiveRecord behaviour when using merge and joins
6 participants