hm:t preloading will respect order set on the RHS association

6 files changed, 68 insertions, 8 deletions

diff --git a/activerecord/lib/active_record/associations/preloader.rb b/activerecord/lib/active_record/associations/preloader.rb
index 4ef361fccf..3add378386 100644
--- a/activerecord/lib/active_record/associations/preloader.rb
+++ b/activerecord/lib/active_record/associations/preloader.rb
@@ -127,7 +127,7 @@ module ActiveRecord
 
         loaders = preloaders_for_one parent, records
 
-        recs = loaders.flat_map(&:target_records).uniq
+        recs = loaders.flat_map(&:preloaded_records).uniq
         loaders.concat Array.wrap(child).flat_map { |assoc|
           preloaders_on assoc, recs
         }
diff --git a/activerecord/lib/active_record/associations/preloader/association.rb b/activerecord/lib/active_record/associations/preloader/association.rb
index ef48e9f174..6c60d03998 100644
--- a/activerecord/lib/active_record/associations/preloader/association.rb
+++ b/activerecord/lib/active_record/associations/preloader/association.rb
@@ -17,6 +17,7 @@ module ActiveRecord
           @owners_by_key = nil
           @type_caster   = IDENTITY_CASTER
           @associated_records_by_owner = nil
+          @loaded = false
         end
 
         def run
@@ -71,20 +72,36 @@ module ActiveRecord
           reflection.options
         end
 
-        def target_records
+        def preloaded_records1
           associated_records_by_owner.values.flatten
         end
 
+        def preloaded_records
+          if owners.first.association(reflection.name).loaded?
+            []
+          else
+            associated_records_by_owner.values.flatten
+          end
+        end
+
+        def loaded?
+          @loaded
+        end
+
         private
 
         def associated_records_by_owner
+          @loaded = true
+
           return @associated_records_by_owner if @associated_records_by_owner
 
           owners_map = owners_by_key
           owner_keys = owners_map.keys.compact
 
           # Each record may have multiple owners, and vice-versa
-          records_by_owner = Hash[owners.map { |owner| [owner, []] }]
+          records_by_owner = Hash.new do |h,owner|
+            h[owner] = []
+          end
 
           if klass && owner_keys.any?
             # Some databases impose a limit on the number of ids in a list (in Oracle it's 1000)
@@ -104,6 +121,9 @@ module ActiveRecord
               end
             end
           end
+          owners.each_with_object(records_by_owner) do |owner,h|
+            h[owner] ||= []
+          end
 
           @associated_records_by_owner = records_by_owner
         end
diff --git a/activerecord/lib/active_record/associations/preloader/has_and_belongs_to_many.rb b/activerecord/lib/active_record/associations/preloader/has_and_belongs_to_many.rb
index 48ae99819b..fce3cb78e0 100644
--- a/activerecord/lib/active_record/associations/preloader/has_and_belongs_to_many.rb
+++ b/activerecord/lib/active_record/associations/preloader/has_and_belongs_to_many.rb
@@ -35,10 +35,10 @@ module ActiveRecord
         # actual records, ensuring that we don't create more than one instances of the same
         # record
         def associated_records_by_owner
-          return @records_by_owner if @records_by_owner
+          return @associated_records_by_owner if @associated_records_by_owner
 
           records = {}
-          @records_by_owner = super.each_value do |rows|
+          @associated_records_by_owner = super.each_value do |rows|
             rows.map! { |row| records[row[klass.primary_key]] ||= klass.instantiate(row) }
           end
         end
diff --git a/activerecord/lib/active_record/associations/preloader/has_many_through.rb b/activerecord/lib/active_record/associations/preloader/has_many_through.rb
index 157b627ad5..39a938c266 100644
--- a/activerecord/lib/active_record/associations/preloader/has_many_through.rb
+++ b/activerecord/lib/active_record/associations/preloader/has_many_through.rb
@@ -5,13 +5,15 @@ module ActiveRecord
         include ThroughAssociation
 
         def associated_records_by_owner
+          return @associated_records_by_owner if @associated_records_by_owner
+
           records_by_owner = super
 
           if reflection_scope.distinct_value
             records_by_owner.each_value { |records| records.uniq! }
           end
 
-          records_by_owner
+          @associated_records_by_owner = records_by_owner
         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 eaceb6c3d9..a62815830c 100644
--- a/activerecord/lib/active_record/associations/preloader/through_association.rb
+++ b/activerecord/lib/active_record/associations/preloader/through_association.rb
@@ -12,6 +12,10 @@ module ActiveRecord
         end
 
         def associated_records_by_owner
+          @loaded = true
+
+          return @associated_records_by_owner if @associated_records_by_owner
+
           left_loader = Preloader.new(owners,
                                       through_reflection.name,
                                       through_scope)
@@ -36,12 +40,31 @@ module ActiveRecord
           preloader = Preloader.new(middle_records,
                                     source_reflection.name,
                                     reflection_scope)
+
           preloader.run
 
-          through_records.each_with_object({}) { |(lhs,middles,assoc),h|
-            h[lhs] = middles.flat_map { |r|
+          middle_to_pl = preloader.preloaders.each_with_object({}) do |pl,h|
+            pl.owners.each { |middle|
+              h[middle] = pl
+            }
+          end
+
+          @associated_records_by_owner = through_records.each_with_object({}) { |(lhs,middles,assoc),h|
+            x = middle_to_pl[middles.first]
+
+            rhs_records = middles.flat_map { |r|
               r.send(source_reflection.name)
             }.compact
+
+            if x && x.loaded?
+              rs = rhs_records.sort_by { |rhs|
+                x.preloaded_records1.index(rhs)
+              }
+            else
+              rs = rhs_records
+            end
+
+            h[lhs] = rs
           }
         end
 
diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb
index dd8b426b25..99c71a1dd9 100644
--- a/activerecord/test/cases/associations/has_many_through_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -41,6 +41,21 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
     Class.new(ActiveRecord::Base) { define_singleton_method(:name) { name } }
   end
 
+  def test_ordered_habtm
+    person_prime = Class.new(ActiveRecord::Base) do
+      def self.name; 'Person'; end
+
+      has_many :readers
+      has_many :posts, -> { order('posts.id DESC') }, :through => :readers
+    end
+    posts = person_prime.includes(:posts).first.posts
+
+    assert_operator posts.length, :>, 1
+    posts.each_cons(2) do |left,right|
+      assert_operator left.id, :>, right.id
+    end
+  end
+
   def test_singleton_has_many_through
     book         = make_model "Book"
     subscription = make_model "Subscription"