Skip to content

Commit 49bcb00

Browse files
kamipokamipo
committed
Fix eager loading polymorphic association with mixed table conditions
This fixes a bug that the `foreign_key` and the `foreign_type` are separated as different table conditions if a polymorphic association has a scope that joins another tables.
1 parent 25b3cbb commit 49bcb00

6 files changed

Lines changed: 43 additions & 26 deletions

File tree

‎activerecord/lib/active_record/associations/join_dependency/join_association.rb‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
# frozen_string_literal: true
22

33
require "active_record/associations/join_dependency/join_part"
4+
require "active_support/core_ext/array/extract"
45

56
module ActiveRecord
67
module Associations
@@ -30,17 +31,21 @@ def join_constraints(foreign_table, foreign_klass, join_type, alias_tracker)
3031
table = tables[-i]
3132
klass = reflection.klass
3233

33-
constraint = reflection.build_join_constraint(table, foreign_table)
34+
join_scope = reflection.join_scope(table, foreign_table, foreign_klass)
3435

35-
joins << table.create_join(table, table.create_on(constraint), join_type)
36-
37-
join_scope = reflection.join_scope(table, foreign_klass)
3836
arel = join_scope.arel(alias_tracker.aliases)
37+
nodes = arel.constraints.first
38+
39+
others = nodes.children.extract! do |node|
40+
Arel.fetch_attribute(node) { |attr| attr.relation.name != table.name }
41+
end
42+
43+
joins << table.create_join(table, table.create_on(nodes), join_type)
3944

40-
if arel.constraints.any?
45+
unless others.empty?
4146
joins.concat arel.join_sources
4247
right = joins.last.right
43-
right.expr = right.expr.and(arel.constraints)
48+
right.expr.children.concat(others)
4449
end
4550

4651
# The current table in this iteration becomes the foreign table in the next

‎activerecord/lib/active_record/reflection.rb‎

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -178,28 +178,24 @@ def scopes
178178
scope ? [scope] : []
179179
end
180180

181-
def build_join_constraint(table, foreign_table)
182-
key = join_keys.key
183-
foreign_key = join_keys.foreign_key
184-
185-
constraint = table[key].eq(foreign_table[foreign_key])
186-
187-
if klass.finder_needs_type_condition?
188-
table.create_and([constraint, klass.send(:type_condition, table)])
189-
else
190-
constraint
191-
end
192-
end
193-
194-
def join_scope(table, foreign_klass)
181+
def join_scope(table, foreign_table, foreign_klass)
195182
predicate_builder = predicate_builder(table)
196183
scope_chain_items = join_scopes(table, predicate_builder)
197184
klass_scope = klass_join_scope(table, predicate_builder)
198185

186+
key = join_keys.key
187+
foreign_key = join_keys.foreign_key
188+
189+
klass_scope.where!(table[key].eq(foreign_table[foreign_key]))
190+
199191
if type
200192
klass_scope.where!(type => foreign_klass.polymorphic_name)
201193
end
202194

195+
if klass.finder_needs_type_condition?
196+
klass_scope.where!(klass.send(:type_condition, table))
197+
end
198+
203199
scope_chain_items.inject(klass_scope, &:merge!)
204200
end
205201

‎activerecord/lib/active_record/relation/where_clause.rb‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -140,11 +140,7 @@ def invert_predicate(node)
140140

141141
def except_predicates(columns)
142142
predicates.reject do |node|
143-
case node
144-
when Arel::Nodes::Between, Arel::Nodes::In, Arel::Nodes::NotIn, Arel::Nodes::Equality, Arel::Nodes::NotEqual, Arel::Nodes::LessThan, Arel::Nodes::LessThanOrEqual, Arel::Nodes::GreaterThan, Arel::Nodes::GreaterThanOrEqual
145-
subrelation = (node.left.kind_of?(Arel::Attributes::Attribute) ? node.left : node.right)
146-
columns.include?(subrelation.name.to_s)
147-
end
143+
Arel.fetch_attribute(node) { |attr| columns.include?(attr.name.to_s) }
148144
end
149145
end
150146

‎activerecord/lib/arel.rb‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,13 @@ def self.arel_node?(value)
3939
value.is_a?(Arel::Node) || value.is_a?(Arel::Attribute) || value.is_a?(Arel::Nodes::SqlLiteral)
4040
end
4141

42+
def self.fetch_attribute(value)
43+
case value
44+
when Arel::Nodes::Between, Arel::Nodes::In, Arel::Nodes::NotIn, Arel::Nodes::Equality, Arel::Nodes::NotEqual, Arel::Nodes::LessThan, Arel::Nodes::LessThanOrEqual, Arel::Nodes::GreaterThan, Arel::Nodes::GreaterThanOrEqual
45+
yield value.left.is_a?(Arel::Attributes::Attribute) ? value.left : value.right
46+
end
47+
end
48+
4249
## Convenience Alias
4350
Node = Arel::Nodes::Node
4451
end

‎activerecord/test/cases/associations/eager_test.rb‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
require "models/post"
55
require "models/tagging"
66
require "models/tag"
7+
require "models/rating"
78
require "models/comment"
89
require "models/author"
910
require "models/essay"
@@ -44,7 +45,7 @@ def test_eager_loading_too_may_ids
4445

4546
class EagerAssociationTest < ActiveRecord::TestCase
4647
fixtures :posts, :comments, :authors, :essays, :author_addresses, :categories, :categories_posts,
47-
:companies, :accounts, :tags, :taggings, :people, :readers, :categorizations,
48+
:companies, :accounts, :tags, :taggings, :ratings, :people, :readers, :categorizations,
4849
:owners, :pets, :author_favorites, :jobs, :references, :subscribers, :subscriptions, :books,
4950
:developers, :projects, :developers_projects, :members, :memberships, :clubs, :sponsors
5051

@@ -89,6 +90,17 @@ def test_loading_conditions_with_or
8990
"expected to find only david's posts"
9091
end
9192

93+
def test_loading_polymorphic_association_with_mixed_table_conditions
94+
rating = Rating.first
95+
assert_equal [taggings(:normal_comment_rating)], rating.taggings_without_tag
96+
97+
rating = Rating.preload(:taggings_without_tag).first
98+
assert_equal [taggings(:normal_comment_rating)], rating.taggings_without_tag
99+
100+
rating = Rating.eager_load(:taggings_without_tag).first
101+
assert_equal [taggings(:normal_comment_rating)], rating.taggings_without_tag
102+
end
103+
92104
def test_loading_with_scope_including_joins
93105
member = Member.first
94106
assert_equal members(:groucho), member

‎activerecord/test/models/rating.rb‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,5 @@
33
class Rating < ActiveRecord::Base
44
belongs_to :comment
55
has_many :taggings, as: :taggable
6+
has_many :taggings_without_tag, -> { left_joins(:tag).where("tags.id": nil) }, as: :taggable, class_name: "Tagging"
67
end

0 commit comments

Comments
 (0)