From e617fb57f5a388d5f0a47fd5e576588dd10066b0 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Mon, 30 Oct 2017 23:55:48 +0900 Subject: Fix preloading polymorphic association when through association has already loaded If through association has already loaded, `source_type` is ignored to loaded through records. The loaded records should be filtered by `source_type` in that case. Fixes #30904. --- .../associations/preloader/through_association.rb | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) (limited to 'activerecord/lib/active_record/associations') diff --git a/activerecord/lib/active_record/associations/preloader/through_association.rb b/activerecord/lib/active_record/associations/preloader/through_association.rb index fa32cc5553..5bd49b041a 100644 --- a/activerecord/lib/active_record/associations/preloader/through_association.rb +++ b/activerecord/lib/active_record/associations/preloader/through_association.rb @@ -13,18 +13,30 @@ module ActiveRecord end def associated_records_by_owner(preloader) + already_loaded = owners.first.association(through_reflection.name).loaded? through_scope = through_scope() - preloader.preload(owners, - through_reflection.name, - through_scope) + unless already_loaded + preloader.preload(owners, through_reflection.name, through_scope) + end through_records = owners.map do |owner| center = owner.association(through_reflection.name).target [owner, Array(center)] end - reset_association(owners, through_reflection.name, through_scope) + if already_loaded + if source_type = reflection.options[:source_type] + through_records.map! do |owner, center| + center = center.select do |record| + record[reflection.foreign_type] == source_type + end + [owner, center] + end + end + else + reset_association(owners, through_reflection.name, through_scope) + end middle_records = through_records.flat_map(&:last) -- cgit v1.2.3 From e0bef22665f93e88f6b2f3ac6bd55543ed0d6343 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Mon, 6 Nov 2017 06:40:36 +0900 Subject: Fix preloading polymorphic multi-level through association This is partially fixed by e617fb57 when through association has already loaded. Otherwise, second level through association should respect `preload_scope`. Fixes #30242. Closes #30076. [Ryuta Kamizono & CicholGricenchos] --- .../active_record/associations/preloader/through_association.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'activerecord/lib/active_record/associations') diff --git a/activerecord/lib/active_record/associations/preloader/through_association.rb b/activerecord/lib/active_record/associations/preloader/through_association.rb index 5bd49b041a..b16fca7dc9 100644 --- a/activerecord/lib/active_record/associations/preloader/through_association.rb +++ b/activerecord/lib/active_record/associations/preloader/through_association.rb @@ -40,7 +40,11 @@ module ActiveRecord middle_records = through_records.flat_map(&:last) - reflection_scope = reflection_scope() if reflection.scope + if preload_scope + reflection_scope = reflection_scope().merge(preload_scope) + elsif reflection.scope + reflection_scope = reflection_scope() + end preloaders = preloader.preload(middle_records, source_reflection.name, @@ -70,6 +74,8 @@ module ActiveRecord rhs_records end end + end.tap do + reset_association(middle_records, source_reflection.name, preload_scope) end end -- cgit v1.2.3 From edcc0a714d911468caf7db0879fc7a3d1185ee3c Mon Sep 17 00:00:00 2001 From: Bogdan Gusiev Date: Mon, 6 Nov 2017 17:32:54 +0200 Subject: Refactor Preloader Code --- .../associations/preloader/association.rb | 4 +- .../associations/preloader/has_many_through.rb | 10 -- .../associations/preloader/through_association.rb | 107 ++++++++------------- 3 files changed, 40 insertions(+), 81 deletions(-) (limited to 'activerecord/lib/active_record/associations') diff --git a/activerecord/lib/active_record/associations/preloader/association.rb b/activerecord/lib/active_record/associations/preloader/association.rb index 607d376a08..b0c41c7d5f 100644 --- a/activerecord/lib/active_record/associations/preloader/association.rb +++ b/activerecord/lib/active_record/associations/preloader/association.rb @@ -17,7 +17,7 @@ module ActiveRecord end def run(preloader) - associated_records_by_owner(preloader).each do |owner, records| + associated_records_by_owner(preloader) do |owner, records| associate_records_to_owner(owner, records) end end @@ -41,7 +41,7 @@ module ActiveRecord end owners.each_with_object({}) do |owner, result| - result[owner] = records[convert_key(owner[owner_key_name])] || [] + yield(owner, records[convert_key(owner[owner_key_name])] || []) end end diff --git a/activerecord/lib/active_record/associations/preloader/has_many_through.rb b/activerecord/lib/active_record/associations/preloader/has_many_through.rb index 0639fdca44..3e17d07a33 100644 --- a/activerecord/lib/active_record/associations/preloader/has_many_through.rb +++ b/activerecord/lib/active_record/associations/preloader/has_many_through.rb @@ -5,16 +5,6 @@ module ActiveRecord class Preloader class HasManyThrough < CollectionAssociation #:nodoc: include ThroughAssociation - - def associated_records_by_owner(preloader) - records_by_owner = super - - if reflection_scope.distinct_value - records_by_owner.each_value(&:uniq!) - end - - records_by_owner - end end end end diff --git a/activerecord/lib/active_record/associations/preloader/through_association.rb b/activerecord/lib/active_record/associations/preloader/through_association.rb index b16fca7dc9..1bb7e98231 100644 --- a/activerecord/lib/active_record/associations/preloader/through_association.rb +++ b/activerecord/lib/active_record/associations/preloader/through_association.rb @@ -13,86 +13,45 @@ module ActiveRecord end def associated_records_by_owner(preloader) - already_loaded = owners.first.association(through_reflection.name).loaded? - through_scope = through_scope() - - unless already_loaded - preloader.preload(owners, through_reflection.name, through_scope) - end - - through_records = owners.map do |owner| - center = owner.association(through_reflection.name).target - [owner, Array(center)] - end + already_loaded = owners.first.association(through_reflection.name).loaded? + through_scope = through_scope() + reflection_scope = target_reflection_scope + through_preloaders = preloader.preload(owners, through_reflection.name, through_scope) + middle_records = through_preloaders.flat_map(&:preloaded_records) + preloaders = preloader.preload(middle_records, source_reflection.name, reflection_scope) + @preloaded_records = preloaders.flat_map(&:preloaded_records) - if already_loaded - if source_type = reflection.options[:source_type] - through_records.map! do |owner, center| - center = center.select do |record| + owners.each do |owner| + through_records = Array(owner.association(through_reflection.name).target) + if already_loaded + if source_type = reflection.options[:source_type] + through_records = through_records.select do |record| record[reflection.foreign_type] == source_type end - [owner, center] end + else + owner.association(through_reflection.name).reset if through_scope end - else - reset_association(owners, through_reflection.name, through_scope) - end - - middle_records = through_records.flat_map(&:last) - - if preload_scope - reflection_scope = reflection_scope().merge(preload_scope) - elsif reflection.scope - reflection_scope = reflection_scope() - end - - preloaders = preloader.preload(middle_records, - source_reflection.name, - reflection_scope) - - @preloaded_records = preloaders.flat_map(&:preloaded_records) - - middle_to_pl = preloaders.each_with_object({}) do |pl, h| - pl.owners.each { |middle| - h[middle] = pl - } - end - - through_records.each_with_object({}) do |(lhs, center), records_by_owner| - pl_to_middle = center.group_by { |record| middle_to_pl[record] } - - records_by_owner[lhs] = pl_to_middle.flat_map do |pl, middles| - rhs_records = middles.flat_map { |r| - r.association(source_reflection.name).target - }.compact - - # Respect the order on `reflection_scope` if it exists, else use the natural order. - if reflection_scope && !reflection_scope.order_values.empty? - @id_map ||= id_to_index_map @preloaded_records - rhs_records.sort_by { |rhs| @id_map[rhs] } - else - rhs_records - end + result = through_records.flat_map do |record| + association = record.association(source_reflection.name) + target = association.target + association.reset if preload_scope + target end - end.tap do - reset_association(middle_records, source_reflection.name, preload_scope) + result.compact! + if reflection_scope + result.sort_by! { |rhs| preload_index[rhs] } if reflection_scope.order_values.any? + result.uniq! if reflection_scope.distinct_value + end + yield(owner, result) end end private - def id_to_index_map(ids) - id_map = {} - ids.each_with_index { |id, index| id_map[id] = index } - id_map - end - - def reset_association(owners, association_name, should_reset) - # Don't cache the association - we would only be caching a subset - if should_reset - owners.each { |owner| - owner.association(association_name).reset - } + def preload_index + @preload_index ||= @preloaded_records.each_with_object({}).with_index do |(id, result), index| + result[id] = index end end @@ -133,6 +92,16 @@ module ActiveRecord scope unless scope.empty_scope? end + + def target_reflection_scope + if preload_scope + reflection_scope.merge(preload_scope) + elsif reflection.scope + reflection_scope + else + nil + end + end end end end -- cgit v1.2.3 From 3f1695bb9c008b7cb1840e09e640f3ec0c59a564 Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Tue, 7 Nov 2017 08:44:44 +0900 Subject: Remove useless `associated_records_by_owner` `associated_records_by_owner` had returned customizing result before calling `associate_records_to_owner` for through association subclasses. Since #22115, `associate_records_to_owner` is called in the method and not returned owner and result pairs. Removing the method will reduce method call and block call nesting. --- .../associations/preloader/association.rb | 22 ++++++++-------------- .../associations/preloader/through_association.rb | 4 ++-- 2 files changed, 10 insertions(+), 16 deletions(-) (limited to 'activerecord/lib/active_record/associations') diff --git a/activerecord/lib/active_record/associations/preloader/association.rb b/activerecord/lib/active_record/associations/preloader/association.rb index b0c41c7d5f..e77761692d 100644 --- a/activerecord/lib/active_record/associations/preloader/association.rb +++ b/activerecord/lib/active_record/associations/preloader/association.rb @@ -17,8 +17,14 @@ module ActiveRecord end def run(preloader) - associated_records_by_owner(preloader) do |owner, records| - associate_records_to_owner(owner, records) + records = load_records do |record| + owner = owners_by_key[convert_key(record[association_key_name])] + association = owner.association(reflection.name) + association.set_inverse_instance(record) + end + + owners.each do |owner| + associate_records_to_owner(owner, records[convert_key(owner[owner_key_name])] || []) end end @@ -33,18 +39,6 @@ module ActiveRecord reflection.join_foreign_key end - def associated_records_by_owner(preloader) - records = load_records do |record| - owner = owners_by_key[convert_key(record[association_key_name])] - association = owner.association(reflection.name) - association.set_inverse_instance(record) - end - - owners.each_with_object({}) do |owner, result| - yield(owner, records[convert_key(owner[owner_key_name])] || []) - end - end - def associate_records_to_owner(owner, records) raise NotImplementedError end diff --git a/activerecord/lib/active_record/associations/preloader/through_association.rb b/activerecord/lib/active_record/associations/preloader/through_association.rb index 1bb7e98231..b1813ba66b 100644 --- a/activerecord/lib/active_record/associations/preloader/through_association.rb +++ b/activerecord/lib/active_record/associations/preloader/through_association.rb @@ -12,7 +12,7 @@ module ActiveRecord reflection.source_reflection end - def associated_records_by_owner(preloader) + def run(preloader) already_loaded = owners.first.association(through_reflection.name).loaded? through_scope = through_scope() reflection_scope = target_reflection_scope @@ -43,7 +43,7 @@ module ActiveRecord result.sort_by! { |rhs| preload_index[rhs] } if reflection_scope.order_values.any? result.uniq! if reflection_scope.distinct_value end - yield(owner, result) + associate_records_to_owner(owner, result) end end -- cgit v1.2.3 From 0c2cb880e34c943275758ca6a6ff84afa7a29fba Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Wed, 8 Nov 2017 20:08:19 +0900 Subject: Don't expose internal methods in `Preloader::ThroughAssociation` `through_reflection` and `source_reflection` are used only in the class. --- .../associations/preloader/through_association.rb | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) (limited to 'activerecord/lib/active_record/associations') diff --git a/activerecord/lib/active_record/associations/preloader/through_association.rb b/activerecord/lib/active_record/associations/preloader/through_association.rb index b1813ba66b..762275fbad 100644 --- a/activerecord/lib/active_record/associations/preloader/through_association.rb +++ b/activerecord/lib/active_record/associations/preloader/through_association.rb @@ -4,14 +4,6 @@ module ActiveRecord module Associations class Preloader module ThroughAssociation #:nodoc: - def through_reflection - reflection.through_reflection - end - - def source_reflection - reflection.source_reflection - end - def run(preloader) already_loaded = owners.first.association(through_reflection.name).loaded? through_scope = through_scope() @@ -48,6 +40,13 @@ module ActiveRecord end private + def through_reflection + reflection.through_reflection + end + + def source_reflection + reflection.source_reflection + end def preload_index @preload_index ||= @preloaded_records.each_with_object({}).with_index do |(id, result), index| -- cgit v1.2.3 From 65aa0b7e3448f849a91b6f510b78d4303ff44dbc Mon Sep 17 00:00:00 2001 From: Ryuta Kamizono Date: Wed, 8 Nov 2017 20:28:52 +0900 Subject: Don't expose accessors which are internal used only --- activerecord/lib/active_record/associations/preloader.rb | 5 +++-- activerecord/lib/active_record/associations/preloader/association.rb | 4 +++- 2 files changed, 6 insertions(+), 3 deletions(-) (limited to 'activerecord/lib/active_record/associations') diff --git a/activerecord/lib/active_record/associations/preloader.rb b/activerecord/lib/active_record/associations/preloader.rb index e1754d4a19..5a93a89d0a 100644 --- a/activerecord/lib/active_record/associations/preloader.rb +++ b/activerecord/lib/active_record/associations/preloader.rb @@ -166,8 +166,6 @@ module ActiveRecord end class AlreadyLoaded # :nodoc: - attr_reader :owners, :reflection - def initialize(klass, owners, reflection, preload_scope) @owners = owners @reflection = reflection @@ -178,6 +176,9 @@ module ActiveRecord def preloaded_records owners.flat_map { |owner| owner.association(reflection.name).target } end + + protected + attr_reader :owners, :reflection end # Returns a class containing the logic needed to load preload the data diff --git a/activerecord/lib/active_record/associations/preloader/association.rb b/activerecord/lib/active_record/associations/preloader/association.rb index e77761692d..19c337dc39 100644 --- a/activerecord/lib/active_record/associations/preloader/association.rb +++ b/activerecord/lib/active_record/associations/preloader/association.rb @@ -4,7 +4,6 @@ module ActiveRecord module Associations class Preloader class Association #:nodoc: - attr_reader :owners, :reflection, :preload_scope, :model, :klass attr_reader :preloaded_records def initialize(klass, owners, reflection, preload_scope) @@ -28,6 +27,9 @@ module ActiveRecord end end + protected + attr_reader :owners, :reflection, :preload_scope, :model, :klass + private # The name of the key on the associated records def association_key_name -- cgit v1.2.3