Merge pull request rails#29413 from kamipo/fix_association_with_scope…

…_including_joins

Fix association with scope including joins

activerecord/CHANGELOG.md

@@ -1,4 +1,11 @@
 ## Rails 5.1.6 (March 29, 2018) ##
+*   Fix eager loading/preloading association with scope including joins.
+
+    Fixes #28324.
+
+    *Ryuta Kamizono*
+
+*   Fix transactions to apply state to child transactions
 
 *   MySQL: Support mysql2 0.5.x.
 

activerecord/lib/active_record/associations/join_dependency/join_association.rb

@@ -34,29 +34,19 @@ def join_constraints(foreign_table, foreign_klass, join_type, tables, chain)
             table = tables.shift
             klass = reflection.klass
 
-            join_keys   = reflection.join_keys
-            key         = join_keys.key
-            foreign_key = join_keys.foreign_key
+            constraint = reflection.build_join_constraint(table, foreign_table)
 
-            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
+            joins << table.create_join(table, table.create_on(constraint), join_type)
 
-            if reflection.type
-              value = foreign_klass.base_class.name
-              column = klass.columns_hash[reflection.type.to_s]
+            join_scope = reflection.join_scope(table, foreign_klass)
 
-              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)
+            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
 
-            joins << table.create_join(table, table.create_on(constraint), join_type)
-
             # The current table in this iteration becomes the foreign table in the next
             foreign_table, foreign_klass = table, klass
           end

activerecord/lib/active_record/associations/preloader.rb

@@ -54,8 +54,6 @@ class Preloader #:nodoc:
         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 @@ class Preloader #:nodoc:
       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?
           []

activerecord/lib/active_record/associations/preloader/association.rb

@@ -11,7 +11,6 @@ def initialize(klass, owners, reflection, preload_scope)
           @reflection    = reflection
           @preload_scope = preload_scope
           @model         = owners.first && owners.first.class
-          @scope         = nil
           @preloaded_records = []
         end
 
@@ -23,29 +22,11 @@ def preload(preloader)
           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
@@ -119,26 +100,34 @@ def load_records(&block)
             # 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.scope_for_association
+            scope = klass.default_scoped
 
             if reflection.type
               scope.where!(reflection.type => model.base_class.sti_name)
             end
 
             scope.merge!(reflection_scope)
-            scope.merge!(preload_scope) if preload_scope != NULL_RELATION
+            scope.merge!(preload_scope) if preload_scope
             scope
           end
       end

activerecord/lib/active_record/reflection.rb

@@ -188,12 +188,29 @@ def scope_chain
       end
       deprecate :scope_chain
 
-      def join_scope(table)
+      def build_join_constraint(table, foreign_table)
+        key         = join_keys.key
+        foreign_key = join_keys.foreign_key
+
+        constraint = table[key].eq(foreign_table[foreign_key])
+
+        if klass.finder_needs_type_condition?
+          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)
 
-        scope_chain_items.inject(klass_scope || scope_chain_items.shift, &:merge!)
+        if type
+          klass_scope.where!(type => foreign_klass.base_class.sti_name)
+        end
+
+        scope_chain_items.inject(klass_scope, &:merge!)
       end
 
       def join_scopes(table, predicate_builder) # :nodoc:

activerecord/test/cases/associations/eager_test.rb

@@ -76,6 +76,7 @@ def test_loading_conditions_with_or
 
   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