Fix n+1 courses api #3169
Fix n+1 courses api #3169
Conversation
OpenAPI ChangesShow/hide ## Changes for v0.yaml:Unexpected changes? Ensure your branch is up-to-date with |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…nline into fix-n+1-courses-api
rhysyngsun
left a comment
rhysyngsun
left a comment
There was a problem hiding this comment.
Looking good, noted a few things, some are nits.
| hasattr(instance, "_prefetched_objects_cache") | ||
| and "courseruns" in instance._prefetched_objects_cache # noqa: SLF001 | ||
| ): | ||
| courseruns = instance._prefetched_objects_cache["courseruns"] # noqa: SLF001 |
There was a problem hiding this comment.
It's always iffy to use internals like this, but the only things I can think of would just make this more convoluted so I think you hit the best approach given the limitations of the library.
There was a problem hiding this comment.
Agreed. I didn't feel great doing it.
|
|
||
| return ( | ||
| queryset.prefetch_related( | ||
| Prefetch("courseruns", queryset=enrollable_courseruns_qs) |
There was a problem hiding this comment.
The removal of this will cause unenrollable course runs to start being returned and I don't thnik we want to change that behavior.
There was a problem hiding this comment.
Will the filter below handle removing unenrollable course runs from what is returned?
There was a problem hiding this comment.
If you're talking about
.filter(courseruns__id__in=enrollable_courseruns_qs.values_list("id", flat=True))the filter only applies to Course records. Then courseruns is preloaded in a separate query that has no knowledge of that filter().
So
prefetch_related("courseruns")is always implicitly
prefetch_related(Prefetch("courseruns", queryset=CourseRun.objects.all()))The behavior might be different if we were doing select_related("courseruns") here, but I'd honestly need to test it out to know exactly what django would automagically do in that case.
| courseruns_qs = CourseRun.objects.unenrollable() | ||
| courseruns_qs = CourseRun.objects.unenrollable().prefetch_related("products") | ||
| return ( | ||
| queryset.prefetch_related(Prefetch("courseruns", queryset=courseruns_qs)) |
There was a problem hiding this comment.
Same as above, the removal of this will cause unenrollable courses to be returned.
| def get_queryset(self): | ||
| return ( | ||
| CourseRunEnrollment.objects.filter(user=self.request.user) | ||
| .select_related("run__course__page", "user", "run") |
There was a problem hiding this comment.
It might be good to move at least run_course_page and possibly run out of the select_related and into prefetch_related too, that's a lot of joins particularly for the page to be doing.
|
|
||
| # Get direct topics from the page | ||
| # Get direct topics from the page (this should be prefetched) | ||
| direct_topics = page_instance.topics.all() |
There was a problem hiding this comment.
This should probably also prefetch parent since that's another set of N+1s.
| ) | ||
|
|
||
| return queryset | ||
| .all() |
There was a problem hiding this comment.
Nit: I'm not sure how the pattern of putting in all() ended up being introduced, but you only need to use all() when you need the default queryset from an objects manager. In this usage, it just returns a copy of the query. It's basically glue-code django implemented so that a queryset can swap in for a manager in cases where all() is called on it.
Course.objects.all() # needed here to get a queryset
Course.objects.filter(id_in=[1,2]) # would be a no-op here because we already have a queryset
Co-authored-by: Nathan Levesque <rhysyngsun@users.noreply.github.com>
What are the relevant tickets?
https://github.com/mitodl/hq/issues/8776
Description (What does it do?)
This PR addresses a lot of different N+1 issues. After resolving the N+1 issue that appears in the linked issue, other's began to emerge.
How can this be tested?
Tests should pass, N+1 issues should no longer be present.