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

merge parent joins for through associations #24682

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion activerecord/CHANGELOG.md
@@ -1,7 +1,13 @@
## Rails 5.0.0.rc1 (May 06, 2016) ##

* No changes.
* Merge parent joins for through associations.

Merges parent associations joins for through associations into a scope. Previously joins in association scope were
taken into account only for root associations with that scope.

Fixes #22538.

*Seva Orlov*

## Rails 5.0.0.beta4 (April 27, 2016) ##

Expand Down
21 changes: 18 additions & 3 deletions activerecord/lib/active_record/associations/association_scope.rb
Expand Up @@ -134,13 +134,13 @@ def add_constraints(scope, owner, association_klass, refl, chain_head, chain_tai
scope = next_chain_scope(scope, table, reflection, association_klass, foreign_table, next_reflection)
end

# Exclude the scope of the association itself, because that
# was already merged in the #scope method.
reflection.constraints.each do |scope_chain_item|
item = eval_scope(reflection.klass, scope_chain_item, owner)
item = eval_scope(reflection.klass, scope_chain_item, owner)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

outdated comment


if scope_chain_item == refl.scope
scope.merge! item.except(:where, :includes)
elsif item.joins_values.any?
merge_joins(scope, item)
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this different than the merge that will occur in the call to scope.merge!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

scope.merge! merges joins with Relation::Merger, which converts inner joins to outer when merge_joins doesn't do that

Copy link
Contributor

Choose a reason for hiding this comment

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

That doesn't appear to be the case. The code is almost exactly identical to what you have here. https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/merger.rb#L107-L129

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it looks pretty much the same, but there are differences at the end. Instead of passing ActiveRecord::Associations::JoinDependency instance to relation.joins!, I pass an array of Arel::Nodes::InnerJoin objects, which are built as inner joins instead of outer for JoinDependency here
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/query_methods.rb#L1015
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/query_methods.rb#L1030
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/relation/query_methods.rb#L1042
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/associations/join_dependency.rb#L122

end

reflection.all_includes do
Expand All @@ -158,6 +158,21 @@ def add_constraints(scope, owner, association_klass, refl, chain_head, chain_tai
scope
end

# Merges inner joins from `other` relation to `scope` relation.
def merge_joins(scope, other)
association_joins, rest_joins = other.joins_values.partition do |join|
case join
when Hash, Symbol, Array
true
else
false
end
end

join_dependency = ActiveRecord::Associations::JoinDependency.new(other.klass, association_joins, rest_joins)
scope.joins! join_dependency.join_constraints([], Arel::Nodes::InnerJoin).map(&:joins)
end

def eval_scope(klass, scope, owner)
klass.unscoped.instance_exec(owner, &scope)
end
Expand Down
Expand Up @@ -1215,4 +1215,13 @@ def test_has_many_through_do_not_cache_association_reader_if_the_though_method_h
ensure
TenantMembership.current_member = nil
end

def test_has_many_through_with_joins_in_scope
developer = Developer.create! name: 'Jamis'
post = Post.first
author = Author.create! name: 'John'
Comment.create! developer: developer, post: post, body: '', author: author

assert_includes post.reload.jamises_developer_comments_authors, author
end
end
3 changes: 3 additions & 0 deletions activerecord/test/models/post.rb
Expand Up @@ -60,6 +60,9 @@ def the_association
end
end

has_many :jamises_developer_comments, -> { joins(:developer).merge(Developer.jamises) }, class_name: 'Comment'
has_many :jamises_developer_comments_authors, through: :jamises_developer_comments, source: :author, source_type: 'Author'

has_many :comments_with_extend, extend: NamedExtension, class_name: "Comment", foreign_key: "post_id" do
def greeting
"hello"
Expand Down