Conversation
Detail responses cache under a self-versioning key derived from the object's `modified` timestamp, so writes invalidate automatically with no explicit cache-busting. List responses cache under a per-model version counter in Redis, bumped by the existing modified-cascade signals (extended here to cover Publisher/Imprint/Universe/Series/Arc/Character/Team/Issue, plus a new Credits -> Issue cascade that was previously missing). User-scoped viewsets (Collection/PullList/WishList/ReadingList) are intentionally excluded from list caching to avoid leaking one user's data to another.
Several detail/action caches keyed off an object's own `modified` missed edits to related data that don't cascade a bump onto it: Arc/Character/Team issue_list didn't see plain issue field edits, and Series retrieve didn't see Publisher/Imprint renames. Both now mix the dependent model's version counter into the cache key. Also fixes a race where CreditSerializer.create() bumped Issue.modified before attaching roles, letting a request cache an issue with an empty role list under a key nothing would ever invalidate; a new m2m_changed signal on Credits.role bumps again once roles actually land. IssueViewSet/SeriesViewSet's cheap (pk, modified) lookup was still carrying the retrieve queryset's annotate() aggregates into an extra JOIN + GROUP BY on every request; get_modified_queryset() now skips them. Also drops the now-unnecessary deferred api.cache imports in signals.py and de-duplicates the per-model cache-bump functions.
…e check Issue retrieve embeds Series/Publisher/Imprint names, and Imprint/Universe retrieve embed their Publisher's name, none of which cascade a `modified` bump onto the cached object. Arc/Character/Team issue_list also nests each issue's Series name without depending on it. All now mix in the relevant model's version counter, same pattern already used for Series. Character/ Team retrieve still omit Creator/Universe on purpose -- those are edited far more often than Publisher/Imprint, so tracking them would tank thecache hit rate for comparatively little benefit; documented inline. Also fixes PublisherViewSet.series_list, which skipped get_object() (and therefore the existence/permission check) entirely on a cache hit -- a deleted publisher's cached series list kept returning 200 instead of 404 until the 2min list-cache TTL caught up.
ModelLabel is now a StrEnum instead of a plain string-constant class, so a typo'd label is caught by type checking instead of silently creating a version counter that's never invalidated. list_cache_key()/detail_cache_key() now fetch all dependent labels' version counters in one cache.get_many() instead of one cache.get() per label, and get_model_version()'s cold-start path drops from 3 Redis round trips to 2 (1 for the winning first caller) by checking cache.add()'s return value instead of re-reading afterward. comicsdb/apps.py's 16 near-identical post_save/post_delete .connect() calls for the 8 "just bump my version counter" models (Arc, Character, Creator, Imprint, Publisher, Series, Team, Universe) collapse into a table-driven loop backed by a single bump_cache() receiver in signals.py, replacing the 8 near-identical wrapper functions it used to require. weak=False is required here since the loop's functools.partial receivers have no other strong reference; verified all 8 models still bump their counter on save and delete before trusting it.
Deciding whether to raise or lower DETAIL_CACHE_TTL/LIST_CACHE_TTL needs actual production data, not guesswork: how much of Redis the API cache accounts for, how it's split across models, and whether Redis is already evicting under memory pressure before the TTL ever gets a say. Scans the full keyspace once (cursor-based, safe against a large production keyspace), buckets keys by api:detail:<model>/api:list:<model>/cachever/other, and estimates per-bucket memory by sampling MEMORY USAGE rather than calling it on every key. Also reports global hit rate and evicted_keys from Redis INFO. Meant to be run manually against production after this caching work deploys, and periodically afterward to compare.
This file contains hidden or 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
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.
Description
Adds Redis-backed response caching to the read-only DRF API to reduce DB load on the heavier viewsets (Issue, Series, etc.), with automatic invalidation on writes — no manual cache-busting to maintain.
Two schemes, matched to endpoint shape:
retrieve, and detail-scoped actions likeissue_list) cache under a key derived from the object'spk+modifiedtimestamp. A write changesmodified, which changes the key, so old entries are simply orphaned and expire via TTL (24h) — no explicit delete needed.list, plusseries_list) cache under a per-model cache-generation counter in Redis, bumped by the existingmodified-cascade signals whenever that model (or Issue, for the Series list's embeddednum_issues) changes.Applied to the 9 public catalog viewsets (Arc, Character, Creator, Imprint, Issue, Publisher, Series, Team, Universe). Deliberately not applied to Collection/PullList/WishList/ReadingList — those are user-scoped (
get_querysetfilters byrequest.user), and a shared list-cache key would leak one user's data to another; that's left uncached for now rather than bolted on in this PR.Also fixes a gap where Credits changes never bumped the parent Issue's
modifiedtimestamp (so a credit could change without invalidating the issue's cached/conditional-request state), and adds the missing cache-invalidation signal wiring for Publisher/Imprint/Universe, which previously had none.DETAIL_CACHE_TTL/LIST_CACHE_TTLare both single constants inapi/cache.py— starting at 24h/2min, to be tuned against production Redis memory usage.Follow-up fixes
A review of the caching design surfaced a few gaps, fixed here:
issue_listand Seriesretrievewere cached under the parent object's ownmodifiedtimestamp, which doesn't change on every edit that affects the cached payload (a plain issue field edit doesn't bump the parent Arc/Character/Team; a Publisher/Imprint rename doesn't bump the owning Series). Both now also key off the dependent model's cache-generation counter.CreditSerializer.create()bumpedIssue.modifiedbefore attaching credit roles, so a request landing in that window could cache an issue with an empty role list under a key nothing would later invalidate. A newm2m_changedsignal onCredits.rolebumps again once roles actually land.(pk, modified)lookup used for conditional requests and cache-key computation was still carrying the retrieve queryset'sannotate()aggregates into an extra JOIN + GROUP BY on every request (cache hits included) for Issue/Series. Fixed via a leanerget_modified_queryset().Further fixes and cleanup
A second review pass caught a few more gaps in the same class as above, plus some efficiency/maintainability polish:
modifiedbump onto the cached object. Arc/Character/Teamissue_listalso nests each issue's Series name without depending on it. All now mix in the relevant model's version counter, same pattern already used for Series. (Character/Team retrieve still omit Creator/Universe on purpose — those are edited/added far more often than Publisher/Imprint, so tracking them would tank the cache hit rate for comparatively little benefit; documented inline.)PublisherViewSet.series_listexistence check: skippedget_object()(and therefore the existence/permission check) entirely on a cache hit — a deleted publisher's cached series list kept returning 200 instead of 404 until the 2min list-cache TTL caught up.ModelLabelis now aStrEnum(a typo'd label is caught by type checking instead of silently creating a dead, never-invalidated counter);list_cache_key()/detail_cache_key()batch dependent-label lookups into one Redisget_many()instead of oneget()per label;get_model_version()'s cold-start path trimmed from 3 Redis round trips to 2; the 16 near-identical per-modelpost_save/post_deleteconnections incomicsdb/apps.pycollapsed into a table-driven loop.audit_response_cachemanagement command: a one-off audit of the response cache's Redis footprint (key counts/estimated memory per category, hit rate, eviction stats), meant to be run against production after this deploys to inform whetherDETAIL_CACHE_TTL/LIST_CACHE_TTLneed tuning rather than guessing.