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

Order of "manual" joins messed up when using .merge #24281

Closed
tbreier opened this issue Mar 22, 2016 · 9 comments · Fixed by #36805
Closed

Order of "manual" joins messed up when using .merge #24281

tbreier opened this issue Mar 22, 2016 · 9 comments · Fixed by #36805

Comments

@tbreier
Copy link

tbreier commented Mar 22, 2016

When using a JOIN like .joins('INNER JOIN posts ON users.id = posts.user_id') instead of .joins(:posts), and then using .merge with a relation with another JOIN defined on (in this case) posts, the JOIN with that other table gets put in the front, which is not something that MySQL is happy about.

This description is probably a bit confusing, but I hope the gist makes clear what I mean (see below).

Steps to reproduce

See https://gist.github.com/tbreier/e388c3f4727f985126a1

Expected behavior

.merge works when the scope to be merged with a) is joined manually (not via symbol describing association) and b) contains a join with another table

Actual behavior

The gist above crashes.

# Running:

SELECT `users`.* FROM `users` INNER JOIN `posts` ON `posts`.`user_id` = `users`.`id` LEFT OUTER JOIN `comments` ON `comments`.`post_id` = `posts`.`id`
SELECT `users`.* FROM `users` LEFT OUTER JOIN `comments` ON `comments`.`post_id` = `posts`.`id` INNER JOIN posts ON users.id = posts.user_id
D, [2016-03-22T14:38:34.101618 #5326] DEBUG -- :   User Load (0.6ms)  SELECT `users`.* FROM `users` INNER JOIN `posts` ON `posts`.`user_id` = `users`.`id` LEFT OUTER JOIN `comments` ON `comments`.`post_id` = `posts`.`id`
D, [2016-03-22T14:38:34.108212 #5326] DEBUG -- :   User Load (0.6ms)  SELECT `users`.* FROM `users` LEFT OUTER JOIN `comments` ON `comments`.`post_id` = `posts`.`id` INNER JOIN posts ON users.id = posts.user_id
E

Finished in 0.025738s, 38.8537 runs/s, 0.0000 assertions/s.

  1) Error:
BugTest#test_association_stuff:
ActiveRecord::StatementInvalid: Mysql2::Error: Unknown column 'posts.id' in 'on clause': SELECT `users`.* FROM `users` LEFT OUTER JOIN `comments` ON `comments`.`post_id` = `posts`.`id` INNER JOIN posts ON users.id = posts.user_id
    /Users/tms/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/mysql2-0.4.3/lib/mysql2/client.rb:107:in `_query'
    /Users/tms/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/mysql2-0.4.3/lib/mysql2/client.rb:107:in `block in query'
    /Users/tms/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/mysql2-0.4.3/lib/mysql2/client.rb:106:in `handle_interrupt'
    /Users/tms/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/mysql2-0.4.3/lib/mysql2/client.rb:106:in `query'
    /Users/tms/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.2.6/lib/active_record/connection_adapters/abstract_mysql_adapter.rb:305:in `block in execute'
    /Users/tms/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.2.6/lib/active_record/connection_adapters/abstract_adapter.rb:472:in `block in log'
    /Users/tms/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activesupport-4.2.6/lib/active_support/notifications/instrumenter.rb:20:in `instrument'
    /Users/tms/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.2.6/lib/active_record/connection_adapters/abstract_adapter.rb:466:in `log'
    /Users/tms/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.2.6/lib/active_record/connection_adapters/abstract_mysql_adapter.rb:305:in `execute'
    /Users/tms/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.2.6/lib/active_record/connection_adapters/mysql2_adapter.rb:231:in `execute'
    /Users/tms/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.2.6/lib/active_record/connection_adapters/mysql2_adapter.rb:235:in `exec_query'
    /Users/tms/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.2.6/lib/active_record/connection_adapters/abstract/database_statements.rb:356:in `select'
    /Users/tms/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.2.6/lib/active_record/connection_adapters/abstract/database_statements.rb:32:in `select_all'
    /Users/tms/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.2.6/lib/active_record/connection_adapters/abstract/query_cache.rb:70:in `select_all'
    /Users/tms/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.2.6/lib/active_record/querying.rb:39:in `find_by_sql'
    /Users/tms/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.2.6/lib/active_record/relation.rb:639:in `exec_queries'
    /Users/tms/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.2.6/lib/active_record/relation.rb:515:in `load'
    /Users/tms/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/activerecord-4.2.6/lib/active_record/relation.rb:243:in `to_a'
    merge_join.rb:63:in `test_association_stuff'

1 runs, 0 assertions, 0 failures, 1 errors, 0 skips

System configuration

ActiveRecord 4.2.6, mysql2 0.4.3, MySQL 5.6.27, Ruby 2.1.2

By the way, I am curious behind the rationale of converting joins contained in the scope to be merged to a LEFT OUTER JOIN. This appears to be intended, as described in #16140.

@sevaorlov
Copy link
Contributor

sevaorlov commented May 7, 2016

Not sure what is the reason, but it seems by design Rails converts inner joins to outer joins when merging.

join_dependency = ActiveRecord::Associations::JoinDependency.new(other.klass,

then these joins are called 'stashed' here

and passed as outer joins here
join_infos = join_dependency.join_constraints stashed_association_joins, join_type

def join_constraints(outer_joins, join_type)

Can someone from Rails team give feedback about this?

@maclover7
Copy link
Contributor

maclover7 commented May 7, 2016

cc @sgrif, assigning to you so it doesn't get lost while you are 🚗 to Canada -- r? @sgrif

@thedarkone
Copy link
Contributor

I have #15461 that attempts to fix this. The PR is a bit stale now, but I'll try to look into it really soon.

@oyeanuj
Copy link

oyeanuj commented Jul 10, 2016

@sgrif @thedarkone Any updates here?

@thedarkone
Copy link
Contributor

I will not be able to work on my PR until after 22nd.

@MaxLap
Copy link
Contributor

MaxLap commented Aug 14, 2016

I think the "left_outer_joins" method also needs to be taken into consideration.

Right now, it is treated as a distinct list of values from the joins_values, so it's impossible to keep the order. The merge of associations also doesn't treat the left_joins the same as the regular joins.

@tibbon
Copy link

tibbon commented Jul 23, 2018

Any updates on this one?

@MaxLap
Copy link
Contributor

MaxLap commented Jul 23, 2018

Shameless plug: I made a gem to do conditions on associations without using JOIN. It uses EXISTS, which avoids lots issues from JOIN, including this issue here. (unless you are using JOIN for some other use than filtering)

You can read about all of the issues with the alternatives here: activerecord_where_assoc - ALTERNATIVES_PROBLEMS

This is the gem: activerecord_where_assoc

@chumakoff
Copy link
Contributor

For those who has the same problem there is a workaround:

    users_with_posts = User.joins('INNER JOIN posts ON users.id = posts.user_id')
    posts_with_comments = Post.joins(:comments)

    User.joins(users_with_posts.join_sources).joins(posts_with_comments.join_sources)

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
Development

Successfully merging a pull request may close this issue.

10 participants