Skip to content

Commit

Permalink
has_many :through with unscope should affect to through scope
Browse files Browse the repository at this point in the history
The order of scope evaluation should be from through scope to the
association's own scope. Otherwise the association's scope cannot affect
to through scope.

Fixes #13677.
Closes #28449.
  • Loading branch information
kamipo committed Sep 6, 2017
1 parent 0a94612 commit 3acc5d6
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 23 deletions.
35 changes: 14 additions & 21 deletions activerecord/lib/active_record/associations/association_scope.rb
Expand Up @@ -24,10 +24,10 @@ def scope(association)
scope = klass.unscoped
owner = association.owner
alias_tracker = AliasTracker.create(klass.connection, klass.table_name)
chain_head, chain_tail = get_chain(reflection, association, alias_tracker)
chain = get_chain(reflection, association, alias_tracker)

scope.extending! reflection.extensions
add_constraints(scope, owner, chain_head, chain_tail)
add_constraints(scope, owner, chain)
end

def join_type
Expand Down Expand Up @@ -98,7 +98,6 @@ def next_chain_scope(scope, table, reflection, foreign_table, next_reflection)
end

class ReflectionProxy < SimpleDelegator # :nodoc:
attr_accessor :next
attr_reader :alias_name

def initialize(reflection, alias_name)
Expand All @@ -111,36 +110,32 @@ def all_includes; nil; end

def get_chain(reflection, association, tracker)
name = reflection.name
runtime_reflection = Reflection::RuntimeReflection.new(reflection, association)
previous_reflection = runtime_reflection
chain = [Reflection::RuntimeReflection.new(reflection, association)]
reflection.chain.drop(1).each do |refl|
alias_name = tracker.aliased_table_for(
refl.table_name,
refl.alias_candidate(name),
refl.klass.type_caster
)
proxy = ReflectionProxy.new(refl, alias_name)
previous_reflection.next = proxy
previous_reflection = proxy
chain << ReflectionProxy.new(refl, alias_name)
end
[runtime_reflection, previous_reflection]
chain
end

def add_constraints(scope, owner, chain_head, chain_tail)
owner_reflection = chain_tail
def add_constraints(scope, owner, chain)
owner_reflection = chain.last
table = owner_reflection.alias_name
scope = last_chain_scope(scope, table, owner_reflection, owner)

reflection = chain_head
while reflection
chain.each_cons(2) do |reflection, next_reflection|
table = reflection.alias_name
next_reflection = reflection.next

unless reflection == chain_tail
foreign_table = next_reflection.alias_name
scope = next_chain_scope(scope, table, reflection, foreign_table, next_reflection)
end
foreign_table = next_reflection.alias_name
scope = next_chain_scope(scope, table, reflection, foreign_table, next_reflection)
end

chain_head = chain.first
chain.reverse_each do |reflection|
table = reflection.alias_name
# Exclude the scope of the association itself, because that
# was already merged in the #scope method.
reflection.constraints.each do |scope_chain_item|
Expand All @@ -158,8 +153,6 @@ def add_constraints(scope, owner, chain_head, chain_tail)
scope.where_clause += item.where_clause
scope.order_values |= item.order_values
end

reflection = next_reflection
end

scope
Expand Down
2 changes: 0 additions & 2 deletions activerecord/lib/active_record/reflection.rb
Expand Up @@ -1077,8 +1077,6 @@ def source_type_info
end

class RuntimeReflection < PolymorphicReflection # :nodoc:
attr_accessor :next

def initialize(reflection, association)
@reflection = reflection
@association = association
Expand Down
Expand Up @@ -1250,6 +1250,10 @@ def test_has_many_through_do_not_cache_association_reader_if_the_though_method_h
TenantMembership.current_member = nil
end

def test_has_many_through_with_unscope_should_affect_to_through_scope
assert_equal [comments(:eager_other_comment1)], authors(:mary).unordered_comments
end

def test_has_many_through_with_scope_should_respect_table_alias
family = Family.create!
users = 3.times.map { User.create! }
Expand Down
1 change: 1 addition & 0 deletions activerecord/test/models/author.rb
Expand Up @@ -40,6 +40,7 @@ def ratings
class_name: "Post"

has_many :comments_desc, -> { order("comments.id DESC") }, through: :posts, source: :comments
has_many :unordered_comments, -> { unscope(:order).distinct }, through: :posts_sorted_by_id_limited, source: :comments
has_many :funky_comments, through: :posts, source: :comments
has_many :ordered_uniq_comments, -> { distinct.order("comments.id") }, through: :posts, source: :comments
has_many :ordered_uniq_comments_desc, -> { distinct.order("comments.id DESC") }, through: :posts, source: :comments
Expand Down

1 comment on commit 3acc5d6

@bogdan
Copy link
Contributor

@bogdan bogdan commented on 3acc5d6 Sep 7, 2017

Choose a reason for hiding this comment

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

❤️

Please sign in to comment.