New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consistency between first() and last() with limit #27597

Merged
merged 1 commit into from Jan 9, 2018

Conversation

Projects
None yet
5 participants
@brchristian
Contributor

brchristian commented Jan 6, 2017

Fixes #23979.

As discussed in #23979, there was an inconsistency between the way that first() and last() would interact with limit. Specifically:

> Topic.limit(1).first(2).size
=> 2
> Topic.limit(1).last(2).size
=> 1

This PR is a refactor and rebase of #24124, with a simpler test suite and simpler implementation.

Discussion with Rails community members as well as DHH in #23598 (comment) showed that the behavior or first should be brought into line with last (rather than vice-versa).

This PR resolves the inconsistency between first and last when used in conjunction with limit.

@rails-bot

This comment has been minimized.

rails-bot commented Jan 6, 2017

r? @pixeltrix

(@rails-bot has picked a reviewer for you, use r? to override)

@kamipo

This comment has been minimized.

Member

kamipo commented Jan 7, 2018

Can you rebase?

@brchristian

This comment has been minimized.

Contributor

brchristian commented Jan 8, 2018

@kamipo Rebased to latest master 👍

activerecord/lib/active_record/relation/finder_methods.rb Outdated
if limit_value.present?
limit = [limit_value - index, limit].min
limit = [limit, 0].max

This comment has been minimized.

@kamipo

kamipo Jan 8, 2018

Member

If limit <= 0, we don't need a query to database.

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 @@ def find_nth_with_limit(index, limit)
         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

This comment has been minimized.

@brchristian

brchristian Jan 8, 2018

Contributor

Great point! I have updated the PR.

activerecord/test/cases/finder_test.rb Outdated
@@ -693,6 +693,11 @@ def test_take_and_first_and_last_with_integer_should_return_an_array
assert_kind_of Array, Topic.last(5)
end
def test_take_and_first_and_last_with_limit_should_return_same_size
topics = Topic.limit(1)
assert_equal topics.first(2).size, topics.last(2).size

This comment has been minimized.

@kamipo

kamipo Jan 8, 2018

Member

first with limit doesn't work as expected, and inconsistent with last is true.
But it is also inconsistent with loaded first with limit.
Can you more exercise tests like test_last_on_relation_with_limit_and_offset?

  def test_find_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_last_on_relation_with_limit_and_offset
post = posts("sti_comments")
comments = post.comments.order(id: :asc)
assert_equal comments.limit(2).to_a.last, comments.limit(2).last
assert_equal comments.limit(2).to_a.last(2), comments.limit(2).last(2)
assert_equal comments.limit(2).to_a.last(3), comments.limit(2).last(3)
assert_equal comments.offset(2).to_a.last, comments.offset(2).last
assert_equal comments.offset(2).to_a.last(2), comments.offset(2).last(2)
assert_equal comments.offset(2).to_a.last(3), comments.offset(2).last(3)
comments = comments.offset(1)
assert_equal comments.limit(2).to_a.last, comments.limit(2).last
assert_equal comments.limit(2).to_a.last(2), comments.limit(2).last(2)
assert_equal comments.limit(2).to_a.last(3), comments.limit(2).last(3)
end

This comment has been minimized.

@brchristian

brchristian Jan 8, 2018

Contributor

No problem, I have added additional tests for compatibility here between limit(x).first(y) and limit(x).to_a.first(y), and they all now pass as well.

@brchristian

This comment has been minimized.

Contributor

brchristian commented Jan 8, 2018

@kamipo Thanks for code review. I have refactored and added the additional tests.

activerecord/test/cases/finder_test.rb Outdated
assert_equal topics.limit(1).to_a.first(2).size, topics.limit(1).first(2).size
assert_equal topics.limit(2).to_a.first(2).size, topics.limit(2).first(2).size
assert_equal topics.limit(3).to_a.first(2).size, topics.limit(3).first(2).size

This comment has been minimized.

@kamipo

kamipo Jan 9, 2018

Member

Can you remove .size? These need to be the same result regardless of whether they are loaded or not.

activerecord/test/cases/finder_test.rb Outdated
assert_equal topics.limit(1).to_a.last(2).size, topics.limit(1).last(2).size
assert_equal topics.limit(2).to_a.last(2).size, topics.limit(2).last(2).size
assert_equal topics.limit(3).to_a.last(2).size, topics.limit(3).last(2).size

This comment has been minimized.

@kamipo

kamipo Jan 9, 2018

Member

These assersions have already been covered by test_last_on_relation_with_limit_and_offset.

def test_last_on_relation_with_limit_and_offset
post = posts("sti_comments")
comments = post.comments.order(id: :asc)
assert_equal comments.limit(2).to_a.last, comments.limit(2).last
assert_equal comments.limit(2).to_a.last(2), comments.limit(2).last(2)
assert_equal comments.limit(2).to_a.last(3), comments.limit(2).last(3)
assert_equal comments.offset(2).to_a.last, comments.offset(2).last
assert_equal comments.offset(2).to_a.last(2), comments.offset(2).last(2)
assert_equal comments.offset(2).to_a.last(3), comments.offset(2).last(3)
comments = comments.offset(1)
assert_equal comments.limit(2).to_a.last, comments.limit(2).last
assert_equal comments.limit(2).to_a.last(2), comments.limit(2).last(2)
assert_equal comments.limit(2).to_a.last(3), comments.limit(2).last(3)
end

activerecord/CHANGELOG.md Outdated
@@ -1,3 +1,11 @@
* Fixed inconsistency between `first(n)` and `last(n)` when
used with `limit()` The `first` finder respects the `limit()`,
making it consistent with `last(n)`.

This comment has been minimized.

@kamipo

kamipo Jan 9, 2018

Member

I think that we should note fixing inconsistency first(n) with either loaded or not.
Previously, we need to use relation.to_a.first(n) as a workaround, but now, we can use relation.first(n) simply.

activerecord/test/cases/finder_test.rb Outdated
assert_equal topics.limit(1).to_a.first(2).size, topics.limit(1).to_a.last(2).size
assert_equal topics.limit(2).to_a.first(2).size, topics.limit(2).to_a.last(2).size
assert_equal topics.limit(3).to_a.first(2).size, topics.limit(3).to_a.last(2).size

This comment has been minimized.

@kamipo

kamipo Jan 9, 2018

Member

Array#first and Array#last are in the Ruby Core. These assertions are not necessary.

http://ruby-doc.org/core-2.5.0/Array.html#method-i-first
http://ruby-doc.org/core-2.5.0/Array.html#method-i-last

activerecord/test/cases/finder_test.rb Outdated
assert_equal topics.limit(1).first(2).size, topics.limit(1).last(2).size
assert_equal topics.limit(2).first(2).size, topics.limit(2).last(2).size
assert_equal topics.limit(3).first(2).size, topics.limit(3).last(2).size

This comment has been minimized.

@kamipo

kamipo Jan 9, 2018

Member

Previously relation.to_a.first(n), relation.to_a.last(n), and relation.last(n) works expected, but only relation.first(n) does not.
Consistency between relation.to_a.last(n) and relation.last(n) have already been covered by test_last_on_relation_with_limit_and_offset.
Whether relation.first(n) works correctly should compare relation.to_a.first(n) rather than relation.last(n).

@brchristian

This comment has been minimized.

Contributor

brchristian commented Jan 9, 2018

@kamipo Great points about the tests and CHANGELOG. I agree with you and I have made all the changes you suggested.

@kamipo

kamipo approved these changes Jan 9, 2018

Looks good to me 👍
Can you squash your commits into one?

@brchristian

This comment has been minimized.

Contributor

brchristian commented Jan 9, 2018

@kamipo Squashed 👍

@kamipo kamipo merged commit d7d6921 into rails:master Jan 9, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
codeclimate All good!
Details

@brchristian brchristian deleted the brchristian:first_last_parity branch Jan 9, 2018

@brchristian

This comment has been minimized.

Contributor

brchristian commented Jan 9, 2018

(PS, Special thanks to @mmlindeboom for helpful discussion!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment