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

Loaded associations should not run a new query when size is called #32617

Merged
merged 1 commit into from Apr 27, 2018

Conversation

@tgturner
Copy link
Contributor

@tgturner tgturner commented Apr 18, 2018

Already loaded associations were running an extra query when size was called on the association.
This fix ensures that an extra query is no longer run.

This addresses #32569

@rails-bot
Copy link

@rails-bot rails-bot commented Apr 18, 2018

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kaspth (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@@ -231,7 +233,7 @@ def size
# loaded and you are going to fetch the records anyway it is better to
# check <tt>collection.length.zero?</tt>.
def empty?
if loaded?
if loaded? || @association_ids

This comment has been minimized.

@chancancode

chancancode Apr 21, 2018
Member

We talked about this briefly in the issue and think this should probably have the !find_target? check as well. Can you investigate why that may matter, and if it does, add a test for it?

This comment has been minimized.

@pixeltrix

pixeltrix Apr 25, 2018
Member

I think we don't need the !find_target? since that will be implicit if @association_ids is defined because it calls size.zero?.

activerecord/test/cases/associations/has_many_associations_test.rb Outdated
car.bulbs.size
end
end

This comment has been minimized.

@chancancode

chancancode Apr 21, 2018
Member

Can you add tests for empty? as well?

activerecord/lib/active_record/associations/collection_association.rb Outdated
@@ -212,6 +212,8 @@ def destroy(*records)
def size
if !find_target? || loaded?
target.size
elsif @association_ids
@association_ids.length

This comment has been minimized.

@kamipo

kamipo Apr 21, 2018
Member

Can you rebase on current master?
This branch will break existing behavior described at e76e477 since ids_reader doesn't respect dirty target when the target is not loaded yet.

@tgturner tgturner force-pushed the tgturner:size-should-use-available-association branch Apr 21, 2018
@tgturner
Copy link
Contributor Author

@tgturner tgturner commented Apr 21, 2018

@chancancode I don't think the !find_target check in empty? is necessary. I added all relevant test cases I could think of.

Thanks to @kamipo, I noticed that the changes I made were causing incorrect results to be returned for models with unsaved associations. Handling that case, I believe, handled the need for !find_target. Please let me know if any of my assumptions are incorrect, and thanks for working on this with me!

@chancancode
Copy link
Member

@chancancode chancancode commented Apr 24, 2018

This seems good to me but I would love if @pixeltrix or @kamipo can double check the more subtle aspects of this as I am not super familiar with them, so I might have missed some things.

@tgturner
Copy link
Contributor Author

@tgturner tgturner commented Apr 24, 2018

Thanks for following up!

@chancancode chancancode assigned pixeltrix and kamipo and unassigned kaspth Apr 24, 2018
activerecord/CHANGELOG.md Outdated
@@ -1,3 +1,8 @@
* Ensure `Associations::CollectionAssociation#size` and `Associations::CollectionAssociation#empty`

This comment has been minimized.

@kamipo

kamipo Apr 26, 2018
Member

s/empty/empty?/

activerecord/lib/active_record/associations/collection_association.rb Outdated
@@ -212,6 +212,8 @@ def destroy(*records)
def size
if !find_target? || loaded?
target.size
elsif @association_ids && target.empty?
@association_ids.length

This comment has been minimized.

@kamipo

kamipo Apr 26, 2018
Member

I prefer @association_ids.size since it is an only place using length in the class.

activerecord/test/cases/associations/has_many_associations_test.rb Outdated
def test_collection_empty_with_dirty_target
post = posts(:thinking)
assert_equal [], post.reader_ids
assert_equal true, post.readers.empty?

This comment has been minimized.

@kamipo

kamipo Apr 26, 2018
Member

assert_empty post.readers

activerecord/test/cases/associations/has_many_associations_test.rb Outdated
post.readers.reset
post.readers.build
assert_equal [], post.reader_ids
assert_equal false, post.readers.empty?

This comment has been minimized.

@kamipo

kamipo Apr 26, 2018
Member

assert_not_empty post.readers

activerecord/test/cases/associations/has_many_associations_test.rb Outdated
car_two = Car.create!
car_two.bulb_ids

assert_queries(0) do

This comment has been minimized.

@kamipo

kamipo Apr 26, 2018
Member

assert_no_queries do

activerecord/test/cases/associations/has_many_associations_test.rb Outdated
assert_equal false, car.bulbs.empty?
end

assert_queries(0) do

This comment has been minimized.

@kamipo

kamipo Apr 26, 2018
Member

assert_no_queries do

activerecord/test/cases/associations/has_many_associations_test.rb Outdated
car_two = Car.create!

assert_queries(1) do
assert_equal false, car.bulbs.empty?

This comment has been minimized.

@kamipo

kamipo Apr 26, 2018
Member

assert_not_empty car.bulbs

activerecord/test/cases/associations/has_many_associations_test.rb Outdated
end

assert_queries(1) do
assert_equal true, car_two.bulbs.empty?

This comment has been minimized.

@kamipo

kamipo Apr 26, 2018
Member

assert_empty car_two.bulbs

activerecord/test/cases/associations/has_many_associations_test.rb Outdated
car_two.bulb_ids

assert_queries(0) do
assert_equal false, car.bulbs.empty?

This comment has been minimized.

@kamipo

kamipo Apr 26, 2018
Member

assert_not_empty car.bulbs

activerecord/test/cases/associations/has_many_associations_test.rb Outdated
end

assert_queries(0) do
assert_equal true, car_two.bulbs.empty?

This comment has been minimized.

@kamipo

kamipo Apr 26, 2018
Member

assert_empty car_two.bulbs

@tgturner tgturner force-pushed the tgturner:size-should-use-available-association branch Apr 26, 2018
@tgturner
Copy link
Contributor Author

@tgturner tgturner commented Apr 26, 2018

@kamipo Thank you for the feedback! Updated PR based on your comments and rebased on top of Master

@kamipo
kamipo approved these changes Apr 26, 2018
Copy link
Member

@kamipo kamipo left a comment

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

Already loaded associations were running an extra query when `size` was called on the association.
This fix ensures that an extra query is no longer run.

Update tests to use proper methods
@tgturner tgturner force-pushed the tgturner:size-should-use-available-association branch to b07b970 Apr 26, 2018
@tgturner
Copy link
Contributor Author

@tgturner tgturner commented Apr 26, 2018

Done!

@kamipo kamipo merged commit 0a8fe15 into rails:master Apr 27, 2018
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chancancode
Copy link
Member

@chancancode chancancode commented Apr 27, 2018

Thank you @tgturner and everyone else who reviewed the PR! ❤️

@tgturner
Copy link
Contributor Author

@tgturner tgturner commented Apr 27, 2018

Thanks to everyone that helped!

@tgturner tgturner deleted the tgturner:size-should-use-available-association branch Apr 27, 2018
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

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