-
-
Notifications
You must be signed in to change notification settings - Fork 934
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
Eliminate n+1 queries in multiple problematic actions #4520
Eliminate n+1 queries in multiple problematic actions #4520
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4520 +/- ##
==========================================
- Coverage 97.15% 97.15% -0.01%
==========================================
Files 391 391
Lines 8257 8255 -2
==========================================
- Hits 8022 8020 -2
Misses 235 235 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work! I'd love to see some graphs of how our requests times change after this.
@@ -44,6 +44,6 @@ def json_return | |||
|
|||
def render_str_call(scope) | |||
local_var = { scope: scope, dependencies: @dependencies, gem_name: @latest_version.rubygem.name } | |||
ActionController::Base.new.render_to_string(partial: "dependencies/dependencies", formats: [:html], locals: local_var) | |||
self.class.renderer.new(request.env).render(partial: "dependencies/dependencies", formats: [:html], locals: local_var) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is something unreasonably satisfying about the way the new version is exactly the same number of characters
order(Arel.sql("case when #{quoted_table_name}.latest AND #{quoted_table_name}.platform = 'ruby' then 2 else 1 end desc")) | ||
.order(Arel.sql("case when #{quoted_table_name}.latest then #{quoted_table_name}.number else NULL end desc")) | ||
.order(id: :desc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol you did it, convert the entire thing into ordering logic, incredible
has_many :dependencies, -> { order("rubygems.name ASC").includes(:rubygem) }, dependent: :destroy, inverse_of: "version" | ||
has_many :dependencies, lambda { | ||
order(Rubygem.arel_table["name"].asc).includes(:rubygem).references(:rubygem) | ||
}, dependent: :destroy, inverse_of: "version" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what's different here... why switch from ->
to lambda, and why use arel_table
? also, what does adding references
do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no functional change here -- i think it got rewritten while I was trying out something else. the nice this is this avoids SQL strings by using the arel table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@indirect references
ensures data are loaded using join and not by second query. In this case eager_load
could be used as well to make it shorter. 🤔
430f4d0
to
5554254
Compare
No description provided.