mirror of
https://github.com/rails/rails.git
synced 2022-11-09 12:12:34 -05:00
Fix incorrect result when eager loading with duplicated through association with join scope
I had found the issue while working on fixing #33525.
That is if duplicated association has a scope which has `where` with
explicit table name condition (e.g. `where("categories.name": "General")`),
that condition in all duplicated associations will filter the first one
only, other all duplicated associations are not filtered, since
duplicated joins will be aliased except the first one (e.g.
`INNER JOIN "categories" "categories_categorizations"`).
```ruby
class Author < ActiveRecord::Base
has_many :general_categorizations, -> { joins(:category).where("categories.name": "General") }, class_name: "Categorization"
has_many :general_posts, through: :general_categorizations, source: :post
end
authors = Author.eager_load(:general_categorizations, :general_posts).to_a
```
Generated eager loading query:
```sql
SELECT "authors"."id" AS t0_r0, ... FROM "authors"
-- `has_many :general_categorizations, -> { joins(:category).where("categories.name": "General") }`
LEFT OUTER JOIN "categorizations" ON "categorizations"."author_id" = "authors"."id"
INNER JOIN "categories" ON "categories"."id" = "categorizations"."category_id" AND "categories"."name" = ?
-- `has_many :general_posts, through: :general_categorizations, source: :post`
---- duplicated `through: :general_categorizations` part
LEFT OUTER JOIN "categorizations" "general_categorizations_authors_join" ON "general_categorizations_authors_join"."author_id" = "authors"."id"
INNER JOIN "categories" "categories_categorizations" ON "categories_categorizations"."id" = "general_categorizations_authors_join"."category_id" AND "categories"."name" = ? -- <-- filtering `"categories"."name" = ?` won't work
---- `source: :post` part
LEFT OUTER JOIN "posts" ON "posts"."id" = "general_categorizations_authors_join"."post_id"
```
Originally eager loading with join scope didn't work before Rails 5.2
(#29413), and duplicated through association with join scope raised a
duplicated alias error before alias tracking is improved in 590b045
.
But now it will potentially be got incorrect result instead of an error,
it is worse than an error.
To fix the issue, it makes eager loading to deduplicate / re-use
duplicated through association if possible, like as `preload`.
```sql
SELECT "authors"."id" AS t0_r0, ... FROM "authors"
-- `has_many :general_categorizations, -> { joins(:category).where("categories.name": "General") }`
LEFT OUTER JOIN "categorizations" ON "categorizations"."author_id" = "authors"."id"
INNER JOIN "categories" ON "categories"."id" = "categorizations"."category_id" AND "categories"."name" = ?
-- `has_many :general_posts, through: :general_categorizations, source: :post`
---- `through: :general_categorizations` part is deduplicated / re-used
LEFT OUTER JOIN "posts" ON "posts"."id" = "categorizations"."post_id"
```
Fixes #32819.
This commit is contained in:
parent
e8cf45dc4a
commit
10b36e81a3
4 changed files with 34 additions and 5 deletions
|
@ -80,6 +80,7 @@ module ActiveRecord
|
||||||
|
|
||||||
def join_constraints(joins_to_add, alias_tracker)
|
def join_constraints(joins_to_add, alias_tracker)
|
||||||
@alias_tracker = alias_tracker
|
@alias_tracker = alias_tracker
|
||||||
|
@joined_tables = {}
|
||||||
|
|
||||||
joins = make_join_constraints(join_root, join_type)
|
joins = make_join_constraints(join_root, join_type)
|
||||||
|
|
||||||
|
@ -173,9 +174,16 @@ module ActiveRecord
|
||||||
foreign_table = parent.table
|
foreign_table = parent.table
|
||||||
foreign_klass = parent.base_klass
|
foreign_klass = parent.base_klass
|
||||||
child.join_constraints(foreign_table, foreign_klass, join_type, alias_tracker) do |reflection|
|
child.join_constraints(foreign_table, foreign_klass, join_type, alias_tracker) do |reflection|
|
||||||
alias_tracker.aliased_table_for(reflection.klass.arel_table) do
|
table = @joined_tables[reflection]
|
||||||
|
|
||||||
|
next table, true if table && reflection != child.reflection
|
||||||
|
|
||||||
|
table = alias_tracker.aliased_table_for(reflection.klass.arel_table) do
|
||||||
table_alias_for(reflection, parent, reflection != child.reflection)
|
table_alias_for(reflection, parent, reflection != child.reflection)
|
||||||
end
|
end
|
||||||
|
|
||||||
|
@joined_tables[reflection] ||= table if join_type == Arel::Nodes::OuterJoin
|
||||||
|
table
|
||||||
end.concat child.children.flat_map { |c| make_constraints(child, c, join_type) }
|
end.concat child.children.flat_map { |c| make_constraints(child, c, join_type) }
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -23,13 +23,23 @@ module ActiveRecord
|
||||||
|
|
||||||
def join_constraints(foreign_table, foreign_klass, join_type, alias_tracker, &block)
|
def join_constraints(foreign_table, foreign_klass, join_type, alias_tracker, &block)
|
||||||
joins = []
|
joins = []
|
||||||
tables = reflection.chain.map(&block)
|
chain = []
|
||||||
@table = tables.first
|
|
||||||
|
reflection.chain.each do |reflection|
|
||||||
|
table, terminated = yield reflection
|
||||||
|
|
||||||
|
if terminated
|
||||||
|
foreign_table, foreign_klass = table, reflection.klass
|
||||||
|
break
|
||||||
|
end
|
||||||
|
|
||||||
|
@table ||= table
|
||||||
|
chain << [reflection, table]
|
||||||
|
end
|
||||||
|
|
||||||
# The chain starts with the target table, but we want to end with it here (makes
|
# The chain starts with the target table, but we want to end with it here (makes
|
||||||
# more sense in this context), so we reverse
|
# more sense in this context), so we reverse
|
||||||
reflection.chain.reverse_each.with_index(1) do |reflection, i|
|
chain.reverse_each do |reflection, table|
|
||||||
table = tables[-i]
|
|
||||||
klass = reflection.klass
|
klass = reflection.klass
|
||||||
|
|
||||||
join_scope = reflection.join_scope(table, foreign_table, foreign_klass)
|
join_scope = reflection.join_scope(table, foreign_table, foreign_klass)
|
||||||
|
|
|
@ -1069,6 +1069,14 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
|
||||||
assert_equal expected, Author.eager_load(:lazy_readers_skimmers_or_not_2).last.lazy_readers_skimmers_or_not_2
|
assert_equal expected, Author.eager_load(:lazy_readers_skimmers_or_not_2).last.lazy_readers_skimmers_or_not_2
|
||||||
end
|
end
|
||||||
|
|
||||||
|
def test_duplicated_has_many_through_with_join_scope
|
||||||
|
Categorization.create!(author: authors(:david), post: posts(:thinking), category: categories(:technology))
|
||||||
|
|
||||||
|
expected = [posts(:welcome)]
|
||||||
|
assert_equal expected, Author.preload(:general_categorizations, :general_posts).first.general_posts
|
||||||
|
assert_equal expected, Author.eager_load(:general_categorizations, :general_posts).first.general_posts
|
||||||
|
end
|
||||||
|
|
||||||
def test_has_many_through_polymorphic_with_rewhere
|
def test_has_many_through_polymorphic_with_rewhere
|
||||||
post = TaggedPost.create!(title: "Tagged", body: "Post")
|
post = TaggedPost.create!(title: "Tagged", body: "Post")
|
||||||
tag = post.tags.create!(name: "Tag")
|
tag = post.tags.create!(name: "Tag")
|
||||||
|
|
|
@ -93,6 +93,9 @@ class Author < ActiveRecord::Base
|
||||||
has_many :special_categories, through: :special_categorizations, source: :category
|
has_many :special_categories, through: :special_categorizations, source: :category
|
||||||
has_one :special_category, through: :special_categorizations, source: :category
|
has_one :special_category, through: :special_categorizations, source: :category
|
||||||
|
|
||||||
|
has_many :general_categorizations, -> { joins(:category).where("categories.name": "General") }, class_name: "Categorization"
|
||||||
|
has_many :general_posts, through: :general_categorizations, source: :post
|
||||||
|
|
||||||
has_many :special_categories_with_conditions, -> { where(categorizations: { special: true }) }, through: :categorizations, source: :category
|
has_many :special_categories_with_conditions, -> { where(categorizations: { special: true }) }, through: :categorizations, source: :category
|
||||||
has_many :nonspecial_categories_with_conditions, -> { where(categorizations: { special: false }) }, through: :categorizations, source: :category
|
has_many :nonspecial_categories_with_conditions, -> { where(categorizations: { special: false }) }, through: :categorizations, source: :category
|
||||||
|
|
||||||
|
|
Loading…
Reference in a new issue