Merge pull request #22053 from Empact/first-loaded

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

activerecord/CHANGELOG.md

+*   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.
+
+    *Ben Woosley*
+
 ## Rails 5.0.0.beta1 (December 18, 2015) ##
 
 *   Order the result of `find(ids)` to match the passed array, if the relation

activerecord/lib/active_record/relation/finder_methods.rb

@@ -117,9 +117,9 @@ def take!
     #
     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, offset_index)
+        find_nth 0
       end
     end
 
@@ -169,7 +169,7 @@ def last!
     #   Person.offset(3).second # returns the second object from OFFSET 3 (which is OFFSET 4)
     #   Person.where(["user_name = :u", { u: user_name }]).second
     def second
-      find_nth(1, offset_index)
+      find_nth 1
     end
 
     # Same as #second but raises ActiveRecord::RecordNotFound if no record
@@ -185,7 +185,7 @@ def second!
     #   Person.offset(3).third # returns the third object from OFFSET 3 (which is OFFSET 5)
     #   Person.where(["user_name = :u", { u: user_name }]).third
     def third
-      find_nth(2, offset_index)
+      find_nth 2
     end
 
     # Same as #third but raises ActiveRecord::RecordNotFound if no record
@@ -201,7 +201,7 @@ def third!
     #   Person.offset(3).fourth # returns the fourth object from OFFSET 3 (which is OFFSET 6)
     #   Person.where(["user_name = :u", { u: user_name }]).fourth
     def fourth
-      find_nth(3, offset_index)
+      find_nth 3
     end
 
     # Same as #fourth but raises ActiveRecord::RecordNotFound if no record
@@ -217,7 +217,7 @@ def fourth!
     #   Person.offset(3).fifth # returns the fifth object from OFFSET 3 (which is OFFSET 7)
     #   Person.where(["user_name = :u", { u: user_name }]).fifth
     def fifth
-      find_nth(4, offset_index)
+      find_nth 4
     end
 
     # Same as #fifth but raises ActiveRecord::RecordNotFound if no record
@@ -233,7 +233,7 @@ def fifth!
     #   Person.offset(3).forty_two # returns the forty-second object from OFFSET 3 (which is OFFSET 44)
     #   Person.where(["user_name = :u", { u: user_name }]).forty_two
     def forty_two
-      find_nth(41, offset_index)
+      find_nth 41
     end
 
     # Same as #forty_two but raises ActiveRecord::RecordNotFound if no record
@@ -488,27 +488,39 @@ def find_take
       end
     end
 
-    def find_nth(index, offset)
+    def find_nth(index, offset = nil)
       if loaded?
         @records[index]
       else
-        offset += index
-        @offsets[offset] ||= find_nth_with_limit(offset, 1).first
+        # 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,
+            please use Relation#offset instead.
+          MSG
+        else
+          offset = offset_index
+        end
+        @offsets[offset + index] ||= find_nth_with_limit_and_offset(index, 1, offset: offset).first
       end
     end
 
     def find_nth!(index)
-      find_nth(index, offset_index) or raise RecordNotFound.new("Couldn't find #{@klass.name} with [#{arel.where_sql(@klass.arel_engine)}]")
+      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
 
@@ -524,5 +536,16 @@ def find_last
           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

activerecord/test/cases/relations_test.rb

@@ -111,15 +111,38 @@ def test_scoped_first
 
   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",
+                    "The Second Topic of the day"], topics.first(2).map(&:title)
+    end
+
+    assert topics.loaded?
+  end
+
+  def test_first_get_more_than_available
+    topics = Topic.all.order('id ASC')
+    unloaded_first = topics.first(10)
+    topics.to_a # force load
+
+    assert_no_queries do
+      loaded_first = topics.first(10)
+      assert_equal unloaded_first, loaded_first
+    end
+  end
+
   def test_reload
     topics = Topic.all
 

guides/source/5_0_release_notes.md

@@ -431,6 +431,10 @@ Please refer to the [Changelog][active-record] for detailed changes.
     `#table_exists?` will check only tables in the future.
     ([Pull Request](https://github.com/rails/rails/pull/21601))
 
+*   Deprecate sending the `offset` argument to `find_nth`. Please use the
+    `offset` method on relation instead.
+    ([Pull Request](https://github.com/rails/rails/pull/22053))
+
 ### Notable changes
 
 *   Added a `foreign_key` option to `references` while creating the table.