From 2c2dde35aa2a8e90ba3f1d4cac7d8aaed4301b3a Mon Sep 17 00:00:00 2001 From: Ari Summer Date: Sat, 16 Dec 2023 12:29:27 -0700 Subject: [PATCH 1/4] Add failing test case for preloading polymorphic association across multiple databases with same table name --- activerecord/test/cases/associations_test.rb | 22 +++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/activerecord/test/cases/associations_test.rb b/activerecord/test/cases/associations_test.rb index 812a6f17030e9..2b5bfb2190517 100644 --- a/activerecord/test/cases/associations_test.rb +++ b/activerecord/test/cases/associations_test.rb @@ -39,6 +39,8 @@ require "models/cpk" require "models/member_detail" require "models/organization" +require "models/dog" +require "models/other_dog" class AssociationsTest < ActiveRecord::TestCase @@ -773,7 +775,8 @@ def test_associations_raise_with_name_error_if_associated_to_classes_that_do_not class PreloaderTest < ActiveRecord::TestCase fixtures :posts, :comments, :books, :authors, :tags, :taggings, :essays, :categories, :author_addresses, :sharded_blog_posts, :sharded_comments, :sharded_blog_posts_tags, :sharded_tags, - :members, :member_details, :organizations, :cpk_orders, :cpk_order_agreements + :members, :member_details, :organizations, :cpk_orders, :cpk_order_agreements, + :dogs, :other_dogs def test_preload_with_scope post = posts(:welcome) @@ -1206,6 +1209,23 @@ def test_preload_does_not_group_same_scope_different_key_name end end + def test_multi_database_polymorphic_preload_with_same_table_name + dog = dogs(:sophie) + dog_comment = comments(:greetings) + dog_comment.origin_type = dog.class.name + dog_comment.origin_id = dog.id + + other_dog = other_dogs(:lassie) + other_dog_comment = comments(:more_greetings) + other_dog_comment.origin_type = other_dog.class.name + other_dog_comment.origin_id = other_dog.id + + assert_queries(2) do + preloader = ActiveRecord::Associations::Preloader.new(records: [dog_comment, other_dog_comment], associations: :origin) + preloader.call + end + end + def test_preload_with_available_records post = posts(:welcome) david = authors(:david) From 9e06d3380ec1c82a4800a320d68d8665ef080fa7 Mon Sep 17 00:00:00 2001 From: Ari Summer Date: Sat, 16 Dec 2023 12:38:59 -0700 Subject: [PATCH 2/4] Update Preloader::Assocation::LoaderQuery to compare using scope.name Using scope.table_name can result in missing preloaded associations if you're preloading polymorphic associations across multiple databases with the same table names. By using the scope name, we're comparing class names, which are guaranteed to be different even if the table_name is equivalent. --- .../lib/active_record/associations/preloader/association.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/associations/preloader/association.rb b/activerecord/lib/active_record/associations/preloader/association.rb index 7c23b7fbe2637..e1974f304b250 100644 --- a/activerecord/lib/active_record/associations/preloader/association.rb +++ b/activerecord/lib/active_record/associations/preloader/association.rb @@ -16,12 +16,12 @@ def initialize(scope, association_key_name) def eql?(other) association_key_name == other.association_key_name && - scope.table_name == other.scope.table_name && + scope.name == other.scope.name && scope.values_for_queries == other.scope.values_for_queries end def hash - [association_key_name, scope.table_name, scope.values_for_queries].hash + [association_key_name, scope.name, scope.values_for_queries].hash end def records_for(loaders) From b8f19c930560ca0a1dce8137098f80de62fd377e Mon Sep 17 00:00:00 2001 From: Ari Summer Date: Sat, 16 Dec 2023 12:48:17 -0700 Subject: [PATCH 3/4] Add explanatory comment to test case --- activerecord/test/cases/associations_test.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/activerecord/test/cases/associations_test.rb b/activerecord/test/cases/associations_test.rb index 2b5bfb2190517..bad3feb09c704 100644 --- a/activerecord/test/cases/associations_test.rb +++ b/activerecord/test/cases/associations_test.rb @@ -1220,6 +1220,14 @@ def test_multi_database_polymorphic_preload_with_same_table_name other_dog_comment.origin_type = other_dog.class.name other_dog_comment.origin_id = other_dog.id + # Both Dog and OtherDog are backed by a table named `dogs`, + # however they are stored in different databases and should + # therefore result in two separate queries rather than be batched + # together. + # + # Expected + # SELECT FROM dogs ... (Dog) + # SELECT FROM dogs ... (OtherDog) assert_queries(2) do preloader = ActiveRecord::Associations::Preloader.new(records: [dog_comment, other_dog_comment], associations: :origin) preloader.call From def54b22d1640a098243d8167a452a5337304373 Mon Sep 17 00:00:00 2001 From: Ari Summer Date: Sat, 16 Dec 2023 17:05:54 -0700 Subject: [PATCH 4/4] Replace scope.name with a comparison on both table_name and connection_specification_name --- .../lib/active_record/associations/preloader/association.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/activerecord/lib/active_record/associations/preloader/association.rb b/activerecord/lib/active_record/associations/preloader/association.rb index e1974f304b250..4ab8ef0558824 100644 --- a/activerecord/lib/active_record/associations/preloader/association.rb +++ b/activerecord/lib/active_record/associations/preloader/association.rb @@ -16,12 +16,13 @@ def initialize(scope, association_key_name) def eql?(other) association_key_name == other.association_key_name && - scope.name == other.scope.name && + scope.table_name == other.scope.table_name && + scope.connection_specification_name == other.scope.connection_specification_name && scope.values_for_queries == other.scope.values_for_queries end def hash - [association_key_name, scope.name, scope.values_for_queries].hash + [association_key_name, scope.table_name, scope.connection_specification_name, scope.values_for_queries].hash end def records_for(loaders)