Sitemap fix, 1 of 2: Add canonical_parent_ids and resource_type to the summary endpoint - #3758
Conversation
OpenAPI Changes2 changes: 0 error, 0 warning, 2 info Unexpected changes? Ensure your branch is up-to-date with |
ChristopherChudzicki
added
the
Needs Review
ChristopherChudzicki
changed the title
There was a problem hiding this comment.
Pull request overview
Extends learning-resource summaries with resource type and URL-forming parent IDs for podcast episodes and videos.
Changes:
- Adds ordered canonical-parent annotations and summary serialization.
- Optimizes pagination counts and introduces response caching.
- Updates tests, OpenAPI output, generated clients, and factories.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
learning_resources/constants.py |
Defines canonical relationships and ordering. |
learning_resources/models.py |
Adds parent-ID annotation and consistent ordering. |
learning_resources/serializers.py |
Exposes the new summary fields. |
learning_resources/views.py |
Applies annotation, pagination, and caching. |
learning_resources/views_test.py |
Tests output, ordering, counts, and caching. |
openapi/specs/v1.yaml |
Documents the expanded response schema. |
frontends/api/src/generated/v1/api.ts |
Regenerates client types. |
frontends/api/src/test-utils/factories/learningResources.ts |
Updates summary test data. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| cache_page_for_anonymous_users( | ||
| cache="redis", | ||
| key_prefix="learning_resources_summary", | ||
| ) |
There was a problem hiding this comment.
moot; decided to remove caching changes from this PR.
|
|
||
| def get_count(self, queryset): | ||
| """Count distinct pks, which the annotations play no part in""" | ||
| return queryset.values(*self.count_fields).distinct().count() |
There was a problem hiding this comment.
Consider adding a one-line note on why this uses .values() rather than the base class's .only().
The base DefaultPagination.get_count uses queryset.only(*self.count_fields).count(), but .only() only defers concrete model columns — it does not strip the canonical_parent_ids annotation, so left alone the subquery would still be evaluated per counted row. Switching to .values(*self.count_fields).distinct() is what actually removes the annotation from the SELECT list (Django keeps only the fields named in .values()), so COUNT(DISTINCT ...) runs over just pk.
That behavior is the entire reason this class exists. It's already covered by test_summary_count_omits_the_parent_ids_annotation (which asserts "canonical_parent_ids" not in count_sql), so a revert to .only() would fail CI rather than break silently — but a short inline comment like # .values("pk") — not .only() — is required to drop the annotation from the count query would make that intent explicit at the call site without having to go find the test. Non-blocking.
There was a problem hiding this comment.
Clarified the comment.
| @method_decorator( | ||
| # no timeout: REDIS_VIEW_CACHE_DURATION is then read per request, so | ||
| # setting it to 0 disables this cache | ||
| cache_page_for_anonymous_users( |
There was a problem hiding this comment.
Question:
This action is now cached under key_prefix="learning_resources_summary", and the cache is invalidated by clear_views_cache() (called with no prefix, so it flushes views.*). That works for the ETL tasks that call it — get_podcast_data, get_ovs_data, the edX/MITx/OLL/xPro pipelines, etc.
The gap is the video/YouTube path: get_youtube_channel_data and get_youtube_playlist_data (learning_resources/tasks.py) load playlists and their video memberships via loaders.load_playlist, but none of them calls clear_views_cache(). Note that load_playlist does call update_index(...) after writing the memberships, so the search index stays fresh — the gap is specifically the Redis view cache, which nothing on the YouTube path touches. test_cache_is_cleared_after_task_run also explicitly documents that get_youtube_data is excluded because "it only queues the fan-out."
Since a video's canonical_parent_ids is exactly its playlist membership, a YouTube ETL run that adds/reorders a video's playlists won't invalidate this newly-cached summary page — it will keep serving stale parent ids until an unrelated ETL task that does call clear_views_cache() next runs.".
Given this PR exists specifically so the sitemaps can rely on canonical_parent_ids, can you confirm the staleness window is acceptable, or add a clear_views_cache() at the point the playlist loader finishes writing? load_playlist (learning_resources/etl/loaders.py) is the natural spot since it already does post-write index maintenance there.
There was a problem hiding this comment.
Decided to remove caching from this PR to keep it simpler. That said, the issue you point out affects regular ETL tasks, and maybe is worth addressing at some point.
(Not sure about putting the cache clear in etl/loaders, though; that would be a first for the repo).
ahtesham-quraish
left a comment
ahtesham-quraish
left a comment
There was a problem hiding this comment.
Left few comments
ahtesham-quraish
added
Waiting on author
and removed
Needs Review
The podcast and video sitemaps can't use /api/v1/learning_resources/summary/ because it doesn't carry enough to build their nested URLs, so they page the full list endpoint instead -- which caps limit at 100 while they request 1000, silently dropping ~87% of their entries (mitodl/mit-learn#3756). This adds what those sitemaps need so they can move over; switching them is follow-up work. - resource_type, so a caller can tell a podcast from an episode. Added to the queryset's .only() as well, or it would be deferred into a query per row. - canonical_parent_ids, the parents that form part of a resource's URL: parent podcasts for an episode, playlists for a video. Empty for everything else -- program and learning-path membership don't appear in a URL, and a learning path is a user-created list that must never leak here. Keeping the endpoint fast was the constraint, since it also serves the catalog-wide resources sitemap (12.4k published resources over 13 shards). canonical_parent_ids is a correlated ArraySubquery: one child_id index scan per row scanned, and no GROUP BY on the outer query. At production scale that is +0.8ms on a 1000-row page at offset 0 and +21ms at offset 12000, since OFFSET discards rows after the subplan has run, so deep pages pay for what they skip. A full 13-shard walk costs on the order of 100ms of extra database time. The pagination count needed handling. Django keeps annotations in the count query when the queryset is .distinct(), so left alone the subquery would run for every row counted rather than only the page -- 17ms against 3ms before this change. SummaryPagination counts distinct pks instead. That is the same number because get_aggregation clears the ordering before the DISTINCT, leaving only the .only() columns, which are all pk-dependent; it is also cheaper than the pre-change count, which hashed a six-column tuple rather than a bare pk. A test pins that the page query carries the annotation and the count query does not. The action is also now cached for anonymous users, like list already was, and ETL's clear_views_cache() invalidates it -- so the page costs land on a miss rather than every crawl. A cache hit is ~48x faster than a miss, mostly because it skips DRF serialization rather than the query. The decorator deliberately omits its timeout argument so REDIS_VIEW_CACHE_DURATION resolves per request instead of at import, which is what makes setting it to 0 actually disable the cache; every other call site in the repo freezes it at import. The detail endpoint's parent prefetches relied on LearningResourceRelationship.Meta.ordering rather than ordering explicitly. They and the new annotation now all name RELATIONSHIP_ORDERING, so the two endpoints agree by construction instead of by inheritance. This is defensive -- Meta.ordering already supplied the same order, and no test can distinguish the two. What is pinned is the agreement itself: a test asserts both endpoints report the same parents in the same order, using positions that are the reverse of creation order so an id-ordered annotation fails it. That agreement matters for 138 of 6,829 videos, the only resources in production with more than one canonical parent. Any member playlist is self-canonical, so a desync wouldn't produce a dead URL -- it would point the sitemap at a different playlist than the bare /video/<id> redirect uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The base DefaultPagination.get_count uses .only(), which defers concrete columns but leaves annotations in the SELECT list. Naming that at the call site, since the override otherwise reads as a gratuitous difference. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The endpoint served the resources and products sitemaps uncached at this scale already; the cache was added here on spec, not to fix a measured problem. An uncached 1000-row page is ~24ms, so a full 13-shard crawl costs about a third of a second. What it cost instead: parent ids go stale whenever an ETL path writes memberships without calling clear_views_cache() -- the YouTube tasks don't -- and cache keys hash the full query string, so any unknown param mints another ~247KB entry. Easy to add back with a deliberate invalidation story if something chattier than a crawler ever points at this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c214005 to
e32b9a0
Compare
What are the relevant tickets?
Description (What does it do?)
Adds
canonical_parent_idsandresource_typeto theapi/v1/learning_resources/summary/endpoint for use in podcast episode + video pagesHow can this be tested?
canonical_parent_ids