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

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

Merged
merged 2 commits into from Dec 28, 2015

Conversation

@Empact
Copy link
Contributor

@Empact Empact commented Oct 23, 2015

I realized that first(2), etc. were unnecessarily querying for the
records when they were already preloaded. This was because
find_nth_with_limit can not know which @records to return because
it conflates the offset and index into a single variable, while
the @records only needs the index itself to select the proper
record.

Because find_nth and find_nth_with_limit are public methods, I
instead introduced a private method find_nth_with_limit_and_offset
which is called internally and handles the loaded? checking.

Once the offset argument is removed from find_nth,
find_nth_with_limit_and_offset can be collapsed into
find_nth_with_limit, with offset always equal to offset_index.

@rails-bot
Copy link

@rails-bot rails-bot commented Oct 23, 2015

r? @chancancode

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

@Empact Empact force-pushed the Empact:first-loaded branch Oct 23, 2015
@Empact Empact changed the title Fix `first(limit)` to take advantage of `loaded?` records if available Fix #first(limit) to take advantage of loaded? records if available Oct 23, 2015
@Empact Empact changed the title Fix #first(limit) to take advantage of loaded? records if available Fix #first(limit) to take advantage of #loaded? records if available Oct 23, 2015
@Empact
Copy link
Contributor Author

@Empact Empact commented Nov 4, 2015

@chancancode Thoughts on this?

@senny
senny reviewed Dec 24, 2015
View changes
activerecord/test/cases/relations_test.rb Outdated
topics = Topic.all.order('id ASC')

assert_queries(1) do
topics.to_a # force load

This comment has been minimized.

@senny

senny Dec 24, 2015
Member

You could move this outside the assert_queries block. Then we don't have to do the actual code 2 times and we can assert that no queries are executed.

@senny
senny reviewed Dec 24, 2015
View changes
activerecord/lib/active_record/relation/finder_methods.rb Outdated
# find_nth_with_limit_and_offset
if offset
ActiveSupport::Deprecation.warn(<<-MSG.squish)
You are passing an offset argument to find_nth. Please instead

This comment has been minimized.

@senny

senny Dec 24, 2015
Member

can we reword this message in the form of:

Passing an offset argument to find_nth is deprecated, please use Relation#offset instead.

@senny
Copy link
Member

@senny senny commented Dec 24, 2015

r? @senny

@Empact I added a few minor comments. The change looks good.

@rails-bot rails-bot assigned senny and unassigned chancancode Dec 24, 2015
@senny senny added the activerecord label Dec 24, 2015
Empact added 2 commits Oct 23, 2015
All uses of the `offset` are passing `offset_index`. Better to push
down the `offset` consideration into `find_nth`.

This also works toward enabling `find_nth_with_limit` to take
advantage of the `loaded?` state of the relation.
I realized that `first(2)`, etc. was unnecessarily querying for the
records when they were already preloaded. This was because
`find_nth_with_limit` can not know which `@records` to return because
it conflates the `offset` and `index` into a single variable, while
the `@records` only needs the `index` itself to select the proper
record.

Because `find_nth` and `find_nth_with_limit` are public methods, I
instead introduced a private method `find_nth_with_limit_and_offset`
which is called internally and handles the `loaded?` checking.

Once the `offset` argument is removed from `find_nth`,
`find_nth_with_limit_and_offset` can be collapsed into
`find_nth_with_limit`, with `offset` always equal to `offset_index`.
@Empact Empact force-pushed the Empact:first-loaded branch to b42c325 Dec 24, 2015
@Empact
Copy link
Contributor Author

@Empact Empact commented Dec 24, 2015

@senny Made the changes you suggested - note test_loaded_first_with_limit was in the style of test_loaded_first, so I changed both.

2 questions:

  • is it helpful to make a separate PR with the removal of the deprecated options, for future application?
  • is there any guideline / thought on when things like this should be removed?
@senny
Copy link
Member

@senny senny commented Dec 24, 2015

@Empact It's not necessary to make a PR for the removal. Once we release a new Major or Minor version we go through all deprecations and remove the ones that are due. Usually they stay for one release cycle. Meaning that the option will be deprecated in 5.0 and will be removed from 5.1.

@senny senny merged commit b42c325 into rails:master Dec 28, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@senny
Copy link
Member

@senny senny commented Dec 28, 2015

@Empact thank you 💛

I've added a test-case to access more records than available to make sure it behaves the same as with a query. I also updated the 5.0 release notes to include the deprecation (0d2675f).

senny added a commit that referenced this pull request Dec 28, 2015
Fix #first(limit) to take advantage of #loaded? records if available
@Empact
Copy link
Contributor Author

@Empact Empact commented Dec 29, 2015

Thanks @senny! 🎆

@bogdan

This comment has been minimized.

Copy link
Contributor

@bogdan bogdan commented on activerecord/lib/active_record/relation/finder_methods.rb in 16a476e Jan 27, 2016

There is some inconsistency here: you should warn unconditionally about offset deprecation. In current implementation if I would do:

u = User.where(...)
u.to_a
u.find_nth(77, 15)

I would not see a deprecation warning.

This comment has been minimized.

Copy link
Contributor Author

@Empact Empact replied Jan 28, 2016

Thanks! #23303

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.