Skip to content

Commit

Permalink
Refactor Preloader to remove AlreadyLoaded class
Browse files Browse the repository at this point in the history
Part 2 of ?

It felt strange to have an `AlreadyLoaded` class when we have 2 classes
for the preloaders. `ThroughAssociation` already super's to
`Association`. Instead of having a new class that provides a confusing
indirection, we can get and set all the information we need on
`Association` inside of `run`.

In addition, the `already_loaded` instance variable on
`ThroughAssociation` actually meant something different than
`already_loaded` that was checked for `AlreadyLoaded`. By pushing this
check down we can get rid of the `initialize` in `ThroughAssociation`
entirely since then all it's doing is calling `super`.

Co-authored-by: John Crepezzi <john.crepezzi@gmail.com>
  • Loading branch information
eileencodes and seejohnrun committed Dec 14, 2020
1 parent 0ecf163 commit 93f37d5
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 35 deletions.
31 changes: 2 additions & 29 deletions activerecord/lib/active_record/associations/preloader.rb
Expand Up @@ -135,7 +135,7 @@ def build_child_preloader(reflection, child, loaders)

def preloaders_for_reflection(reflection, reflection_records)
reflection_records.group_by { |record| record.association(reflection.name).klass }.map do |rhs_klass, rs|
preloader_for(reflection, rs).new(rhs_klass, rs, reflection, scope, associate_by_default).run
preloader_for(reflection).new(rhs_klass, rs, reflection, scope, associate_by_default).run
end
end

Expand All @@ -149,37 +149,10 @@ def grouped_records(association)
h
end

class AlreadyLoaded # :nodoc:
def initialize(klass, owners, reflection, preload_scope, associate_by_default = true)
@owners = owners
@reflection = reflection
end

def run
self
end

def preloaded_records
@preloaded_records ||= records_by_owner.flat_map(&:last)
end

def records_by_owner
@records_by_owner ||= owners.index_with do |owner|
Array(owner.association(reflection.name).target)
end
end

private
attr_reader :owners, :reflection
end

# Returns a class containing the logic needed to load preload the data
# and attach it to a relation. The class returned implements a `run` method
# that accepts a preloader.
def preloader_for(reflection, owners)
if owners.all? { |o| o.association(reflection.name).loaded? }
return AlreadyLoaded
end
def preloader_for(reflection)
reflection.check_preloadable!

if reflection.options[:through]
Expand Down
Expand Up @@ -11,9 +11,16 @@ def initialize(klass, owners, reflection, preload_scope, associate_by_default =
@preload_scope = preload_scope
@associate = associate_by_default || !preload_scope || preload_scope.empty_scope?
@model = owners.first && owners.first.class

@already_loaded = owners.all? { |o| o.association(reflection.name).loaded? }
end

def run
if @already_loaded
fetch_from_preloaded_records
return self
end

records = records_by_owner

owners.each do |owner|
Expand All @@ -38,6 +45,14 @@ def preloaded_records
private
attr_reader :owners, :reflection, :preload_scope, :model, :klass

def fetch_from_preloaded_records
@records_by_owner = owners.index_with do |owner|
Array(owner.association(reflection.name).target)
end

@preloaded_records = records_by_owner.flat_map(&:last)
end

def load_records
# owners can be duplicated when a relation has a collection association join
# #compare_by_identity makes such owners different hash keys
Expand Down
Expand Up @@ -4,11 +4,6 @@ module ActiveRecord
module Associations
class Preloader
class ThroughAssociation < Association # :nodoc:
def initialize(*)
super
@already_loaded = owners.first.association(through_reflection.name).loaded?
end

def preloaded_records
@preloaded_records ||= source_preloaders.flat_map(&:preloaded_records)
end
Expand All @@ -21,7 +16,7 @@ def records_by_owner
@records_by_owner = owners.each_with_object({}) do |owner, result|
through_records = through_records_by_owner[owner] || []

if @already_loaded
if owners.first.association(through_reflection.name).loaded?
if source_type = reflection.options[:source_type]
through_records = through_records.select do |record|
record[reflection.foreign_type] == source_type
Expand Down

0 comments on commit 93f37d5

Please sign in to comment.