diff --git a/api/cache.py b/api/cache.py new file mode 100644 index 00000000..4dca6d2b --- /dev/null +++ b/api/cache.py @@ -0,0 +1,121 @@ +"""Redis-backed response caching for the read-only DRF API. + +Two independent invalidation schemes are used, depending on endpoint shape: + +* Detail responses (retrieve, and detail-scoped actions like issue_list) are + cached under a self-versioning key derived from the object's ``modified`` + timestamp -- when ``modified`` changes, the key changes with it, so old + entries are simply orphaned and expire via TTL. No explicit invalidation + is needed. +* List responses (list, and collection-scoped actions like series_list) are + cached under a key that includes a per-model cache-generation counter in + Redis, bumped by signal handlers (see comicsdb/signals.py) whenever data a + list response could embed changes. +""" + +import hashlib +from collections.abc import Iterable +from enum import StrEnum +from typing import Any + +from django.core.cache import cache + +DETAIL_CACHE_TTL = 60 * 60 * 24 # 24h safety net; live keys self-invalidate on write. +LIST_CACHE_TTL = 60 * 2 # 2min; bounds staleness from nested-object changes we don't chase. + +_VERSION_KEY_PREFIX = "cachever" + + +class ModelLabel(StrEnum): + """Stable cache-key labels shared between signal handlers and views.""" + + ARC = "arc" + CHARACTER = "character" + CREATOR = "creator" + IMPRINT = "imprint" + ISSUE = "issue" + PUBLISHER = "publisher" + SERIES = "series" + TEAM = "team" + UNIVERSE = "universe" + + +def detail_cache_key(model_label: str, pk: Any, modified, *dependent_labels: str) -> str: + """Cache key for a single object's serialized detail response. + + Self-invalidating: a change to `modified` produces a new key, so old + entries are simply orphaned and expire via TTL. + + `dependent_labels` (optional) mix in other models' version counters, for + responses that embed data from a related object whose own edits don't + cascade a `modified` bump onto this one (e.g. a Series response embeds + its Publisher's name, but renaming the Publisher doesn't touch the + Series row). + """ + key = f"api:detail:{model_label}:{pk}:{modified.timestamp()}" + if dependent_labels: + version_map = get_model_versions(dependent_labels) + versions = "-".join(str(version_map[lbl]) for lbl in dependent_labels) + key = f"{key}:{versions}" + return key + + +def get_model_version(model_label: str) -> int: + """Return the current cache-generation counter for a model, initializing + it to 1 on first use.""" + key = f"{_VERSION_KEY_PREFIX}:{model_label}" + version = cache.get(key) + if version is not None: + return version + if cache.add(key, 1, timeout=None): + return 1 + # Lost the initialization race to another caller -- read back what they set. + return cache.get(key) or 1 + + +def get_model_versions(model_labels: Iterable[str]) -> dict[str, int]: + """Batch form of get_model_version(): one Redis round trip (get_many) + for the common case where every counter already exists, instead of one + round trip per label.""" + labels = list(dict.fromkeys(model_labels)) # de-dupe, preserve order + keys = {lbl: f"{_VERSION_KEY_PREFIX}:{lbl}" for lbl in labels} + cached = cache.get_many(keys.values()) + return { + lbl: cached[key] if key in cached else get_model_version(lbl) for lbl, key in keys.items() + } + + +def bump_model_version(model_label: str) -> None: + """Invalidate list caches that depend on `model_label` by advancing its + generation counter.""" + key = f"{_VERSION_KEY_PREFIX}:{model_label}" + try: + cache.incr(key) + except ValueError: + # Key doesn't exist yet. At most one concurrent caller's `add` wins; + # the other's bump is harmlessly absorbed, since a version key that + # didn't exist means no list cache entry was ever computed under any + # version of it either. + cache.add(key, 1, timeout=None) + + +def list_cache_key( + model_label: str, + *dependent_labels: str, + query: Iterable[tuple[str, list[str]]], + scope: str = "", +) -> str: + """Cache key for a list-type response: one or more model versions plus a + normalized hash of the request's query params. + + `query` should come from `request.query_params.lists()` (multi-value), + not `.dict()` -- `.dict()` silently drops all-but-the-last value for + repeated params (e.g. IssueFilter's `role_id`), which would let distinct + multi-value requests collide on the same key. + """ + labels = (model_label, *dependent_labels) + version_map = get_model_versions(labels) + versions = "-".join(str(version_map[lbl]) for lbl in labels) + normalized = "&".join(f"{k}={v}" for k, v in sorted(query)) + digest = hashlib.sha256(normalized.encode()).hexdigest()[:16] + return f"api:list:{model_label}:{scope}:{versions}:{digest}" diff --git a/api/management/commands/audit_response_cache.py b/api/management/commands/audit_response_cache.py new file mode 100644 index 00000000..ee457a9a --- /dev/null +++ b/api/management/commands/audit_response_cache.py @@ -0,0 +1,166 @@ +import random +import time +from collections import defaultdict + +from django.core.cache import cache +from django.core.management.base import BaseCommand + +_DETAIL_PREFIX = "api:detail:" +_LIST_PREFIX = "api:list:" +_VERSION_PREFIX = "cachever:" +_BYTES_PER_UNIT = 1024 + + +class Command(BaseCommand): + help = ( + "One-off audit of the Redis-backed API response cache -- key counts and " + "estimated memory footprint per category (api:detail:, " + "api:list:, cachever, everything else), plus global hit-rate and " + "eviction stats. Meant to be run against production shortly after the " + "caching PR deploys (and again later) to see whether DETAIL_CACHE_TTL/" + "LIST_CACHE_TTL (api/cache.py) need tuning, rather than guessing." + ) + + def add_arguments(self, parser) -> None: + parser.add_argument( + "--sample-size", + type=int, + default=300, + help=( + "Max keys to sample per category for MEMORY USAGE, so the " + "estimate doesn't require calling it on every key in a large " + "keyspace (default: 300)" + ), + ) + parser.add_argument( + "--scan-count", + type=int, + default=1000, + help="COUNT hint passed to Redis SCAN per iteration (default: 1000)", + ) + + def handle(self, *args, **options) -> None: + # Django's generic cache API has no SCAN/MEMORY USAGE/INFO -- those + # require the raw redis-py client the RedisCache backend wraps. + client = cache._cache.get_client() + + self._print_global_stats(client) + categories, total_keys, elapsed = self._scan_and_categorize(client, options["scan_count"]) + self.stdout.write(f"\nScanned {total_keys:,} keys in {elapsed:.1f}s.") + self._print_category_report(client, categories, options["sample_size"]) + + def _print_global_stats(self, client) -> None: + try: + memory = client.info("memory") + stats = client.info("stats") + except Exception as exc: # noqa: BLE001 -- best-effort diagnostics + self.stdout.write( + self.style.WARNING(f"INFO command unavailable ({exc}); skipping global stats.") + ) + return + + hits = stats.get("keyspace_hits", 0) + misses = stats.get("keyspace_misses", 0) + total = hits + misses + hit_rate = f"{hits / total:.1%}" if total else "n/a" + + self.stdout.write( + self.style.MIGRATE_HEADING("Redis instance (global -- all keys, not just ours)") + ) + self.stdout.write(f" used_memory: {memory.get('used_memory_human', '?')}") + maxmemory = memory.get("maxmemory", 0) + unbounded = " (unbounded -- no eviction policy in effect)" if not maxmemory else "" + self.stdout.write(f" maxmemory: {memory.get('maxmemory_human', '?')}{unbounded}") + self.stdout.write(f" maxmemory_policy: {memory.get('maxmemory_policy', '?')}") + self.stdout.write(f" mem_fragmentation: {memory.get('mem_fragmentation_ratio', '?')}") + self.stdout.write(f" keyspace hit rate: {hit_rate} ({hits:,} hits / {misses:,} misses)") + + evicted = stats.get("evicted_keys", 0) + evicted_line = f" evicted_keys: {evicted:,}" + if evicted: + evicted_line = self.style.WARNING( + f"{evicted_line} <-- Redis is evicting under memory pressure; " + "TTLs alone aren't controlling memory here, lower them or add memory" + ) + self.stdout.write(evicted_line) + + def _scan_and_categorize(self, client, scan_count: int): + categories: dict[str, list[bytes]] = defaultdict(list) + total_keys = 0 + cursor = 0 + start = time.monotonic() + while True: + cursor, keys = client.scan(cursor=cursor, count=scan_count) + for raw_key in keys: + total_keys += 1 + key = raw_key.decode() if isinstance(raw_key, bytes) else raw_key + categories[self._categorize(key)].append(raw_key) + if cursor == 0: + break + return categories, total_keys, time.monotonic() - start + + @staticmethod + def _categorize(key: str) -> str: + # Substring match rather than startswith(): robust to however Django's + # RedisCache backend wraps the logical key (e.g. a "::" + # prefix), without needing to know its exact format. + if _DETAIL_PREFIX in key: + model = key.split(_DETAIL_PREFIX, 1)[1].split(":", 1)[0] + return f"api:detail:{model}" + if _LIST_PREFIX in key: + model = key.split(_LIST_PREFIX, 1)[1].split(":", 1)[0] + return f"api:list:{model}" + if _VERSION_PREFIX in key: + return "cachever" + return "other (Select2, throttling, etc.)" + + def _print_category_report(self, client, categories: dict, sample_size: int) -> None: + self.stdout.write(self.style.MIGRATE_HEADING("\nBy category")) + if not categories: + self.stdout.write(" No keys found.") + return + + rows = [] + for label, keys in categories.items(): + count = len(keys) + avg_bytes = self._sample_avg_memory(client, keys, sample_size) + est_total = avg_bytes * count if avg_bytes is not None else None + rows.append((label, count, avg_bytes, est_total)) + rows.sort(key=lambda row: row[3] or 0, reverse=True) + + for label, count, avg_bytes, est_total in rows: + avg_str = f"~{avg_bytes:,.0f} B/key" if avg_bytes is not None else "n/a" + total_str = self._human_bytes(est_total) + self.stdout.write(f" {label:<32} {count:>8,} keys {avg_str:>14} est. {total_str}") + + known = sum(est for *_ignored, est in rows if est is not None) + self.stdout.write( + f"\n Estimated total across sampled categories: {self._human_bytes(known)}" + ) + self.stdout.write( + " (Estimates extrapolate from a random sample's MEMORY USAGE -- " + "re-run periodically and compare, not just once.)" + ) + + @staticmethod + def _sample_avg_memory(client, keys: list, sample_size: int) -> float | None: + sample = keys if len(keys) <= sample_size else random.sample(keys, sample_size) + sizes = [] + for key in sample: + try: + size = client.memory_usage(key) + except Exception: # noqa: BLE001, S112 -- best-effort; key may have expired mid-scan + continue + if size is not None: + sizes.append(size) + return sum(sizes) / len(sizes) if sizes else None + + @staticmethod + def _human_bytes(n: float | None) -> str: + if n is None: + return "n/a" + for unit in ("B", "KB", "MB", "GB"): + if n < _BYTES_PER_UNIT: + return f"{n:,.1f} {unit}" + n /= _BYTES_PER_UNIT + return f"{n:,.1f} TB" diff --git a/api/views.py b/api/views.py index 297a3d41..ca66ca2d 100644 --- a/api/views.py +++ b/api/views.py @@ -1,3 +1,4 @@ +from django.core.cache import cache from django.db import models from django.db.models import ( Avg, @@ -24,6 +25,13 @@ from rest_framework.response import Response from rest_framework_condition import last_modified +from api.cache import ( + DETAIL_CACHE_TTL, + LIST_CACHE_TTL, + ModelLabel, + detail_cache_key, + list_cache_key, +) from api.v1_0.serializers import ( ArcListSerializer, ArcSerializer, @@ -112,26 +120,109 @@ class ReadingListItemsPagination(PageNumberPagination): class CachedObjectMixin: + #: Set on concrete viewsets to enable response caching; None disables it + #: (fail-open -- behaves exactly as before this attribute existed). + cache_model_label: str | None = None + def get_object(self): if not hasattr(self, "_cached_object"): self._cached_object = super().get_object() return self._cached_object + def get_modified_queryset(self): + """Queryset used by get_object_modified() for its (pk, modified) + lookup. Defaults to get_queryset(), but a viewset whose + get_queryset() adds annotate() aggregates should override this to + return a lean, unannotated queryset with the same row-level + permission scoping -- an annotated aggregate still forces a JOIN + + GROUP BY even when values_list() doesn't select the annotated + field. + """ + return self.get_queryset() + + def get_object_modified(self): + """Cheap (pk, modified) lookup for conditional-request checks and + cache-key computation -- does NOT trigger the full get_object() + queryset (select_related/prefetch_related/annotate). That heavier + query only runs lazily if get_object() is later called for an + actual cache-miss render. + """ + if not hasattr(self, "_cached_object_modified"): + if hasattr(self, "_cached_object"): + # get_object() already ran this request -- reuse it instead + # of doing a second query. + obj = self._cached_object + self._cached_object_modified = ( + getattr(obj, "pk", None), + getattr(obj, "modified", None), + ) + else: + lookup_url_kwarg = self.lookup_url_kwarg or self.lookup_field + pk = self.kwargs[lookup_url_kwarg] + # get_modified_queryset()/filter_queryset(), not a bare + # Model.objects, so this still respects per-viewset row + # scoping (e.g. CollectionViewSet/PullListViewSet/ + # WishListViewSet filtering by user). + row = ( + self.filter_queryset(self.get_modified_queryset()) + .filter(**{self.lookup_field: pk}) + .values_list(self.lookup_field, "modified") + .first() + ) + self._cached_object_modified = row if row else (None, None) + + return self._cached_object_modified + class ConditionalRetrieveModelMixin(CachedObjectMixin, mixins.RetrieveModelMixin): + #: Other models' cache-generation counters (see api/cache.py) to mix + #: into the detail cache key, for responses that embed a related + #: object's fields where an edit to that related object doesn't cascade + #: a `modified` bump onto this one. + cache_detail_dependent_labels: tuple[str, ...] = () + def retrieve(self, request, *args, **kwargs): - retrieve = last_modified(last_modified_func=self._retrieve_last_modified)(super().retrieve) + retrieve = last_modified(last_modified_func=self._retrieve_last_modified)( + self._cached_retrieve + ) return retrieve(self, request, *args, **kwargs) def _retrieve_last_modified(self, *args, **kwargs): - obj = self.get_object() + _pk, modified = self.get_object_modified() + + return modified + + def _cached_retrieve(self, request, *args, **kwargs): + """Reached only once rest_framework_condition's last_modified() has + already ruled out a 304 -- i.e. exactly where a cache lookup is + worth doing. + + Note: a cache hit returns without calling get_object(), so + check_object_permissions() never runs on that path. Every + permission class in this project is view-level only (none override + has_object_permission), so this is currently inert -- but a future + object-level permission class on a cache_model_label-enabled + viewset would need get_object_modified() (or this method) to also + enforce it explicitly, since DRF's default only calls it from + get_object(). + """ + pk, modified = self.get_object_modified() + if not self.cache_model_label or modified is None: + return mixins.RetrieveModelMixin.retrieve(self, request, *args, **kwargs) - if obj and getattr(obj, "modified", None): - return obj.modified + key = detail_cache_key( + self.cache_model_label, pk, modified, *self.cache_detail_dependent_labels + ) + cached = cache.get(key) + if cached is not None: + return Response(cached) - return None + response = mixins.RetrieveModelMixin.retrieve(self, request, *args, **kwargs) + if response.status_code == status.HTTP_200_OK: + cache.set(key, response.data, DETAIL_CACHE_TTL) + return response class UserTrackingMixin: @@ -144,7 +235,73 @@ def perform_update(self, serializer): serializer.save(edited_by=self.request.user) -class IssueListMixin(CachedObjectMixin): +class CachedListModelMixin(mixins.ListModelMixin): + """Drop-in replacement for mixins.ListModelMixin that caches list() + responses under a per-model cache-generation counter (see api/cache.py). + """ + + cache_model_label: str | None = None + cache_dependent_labels: tuple[str, ...] = () + + def list(self, request, *args, **kwargs): + if not self.cache_model_label: + return super().list(request, *args, **kwargs) + + key = list_cache_key( + self.cache_model_label, + *self.cache_dependent_labels, + query=request.query_params.lists(), + ) + cached = cache.get(key) + if cached is not None: + return Response(cached) + + response = super().list(request, *args, **kwargs) + if response.status_code == status.HTTP_200_OK: + cache.set(key, response.data, LIST_CACHE_TTL) + return response + + +class CachedDetailActionMixin(CachedObjectMixin): + """Shared cache-wrapping logic for detail-scoped list actions + (issue_list, etc.) that paginate a related queryset off a parent object. + Cached under the parent's own `modified` timestamp, since that already + captures "has this parent's related collection changed" via the + existing modified-cascade signals. + """ + + #: Other models' cache-generation counters (see api/cache.py) to mix + #: into the action's cache key, for actions whose payload can change + #: without the parent object's own `modified` being touched (e.g. + #: issue_list embeds fields from Issue rows that don't cascade a + #: `modified` bump onto the parent Arc/Character/Team on every edit -- + #: only on M2M add/remove/clear). + cache_action_dependent_labels: tuple[str, ...] = () + + def _cached_paginated_action(self, *, build_queryset, serializer_class): + pk, modified = self.get_object_modified() + key = None + if self.cache_model_label and modified is not None: + key = detail_cache_key( + self.cache_model_label, pk, modified, *self.cache_action_dependent_labels + ) + cached = cache.get(key) + if cached is not None: + return Response(cached) + + obj = self.get_object() + queryset = build_queryset(obj) + page = self.paginate_queryset(queryset) + if page is None: + raise Http404 + serializer = serializer_class(page, many=True, context={"request": self.request}) + response = self.get_paginated_response(serializer.data) + if key is not None: + cache.set(key, response.data, DETAIL_CACHE_TTL) + return response + + +class IssueListMixin(CachedDetailActionMixin): """Mixin to provide a standard issue_list action for related models.""" def get_issue_queryset(self, obj): @@ -164,21 +321,15 @@ def issue_list(self, request, *args, **kwargs): def _issue_list(self, request, *args, **kwargs): """Returns a list of issues for this object.""" - obj = self.get_object() - queryset = self.get_issue_queryset(obj) - page = self.paginate_queryset(queryset) - if page is not None: - serializer = IssueListSerializer(page, many=True, context={"request": self.request}) - return self.get_paginated_response(serializer.data) - raise Http404 + return self._cached_paginated_action( + build_queryset=self.get_issue_queryset, + serializer_class=IssueListSerializer, + ) def _issue_list_last_modified(self, *args, **kwargs): - obj = self.get_object() - - if obj and getattr(obj, "modified", None): - return obj.modified + _pk, modified = self.get_object_modified() - return None + return modified class ArcViewSet( @@ -186,7 +337,7 @@ class ArcViewSet( IssueListMixin, mixins.CreateModelMixin, ConditionalRetrieveModelMixin, - mixins.ListModelMixin, + CachedListModelMixin, mixins.UpdateModelMixin, viewsets.GenericViewSet, ): @@ -201,6 +352,11 @@ class ArcViewSet( queryset = Arc.objects.all() filterset_class = ComicVineFilter parser_classes = (MultiPartParser, FormParser) + cache_model_label = ModelLabel.ARC + # issue_list embeds fields from Issue rows (and their Series) that + # don't cascade a `modified` bump onto this Arc except on M2M + # add/remove/clear. + cache_action_dependent_labels = (ModelLabel.ISSUE, ModelLabel.SERIES) def get_serializer_class(self): match self.action: @@ -217,7 +373,7 @@ class CharacterViewSet( IssueListMixin, mixins.CreateModelMixin, ConditionalRetrieveModelMixin, - mixins.ListModelMixin, + CachedListModelMixin, mixins.UpdateModelMixin, viewsets.GenericViewSet, ): @@ -232,6 +388,19 @@ class CharacterViewSet( queryset = Character.objects.all() filterset_class = ComicVineFilter parser_classes = (MultiPartParser, FormParser) + cache_model_label = ModelLabel.CHARACTER + # retrieve also embeds Creator/Universe names (CharacterReadSerializer) + # that don't cascade a `modified` bump onto this Character either, but + # those are deliberately left as bounded (TTL-limited) staleness rather + # than a cache_detail_dependent_labels dependency -- Creators in + # particular are edited/added far more often than Publishers/Imprints, + # so tying this key to CREATOR's version would invalidate every cached + # Character detail on essentially any creator edit anywhere. + # + # issue_list embeds fields from Issue rows (and their Series) that + # don't cascade a `modified` bump onto this Character except on M2M + # add/remove/clear. + cache_action_dependent_labels = (ModelLabel.ISSUE, ModelLabel.SERIES) def get_queryset(self): queryset = super().get_queryset() @@ -255,7 +424,7 @@ class CreatorViewSet( UserTrackingMixin, mixins.CreateModelMixin, ConditionalRetrieveModelMixin, - mixins.ListModelMixin, + CachedListModelMixin, mixins.UpdateModelMixin, viewsets.GenericViewSet, ): @@ -270,6 +439,7 @@ class CreatorViewSet( queryset = Creator.objects.all() filterset_class = ComicVineFilter parser_classes = (MultiPartParser, FormParser) + cache_model_label = ModelLabel.CREATOR def get_serializer_class(self): match self.action: @@ -305,7 +475,7 @@ class ImprintViewSet( UserTrackingMixin, mixins.CreateModelMixin, ConditionalRetrieveModelMixin, - mixins.ListModelMixin, + CachedListModelMixin, mixins.UpdateModelMixin, viewsets.GenericViewSet, ): @@ -326,6 +496,10 @@ class ImprintViewSet( queryset = Imprint.objects.all() filterset_class = ComicVineFilter parser_classes = (MultiPartParser, FormParser) + cache_model_label = ModelLabel.IMPRINT + # Imprint retrieve embeds its Publisher's name, which doesn't cascade a + # `modified` bump onto this Imprint when renamed. + cache_detail_dependent_labels = (ModelLabel.PUBLISHER,) def get_queryset(self): queryset = super().get_queryset() @@ -347,7 +521,7 @@ class IssueViewSet( UserTrackingMixin, mixins.CreateModelMixin, ConditionalRetrieveModelMixin, - mixins.ListModelMixin, + CachedListModelMixin, mixins.UpdateModelMixin, viewsets.GenericViewSet, ): @@ -365,6 +539,23 @@ class IssueViewSet( queryset = Issue.objects.all() filterset_class = IssueFilter parser_classes = (JSONParser, MultiPartParser, FormParser) + cache_model_label = ModelLabel.ISSUE + # Issue retrieve embeds its Series/Publisher/Imprint names, which don't + # cascade a `modified` bump onto this Issue when renamed. Arc/ + # Character/Team/Universe/Creator names are also embedded but + # deliberately excluded here -- those are edited/created far more + # often, and mixing them in would invalidate every cached issue detail + # response on essentially every catalog edit anywhere, not just ones + # affecting this issue. + cache_detail_dependent_labels = (ModelLabel.PUBLISHER, ModelLabel.IMPRINT, ModelLabel.SERIES) + + def get_modified_queryset(self): + # get_queryset() annotates average_rating/rating_count for the + # retrieve action -- an aggregate that survives into (pk, modified) + # values_list() as an extra JOIN + GROUP BY even though neither + # annotated field is selected. This lookup only needs the row's own + # pk/modified, so skip the annotation entirely. + return Issue.objects.all() def get_queryset(self): if self.action == "list": @@ -418,7 +609,7 @@ class PublisherViewSet( UserTrackingMixin, mixins.CreateModelMixin, ConditionalRetrieveModelMixin, - mixins.ListModelMixin, + CachedListModelMixin, mixins.UpdateModelMixin, viewsets.GenericViewSet, ): @@ -439,6 +630,7 @@ class PublisherViewSet( queryset = Publisher.objects.all() filterset_class = PublisherFilter parser_classes = (MultiPartParser, FormParser) + cache_model_label = ModelLabel.PUBLISHER def get_serializer_class(self): match self.action: @@ -455,17 +647,39 @@ def series_list(self, request, pk=None): """ Returns a list of series for a publisher. """ + # get_object() (existence + permission check) first, same as any + # other detail-scoped action -- a cache hit must not bypass either + # of those, and must not keep serving a 200 once the publisher + # itself has been deleted. publisher = self.get_object() + + # series_list's payload depends on the Series/Issue graph, not on + # Publisher.modified (adding a series, or issues under one, doesn't + # touch the publisher row) -- so this uses the version-counter list + # scheme, scoped to this publisher, rather than a parent-modified + # detail key. + key = list_cache_key( + ModelLabel.SERIES, + ModelLabel.ISSUE, + query=self.request.query_params.lists(), + scope=f"publisher:{pk}:series_list", + ) + cached = cache.get(key) + if cached is not None: + return Response(cached) + queryset = ( publisher.series.select_related("series_type") .annotate(num_issues=Count("issues", distinct=True)) .order_by("sort_name", "year_began") ) page = self.paginate_queryset(queryset) - if page is not None: - serializer = SeriesListSerializer(page, many=True, context={"request": request}) - return self.get_paginated_response(serializer.data) - raise Http404 + if page is None: + raise Http404 + serializer = SeriesListSerializer(page, many=True, context={"request": request}) + response = self.get_paginated_response(serializer.data) + cache.set(key, response.data, LIST_CACHE_TTL) + return response class RoleViewset(mixins.ListModelMixin, viewsets.GenericViewSet): @@ -484,7 +698,7 @@ class SeriesViewSet( IssueListMixin, mixins.CreateModelMixin, ConditionalRetrieveModelMixin, - mixins.ListModelMixin, + CachedListModelMixin, mixins.UpdateModelMixin, viewsets.GenericViewSet, ): @@ -504,6 +718,19 @@ class SeriesViewSet( queryset = Series.objects.select_related("series_type", "publisher") filterset_class = SeriesFilter + cache_model_label = ModelLabel.SERIES + # Series list embeds num_issues, which changes on every Issue write. + cache_dependent_labels = (ModelLabel.ISSUE,) + # Series retrieve embeds its Publisher/Imprint name, which don't cascade + # a `modified` bump onto this Series when renamed. + cache_detail_dependent_labels = (ModelLabel.PUBLISHER, ModelLabel.IMPRINT) + + def get_modified_queryset(self): + # get_queryset() annotates num_issues for list/retrieve -- an + # aggregate that survives into (pk, modified) values_list() as an + # extra JOIN + GROUP BY even though the annotated field isn't + # selected. This lookup only needs the row's own pk/modified. + return Series.objects.all() def get_queryset(self): queryset = super().get_queryset() @@ -567,7 +794,7 @@ class TeamViewSet( IssueListMixin, mixins.CreateModelMixin, ConditionalRetrieveModelMixin, - mixins.ListModelMixin, + CachedListModelMixin, mixins.UpdateModelMixin, viewsets.GenericViewSet, ): @@ -581,7 +808,20 @@ class TeamViewSet( queryset = Team.objects.all() filterset_class = ComicVineFilter + cache_model_label = ModelLabel.TEAM parser_classes = (MultiPartParser, FormParser) + # retrieve also embeds Creator/Universe names (TeamReadSerializer) that + # don't cascade a `modified` bump onto this Team either, but those are + # deliberately left as bounded (TTL-limited) staleness rather than a + # cache_detail_dependent_labels dependency -- Creators in particular + # are edited/added far more often than Publishers/Imprints, so tying + # this key to CREATOR's version would invalidate every cached Team + # detail on essentially any creator edit anywhere. + # + # issue_list embeds fields from Issue rows (and their Series) that + # don't cascade a `modified` bump onto this Team except on M2M + # add/remove/clear. + cache_action_dependent_labels = (ModelLabel.ISSUE, ModelLabel.SERIES) def get_queryset(self): queryset = super().get_queryset() @@ -605,7 +845,7 @@ class UniverseViewSet( UserTrackingMixin, mixins.CreateModelMixin, ConditionalRetrieveModelMixin, - mixins.ListModelMixin, + CachedListModelMixin, mixins.UpdateModelMixin, viewsets.GenericViewSet, ): @@ -620,6 +860,10 @@ class UniverseViewSet( queryset = Universe.objects.all() filterset_class = UniverseFilter parser_classes = (MultiPartParser, FormParser) + cache_model_label = ModelLabel.UNIVERSE + # Universe retrieve embeds its Publisher's name, which doesn't cascade a + # `modified` bump onto this Universe when renamed. + cache_detail_dependent_labels = (ModelLabel.PUBLISHER,) def get_queryset(self): queryset = super().get_queryset() diff --git a/comicsdb/apps.py b/comicsdb/apps.py index 2f6b4eb8..c9c03359 100644 --- a/comicsdb/apps.py +++ b/comicsdb/apps.py @@ -1,11 +1,17 @@ +from functools import partial + from django.apps import AppConfig from django.db.models.signals import m2m_changed, post_delete, post_save, pre_delete +from api.cache import ModelLabel from comicsdb.signals import ( + bump_cache, pre_delete_credit, pre_delete_image, update_arc_modified, update_character_modified, + update_issue_modified_on_credit_change, + update_issue_modified_on_credit_role_change, update_series_modified_on_issue_delete, update_series_modified_on_issue_save, update_team_modified, @@ -54,14 +60,64 @@ def ready(self): dispatch_uid="m2m_changed_issue_team_modified", ) + imprint = self.get_model("Imprint") + publisher = self.get_model("Publisher") pre_delete.connect(pre_delete_image, sender=publisher, dispatch_uid="pre_delete_publisher") + series = self.get_model("Series") + team = self.get_model("Team") pre_delete.connect(pre_delete_image, sender=team, dispatch_uid="pre_delete_team") + universe = self.get_model("Universe") + variant = self.get_model("Variant") pre_delete.connect(pre_delete_image, sender=variant, dispatch_uid="pre_delete_variant") credits_ = self.get_model("Credits") pre_delete.connect(pre_delete_credit, sender=credits_, dispatch_uid="pre_delete_credits") + post_save.connect( + update_issue_modified_on_credit_change, + sender=credits_, + dispatch_uid="post_save_credit_issue_modified", + ) + post_delete.connect( + update_issue_modified_on_credit_change, + sender=credits_, + dispatch_uid="post_delete_credit_issue_modified", + ) + m2m_changed.connect( + update_issue_modified_on_credit_role_change, + sender=credits_.role.through, + dispatch_uid="m2m_changed_credit_role_modified", + ) + + # Models whose cache invalidation is *only* "bump my own version + # counter" on every save/delete -- see bump_cache() in + # comicsdb/signals.py. Everything above this point wires up + # handlers with model-specific behavior (image cleanup, modified + # cascades); this is the uniform remainder. + cache_bump_models = ( + (arc, ModelLabel.ARC), + (character, ModelLabel.CHARACTER), + (creator, ModelLabel.CREATOR), + (imprint, ModelLabel.IMPRINT), + (publisher, ModelLabel.PUBLISHER), + (series, ModelLabel.SERIES), + (team, ModelLabel.TEAM), + (universe, ModelLabel.UNIVERSE), + ) + for model, label in cache_bump_models: + bumper = partial(bump_cache, label) + # weak=False: `bumper` is a local `partial` with no other + # strong reference, so Django's default weak-reference + # receiver storage would let it be garbage-collected right + # after this loop iteration ends, silently dropping the + # connection. + post_save.connect( + bumper, sender=model, weak=False, dispatch_uid=f"post_save_{label}_cache" + ) + post_delete.connect( + bumper, sender=model, weak=False, dispatch_uid=f"post_delete_{label}_cache" + ) diff --git a/comicsdb/signals.py b/comicsdb/signals.py index a8507dc2..8e454a51 100644 --- a/comicsdb/signals.py +++ b/comicsdb/signals.py @@ -3,6 +3,8 @@ from django.utils import timezone from sorl.thumbnail import delete +from api.cache import ModelLabel, bump_model_version + LOGGER = logging.getLogger(__name__) @@ -19,12 +21,16 @@ def update_series_modified_on_issue_save(sender, instance, **kwargs): from comicsdb.models import Series # noqa: PLC0415 Series.objects.filter(pk=instance.series_id).update(modified=timezone.now()) + bump_model_version(ModelLabel.ISSUE) + bump_model_version(ModelLabel.SERIES) def update_series_modified_on_issue_delete(sender, instance, **kwargs): from comicsdb.models import Series # noqa: PLC0415 Series.objects.filter(pk=instance.series_id).update(modified=timezone.now()) + bump_model_version(ModelLabel.ISSUE) + bump_model_version(ModelLabel.SERIES) def update_related_modified(parent_model, instance, action, pk_set): @@ -47,15 +53,58 @@ def update_arc_modified(sender, instance, action, pk_set, **kwargs): from comicsdb.models import Arc # noqa: PLC0415 update_related_modified(Arc, instance, action, pk_set) + if action in ("post_add", "post_remove", "post_clear"): + bump_model_version(ModelLabel.ARC) def update_character_modified(sender, instance, action, pk_set, **kwargs): from comicsdb.models import Character # noqa: PLC0415 update_related_modified(Character, instance, action, pk_set) + if action in ("post_add", "post_remove", "post_clear"): + bump_model_version(ModelLabel.CHARACTER) def update_team_modified(sender, instance, action, pk_set, **kwargs): from comicsdb.models import Team # noqa: PLC0415 update_related_modified(Team, instance, action, pk_set) + if action in ("post_add", "post_remove", "post_clear"): + bump_model_version(ModelLabel.TEAM) + + +def update_issue_modified_on_credit_change(sender, instance, **kwargs): + """Credits changes aren't reflected on the parent Issue's `modified` by + default; bump it explicitly so the issue's cached detail response + (which embeds credits) invalidates.""" + from comicsdb.models import Issue # noqa: PLC0415 + + Issue.objects.filter(pk=instance.issue_id).update(modified=timezone.now()) + bump_model_version(ModelLabel.ISSUE) + + +def update_issue_modified_on_credit_role_change(sender, instance, action, pk_set, **kwargs): + """Credits.role is a M2M -- .add()/.remove()/.set() don't call + Credits.save(), so update_issue_modified_on_credit_change (a post_save + hook) never fires for role-only changes. In particular, + CreditSerializer.create() calls Credits.objects.create() (bumping + Issue.modified with an empty role list) and only then calls + credit.role.add(...): without this second bump, a request landing in + that window could cache the issue with an empty role list under a + `modified` key that's never touched again.""" + if action not in ("post_add", "post_remove", "post_clear"): + return + + from comicsdb.models import Issue # noqa: PLC0415 + + Issue.objects.filter(pk=instance.issue_id).update(modified=timezone.now()) + bump_model_version(ModelLabel.ISSUE) + + +def bump_cache(label, sender, instance, **kwargs): + """Generic post_save/post_delete receiver for the models whose cache + invalidation is *only* "bump my own version counter" -- Arc, Character, + Creator, Imprint, Publisher, Series, Team, Universe. Wired up via + functools.partial(bump_cache, label) in comicsdb/apps.py so one + function covers all eight instead of eight near-identical wrappers.""" + bump_model_version(label) diff --git a/tests/api/test_audit_response_cache.py b/tests/api/test_audit_response_cache.py new file mode 100644 index 00000000..37b405cc --- /dev/null +++ b/tests/api/test_audit_response_cache.py @@ -0,0 +1,82 @@ +import io +import uuid +from unittest.mock import patch + +from django.core.cache import cache +from django.core.management import call_command + +from api.management.commands.audit_response_cache import Command + +COMMAND = "audit_response_cache" + + +def test_categorize_detail_key_extracts_model_label(): + key = ":1:api:detail:issue:42:1704085200.0" + assert Command._categorize(key) == "api:detail:issue" + + +def test_categorize_detail_key_with_dependent_labels_still_extracts_model(): + key = ":1:api:detail:series:7:1704085200.0:12-3" + assert Command._categorize(key) == "api:detail:series" + + +def test_categorize_list_key_extracts_model_label(): + key = ":1:api:list:arc::4-2:abcdef0123456789" + assert Command._categorize(key) == "api:list:arc" + + +def test_categorize_cachever_key(): + assert Command._categorize(":1:cachever:issue") == "cachever" + + +def test_categorize_unrelated_key_falls_back_to_other(): + assert Command._categorize(":1:django_select2:some-widget-id") == ( + "other (Select2, throttling, etc.)" + ) + + +def test_human_bytes_formats_across_units(): + assert Command._human_bytes(512) == "512.0 B" + assert Command._human_bytes(2048) == "2.0 KB" + assert Command._human_bytes(5 * 1024 * 1024) == "5.0 MB" + assert Command._human_bytes(None) == "n/a" + + +def test_report_counts_and_sizes_a_seeded_category(): + """Seed a handful of keys under a category unique to this test run (a + random model label) so the assertion is immune to whatever other keys + real Redis happens to hold -- other test workers/dev usage share this + same Redis instance, and the command intentionally scans everything.""" + label = f"audit-test-{uuid.uuid4().hex[:8]}" + keys = [f"api:detail:{label}:{i}:1704085200.0" for i in range(3)] + for key in keys: + cache.set(key, {"payload": "x" * 50}, 60) + + out = io.StringIO() + try: + call_command(COMMAND, stdout=out) + finally: + cache.delete_many(keys) + + output = out.getvalue() + assert f"api:detail:{label}" in output + assert "3 keys" in output + assert "Redis instance (global" in output + assert "By category" in output + + +def test_info_failure_is_reported_without_crashing(): + class _BrokenClient: + def info(self, *_args, **_kwargs): + raise ConnectionError("redis unavailable") + + def scan(self, cursor=0, **_kwargs): + return 0, [] + + with patch("django.core.cache.cache._cache.get_client", return_value=_BrokenClient()): + out = io.StringIO() + call_command(COMMAND, stdout=out) + + output = out.getvalue() + assert "INFO command unavailable" in output + assert "redis unavailable" in output diff --git a/tests/comicsdb/test_api_response_caching.py b/tests/comicsdb/test_api_response_caching.py new file mode 100644 index 00000000..a44f36a1 --- /dev/null +++ b/tests/comicsdb/test_api_response_caching.py @@ -0,0 +1,306 @@ +import uuid +from unittest.mock import patch + +import pytest +from django.core.cache.backends.locmem import LocMemCache +from django.db import connection +from django.test.utils import CaptureQueriesContext +from django.urls import reverse +from rest_framework import status + +from api.views import CollectionViewSet, PullListViewSet, WishListViewSet +from comicsdb.models import Credits + + +@pytest.fixture +def local_cache(): + """Isolate the response cache so assertions about exact hit/miss/version + behavior aren't affected by concurrent xdist workers sharing the real + Redis backend (the `cachever:*` counters are global-per-model).""" + test_cache = LocMemCache(f"test-api-response-caching-{uuid.uuid4()}", {}) + with patch("api.views.cache", test_cache), patch("api.cache.cache", test_cache): + yield test_cache + + +def test_issue_retrieve_cache_hit_skips_heavy_query( + api_client_with_credentials, basic_issue, local_cache +): + url = reverse("api:issue-detail", kwargs={"pk": basic_issue.pk}) + resp = api_client_with_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + first_body = resp.json() + + with CaptureQueriesContext(connection) as queries: + resp = api_client_with_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json() == first_body + + # Cache hit: none of the heavy prefetch/join queries the full retrieve + # queryset would run should appear -- only the cheap (pk, modified) + # lookup used for the conditional-request/cache-key check. + sql_statements = [q["sql"] for q in queries.captured_queries] + assert not any("comicsdb_credits" in sql for sql in sql_statements) + assert not any("comicsdb_issue_arcs" in sql for sql in sql_statements) + + +def test_issue_retrieve_after_update_is_not_stale( + api_client_with_staff_credentials, basic_issue, local_cache +): + url = reverse("api:issue-detail", kwargs={"pk": basic_issue.pk}) + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["desc"] != "Updated description" + + resp = api_client_with_staff_credentials.patch(url, {"desc": "Updated description"}) + assert resp.status_code == status.HTTP_200_OK + + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["desc"] == "Updated description" + + +def test_credits_change_busts_issue_detail_cache( + api_client_with_staff_credentials, basic_issue, john_byrne, writer, local_cache +): + url = reverse("api:issue-detail", kwargs={"pk": basic_issue.pk}) + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["credits"] == [] + + credit = Credits.objects.create(issue=basic_issue, creator=john_byrne) + credit.role.add(writer) + + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert len(resp.json()["credits"]) == 1 + assert resp.json()["credits"][0]["creator"] == john_byrne.name + + +def test_arc_list_reflects_newly_created_arc( + api_client_with_staff_credentials, wwh_arc, local_cache +): + url = reverse("api:arc-list") + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["count"] == 1 + + resp = api_client_with_staff_credentials.post(url, {"name": "Final Crisis", "desc": "New arc"}) + assert resp.status_code == status.HTTP_201_CREATED + + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["count"] == 2 + + +def test_arc_list_reflects_deleted_arc_via_admin( + api_client_with_credentials, wwh_arc, fc_arc, local_cache +): + url = reverse("api:arc-list") + resp = api_client_with_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["count"] == 2 + + fc_arc.delete() + + resp = api_client_with_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["count"] == 1 + + +def test_series_list_reflects_new_issue_num_issues( + api_client_with_staff_credentials, fc_series, basic_issue, local_cache +): + url = reverse("api:series-list") + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + result = next(r for r in resp.json()["results"] if r["id"] == fc_series.pk) + assert result["issue_count"] == 1 + + resp = api_client_with_staff_credentials.post( + reverse("api:issue-list"), + {"series": fc_series.pk, "number": "2", "cover_date": "2008-01-01"}, + ) + assert resp.status_code == status.HTTP_201_CREATED + + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + result = next(r for r in resp.json()["results"] if r["id"] == fc_series.pk) + assert result["issue_count"] == 2 + + +def test_publisher_series_list_reflects_new_series( + api_client_with_staff_credentials, dc_comics, fc_series, single_issue_type, local_cache +): + url = reverse("api:publisher-series-list", kwargs={"pk": dc_comics.pk}) + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["count"] == 1 + + resp = api_client_with_staff_credentials.post( + reverse("api:series-list"), + { + "name": "New Series", + "sort_name": "New Series", + "volume": 1, + "publisher": dc_comics.pk, + "series_type": single_issue_type.pk, + "year_began": 2024, + "status": 4, + }, + ) + assert resp.status_code == status.HTTP_201_CREATED + + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["count"] == 2 + + +def test_publisher_series_list_404s_after_publisher_deleted( + api_client_with_staff_credentials, dc_comics, fc_series, local_cache +): + """series_list must call get_object() (existence check) before serving + a cache hit -- otherwise a deleted publisher's cached series list would + keep returning 200 instead of 404 until the list cache TTL expires.""" + url = reverse("api:publisher-series-list", kwargs={"pk": dc_comics.pk}) + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + + fc_series.delete() + dc_comics.delete() + + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_404_NOT_FOUND + + +def test_arc_issue_list_reflects_issue_field_edit( + api_client_with_staff_credentials, issue_with_arc, fc_arc, local_cache +): + """issue_list is cached under the parent Arc's own `modified`, which + only bumps on M2M add/remove/clear -- not on a plain field edit to one + of the linked issues. cache_action_dependent_labels ties the cache key + to the Issue model's version counter too, so an edit like this should + still be visible without waiting for the parent's `modified` to catch + up.""" + url = reverse("api:arc-issue-list", kwargs={"pk": fc_arc.pk}) + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["results"][0]["number"] == "1" + + issue_with_arc.number = "2" + issue_with_arc.save() + + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["results"][0]["number"] == "2" + + +def test_series_retrieve_after_publisher_rename_is_not_stale( + api_client_with_staff_credentials, fc_series, dc_comics, local_cache +): + """Series retrieve embeds the Publisher's name, but renaming a + Publisher doesn't bump the owning Series' `modified`. + cache_detail_dependent_labels ties the Series detail cache key to the + Publisher model's version counter too, so the rename should show up + without waiting on the Series' own `modified`.""" + url = reverse("api:series-detail", kwargs={"pk": fc_series.pk}) + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["publisher"]["name"] == "DC Comics" + + dc_comics.name = "DC Comics Renamed" + dc_comics.save() + + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["publisher"]["name"] == "DC Comics Renamed" + + +def test_credit_role_add_after_create_does_not_stick_empty_roles( + api_client_with_staff_credentials, basic_issue, john_byrne, writer, local_cache +): + """CreditSerializer.create() calls Credits.objects.create() (bumping + Issue.modified once) and only then credit.role.add(...). A request + landing between those two steps -- simulated here by fetching before + role.add() runs -- must not permanently cache the issue with an empty + role list: the m2m_changed bump on role.add() has to produce a new + `modified` value so the stale entry is orphaned rather than reused.""" + url = reverse("api:issue-detail", kwargs={"pk": basic_issue.pk}) + + credit = Credits.objects.create(issue=basic_issue, creator=john_byrne) + + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["credits"][0]["role"] == [] + + credit.role.add(writer) + + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["credits"][0]["role"][0]["name"] == "Writer" + + +def test_issue_retrieve_after_series_rename_is_not_stale( + api_client_with_staff_credentials, basic_issue, fc_series, local_cache +): + """Issue retrieve embeds its Series' name, but renaming a Series + doesn't bump the owning Issue's `modified`. cache_detail_dependent_labels + ties the Issue detail cache key to the Series model's version counter + too, so the rename should show up without waiting on the Issue's own + `modified`.""" + url = reverse("api:issue-detail", kwargs={"pk": basic_issue.pk}) + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["series"]["name"] == "Final Crisis" + + fc_series.name = "Final Crisis Renamed" + fc_series.save() + + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["series"]["name"] == "Final Crisis Renamed" + + +def test_imprint_retrieve_after_publisher_rename_is_not_stale( + api_client_with_staff_credentials, vertigo_imprint, dc_comics, local_cache +): + """Imprint retrieve embeds its Publisher's name, but renaming a + Publisher doesn't bump the owning Imprint's `modified`.""" + url = reverse("api:imprint-detail", kwargs={"pk": vertigo_imprint.pk}) + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["publisher"]["name"] == "DC Comics" + + dc_comics.name = "DC Comics Renamed" + dc_comics.save() + + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["publisher"]["name"] == "DC Comics Renamed" + + +def test_arc_issue_list_reflects_series_rename( + api_client_with_staff_credentials, issue_with_arc, fc_arc, fc_series, local_cache +): + """issue_list nests each issue's Series name; renaming the Series + doesn't bump the parent Arc's `modified`.""" + url = reverse("api:arc-issue-list", kwargs={"pk": fc_arc.pk}) + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["results"][0]["series"]["name"] == "Final Crisis" + + fc_series.name = "Final Crisis Renamed" + fc_series.save() + + resp = api_client_with_staff_credentials.get(url) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["results"][0]["series"]["name"] == "Final Crisis Renamed" + + +def test_user_scoped_viewsets_are_not_list_cached(): + """CollectionViewSet/PullListViewSet/WishListViewSet are user-scoped + (get_queryset filters by request.user) -- they must never use + CachedListModelMixin, since a shared list cache key would serve one + user's private data to another. This is a cheap regression guard for + that intentional exclusion; see api/views.py.""" + for viewset in (CollectionViewSet, PullListViewSet, WishListViewSet): + assert getattr(viewset, "cache_model_label", None) is None