Merge pull request #23598 from brchristian/activerecord_second_to_last

ActiveRecord #second_to_last tests and finder methods

2 files changed, 83 insertions, 4 deletions

diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb
index 0037398554..c3053f0b13 100644
--- a/activerecord/lib/active_record/relation/finder_methods.rb
+++ b/activerecord/lib/active_record/relation/finder_methods.rb
@@ -255,13 +255,13 @@ module ActiveRecord
     #   Person.offset(3).third_to_last # returns the third-to-last object from OFFSET 3
     #   Person.where(["user_name = :u", { u: user_name }]).third_to_last
     def third_to_last
-      find_nth(-3)
+      find_nth_from_last 3
     end
 
     # Same as #third_to_last but raises ActiveRecord::RecordNotFound if no record
     # is found.
     def third_to_last!
-      find_nth!(-3)
+      find_nth_from_last 3 or raise RecordNotFound.new("Couldn't find #{@klass.name} with [#{arel.where_sql(@klass.arel_engine)}]")
     end
 
     # Find the second-to-last record.
@@ -271,13 +271,13 @@ module ActiveRecord
     #   Person.offset(3).second_to_last # returns the second-to-last object from OFFSET 3
     #   Person.where(["user_name = :u", { u: user_name }]).second_to_last
     def second_to_last
-      find_nth(-2)
+      find_nth_from_last 2
     end
 
     # Same as #second_to_last but raises ActiveRecord::RecordNotFound if no record
     # is found.
     def second_to_last!
-      find_nth!(-2)
+      find_nth_from_last 2 or raise RecordNotFound.new("Couldn't find #{@klass.name} with [#{arel.where_sql(@klass.arel_engine)}]")
     end
 
     # Returns true if a record exists in the table that matches the +id+ or
@@ -561,6 +561,25 @@ module ActiveRecord
       relation.limit(limit).to_a
     end
 
+    def find_nth_from_last(index)
+      if loaded?
+        @records[-index]
+      else
+        relation = if order_values.empty? && primary_key
+                     order(arel_attribute(primary_key).asc)
+                   else
+                     self
+                   end
+
+        relation.to_a[-index]
+        # TODO: can be made more performant on large result sets by
+        # for instance, last(index)[-index] (which would require
+        # refactoring the last(n) finder method to make test suite pass),
+        # or by using a combination of reverse_order, limit, and offset,
+        # e.g., reverse_order.offset(index-1).first
+      end
+    end
+    
     private
 
     def find_nth_with_limit_and_offset(index, limit, offset:) # :nodoc:
diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb
index 3e31874455..692c6bf2d0 100644
--- a/activerecord/test/cases/finder_test.rb
+++ b/activerecord/test/cases/finder_test.rb
@@ -486,6 +486,66 @@ class FinderTest < ActiveRecord::TestCase
     end
   end
 
+  def test_second_to_last
+    assert_equal topics(:fourth).title, Topic.second_to_last.title
+
+    # test with offset
+    assert_equal topics(:fourth), Topic.offset(1).second_to_last
+    assert_equal topics(:fourth), Topic.offset(2).second_to_last
+    assert_equal topics(:fourth), Topic.offset(3).second_to_last
+    assert_equal nil, Topic.offset(4).second_to_last
+    assert_equal nil, Topic.offset(5).second_to_last
+
+    #test with limit
+    # assert_equal nil, Topic.limit(1).second # TODO: currently failing
+    assert_equal nil, Topic.limit(1).second_to_last
+  end
+
+  def test_second_to_last_have_primary_key_order_by_default
+    expected = topics(:fourth)
+    expected.touch # PostgreSQL changes the default order if no order clause is used
+    assert_equal expected, Topic.second_to_last
+  end
+
+  def test_model_class_responds_to_second_to_last_bang
+    assert Topic.second_to_last!
+    Topic.delete_all
+    assert_raises_with_message ActiveRecord::RecordNotFound, "Couldn't find Topic" do
+      Topic.second_to_last!
+    end
+  end
+
+  def test_third_to_last
+    assert_equal topics(:third).title, Topic.third_to_last.title
+
+    # test with offset
+    assert_equal topics(:third), Topic.offset(1).third_to_last
+    assert_equal topics(:third), Topic.offset(2).third_to_last
+    assert_equal nil, Topic.offset(3).third_to_last
+    assert_equal nil, Topic.offset(4).third_to_last
+    assert_equal nil, Topic.offset(5).third_to_last
+
+    # test with limit
+    # assert_equal nil, Topic.limit(1).third # TODO: currently failing
+    assert_equal nil, Topic.limit(1).third_to_last
+    # assert_equal nil, Topic.limit(2).third # TODO: currently failing
+    assert_equal nil, Topic.limit(2).third_to_last
+  end
+
+  def test_third_to_last_have_primary_key_order_by_default
+    expected = topics(:third)
+    expected.touch # PostgreSQL changes the default order if no order clause is used
+    assert_equal expected, Topic.third_to_last
+  end
+
+  def test_model_class_responds_to_third_to_last_bang
+    assert Topic.third_to_last!
+    Topic.delete_all
+    assert_raises_with_message ActiveRecord::RecordNotFound, "Couldn't find Topic" do
+      Topic.third_to_last!
+    end
+  end
+
   def test_last_bang_present
     assert_nothing_raised do
       assert_equal topics(:second), Topic.where("title = 'The Second Topic of the day'").last!