Merge pull request #18766 from yasyf/issue_17864

Honour joining model order in `has_many :through` associations when
eager loading

4 files changed, 68 insertions, 8 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 39fe217777..cd4b297c8c 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,18 @@
+*   Honour the order of the joining model in a `has_many :through` association when eager loading.
+
+    Example:
+
+    The below will now follow the order of `by_lines` when eager loading `authors`.
+
+        class Article < ActiveRecord::Base
+          has_many :by_lines, -> { order(:position) }
+          has_many :authors, through: :by_lines
+        end
+
+    Fixes #17864.
+
+    *Yasyf Mohamedali*, *Joel Turkel*
+
 *   Ensure that the Suppressor runs before validations.
 
     This moves the suppressor up to be run before validations rather than after
diff --git a/activerecord/lib/active_record/associations/preloader/through_association.rb b/activerecord/lib/active_record/associations/preloader/through_association.rb
index 6c83058202..b0203909ce 100644
--- a/activerecord/lib/active_record/associations/preloader/through_association.rb
+++ b/activerecord/lib/active_record/associations/preloader/through_association.rb
@@ -38,12 +38,7 @@ module ActiveRecord
             }
           end
 
-          record_offset = {}
-          @preloaded_records.each_with_index do |record,i|
-            record_offset[record] = i
-          end
-
-          through_records.each_with_object({}) { |(lhs,center),records_by_owner|
+          through_records.each_with_object({}) do |(lhs,center), records_by_owner|
             pl_to_middle = center.group_by { |record| middle_to_pl[record] }
 
             records_by_owner[lhs] = pl_to_middle.flat_map do |pl, middles|
@@ -53,13 +48,25 @@ module ActiveRecord
                 target_records_from_association(association)
               }.compact
 
-              rhs_records.sort_by { |rhs| record_offset[rhs] }
+              # Respect the order on `reflection_scope` if it exists, else use the natural order.
+              if reflection_scope.values[:order].present?
+                @id_map ||= id_to_index_map @preloaded_records
+                rhs_records.sort_by { |rhs| @id_map[rhs] }
+              else
+                rhs_records
+              end
             end
-          }
+          end
         end
 
         private
 
+        def id_to_index_map(ids)
+          id_map = {}
+          ids.each_with_index { |id, index| id_map[id] = index }
+          id_map
+        end
+
         def reset_association(owners, association_name)
           should_reset = (through_scope != through_reflection.klass.unscoped) ||
              (reflection.options[:source_type] && through_reflection.collection?)
diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb
index ac478cbb01..3ee84fb66c 100644
--- a/activerecord/test/cases/associations/eager_test.rb
+++ b/activerecord/test/cases/associations/eager_test.rb
@@ -749,6 +749,38 @@ class EagerAssociationTest < ActiveRecord::TestCase
     }
   end
 
+  def test_eager_has_many_through_with_order
+    tag = OrderedTag.create(name: 'Foo')
+    post1 = Post.create!(title: 'Beaches', body: "I like beaches!")
+    post2 = Post.create!(title: 'Pools', body: "I like pools!")
+
+    Tagging.create!(taggable_type: 'Post', taggable_id: post1.id, tag: tag)
+    Tagging.create!(taggable_type: 'Post', taggable_id: post2.id, tag: tag)
+
+    tag_with_includes = OrderedTag.includes(:tagged_posts).find(tag.id)
+    assert_equal(tag_with_includes.taggings.map(&:taggable).map(&:title), tag_with_includes.tagged_posts.map(&:title))
+  end
+
+  def test_eager_has_many_through_multiple_with_order
+    tag1 = OrderedTag.create!(name: 'Bar')
+    tag2 = OrderedTag.create!(name: 'Foo')
+
+    post1 = Post.create!(title: 'Beaches', body: "I like beaches!")
+    post2 = Post.create!(title: 'Pools', body: "I like pools!")
+
+    Tagging.create!(taggable: post1, tag: tag1)
+    Tagging.create!(taggable: post2, tag: tag1)
+    Tagging.create!(taggable: post2, tag: tag2)
+    Tagging.create!(taggable: post1, tag: tag2)
+
+    tags_with_includes = OrderedTag.where(id: [tag1, tag2].map(&:id)).includes(:tagged_posts).order(:id).to_a
+    tag1_with_includes = tags_with_includes.first
+    tag2_with_includes = tags_with_includes.last
+
+    assert_equal([post2, post1].map(&:title), tag1_with_includes.tagged_posts.map(&:title))
+    assert_equal([post1, post2].map(&:title), tag2_with_includes.tagged_posts.map(&:title))
+  end
+
   def test_eager_with_default_scope
     developer = EagerDeveloperWithDefaultScope.where(:name => 'David').first
     projects = Project.order(:id).to_a
diff --git a/activerecord/test/models/tag.rb b/activerecord/test/models/tag.rb
index 80d4725f7e..b48b9a2155 100644
--- a/activerecord/test/models/tag.rb
+++ b/activerecord/test/models/tag.rb
@@ -5,3 +5,9 @@ class Tag < ActiveRecord::Base
 
   has_many :tagged_posts, :through => :taggings, :source => 'taggable', :source_type => 'Post'
 end
+
+class OrderedTag < Tag
+  self.table_name = "tags"
+
+  has_many :taggings, -> { order('taggings.id DESC') }, foreign_key: 'tag_id'
+end