Merge pull request #29589 from kamipo/refactor_join_scope

Refactor join dependency to move building constraints to `join_scope` in `Reflection`

3 files changed, 21 insertions, 54 deletions

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 95f16c622e..005410d598 100644
--- a/activerecord/lib/active_record/associations/join_dependency/join_association.rb
+++ b/activerecord/lib/active_record/associations/join_dependency/join_association.rb
@@ -34,26 +34,10 @@ module ActiveRecord
             table = tables.shift
             klass = reflection.klass
 
-            join_keys   = reflection.join_keys
-            key         = join_keys.key
-            foreign_key = join_keys.foreign_key
+            join_scope = reflection.join_scope(table, foreign_table, foreign_klass)
 
-            constraint = build_constraint(klass, table, key, foreign_table, foreign_key)
-
-            rel = reflection.join_scope(table)
-
-            if rel && !rel.arel.constraints.empty?
-              binds += rel.bound_attributes
-              constraint = constraint.and rel.arel.constraints
-            end
-
-            if reflection.type
-              value = foreign_klass.base_class.name
-              column = klass.columns_hash[reflection.type.to_s]
-
-              binds << Relation::QueryAttribute.new(column.name, value, klass.type_for_attribute(column.name))
-              constraint = constraint.and klass.arel_attribute(reflection.type, table).eq(Arel::Nodes::BindParam.new)
-            end
+            binds.concat join_scope.bound_attributes
+            constraint = join_scope.arel.constraints
 
             joins << table.create_join(table, table.create_on(constraint), join_type)
 
@@ -64,34 +48,6 @@ module ActiveRecord
           JoinInformation.new joins, binds
         end
 
-        #  Builds equality condition.
-        #
-        #  Example:
-        #
-        #  class Physician < ActiveRecord::Base
-        #    has_many :appointments
-        #  end
-        #
-        #  If I execute `Physician.joins(:appointments).to_a` then
-        #    klass         # => Physician
-        #    table         # => #<Arel::Table @name="appointments" ...>
-        #    key           # =>  physician_id
-        #    foreign_table # => #<Arel::Table @name="physicians" ...>
-        #    foreign_key   # => id
-        #
-        def build_constraint(klass, table, key, foreign_table, foreign_key)
-          constraint = table[key].eq(foreign_table[foreign_key])
-
-          if klass.finder_needs_type_condition?
-            constraint = table.create_and([
-              constraint,
-              klass.send(:type_condition, table)
-            ])
-          end
-
-          constraint
-        end
-
         def table
           tables.first
         end
diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb
index c6aa2ed720..3d3ec862a3 100644
--- a/activerecord/lib/active_record/reflection.rb
+++ b/activerecord/lib/active_record/reflection.rb
@@ -171,7 +171,7 @@ module ActiveRecord
       JoinKeys = Struct.new(:key, :foreign_key) # :nodoc:
 
       def join_keys
-        get_join_keys klass
+        @join_keys ||= get_join_keys(klass)
       end
 
       # Returns a list of scopes that should be applied for this Reflection
@@ -185,12 +185,25 @@ module ActiveRecord
       end
       deprecate :scope_chain
 
-      def join_scope(table)
+      def join_scope(table, foreign_table, foreign_klass)
         predicate_builder = predicate_builder(table)
         scope_chain_items = join_scopes(table, predicate_builder)
         klass_scope       = klass_join_scope(table, predicate_builder)
 
-        scope_chain_items.inject(klass_scope || scope_chain_items.shift, &:merge!)
+        key         = join_keys.key
+        foreign_key = join_keys.foreign_key
+
+        klass_scope.where!(table[key].eq(foreign_table[foreign_key]))
+
+        if klass.finder_needs_type_condition?
+          klass_scope.where!(klass.send(:type_condition, table))
+        end
+
+        if type
+          klass_scope.where!(type => foreign_klass.base_class.name)
+        end
+
+        scope_chain_items.inject(klass_scope, &:merge!)
       end
 
       def join_scopes(table, predicate_builder) # :nodoc:
@@ -207,8 +220,7 @@ module ActiveRecord
             scope.joins_values = scope.left_outer_joins_values = [].freeze
           }
         else
-          relation = build_scope(table, predicate_builder)
-          klass.send(:build_default_scope, relation)
+          klass.default_scoped(build_scope(table, predicate_builder))
         end
       end
 
diff --git a/activerecord/lib/active_record/scoping/named.rb b/activerecord/lib/active_record/scoping/named.rb
index a61fdd6454..388f471bf5 100644
--- a/activerecord/lib/active_record/scoping/named.rb
+++ b/activerecord/lib/active_record/scoping/named.rb
@@ -29,8 +29,7 @@ module ActiveRecord
           end
         end
 
-        def default_scoped # :nodoc:
-          scope = relation
+        def default_scoped(scope = relation) # :nodoc:
           build_default_scope(scope) || scope
         end