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

Detect n+1 count queries from ActiveRecord::Associations::CollectionProxy#count #655

Merged
merged 2 commits into from
Apr 16, 2023
Merged

Detect n+1 count queries from ActiveRecord::Associations::CollectionProxy#count #655

merged 2 commits into from
Apr 16, 2023

Conversation

quintinm-dev
Copy link
Contributor

@quintinm-dev quintinm-dev commented Apr 6, 2023

Just gauging interest here, as there may be a lot of considerations that I've missed. Generally people should use size, but if they use count the n+1 goes undetected. If you do want this in, we could backport it to older Rails versions.

Minimal example:

class Blog < ApplicationRecord
  has_many :comments, dependent: :destroy
end

MAX = 5

# currently undetected
Blog.all.filter { |b| b.comments.count < MAX }

# gets detected as counter cache
Blog.all.filter { |b| b.comments.size < MAX }

# gets detected as n+1
Blog.all.filter { |b| b.comments.length < MAX }

# gets detected as n+1
Blog.all.filter { |b| b.comments.to_a.count < MAX }

@quintinm-dev quintinm-dev marked this pull request as ready for review April 6, 2023 23:48
@flyerhzm
Copy link
Owner

@quintinm-dev yes, we should detect n+1 for count method, I'll merge it if you can backport it to older Rails versions.

@quintinm-dev quintinm-dev marked this pull request as draft April 11, 2023 16:29
@quintinm-dev quintinm-dev marked this pull request as ready for review April 11, 2023 17:46
@quintinm-dev
Copy link
Contributor Author

@flyerhzm done, should be ready for a review now 😎

@flyerhzm flyerhzm merged commit 520278f into flyerhzm:main Apr 16, 2023
@flyerhzm
Copy link
Owner

@quintinm-dev it looks good, thanks

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

Successfully merging this pull request may close these issues.

2 participants