Skip to content
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

ActiveRecord find_nth finder respects limit() #24124

Closed
wants to merge 4 commits into from

Conversation

brchristian
Copy link
Contributor

Summary

Fixes #23979.

Issue #23979 describes behavior where the ActiveRecord first and last would behave differently following a limit(), and DHH's comment in #23598 (comment) pointed to the first method being the one that was behaving incorrectly.

Specifically, limit(1).first(2) would return a set of size 2 (incorrect), while limit(1).last(2) would return a record set of size 1 (correct).

This PR introduces a test suite to capture the interrelationships between limit and the finder methods (both first(n) as well as second, third, and so on).

It also adjusts those finder methods to make the test suite pass: making them respect the limit and making them consistent with the behavior of the last finders.

@rails-bot
Copy link

r? @schneems

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

@brchristian
Copy link
Contributor Author

@schneems Let me know if there’s anything else I can do to help prepare this PR!

@schneems
Copy link
Member

schneems commented Sep 9, 2016

@sgrif missed this one coming in awhile ago, can you take a look?

@brchristian
Copy link
Contributor Author

The change in c9220bf required a significant refactor, so I am closing this PR in favor of #27597.

@brchristian brchristian closed this Jan 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants