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

Strange ActiveRecord behaviour when using merge and joins #12953

Closed
iwiznia opened this issue Nov 19, 2013 · 6 comments · Fixed by akhiln/rails#1 or #36805
Closed

Strange ActiveRecord behaviour when using merge and joins #12953

iwiznia opened this issue Nov 19, 2013 · 6 comments · Fixed by akhiln/rails#1 or #36805

Comments

@iwiznia
Copy link
Contributor

iwiznia commented Nov 19, 2013

There's a strange behaviour on AR in which using merge on a scope varies the order of the joins depending on how you define the join (string vs symbol).
Here's the testcase:
https://gist.github.com/iwiznia/7547713

I know that in the test there's a table that doesn't exist, it's there only to check the order of the join clauses.
There are 3 tests which should yield the same result, but when you combine joins as strings and joins as symbols or classes, it mangles the order of the joins.
Maybe it has to do with the fact that it's using the join_sources of the Arel::SelectManager class, but I thought that was a valid use case, and doing the same thing, but calling to_sql first solves the error.

What do you think? Is this a bug? I think it is since the three join statements should produce the same sql.

@take
Copy link
Contributor

take commented Dec 3, 2013

Hi, I looked into the test code and,

but when you combine joins as strings and joins as symbols or classes, it mangles the order of the joins.

since the following test case passes(which you wrote), isn't the combination which fails is only joins as string, and joins as classes ?

  def test_joins_on_correct_order_works
     assert_equal Post.joins(:comments).merge(Comment.fails).to_sql, 'SELECT "posts".* FROM "posts" INNER JOIN "comments" ON "comments"."post_id" = "posts"."id" INNER JOIN "other_table"'
  end

@iwiznia
Copy link
Contributor Author

iwiznia commented Dec 3, 2013

I don't understand your question...There's only one failing test case, the others are there to help pinpoint the problem, since they should be doing outputing the same as the failing test

@iwiznia iwiznia added the stale label Apr 23, 2014
@rafaelfranca
Copy link
Member

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@rails-bot
Copy link

This issue has been automatically closed because of inactivity.

If you can still reproduce this error on the 4-1-stable, 4-0-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

@rafaelfranca rafaelfranca reopened this Jun 2, 2014
thedarkone added a commit to thedarkone/rails that referenced this issue Jun 6, 2014
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.
thedarkone added a commit to thedarkone/rails that referenced this issue Sep 12, 2014
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.
@magikid
Copy link

magikid commented Mar 13, 2016

I'm not sure what versions @iwiznia was using but when I run this using ruby 2.3 and rails 4.2.6, all of the tests pass.

@tibbon
Copy link

tibbon commented Jul 23, 2018

We're running into this problem ourselves. Curious to see if there's any new thoughts on it.

@kamipo kamipo added pinned and removed attached PR labels Jul 24, 2018
kamipo added a commit to kamipo/rails that referenced this issue Jul 29, 2019
Currently, string joins are always applied as last joins part, and Arel
join nodes are always applied as leading joins part (since rails#36304), it
makes people struggled to preserve user supplied joins order.

To mitigate this problem, preserve the order of string joins and Arel
join nodes either before or after of association joins.

Fixes rails#36761.
Fixes rails#34328.
Fixes rails#24281.
Fixes rails#12953.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants