Merge pull request #27597 from brchristian/first_last_parity

Consistency between first() and last() with limit

3 files changed, 31 insertions, 1 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index cbbfad615d..167bd8c82c 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,11 @@
+*   Fixed inconsistency with `first(n)` when used with `limit()`.
+    The `first(n)` finder now respects the `limit()`, making it consistent
+    with `relation.to_a.first(n)`, and also with the behavior of `last(n)`.
+
+    Fixes #23979.
+
+    *Brian Christian*
+
 *   Use `count(:all)` in `HasManyAssociation#count_records` to prevent invalid
     SQL queries for association counting.
 
diff --git a/activerecord/lib/active_record/relation/finder_methods.rb b/activerecord/lib/active_record/relation/finder_methods.rb
index e61cacf6a7..50d0f14b98 100644
--- a/activerecord/lib/active_record/relation/finder_methods.rb
+++ b/activerecord/lib/active_record/relation/finder_methods.rb
@@ -532,7 +532,11 @@ module ActiveRecord
         else
           relation = ordered_relation
 
-          if limit_value.nil? || index < limit_value
+          if limit_value
+            limit = [limit_value - index, limit].min
+          end
+
+          if limit > 0
             relation = relation.offset(offset_index + index) unless index.zero?
             relation.limit(limit).to_a
           else
diff --git a/activerecord/test/cases/finder_test.rb b/activerecord/test/cases/finder_test.rb
index c0485a7be7..95bd987551 100644
--- a/activerecord/test/cases/finder_test.rb
+++ b/activerecord/test/cases/finder_test.rb
@@ -687,6 +687,24 @@ class FinderTest < ActiveRecord::TestCase
     assert_equal comments.limit(2).to_a.last(3), comments.limit(2).last(3)
   end
 
+  def test_first_on_relation_with_limit_and_offset
+    post = posts("sti_comments")
+
+    comments = post.comments.order(id: :asc)
+    assert_equal comments.limit(2).to_a.first, comments.limit(2).first
+    assert_equal comments.limit(2).to_a.first(2), comments.limit(2).first(2)
+    assert_equal comments.limit(2).to_a.first(3), comments.limit(2).first(3)
+
+    assert_equal comments.offset(2).to_a.first, comments.offset(2).first
+    assert_equal comments.offset(2).to_a.first(2), comments.offset(2).first(2)
+    assert_equal comments.offset(2).to_a.first(3), comments.offset(2).first(3)
+
+    comments = comments.offset(1)
+    assert_equal comments.limit(2).to_a.first, comments.limit(2).first
+    assert_equal comments.limit(2).to_a.first(2), comments.limit(2).first(2)
+    assert_equal comments.limit(2).to_a.first(3), comments.limit(2).first(3)
+  end
+
   def test_take_and_first_and_last_with_integer_should_return_an_array
     assert_kind_of Array, Topic.take(5)
     assert_kind_of Array, Topic.first(5)