From 10b36e81a357f8d7fa3665630c4d41c057fe59d9 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Tue, 26 May 2020 01:29:10 +0900 Subject: [PATCH] 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. --- .../associations/join_dependency.rb | 10 +++++++++- .../join_dependency/join_association.rb | 18 ++++++++++++++---- .../has_many_through_associations_test.rb | 8 ++++++++ activerecord/test/models/author.rb | 3 +++ 4 files changed, 34 insertions(+), 5 deletions(-) diff --git a/activerecord/lib/active_record/associations/join_dependency.rb b/activerecord/lib/active_record/associations/join_dependency.rb index eb3fb0b74f..459ed193be 100644 --- a/activerecord/lib/active_record/associations/join_dependency.rb +++ b/activerecord/lib/active_record/associations/join_dependency.rb @@ -80,6 +80,7 @@ module ActiveRecord def join_constraints(joins_to_add, alias_tracker) @alias_tracker = alias_tracker + @joined_tables = {} joins = make_join_constraints(join_root, join_type) @@ -173,9 +174,16 @@ module ActiveRecord foreign_table = parent.table foreign_klass = parent.base_klass 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) 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 diff --git a/activerecord/lib/active_record/associations/join_dependency/join_association.rb b/activerecord/lib/active_record/associations/join_dependency/join_association.rb index 98b18e53b6..74817ec7fa 100644 --- a/activerecord/lib/active_record/associations/join_dependency/join_association.rb +++ b/activerecord/lib/active_record/associations/join_dependency/join_association.rb @@ -23,13 +23,23 @@ module ActiveRecord def join_constraints(foreign_table, foreign_klass, join_type, alias_tracker, &block) joins = [] - tables = reflection.chain.map(&block) - @table = tables.first + chain = [] + + 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 # more sense in this context), so we reverse - reflection.chain.reverse_each.with_index(1) do |reflection, i| - table = tables[-i] + chain.reverse_each do |reflection, table| klass = reflection.klass join_scope = reflection.join_scope(table, foreign_table, foreign_klass) diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb index 284075fb5c..10f4a7c8a3 100644 --- a/activerecord/test/cases/associations/has_many_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb @@ -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 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 post = TaggedPost.create!(title: "Tagged", body: "Post") tag = post.tags.create!(name: "Tag") diff --git a/activerecord/test/models/author.rb b/activerecord/test/models/author.rb index 373f6d2af6..a19890a0a5 100644 --- a/activerecord/test/models/author.rb +++ b/activerecord/test/models/author.rb @@ -93,6 +93,9 @@ class Author < ActiveRecord::Base has_many :special_categories, 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 :nonspecial_categories_with_conditions, -> { where(categorizations: { special: false }) }, through: :categorizations, source: :category