From b42c3255bf22e54f459751d5370e8befc33e84ea Mon Sep 17 00:00:00 2001
From: Ben Woosley <ben.woosley@gmail.com>
Date: Fri, 23 Oct 2015 16:13:36 -0500
Subject: 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`.
---
 .../lib/active_record/relation/finder_methods.rb   | 25 +++++++++++++++++-----
 1 file changed, 20 insertions(+), 5 deletions(-)

(limited to 'activerecord/lib')

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
-- 
cgit v1.2.3