Merge pull request #22653 from matthewd/find_array_ordered

ActiveRecord::Base#find(array) returning result in the same order as the array passed

4 files changed, 100 insertions, 2 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 2c5b670469..dc5d2632dc 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,10 @@
+*   Order the result of `find(ids)` to match the passed array, if the relation
+    has no explicit order defined.
+
+    Fixes #20338.
+
+    *Miguel Grazziotin*, *Matthew Draper*
+
 *   Omit default limit values in dumped schema. It's tidier, and if the defaults
     change in the future, we can address that via Migration API Versioning.
 
diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb
index 435cef901b..19244bcf95 100644
--- a/activerecord/lib/active_record/relation/finder_methods.rb
+++ b/activerecord/lib/active_record/relation/finder_methods.rb
@@ -442,6 +442,8 @@ module ActiveRecord
     end
 
     def find_some(ids)
+      return find_some_ordered(ids) unless order_values.present?
+
       result = where(primary_key => ids).to_a
 
       expected_size =
@@ -463,6 +465,21 @@ module ActiveRecord
       end
     end
 
+    def find_some_ordered(ids)
+      ids = ids.slice(offset_value || 0, limit_value || ids.size) || []
+
+      result = except(:limit, :offset).where(primary_key => ids).to_a
+
+      if result.size == ids.size
+        pk_type = @klass.type_for_attribute(primary_key)
+
+        records_by_id = result.index_by(&:id)
+        ids.map { |id| records_by_id.fetch(pk_type.cast(id)) }
+      else
+        raise_record_not_found_exception!(ids, result.size, ids.size)
+      end
+    end
+
     def find_take
       if loaded?
         @records.first
diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb
index e628254b44..79791af187 100644
--- a/activerecord/test/cases/base_test.rb
+++ b/activerecord/test/cases/base_test.rb
@@ -525,7 +525,8 @@ class BasicsTest < ActiveRecord::TestCase
   end
 
   def test_find_by_slug_with_array
-    assert_equal Topic.find(['1-meowmeow', '2-hello']), Topic.find([1, 2])
+    assert_equal Topic.find([1, 2]), Topic.find(['1-meowmeow', '2-hello'])
+    assert_equal 'The Second Topic of the day', Topic.find(['2-hello', '1-meowmeow']).first.title
   end
 
   def test_find_by_slug_with_range
diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb
index 73f5312eba..9f90ab79a1 100644
--- a/activerecord/test/cases/finder_test.rb
+++ b/activerecord/test/cases/finder_test.rb
@@ -48,6 +48,75 @@ class FinderTest < ActiveRecord::TestCase
     end
   end
 
+  def test_find_with_ids_returning_ordered
+    records = Topic.find([4,2,5])
+    assert_equal 'The Fourth Topic of the day', records[0].title
+    assert_equal 'The Second Topic of the day', records[1].title
+    assert_equal 'The Fifth Topic of the day', records[2].title
+
+    records = Topic.find(4,2,5)
+    assert_equal 'The Fourth Topic of the day', records[0].title
+    assert_equal 'The Second Topic of the day', records[1].title
+    assert_equal 'The Fifth Topic of the day', records[2].title
+
+    records = Topic.find(['4','2','5'])
+    assert_equal 'The Fourth Topic of the day', records[0].title
+    assert_equal 'The Second Topic of the day', records[1].title
+    assert_equal 'The Fifth Topic of the day', records[2].title
+
+    records = Topic.find('4','2','5')
+    assert_equal 'The Fourth Topic of the day', records[0].title
+    assert_equal 'The Second Topic of the day', records[1].title
+    assert_equal 'The Fifth Topic of the day', records[2].title
+  end
+
+  def test_find_with_ids_and_order_clause
+    # The order clause takes precedence over the informed ids
+    records = Topic.order(:author_name).find([5,3,1])
+    assert_equal 'The Third Topic of the day', records[0].title
+    assert_equal 'The First Topic',            records[1].title
+    assert_equal 'The Fifth Topic of the day', records[2].title
+
+    records = Topic.order(:id).find([5,3,1])
+    assert_equal 'The First Topic',            records[0].title
+    assert_equal 'The Third Topic of the day', records[1].title
+    assert_equal 'The Fifth Topic of the day', records[2].title
+  end
+
+  def test_find_with_ids_with_limit_and_order_clause
+    # The order clause takes precedence over the informed ids
+    records = Topic.limit(2).order(:id).find([5,3,1])
+    assert_equal 2, records.size
+    assert_equal 'The First Topic',            records[0].title
+    assert_equal 'The Third Topic of the day', records[1].title
+  end
+
+  def test_find_with_ids_and_limit
+    records = Topic.limit(3).find([3,2,5,1,4])
+    assert_equal 3, records.size
+    assert_equal 'The Third Topic of the day',  records[0].title
+    assert_equal 'The Second Topic of the day', records[1].title
+    assert_equal 'The Fifth Topic of the day',  records[2].title
+  end
+
+  def test_find_with_ids_where_and_limit
+    # Please note that Topic 1 is the only not approved so
+    # if it were among the first 3 it would raise a ActiveRecord::RecordNotFound
+    records = Topic.where(approved: true).limit(3).find([3,2,5,1,4])
+    assert_equal 3, records.size
+    assert_equal 'The Third Topic of the day',  records[0].title
+    assert_equal 'The Second Topic of the day', records[1].title
+    assert_equal 'The Fifth Topic of the day',  records[2].title
+  end
+
+  def test_find_with_ids_and_offset
+    records = Topic.offset(2).find([3,2,5,1,4])
+    assert_equal 3, records.size
+    assert_equal 'The Fifth Topic of the day',  records[0].title
+    assert_equal 'The First Topic',             records[1].title
+    assert_equal 'The Fourth Topic of the day', records[2].title
+  end
+
   def test_find_passing_active_record_object_is_deprecated
     assert_deprecated do
       Topic.find(Topic.last)
@@ -195,7 +264,9 @@ class FinderTest < ActiveRecord::TestCase
 
   def test_find_by_ids_with_limit_and_offset
     assert_equal 2, Entrant.limit(2).find([1,3,2]).size
-    assert_equal 1, Entrant.limit(3).offset(2).find([1,3,2]).size
+    entrants = Entrant.limit(3).offset(2).find([1,3,2])
+    assert_equal 1, entrants.size
+    assert_equal 'Ruby Guru', entrants.first.name
 
     # Also test an edge case: If you have 11 results, and you set a
     #   limit of 3 and offset of 9, then you should find that there
@@ -203,6 +274,8 @@ class FinderTest < ActiveRecord::TestCase
     devs = Developer.all
     last_devs = Developer.limit(3).offset(9).find devs.map(&:id)
     assert_equal 2, last_devs.size
+    assert_equal 'fixture_10', last_devs[0].name
+    assert_equal 'Jamis', last_devs[1].name
   end
 
   def test_find_with_large_number