Refactor to lazy construct join table aliases

Early constructed join table aliases are not always used for eager loading, and it is required refactoring to improve eager loading duplicated through associations.
rails · May 25, 2020 · 590b045 · 590b045
1 parent c179a06
commit 590b045
Show file tree

Hide file tree

Showing 4 changed files with 14 additions and 29 deletions.
diff --git a/activerecord/lib/active_record/associations/alias_tracker.rb b/activerecord/lib/active_record/associations/alias_tracker.rb
@@ -6,9 +6,14 @@ module ActiveRecord
   module Associations
     # Keeps track of table aliases for ActiveRecord::Associations::JoinDependency
     class AliasTracker # :nodoc:
-      def self.create(connection, initial_table, joins)
+      def self.create(connection, initial_table, joins, aliases = nil)
         if joins.empty?
-          aliases = Hash.new(0)
+          aliases ||= Hash.new(0)
+        elsif aliases
+          default_proc = aliases.default_proc || proc { 0 }
+          aliases.default_proc = proc { |h, k|
+            h[k] = initial_count_for(connection, k, joins) + default_proc.call(h, k)
+          }
         else
           aliases = Hash.new { |h, k|
             h[k] = initial_count_for(connection, k, joins)
@@ -32,8 +37,6 @@ def self.initial_count_for(connection, name, table_joins)
             ).size
           elsif join.is_a?(Arel::Nodes::Join)
             join.left.name == name ? 1 : 0
-          elsif join.is_a?(Hash)
-            join[name]
           else
             raise ArgumentError, "joins list should be initialized by list of Arel::Nodes::Join"
           end

diff --git a/activerecord/lib/active_record/associations/join_dependency.rb b/activerecord/lib/active_record/associations/join_dependency.rb
@@ -81,11 +81,9 @@ def reflections
       def join_constraints(joins_to_add, alias_tracker)
         @alias_tracker = alias_tracker
 
-        construct_tables!(join_root)
         joins = make_join_constraints(join_root, join_type)
 
         joins.concat joins_to_add.flat_map { |oj|
-          construct_tables!(oj.join_root)
           if join_root.match? oj.join_root
             walk(join_root, oj.join_root, oj.join_type)
           else
@@ -157,12 +155,6 @@ def aliases
           }
         end
 
-        def construct_tables!(join_root)
-          join_root.each_children do |parent, child|
-            child.tables = table_aliases_for(parent, child)
-          end
-        end
-
         def make_join_constraints(join_root, join_type)
           join_root.children.flat_map do |child|
             make_constraints(join_root, child, join_type)
@@ -172,18 +164,13 @@ def make_join_constraints(join_root, join_type)
         def make_constraints(parent, child, join_type)
           foreign_table = parent.table
           foreign_klass = parent.base_klass
-          joins = child.join_constraints(foreign_table, foreign_klass, join_type, alias_tracker)
-          joins.concat child.children.flat_map { |c| make_constraints(child, c, join_type) }
-        end
-
-        def table_aliases_for(parent, node)
-          node.reflection.chain.map { |reflection|
+          child.join_constraints(foreign_table, foreign_klass, join_type, alias_tracker) do |reflection|
             alias_tracker.aliased_table_for(
               reflection.table_name,
-              table_alias_for(reflection, parent, reflection != node.reflection),
+              table_alias_for(reflection, parent, reflection != child.reflection),
               reflection.klass.type_caster
             )
-          }
+          end.concat child.children.flat_map { |c| make_constraints(child, c, join_type) }
         end
 
         def table_alias_for(reflection, parent, join)

diff --git a/activerecord/lib/active_record/associations/join_dependency/join_association.rb b/activerecord/lib/active_record/associations/join_dependency/join_association.rb
@@ -14,16 +14,17 @@ def initialize(reflection, children)
           super(reflection.klass, children)
 
           @reflection = reflection
-          @tables     = nil
         end
 
         def match?(other)
           return true if self == other
           super && reflection == other.reflection
         end
 
-        def join_constraints(foreign_table, foreign_klass, join_type, alias_tracker)
+        def join_constraints(foreign_table, foreign_klass, join_type, alias_tracker, &block)
           joins = []
+          tables = reflection.chain.map(&block)
+          @table = tables.first
 
           # The chain starts with the target table, but we want to end with it here (makes
           # more sense in this context), so we reverse
@@ -63,11 +64,6 @@ def join_constraints(foreign_table, foreign_klass, join_type, alias_tracker)
           joins
         end
 
-        def tables=(tables)
-          @tables = tables
-          @table  = tables.first
-        end
-
         def readonly?
           return @readonly if defined?(@readonly)
 

diff --git a/activerecord/lib/active_record/relation.rb b/activerecord/lib/active_record/relation.rb
@@ -749,8 +749,7 @@ def has_limit_or_offset? # :nodoc:
     end
 
     def alias_tracker(joins = [], aliases = nil) # :nodoc:
-      joins += [aliases] if aliases
-      ActiveRecord::Associations::AliasTracker.create(connection, table.name, joins)
+      ActiveRecord::Associations::AliasTracker.create(connection, table.name, joins, aliases)
     end
 
     class StrictLoadingScope