Fix `first(limit)` to take advantage of `loaded?` records if available

I realized that `first(2)`, etc. was unnecessarily querying for the
records when they were already preloaded. This was because
`find_nth_with_limit` can not know which `@records` to return because
it conflates the `offset` and `index` into a single variable, while
the `@records` only needs the `index` itself to select the proper
record.

Because `find_nth` and `find_nth_with_limit` are public methods, I
instead introduced a private method `find_nth_with_limit_and_offset`
which is called internally and handles the `loaded?` checking.

Once the `offset` argument is removed from `find_nth`,
`find_nth_with_limit_and_offset` can be collapsed into
`find_nth_with_limit`, with `offset` always equal to `offset_index`.

3 files changed, 39 insertions, 8 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 5daec6fb34..9144ab6695 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,8 @@
+*   When calling `first` with a `limit` argument, return directly from the
+    `loaded?` records if available.
+
+    *Ben Woosley*
+
 *   Deprecate sending the `offset` argument to `find_nth`. Please use the
     `offset` method on relation instead.
 
diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb
index 4a0867289e..3cbb12a09d 100644
--- a/activerecord/lib/active_record/relation/finder_methods.rb
+++ b/activerecord/lib/active_record/relation/finder_methods.rb
@@ -117,7 +117,7 @@ module ActiveRecord
     #
     def first(limit = nil)
       if limit
-        find_nth_with_limit(offset_index, limit)
+        find_nth_with_limit_and_offset(0, limit, offset: offset_index)
       else
         find_nth 0
       end
@@ -492,6 +492,9 @@ module ActiveRecord
       if loaded?
         @records[index]
       else
+        # TODO: once the offset argument is removed we rely on offset_index
+        # within find_nth_with_limit, rather than pass it in via
+        # find_nth_with_limit_and_offset
         if offset
           ActiveSupport::Deprecation.warn(<<-MSG.squish)
             Passing an offset argument to find_nth is deprecated,
@@ -500,8 +503,7 @@ module ActiveRecord
         else
           offset = offset_index
         end
-        offset += index
-        @offsets[offset] ||= find_nth_with_limit(offset, 1).first
+        @offsets[offset + index] ||= find_nth_with_limit_and_offset(index, 1, offset: offset).first
       end
     end
 
@@ -509,14 +511,16 @@ module ActiveRecord
       find_nth(index) or raise RecordNotFound.new("Couldn't find #{@klass.name} with [#{arel.where_sql(@klass.arel_engine)}]")
     end
 
-    def find_nth_with_limit(offset, limit)
+    def find_nth_with_limit(index, limit)
+      # TODO: once the offset argument is removed from find_nth,
+      # find_nth_with_limit_and_offset can be merged into this method
       relation = if order_values.empty? && primary_key
                    order(arel_table[primary_key].asc)
                  else
                    self
                  end
 
-      relation = relation.offset(offset) unless offset.zero?
+      relation = relation.offset(index) unless index.zero?
       relation.limit(limit).to_a
     end
 
@@ -532,5 +536,16 @@ module ActiveRecord
           end
       end
     end
+
+    private
+
+    def find_nth_with_limit_and_offset(index, limit, offset:) # :nodoc:
+      if loaded?
+        @records[index, limit]
+      else
+        index += offset
+        find_nth_with_limit(index, limit)
+      end
+    end
   end
 end
diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index 7149c7d072..df0c2f1f92 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -111,10 +111,21 @@ class RelationTest < ActiveRecord::TestCase
 
   def test_loaded_first
     topics = Topic.all.order('id ASC')
+    topics.to_a # force load
 
-    assert_queries(1) do
-      topics.to_a # force load
-      2.times { assert_equal "The First Topic", topics.first.title }
+    assert_no_queries do
+      assert_equal "The First Topic", topics.first.title
+    end
+
+    assert topics.loaded?
+  end
+
+  def test_loaded_first_with_limit
+    topics = Topic.all.order('id ASC')
+    topics.to_a # force load
+
+    assert_no_queries do
+      assert_equal "The First Topic", topics.first(2).first.title
     end
 
     assert topics.loaded?