Fix polymorphic assocation with nil type N+1 false alarm issue #70
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wendy-clio
force-pushed
the
fix_poly_n_plus_one_false_alarm
branch
4 times, most recently
from
6f18633
to
091f920
Compare
wendy-clio
force-pushed
the
fix_poly_n_plus_one_false_alarm
branch
from
091f920
to
41abfe4
Compare
wendy-clio
requested review from
apalmblad,
fimmtiu,
deanna-cooney,
MrJakeReid,
tudorbertiean,
oliviawerre and
Vtown121
lib/jit_preloader/active_record/associations/singular_association.rb
Outdated
Show resolved
Hide resolved
… with nil type to be include
wendy-clio
force-pushed
the
fix_poly_n_plus_one_false_alarm
branch
from
41abfe4
to
275c00a
Compare
Vtown121
previously approved these changes
Jun 10, 2024
tudorbertiean
previously approved these changes
Jun 10, 2024
oliviawerre
previously approved these changes
Jun 10, 2024
Co-authored-by: Olivia Werre <[email protected]>
wendy-clio
dismissed stale reviews from oliviawerre, tudorbertiean, and Vtown121
via
94c257e
deanna-cooney
approved these changes
Jun 10, 2024
| @@ -18,13 +18,17 @@ def load_target | |||
| # always an N+1 query. | |||
| record.jit_n_plus_one_tracking ||= owner.jit_n_plus_one_tracking if record | |||
|
|
|||
| if !jit_loaded && owner.jit_n_plus_one_tracking | |||
| if !jit_loaded && owner.jit_n_plus_one_tracking && !is_polymorphic_association_without_type | |||
There was a problem hiding this comment.
Nit for positive logic and methods returning boolean results ending with a ?.
Reading "is not an association without a type" is a little confusing at first. Perhaps polymorphic_association_has_type? and checking self.klass.present? instead? What you have here isn't wrong though, so take it or leave it!
There was a problem hiding this comment.
That's another approach to simplify the name but may complex the logic. So I will just keep it as is.
oliviawerre
approved these changes
Jun 10, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By adding a check
is_polymorphic_association_without_typebefore publishing N+1 notification, it filters out the scenario if a record has a polymorphic association with a nil type.