Merge pull request #29413 from kamipo/fix_association_with_scope_including_joins

Fix association with scope including joins

9 files changed, 60 insertions, 70 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 7404c1bd8f..8d900d9669 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,9 @@
+*   Fix eager loading/preloading association with scope including joins.
+
+    Fixes #28324.
+
+    *Ryuta Kamizono*
+
 *   Fix transactions to apply state to child transactions
 
     Previously if you had a nested transaction and the outer transaction was rolledback the record from the
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 d0c1848079..b14ddfeeeb 100644
--- a/activerecord/lib/active_record/associations/join_dependency/join_association.rb
+++ b/activerecord/lib/active_record/associations/join_dependency/join_association.rb
@@ -34,13 +34,19 @@ module ActiveRecord
             table = tables.shift
             klass = reflection.klass
 
-            join_scope = reflection.join_scope(table, foreign_table, foreign_klass)
-
-            binds.concat join_scope.bound_attributes
-            constraint = join_scope.arel.constraints
+            constraint = reflection.build_join_constraint(table, foreign_table)
 
             joins << table.create_join(table, table.create_on(constraint), join_type)
 
+            join_scope = reflection.join_scope(table, foreign_klass)
+
+            if join_scope.arel.constraints.any?
+              binds.concat join_scope.bound_attributes
+              joins.concat join_scope.arel.join_sources
+              right = joins.last.right
+              right.expr = right.expr.and(join_scope.arel.constraints)
+            end
+
             # The current table in this iteration becomes the foreign table in the next
             foreign_table, foreign_klass = table, klass
           end
diff --git a/activerecord/lib/active_record/associations/preloader.rb b/activerecord/lib/active_record/associations/preloader.rb
index 208d1b2670..a18994cec4 100644
--- a/activerecord/lib/active_record/associations/preloader.rb
+++ b/activerecord/lib/active_record/associations/preloader.rb
@@ -54,8 +54,6 @@ module ActiveRecord
         autoload :BelongsTo,           "active_record/associations/preloader/belongs_to"
       end
 
-      NULL_RELATION = Struct.new(:values, :where_clause, :joins_values).new({}, Relation::WhereClause.empty, [])
-
       # Eager loads the named associations for the given Active Record record(s).
       #
       # In this description, 'association name' shall refer to the name passed
@@ -93,7 +91,6 @@ module ActiveRecord
       def preload(records, associations, preload_scope = nil)
         records       = Array.wrap(records).compact.uniq
         associations  = Array.wrap(associations)
-        preload_scope = preload_scope || NULL_RELATION
 
         if records.empty?
           []
diff --git a/activerecord/lib/active_record/associations/preloader/association.rb b/activerecord/lib/active_record/associations/preloader/association.rb
index 63ef3f2d8c..85343040db 100644
--- a/activerecord/lib/active_record/associations/preloader/association.rb
+++ b/activerecord/lib/active_record/associations/preloader/association.rb
@@ -11,7 +11,6 @@ module ActiveRecord
           @reflection    = reflection
           @preload_scope = preload_scope
           @model         = owners.first && owners.first.class
-          @scope         = nil
           @preloaded_records = []
         end
 
@@ -23,29 +22,11 @@ module ActiveRecord
           raise NotImplementedError
         end
 
-        def scope
-          @scope ||= build_scope
-        end
-
-        def records_for(ids)
-          scope.where(association_key_name => ids)
-        end
-
-        def table
-          klass.arel_table
-        end
-
         # The name of the key on the associated records
         def association_key_name
           raise NotImplementedError
         end
 
-        # This is overridden by HABTM as the condition should be on the foreign_key column in
-        # the join table
-        def association_key
-          klass.arel_attribute(association_key_name, table)
-        end
-
         # The name of the key on the model which declares the association
         def owner_key_name
           raise NotImplementedError
@@ -114,54 +95,35 @@ module ActiveRecord
             # Make several smaller queries if necessary or make one query if the adapter supports it
             slices = owner_keys.each_slice(klass.connection.in_clause_length || owner_keys.size)
             @preloaded_records = slices.flat_map do |slice|
-              records_for(slice).load(&block)
+              records_for(slice, &block)
             end
             @preloaded_records.group_by do |record|
               convert_key(record[association_key_name])
             end
           end
 
+          def records_for(ids, &block)
+            scope.where(association_key_name => ids).load(&block)
+          end
+
+          def scope
+            @scope ||= build_scope
+          end
+
           def reflection_scope
             @reflection_scope ||= reflection.scope_for(klass)
           end
 
           def build_scope
-            scope = klass.unscoped
-
-            values = reflection_scope.values
-            preload_values = preload_scope.values
-
-            scope.where_clause = reflection_scope.where_clause + preload_scope.where_clause
-            scope.references_values = Array(values[:references]) + Array(preload_values[:references])
-
-            if preload_values[:select] || values[:select]
-              scope._select!(preload_values[:select] || values[:select])
-            end
-            scope.includes! preload_values[:includes] || values[:includes]
-            if preload_scope.joins_values.any?
-              scope.joins!(preload_scope.joins_values)
-            else
-              scope.joins!(reflection_scope.joins_values)
-            end
-
-            if order_values = preload_values[:order] || values[:order]
-              scope.order!(order_values)
-            end
-
-            if preload_values[:reordering] || values[:reordering]
-              scope.reordering_value = true
-            end
-
-            if preload_values[:readonly] || values[:readonly]
-              scope.readonly!
-            end
+            scope = klass.default_scoped
 
-            if options[:as]
-              scope.where!(klass.table_name => { reflection.type => model.base_class.sti_name })
+            if reflection.type
+              scope.where!(reflection.type => model.base_class.sti_name)
             end
 
-            scope.unscope_values = Array(values[:unscope]) + Array(preload_values[:unscope])
-            klass.default_scoped.merge(scope)
+            scope.merge!(reflection_scope)
+            scope.merge!(preload_scope) if preload_scope
+            scope
           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 34587fd3f5..0999746cd5 100644
--- a/activerecord/lib/active_record/associations/preloader/through_association.rb
+++ b/activerecord/lib/active_record/associations/preloader/through_association.rb
@@ -79,17 +79,24 @@ module ActiveRecord
 
           def through_scope
             scope = through_reflection.klass.unscoped
+            values = reflection_scope.values
 
             if options[:source_type]
               scope.where! reflection.foreign_type => options[:source_type]
             else
               unless reflection_scope.where_clause.empty?
-                scope.includes_values = Array(reflection_scope.values[:includes] || options[:source])
+                scope.includes_values = Array(values[:includes] || options[:source])
                 scope.where_clause = reflection_scope.where_clause
+                if joins = values[:joins]
+                  scope.joins!(source_reflection.name => joins)
+                end
+                if left_outer_joins = values[:left_outer_joins]
+                  scope.left_outer_joins!(source_reflection.name => left_outer_joins)
+                end
               end
 
-              scope.references! reflection_scope.values[:references]
-              if scope.eager_loading? && order_values = reflection_scope.values[:order]
+              scope.references! values[:references]
+              if scope.eager_loading? && order_values = values[:order]
                 scope = scope.order(order_values)
               end
             end
diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb
index 8ff2f50fdb..a453ca55c7 100644
--- a/activerecord/lib/active_record/reflection.rb
+++ b/activerecord/lib/active_record/reflection.rb
@@ -185,19 +185,23 @@ module ActiveRecord
       end
       deprecate :scope_chain
 
-      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)
-
+      def build_join_constraint(table, foreign_table)
         key         = join_keys.key
         foreign_key = join_keys.foreign_key
 
-        klass_scope.where!(table[key].eq(foreign_table[foreign_key]))
+        constraint = table[key].eq(foreign_table[foreign_key])
 
         if klass.finder_needs_type_condition?
-          klass_scope.where!(klass.send(:type_condition, table))
+          table.create_and([constraint, klass.send(:type_condition, table)])
+        else
+          constraint
         end
+      end
+
+      def join_scope(table, foreign_klass)
+        predicate_builder = predicate_builder(table)
+        scope_chain_items = join_scopes(table, predicate_builder)
+        klass_scope       = klass_join_scope(table, predicate_builder)
 
         if type
           klass_scope.where!(type => foreign_klass.base_class.sti_name)
diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb
index 55b294cfaa..c0bab19e82 100644
--- a/activerecord/test/cases/associations/eager_test.rb
+++ b/activerecord/test/cases/associations/eager_test.rb
@@ -68,6 +68,11 @@ class EagerAssociationTest < ActiveRecord::TestCase
       "expected to find only david's posts"
   end
 
+  def test_loading_with_scope_including_joins
+    assert_equal clubs(:boring_club), Member.preload(:general_club).find(1).general_club
+    assert_equal clubs(:boring_club), Member.eager_load(:general_club).find(1).general_club
+  end
+
   def test_with_ordering
     list = Post.all.merge!(includes: :comments, order: "posts.id DESC").to_a
     [:other_by_mary, :other_by_bob, :misc_by_mary, :misc_by_bob, :eager_other,
diff --git a/activerecord/test/models/club.rb b/activerecord/test/models/club.rb
index 49d7b24a3b..3d441b1d48 100644
--- a/activerecord/test/models/club.rb
+++ b/activerecord/test/models/club.rb
@@ -8,6 +8,8 @@ class Club < ActiveRecord::Base
 
   has_many :favourites, -> { where(memberships: { favourite: true }) }, through: :memberships, source: :member
 
+  scope :general, -> { left_joins(:category).where(categories: { name: "General" }) }
+
   private
 
     def private_method
diff --git a/activerecord/test/models/member.rb b/activerecord/test/models/member.rb
index 36f2461b84..b9597c6b9a 100644
--- a/activerecord/test/models/member.rb
+++ b/activerecord/test/models/member.rb
@@ -22,6 +22,7 @@ class Member < ActiveRecord::Base
   has_many :organization_member_details_2, through: :organization, source: :member_details
 
   has_one :club_category, through: :club, source: :category
+  has_one :general_club, -> { general }, through: :current_membership, source: :club
 
   has_many :current_memberships, -> { where favourite: true }
   has_many :clubs, through: :current_memberships