Merge pull request #28276 from rails/pkwip

Various refactoring to association scopes and joins

3 files changed, 78 insertions, 46 deletions

diff --git a/activerecord/lib/active_record/associations/association_scope.rb b/activerecord/lib/active_record/associations/association_scope.rb
index badde9973f..120d75416c 100644
--- a/activerecord/lib/active_record/associations/association_scope.rb
+++ b/activerecord/lib/active_record/associations/association_scope.rb
@@ -25,7 +25,7 @@ module ActiveRecord
         chain_head, chain_tail = get_chain(reflection, association, alias_tracker)
 
         scope.extending! Array(reflection.options[:extend])
-        add_constraints(scope, owner, klass, reflection, chain_head, chain_tail)
+        add_constraints(scope, owner, reflection, chain_head, chain_tail)
       end
 
       def join_type
@@ -60,8 +60,8 @@ module ActiveRecord
           table.create_join(table, table.create_on(constraint), join_type)
         end
 
-        def last_chain_scope(scope, table, reflection, owner, association_klass)
-          join_keys = reflection.join_keys(association_klass)
+        def last_chain_scope(scope, table, reflection, owner)
+          join_keys = reflection.join_keys
           key = join_keys.key
           foreign_key = join_keys.foreign_key
 
@@ -80,8 +80,8 @@ module ActiveRecord
           value_transformation.call(value)
         end
 
-        def next_chain_scope(scope, table, reflection, association_klass, foreign_table, next_reflection)
-          join_keys = reflection.join_keys(association_klass)
+        def next_chain_scope(scope, table, reflection, foreign_table, next_reflection)
+          join_keys = reflection.join_keys
           key = join_keys.key
           foreign_key = join_keys.foreign_key
 
@@ -120,10 +120,10 @@ module ActiveRecord
           [runtime_reflection, previous_reflection]
         end
 
-        def add_constraints(scope, owner, association_klass, refl, chain_head, chain_tail)
+        def add_constraints(scope, owner, refl, chain_head, chain_tail)
           owner_reflection = chain_tail
           table = owner_reflection.alias_name
-          scope = last_chain_scope(scope, table, owner_reflection, owner, association_klass)
+          scope = last_chain_scope(scope, table, owner_reflection, owner)
 
           reflection = chain_head
           while reflection
@@ -132,7 +132,7 @@ module ActiveRecord
 
             unless reflection == chain_tail
               foreign_table = next_reflection.alias_name
-              scope = next_chain_scope(scope, table, reflection, association_klass, foreign_table, next_reflection)
+              scope = next_chain_scope(scope, table, reflection, foreign_table, next_reflection)
             end
 
             # Exclude the scope of the association itself, because that
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 935777d485..97cfec0302 100644
--- a/activerecord/lib/active_record/associations/join_dependency/join_association.rb
+++ b/activerecord/lib/active_record/associations/join_dependency/join_association.rb
@@ -34,35 +34,16 @@ module ActiveRecord
             table = tables.shift
             klass = reflection.klass
 
-            join_keys   = reflection.join_keys(klass)
+            join_keys   = reflection.join_keys
             key         = join_keys.key
             foreign_key = join_keys.foreign_key
 
             constraint = build_constraint(klass, table, key, foreign_table, foreign_key)
 
             predicate_builder = PredicateBuilder.new(TableMetadata.new(klass, table))
-            scope_chain_items = reflection.scopes.map do |item|
-              if item.is_a?(Relation)
-                item
-              else
-                ActiveRecord::Relation.create(klass, table, predicate_builder)
-                  .instance_exec(&item)
-              end
-            end
+            scope_chain_items = reflection.join_scopes(table, predicate_builder)
+            klass_scope       = reflection.klass_join_scope(table, predicate_builder)
 
-            klass_scope =
-              if klass.current_scope
-                klass.current_scope.clone.tap { |scope|
-                  scope.joins_values = []
-                }
-              else
-                relation = ActiveRecord::Relation.create(
-                  klass,
-                  table,
-                  predicate_builder,
-                )
-                klass.send(:build_default_scope, relation)
-              end
             scope_chain_items.concat [klass_scope].compact
 
             rel = scope_chain_items.inject(scope_chain_items.shift) do |left, right|
diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb
index 61a2279292..24ca8b0be4 100644
--- a/activerecord/lib/active_record/reflection.rb
+++ b/activerecord/lib/active_record/reflection.rb
@@ -172,8 +172,8 @@ module ActiveRecord
 
       JoinKeys = Struct.new(:key, :foreign_key) # :nodoc:
 
-      def join_keys(association_klass)
-        JoinKeys.new(foreign_key, active_record_primary_key)
+      def join_keys
+        get_join_keys klass
       end
 
       # Returns a list of scopes that should be applied for this Reflection
@@ -187,6 +187,30 @@ module ActiveRecord
       end
       deprecate :scope_chain
 
+      def join_scopes(table, predicate_builder) # :nodoc:
+        if scope
+          [ActiveRecord::Relation.create(klass, table, predicate_builder)
+            .instance_exec(&scope)]
+        else
+          []
+        end
+      end
+
+      def klass_join_scope(table, predicate_builder) # :nodoc:
+        if klass.current_scope
+          klass.current_scope.clone.tap { |scope|
+            scope.joins_values = []
+          }
+        else
+          relation = ActiveRecord::Relation.create(
+            klass,
+            table,
+            predicate_builder,
+          )
+          klass.send(:build_default_scope, relation)
+        end
+      end
+
       def constraints
         chain.map(&:scopes).flatten
       end
@@ -260,6 +284,20 @@ module ActiveRecord
       def chain
         collect_join_chain
       end
+
+      def get_join_keys(association_klass)
+        JoinKeys.new(join_pk(association_klass), join_fk)
+      end
+
+      private
+
+        def join_pk(_)
+          foreign_key
+        end
+
+        def join_fk
+          active_record_primary_key
+        end
     end
 
     # Base class for AggregateReflection and AssociationReflection. Objects of
@@ -687,11 +725,6 @@ module ActiveRecord
         end
       end
 
-      def join_keys(association_klass)
-        key = polymorphic? ? association_primary_key(association_klass) : association_primary_key
-        JoinKeys.new(key, foreign_key)
-      end
-
       def join_id_for(owner) # :nodoc:
         owner[foreign_key]
       end
@@ -701,6 +734,14 @@ module ActiveRecord
         def calculate_constructable(macro, options)
           !polymorphic?
         end
+
+        def join_fk
+          foreign_key
+        end
+
+        def join_pk(klass)
+          polymorphic? ? association_primary_key(klass) : association_primary_key
+        end
     end
 
     class HasAndBelongsToManyReflection < AssociationReflection # :nodoc:
@@ -720,7 +761,7 @@ module ActiveRecord
     class ThroughReflection < AbstractReflection #:nodoc:
       attr_reader :delegate_reflection
       delegate :foreign_key, :foreign_type, :association_foreign_key,
-               :active_record_primary_key, :type, to: :source_reflection
+               :active_record_primary_key, :type, :get_join_keys, to: :source_reflection
 
       def initialize(delegate_reflection)
         @delegate_reflection = delegate_reflection
@@ -806,6 +847,10 @@ module ActiveRecord
         source_reflection.scopes + super
       end
 
+      def join_scopes(table, predicate_builder) # :nodoc:
+        source_reflection.join_scopes(table, predicate_builder) + super
+      end
+
       def source_type_scope
         through_reflection.klass.where(foreign_type => options[:source_type])
       end
@@ -816,10 +861,6 @@ module ActiveRecord
           through_reflection.has_scope?
       end
 
-      def join_keys(association_klass)
-        source_reflection.join_keys(association_klass)
-      end
-
       # A through association is nested if there would be more than one join table
       def nested?
         source_reflection.through_reflection? || through_reflection.through_reflection?
@@ -954,6 +995,7 @@ module ActiveRecord
       end
 
       private
+
         def actual_source_reflection # FIXME: this is a horrible name
           source_reflection.send(:actual_source_reflection)
         end
@@ -990,6 +1032,15 @@ module ActiveRecord
         end
       end
 
+      def join_scopes(table, predicate_builder) # :nodoc:
+        scopes = @previous_reflection.join_scopes(table, predicate_builder) + super
+        if @previous_reflection.options[:source_type]
+          scopes + [@previous_reflection.source_type_scope]
+        else
+          scopes
+        end
+      end
+
       def klass
         @reflection.klass
       end
@@ -1006,10 +1057,6 @@ module ActiveRecord
         @reflection.plural_name
       end
 
-      def join_keys(association_klass)
-        @reflection.join_keys(association_klass)
-      end
-
       def type
         @reflection.type
       end
@@ -1023,6 +1070,10 @@ module ActiveRecord
         source_type = @previous_reflection.options[:source_type]
         lambda { |object| where(type => source_type) }
       end
+
+      def get_join_keys(association_klass)
+        @reflection.get_join_keys(association_klass)
+      end
     end
 
     class RuntimeReflection < PolymorphicReflection # :nodoc: