diff --git a/api/views.py b/api/views.py index eac5651e..f837da0e 100644 --- a/api/views.py +++ b/api/views.py @@ -14,6 +14,7 @@ from django.http import Http404 from django.shortcuts import get_object_or_404 from django.utils import timezone +from django.utils.http import parse_http_date_safe from djmoney.money import Money from drf_spectacular.utils import OpenApiParameter, extend_schema from rest_framework import mixins, status, viewsets @@ -77,6 +78,7 @@ WishListItemReadSerializer, WishListSerializer, ) +from comicsdb.cache import get_last_modified, set_last_modified from comicsdb.filters.collection import CollectionFilter from comicsdb.filters.issue import IssueFilter from comicsdb.filters.name import ComicVineFilter, NameFilter, UniverseFilter @@ -111,6 +113,8 @@ class ReadingListItemsPagination(PageNumberPagination): class CachedObjectMixin: + """Memoizes get_object() per request.""" + def get_object(self): if not hasattr(self, "_cached_object"): self._cached_object = super().get_object() @@ -118,19 +122,45 @@ def get_object(self): return self._cached_object -class ConditionalRetrieveModelMixin(CachedObjectMixin, mixins.RetrieveModelMixin): - def retrieve(self, request, *args, **kwargs): - retrieve = last_modified(last_modified_func=self._retrieve_last_modified)(super().retrieve) - - return retrieve(self, request, *args, **kwargs) +class LastModifiedMixin(CachedObjectMixin): + """Supplies `_last_modified` via a plain DB fetch.""" - def _retrieve_last_modified(self, *args, **kwargs): + def _last_modified(self, *args, **kwargs): obj = self.get_object() - if obj and getattr(obj, "modified", None): - return obj.modified + return getattr(obj, "modified", None) if obj else None + + +class CachedLastModifiedMixin(LastModifiedMixin): + """Answers conditional-GET checks from Redis, skipping the DB on a cache hit. + + Only use on viewsets whose queryset isn't filtered by request.user - a cache + hit skips that filtering, which would leak other users' rows via a 304. + """ + + def _last_modified(self, request, *args, **kwargs): + if_modified_since = parse_http_date_safe(request.META.get("HTTP_IF_MODIFIED_SINCE", "")) + + if if_modified_since is not None: + pk = self.kwargs.get(self.lookup_url_kwarg or self.lookup_field) + cached = get_last_modified(self.get_queryset().model, pk) if pk else None + + if cached is not None and int(cached.timestamp()) <= if_modified_since: + return cached + + dt = super()._last_modified(request, *args, **kwargs) - return None + if if_modified_since is not None and dt is not None: + set_last_modified(self.get_object()) + + return dt + + +class ConditionalRetrieveModelMixin(LastModifiedMixin, mixins.RetrieveModelMixin): + def retrieve(self, request, *args, **kwargs): + retrieve = last_modified(last_modified_func=self._last_modified)(super().retrieve) + + return retrieve(self, request, *args, **kwargs) class UserTrackingMixin: @@ -143,7 +173,7 @@ def perform_update(self, serializer): serializer.save(edited_by=self.request.user) -class IssueListMixin(CachedObjectMixin): +class IssueListMixin(LastModifiedMixin): """Mixin to provide a standard issue_list action for related models.""" def get_issue_queryset(self, obj): @@ -155,9 +185,7 @@ def get_issue_queryset(self, obj): @extend_schema(responses={200: IssueListSerializer(many=True)}, filters=False) @action(detail=True) def issue_list(self, request, *args, **kwargs): - issue_list = last_modified(last_modified_func=self._issue_list_last_modified)( - self._issue_list - ) + issue_list = last_modified(last_modified_func=self._last_modified)(self._issue_list) return issue_list(self, request, *args, **kwargs) @@ -171,16 +199,9 @@ def _issue_list(self, request, *args, **kwargs): return self.get_paginated_response(serializer.data) raise Http404 - def _issue_list_last_modified(self, *args, **kwargs): - obj = self.get_object() - - if obj and getattr(obj, "modified", None): - return obj.modified - - return None - class ArcViewSet( + CachedLastModifiedMixin, UserTrackingMixin, IssueListMixin, mixins.CreateModelMixin, @@ -212,6 +233,7 @@ def get_serializer_class(self): class CharacterViewSet( + CachedLastModifiedMixin, UserTrackingMixin, IssueListMixin, mixins.CreateModelMixin, @@ -251,6 +273,7 @@ def get_serializer_class(self): class CreatorViewSet( + CachedLastModifiedMixin, UserTrackingMixin, mixins.CreateModelMixin, ConditionalRetrieveModelMixin, @@ -301,6 +324,7 @@ def create(self, request, *args, **kwargs) -> Response: class ImprintViewSet( + CachedLastModifiedMixin, UserTrackingMixin, mixins.CreateModelMixin, ConditionalRetrieveModelMixin, @@ -343,6 +367,7 @@ def get_serializer_class(self): class IssueViewSet( + CachedLastModifiedMixin, UserTrackingMixin, mixins.CreateModelMixin, ConditionalRetrieveModelMixin, @@ -414,6 +439,7 @@ def get_serializer_class(self): class PublisherViewSet( + CachedLastModifiedMixin, UserTrackingMixin, mixins.CreateModelMixin, ConditionalRetrieveModelMixin, @@ -479,6 +505,7 @@ class RoleViewset(mixins.ListModelMixin, viewsets.GenericViewSet): class SeriesViewSet( + CachedLastModifiedMixin, UserTrackingMixin, IssueListMixin, mixins.CreateModelMixin, @@ -562,6 +589,7 @@ class SeriesTypeViewSet(mixins.ListModelMixin, viewsets.GenericViewSet): class TeamViewSet( + CachedLastModifiedMixin, UserTrackingMixin, IssueListMixin, mixins.CreateModelMixin, @@ -601,6 +629,7 @@ def get_serializer_class(self): class UniverseViewSet( + CachedLastModifiedMixin, UserTrackingMixin, mixins.CreateModelMixin, ConditionalRetrieveModelMixin, diff --git a/comicsdb/apps.py b/comicsdb/apps.py index 2f6b4eb8..675ea044 100644 --- a/comicsdb/apps.py +++ b/comicsdb/apps.py @@ -2,6 +2,7 @@ from django.db.models.signals import m2m_changed, post_delete, post_save, pre_delete from comicsdb.signals import ( + post_delete_last_modified, pre_delete_credit, pre_delete_image, update_arc_modified, @@ -65,3 +66,14 @@ def ready(self): credits_ = self.get_model("Credits") pre_delete.connect(pre_delete_credit, sender=credits_, dispatch_uid="pre_delete_credits") + + # Clear the cache entry on delete, so a removed row 404s instead of 304ing. + from comicsdb.models.common import LastModifiedCacheMixin # noqa: PLC0415 + + for model in self.get_models(): + if issubclass(model, LastModifiedCacheMixin): + post_delete.connect( + post_delete_last_modified, + sender=model, + dispatch_uid=f"post_delete_last_modified_{model._meta.model_name}", + ) diff --git a/comicsdb/cache.py b/comicsdb/cache.py new file mode 100644 index 00000000..c8719802 --- /dev/null +++ b/comicsdb/cache.py @@ -0,0 +1,65 @@ +"""Redis-backed cache of each cacheable model's `modified` timestamp.""" + +import logging +from datetime import UTC, datetime + +from django.core.cache import cache + +LOGGER = logging.getLogger(__name__) + +LAST_MODIFIED_CACHE_TTL = 60 * 60 * 24 * 30 # 30 days + + +def last_modified_cache_key(model, pk) -> str: + return f"modified:{model._meta.label_lower}:{pk}" + + +def _safe_get(key): + try: + return cache.get(key) + except Exception: # noqa: BLE001 + LOGGER.warning("Failed to read cache key %s", key, exc_info=True) + return None + + +def _safe_set(key, value, timeout): + try: + cache.set(key, value, timeout) + except Exception: # noqa: BLE001 + LOGGER.warning("Failed to write cache key %s", key, exc_info=True) + + +def _safe_delete_many(keys): + try: + cache.delete_many(keys) + except Exception: # noqa: BLE001 + LOGGER.warning("Failed to delete cache keys %s", keys, exc_info=True) + + +def get_last_modified(model, pk) -> datetime | None: + """Cached `modified` for `model`/`pk`, or None on a miss.""" + value = _safe_get(last_modified_cache_key(model, pk)) + + if not isinstance(value, int): + return None + + return datetime.fromtimestamp(value, tz=UTC) + + +def set_last_modified(instance) -> None: + """Write-through cache update for a model instance.""" + modified = getattr(instance, "modified", None) + + if modified is None: + return + + key = last_modified_cache_key(instance.__class__, instance.pk) + _safe_set(key, int(modified.timestamp()), LAST_MODIFIED_CACHE_TTL) + + +def delete_last_modified(model, pk) -> None: + _safe_delete_many([last_modified_cache_key(model, pk)]) + + +def delete_last_modified_many(model, pks) -> None: + _safe_delete_many([last_modified_cache_key(model, pk) for pk in pks]) diff --git a/comicsdb/models/arc.py b/comicsdb/models/arc.py index c17c8641..1337eb60 100644 --- a/comicsdb/models/arc.py +++ b/comicsdb/models/arc.py @@ -13,13 +13,13 @@ from sorl.thumbnail import ImageField from comicsdb.models.attribution import Attribution -from comicsdb.models.common import CommonInfo, pre_save_slug +from comicsdb.models.common import CommonInfo, LastModifiedCacheMixin, pre_save_slug from users.models import CustomUser LOGGER = logging.getLogger(__name__) -class Arc(CommonInfo): +class Arc(LastModifiedCacheMixin, CommonInfo): image = ImageField(upload_to="arc/%Y/%m/%d/", blank=True) attribution = GenericRelation(Attribution, related_query_name="arcs") created_by = models.ForeignKey( diff --git a/comicsdb/models/character.py b/comicsdb/models/character.py index 1e1983f9..8e0c628b 100644 --- a/comicsdb/models/character.py +++ b/comicsdb/models/character.py @@ -14,7 +14,7 @@ from sorl.thumbnail import ImageField from comicsdb.models.attribution import Attribution -from comicsdb.models.common import CommonInfo, pre_save_slug +from comicsdb.models.common import CommonInfo, LastModifiedCacheMixin, pre_save_slug from comicsdb.models.creator import Creator from comicsdb.models.team import Team from comicsdb.models.universe import Universe @@ -23,7 +23,7 @@ LOGGER = logging.getLogger(__name__) -class Character(CommonInfo): +class Character(LastModifiedCacheMixin, CommonInfo): image = ImageField(upload_to="character/%Y/%m/%d/", blank=True) alias = ArrayField(models.CharField(max_length=100), blank=True, default=list) creators = models.ManyToManyField(Creator, blank=True, related_name="characters") diff --git a/comicsdb/models/common.py b/comicsdb/models/common.py index 318f2c59..598c38ee 100644 --- a/comicsdb/models/common.py +++ b/comicsdb/models/common.py @@ -4,6 +4,8 @@ from django.db.models.functions import Now from django.utils.text import slugify +from comicsdb.cache import set_last_modified + MIN_RATING = 1 MAX_RATING = 5 RATING_CHOICES = [(i, str(i)) for i in range(MIN_RATING, MAX_RATING + 1)] @@ -37,6 +39,20 @@ def pre_save_slug(sender, instance, **kwargs): instance.slug = generate_slug_from_name(instance) +class LastModifiedCacheMixin(models.Model): + """Writes `modified` to comicsdb.cache on every save(). + + Use only on models whose viewset reads it via CachedLastModifiedMixin. + """ + + class Meta: + abstract = True + + def save(self, *args, **kwargs): + super().save(*args, **kwargs) + set_last_modified(self) + + class CommonInfo(models.Model): name = models.CharField(max_length=255) slug = models.SlugField(max_length=255, unique=True) diff --git a/comicsdb/models/creator.py b/comicsdb/models/creator.py index 81b47bb4..f4b82f44 100644 --- a/comicsdb/models/creator.py +++ b/comicsdb/models/creator.py @@ -14,13 +14,13 @@ from sorl.thumbnail import ImageField from comicsdb.models.attribution import Attribution -from comicsdb.models.common import CommonInfo, pre_save_slug +from comicsdb.models.common import CommonInfo, LastModifiedCacheMixin, pre_save_slug from users.models import CustomUser LOGGER = logging.getLogger(__name__) -class Creator(CommonInfo): +class Creator(LastModifiedCacheMixin, CommonInfo): birth = models.DateField("Date of Birth", null=True, blank=True) death = models.DateField("Date of Death", null=True, blank=True) image = ImageField(upload_to="creator/%Y/%m/%d/", blank=True) diff --git a/comicsdb/models/imprint.py b/comicsdb/models/imprint.py index 59830a17..e514067a 100644 --- a/comicsdb/models/imprint.py +++ b/comicsdb/models/imprint.py @@ -13,14 +13,14 @@ from sorl.thumbnail import ImageField from comicsdb.models.attribution import Attribution -from comicsdb.models.common import CommonInfo, pre_save_slug +from comicsdb.models.common import CommonInfo, LastModifiedCacheMixin, pre_save_slug from comicsdb.models.publisher import Publisher from users.models import CustomUser LOGGER = logging.getLogger(__name__) -class Imprint(CommonInfo): +class Imprint(LastModifiedCacheMixin, CommonInfo): publisher = models.ForeignKey(Publisher, on_delete=models.CASCADE, related_name="imprints") founded = models.PositiveSmallIntegerField("Year Founded", null=True, blank=True) image = ImageField("Logo", upload_to="imprint/%Y/%m/%d", null=True, blank=True) diff --git a/comicsdb/models/issue.py b/comicsdb/models/issue.py index a3091898..0a20b1d5 100644 --- a/comicsdb/models/issue.py +++ b/comicsdb/models/issue.py @@ -20,7 +20,7 @@ from comicsdb.models.arc import Arc from comicsdb.models.attribution import Attribution from comicsdb.models.character import Character -from comicsdb.models.common import CommonInfo +from comicsdb.models.common import CommonInfo, LastModifiedCacheMixin from comicsdb.models.creator import Creator from comicsdb.models.rating import Rating from comicsdb.models.series import Series @@ -51,7 +51,7 @@ def get_queryset(self): ) -class Issue(CommonInfo): +class Issue(LastModifiedCacheMixin, CommonInfo): series = models.ForeignKey(Series, on_delete=models.CASCADE, related_name="issues") name = ArrayField(models.CharField("Story Title", max_length=150), blank=True, default=list) title = models.CharField("Collection Title", max_length=255, blank=True) diff --git a/comicsdb/models/publisher.py b/comicsdb/models/publisher.py index 62f0d1fe..0406a4de 100644 --- a/comicsdb/models/publisher.py +++ b/comicsdb/models/publisher.py @@ -14,13 +14,13 @@ from sorl.thumbnail import ImageField from comicsdb.models.attribution import Attribution -from comicsdb.models.common import CommonInfo, pre_save_slug +from comicsdb.models.common import CommonInfo, LastModifiedCacheMixin, pre_save_slug from users.models import CustomUser LOGGER = logging.getLogger(__name__) -class Publisher(CommonInfo): +class Publisher(LastModifiedCacheMixin, CommonInfo): founded = models.PositiveSmallIntegerField("Year Founded", null=True, blank=True) country = CountryField(default="US") image = ImageField("Logo", upload_to="publisher/%Y/%m/%d/", blank=True) diff --git a/comicsdb/models/series.py b/comicsdb/models/series.py index f4b31c3b..70799947 100644 --- a/comicsdb/models/series.py +++ b/comicsdb/models/series.py @@ -14,7 +14,7 @@ from comicsdb.db_functions import ArrayToString from comicsdb.models.attribution import Attribution -from comicsdb.models.common import CommonInfo +from comicsdb.models.common import CommonInfo, LastModifiedCacheMixin from comicsdb.models.genre import Genre from comicsdb.models.imprint import Imprint from comicsdb.models.publisher import Publisher @@ -36,7 +36,7 @@ def __str__(self) -> str: return self.name -class Series(CommonInfo): +class Series(LastModifiedCacheMixin, CommonInfo): class Status(models.IntegerChoices): CANCELLED = 1 COMPLETED = 2 diff --git a/comicsdb/models/team.py b/comicsdb/models/team.py index b188ebf4..f5434512 100644 --- a/comicsdb/models/team.py +++ b/comicsdb/models/team.py @@ -13,7 +13,7 @@ from sorl.thumbnail import ImageField from comicsdb.models.attribution import Attribution -from comicsdb.models.common import CommonInfo, pre_save_slug +from comicsdb.models.common import CommonInfo, LastModifiedCacheMixin, pre_save_slug from comicsdb.models.creator import Creator from comicsdb.models.universe import Universe from users.models import CustomUser @@ -21,7 +21,7 @@ LOGGER = logging.getLogger(__name__) -class Team(CommonInfo): +class Team(LastModifiedCacheMixin, CommonInfo): image = ImageField(upload_to="team/%Y/%m/%d/", blank=True) creators = models.ManyToManyField(Creator, blank=True, related_name="teams") universes = models.ManyToManyField(Universe, blank=True, related_name="teams") diff --git a/comicsdb/models/universe.py b/comicsdb/models/universe.py index 59d860ea..17eb1af2 100644 --- a/comicsdb/models/universe.py +++ b/comicsdb/models/universe.py @@ -13,14 +13,14 @@ from sorl.thumbnail import ImageField from comicsdb.models.attribution import Attribution -from comicsdb.models.common import CommonInfo, pre_save_slug +from comicsdb.models.common import CommonInfo, LastModifiedCacheMixin, pre_save_slug from comicsdb.models.publisher import Publisher from users.models import CustomUser LOGGER = logging.getLogger(__name__) -class Universe(CommonInfo): +class Universe(LastModifiedCacheMixin, CommonInfo): publisher = models.ForeignKey(Publisher, on_delete=models.CASCADE, related_name="universes") image = ImageField(upload_to="universe/%Y/%m/%d/", blank=True) designation = models.CharField(max_length=255, blank=True) diff --git a/comicsdb/signals.py b/comicsdb/signals.py index a8507dc2..b1b53f92 100644 --- a/comicsdb/signals.py +++ b/comicsdb/signals.py @@ -1,8 +1,11 @@ import logging +from django.db import transaction from django.utils import timezone from sorl.thumbnail import delete +from comicsdb.cache import delete_last_modified, delete_last_modified_many + LOGGER = logging.getLogger(__name__) @@ -19,43 +22,65 @@ 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()) + transaction.on_commit(lambda pk=instance.series_id: delete_last_modified(Series, pk)) 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()) + transaction.on_commit(lambda pk=instance.series_id: delete_last_modified(Series, pk)) -def update_related_modified(parent_model, instance, action, pk_set): - """Shared logic for M2M post_add/post_remove/post_clear on Arc, Character, Team.""" - if action not in ("post_add", "post_remove", "post_clear"): +def update_related_modified(parent_model, field_name, instance, action, pk_set): + """Shared logic for M2M pre_clear/post_add/post_remove/post_clear on Arc, Character, Team.""" + if action not in ("pre_clear", "post_add", "post_remove", "post_clear"): return from comicsdb.models import Issue # noqa: PLC0415 if isinstance(instance, Issue): - # pk_set is None for post_clear; skip since affected parents are unknown + if action == "pre_clear": + # pk_set is None for post_clear; snapshot the affected parents now, + # before the through rows are removed, so post_clear can invalidate them. + instance._cleared_pks = set(getattr(instance, field_name).values_list("pk", flat=True)) + return + + if action == "post_clear": + pk_set = getattr(instance, "_cleared_pks", None) + if hasattr(instance, "_cleared_pks"): + del instance._cleared_pks + if pk_set: parent_model.objects.filter(pk__in=pk_set).update(modified=timezone.now()) - else: + transaction.on_commit( + lambda pks=frozenset(pk_set): delete_last_modified_many(parent_model, pks) + ) + elif action != "pre_clear": # instance is the parent (e.g. arc.issues.add/clear(...)) parent_model.objects.filter(pk=instance.pk).update(modified=timezone.now()) + transaction.on_commit(lambda pk=instance.pk: delete_last_modified(parent_model, pk)) + + +def post_delete_last_modified(sender, instance, **kwargs): + # instance.pk is reset to None by Model.delete() right after this signal + # fires, so it must be captured now rather than read when on_commit runs. + transaction.on_commit(lambda pk=instance.pk: delete_last_modified(sender, pk)) 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) + update_related_modified(Arc, "arcs", instance, action, pk_set) 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) + update_related_modified(Character, "characters", instance, action, pk_set) 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) + update_related_modified(Team, "teams", instance, action, pk_set) diff --git a/tests/comicsdb/conftest.py b/tests/comicsdb/conftest.py index 7693d804..d4ea0103 100644 --- a/tests/comicsdb/conftest.py +++ b/tests/comicsdb/conftest.py @@ -1,10 +1,12 @@ # ruff: noqa: PLC0415 import uuid from datetime import date, datetime +from unittest.mock import patch import pytest from django.contrib.auth.models import Group, Permission from django.contrib.contenttypes.models import ContentType +from django.core.cache.backends.locmem import LocMemCache from django.core.management import call_command from django.utils import timezone @@ -22,6 +24,14 @@ NUMBER_OF_ISSUES = 35 +@pytest.fixture(autouse=True) +def isolated_last_modified_cache(): + """Isolates comicsdb.cache from the shared Redis (see test_auth_method_tracking.py).""" + test_cache = LocMemCache(f"test-last-modified-cache-{uuid.uuid4()}", {}) + with patch("comicsdb.cache.cache", test_cache): + yield test_cache + + @pytest.fixture def test_password(): return "strong-test-pass" diff --git a/tests/comicsdb/test_api_conditional_requests.py b/tests/comicsdb/test_api_conditional_requests.py index 6999ff4a..eb16a6e7 100644 --- a/tests/comicsdb/test_api_conditional_requests.py +++ b/tests/comicsdb/test_api_conditional_requests.py @@ -1,6 +1,20 @@ +from datetime import timedelta +from unittest.mock import Mock, patch + from django.urls import reverse +from django.utils import timezone +from django.utils.http import http_date, parse_http_date from rest_framework import status +from comicsdb.cache import get_last_modified, set_last_modified +from comicsdb.models.arc import Arc +from comicsdb.models.issue import Issue + + +def _stale_since(resp): + """Backdate a Last-Modified header by 2s, to dodge HTTP's 1s date resolution.""" + return http_date(parse_http_date(resp["Last-Modified"]) - 2) + def test_arc_returns_last_modified_header(api_client_with_credentials, wwh_arc): resp = api_client_with_credentials.get(reverse("api:arc-detail", kwargs={"pk": wwh_arc.pk})) @@ -225,3 +239,207 @@ def test_issue_list_conditional_request_with_old_date_returns_200( HTTP_IF_MODIFIED_SINCE="Wed, 01 Jan 2020 00:00:00 GMT", ) assert resp.status_code == status.HTTP_200_OK + + +def test_arc_detail_hit_avoids_db_fetch(api_client_with_credentials, wwh_arc): + """A cache hit must short-circuit to 304 without calling get_object().""" + resp = api_client_with_credentials.get(reverse("api:arc-detail", kwargs={"pk": wwh_arc.pk})) + assert resp.status_code == status.HTTP_200_OK + last_modified = resp["Last-Modified"] + + with patch( + "api.views.CachedObjectMixin.get_object", + side_effect=AssertionError("get_object() was called; cache was not consulted"), + ): + resp = api_client_with_credentials.get( + reverse("api:arc-detail", kwargs={"pk": wwh_arc.pk}), + HTTP_IF_MODIFIED_SINCE=last_modified, + ) + assert resp.status_code == status.HTTP_304_NOT_MODIFIED + + +def test_arc_detail_hit_does_not_rewrite_cache(api_client_with_credentials, wwh_arc): + """A cache hit must not re-write the unchanged value on every request.""" + resp = api_client_with_credentials.get(reverse("api:arc-detail", kwargs={"pk": wwh_arc.pk})) + assert resp.status_code == status.HTTP_200_OK + last_modified = resp["Last-Modified"] + + with patch("comicsdb.cache.cache.set") as mock_set: + resp = api_client_with_credentials.get( + reverse("api:arc-detail", kwargs={"pk": wwh_arc.pk}), + HTTP_IF_MODIFIED_SINCE=last_modified, + ) + assert resp.status_code == status.HTTP_304_NOT_MODIFIED + mock_set.assert_not_called() + + +def test_arc_plain_get_does_not_touch_cache(api_client_with_credentials, wwh_arc): + """A plain (non-conditional) GET - the majority of real traffic - always + fetches from the DB anyway, so it should skip the cache entirely rather than + read a value it won't use or write one back that's already correct.""" + resp = api_client_with_credentials.get(reverse("api:arc-detail", kwargs={"pk": wwh_arc.pk})) + assert resp.status_code == status.HTTP_200_OK + + with patch("comicsdb.cache.cache") as mock_cache: + resp = api_client_with_credentials.get(reverse("api:arc-detail", kwargs={"pk": wwh_arc.pk})) + assert resp.status_code == status.HTTP_200_OK + mock_cache.get.assert_not_called() + mock_cache.set.assert_not_called() + + +def test_arc_conditional_request_self_heals_after_cache_flush( + api_client_with_credentials, wwh_arc, isolated_last_modified_cache +): + resp = api_client_with_credentials.get(reverse("api:arc-detail", kwargs={"pk": wwh_arc.pk})) + assert resp.status_code == status.HTTP_200_OK + last_modified = resp["Last-Modified"] + + isolated_last_modified_cache.clear() + assert get_last_modified(Arc, wwh_arc.pk) is None + + resp = api_client_with_credentials.get( + reverse("api:arc-detail", kwargs={"pk": wwh_arc.pk}), + HTTP_IF_MODIFIED_SINCE=last_modified, + ) + assert resp.status_code == status.HTTP_304_NOT_MODIFIED + assert get_last_modified(Arc, wwh_arc.pk) is not None + + +def test_arc_conditional_request_degrades_gracefully_when_cache_read_fails( + api_client_with_credentials, wwh_arc +): + resp = api_client_with_credentials.get(reverse("api:arc-detail", kwargs={"pk": wwh_arc.pk})) + assert resp.status_code == status.HTTP_200_OK + last_modified = resp["Last-Modified"] + + with patch("comicsdb.cache.cache.get", side_effect=Exception("redis down")): + resp = api_client_with_credentials.get( + reverse("api:arc-detail", kwargs={"pk": wwh_arc.pk}), + HTTP_IF_MODIFIED_SINCE=last_modified, + ) + assert resp.status_code == status.HTTP_304_NOT_MODIFIED + + +def test_arc_conditional_request_returns_200_after_save(api_client_with_credentials, wwh_arc): + resp = api_client_with_credentials.get(reverse("api:arc-detail", kwargs={"pk": wwh_arc.pk})) + assert resp.status_code == status.HTTP_200_OK + last_modified = _stale_since(resp) + + wwh_arc.desc = "updated description" + wwh_arc.save() + + resp = api_client_with_credentials.get( + reverse("api:arc-detail", kwargs={"pk": wwh_arc.pk}), + HTTP_IF_MODIFIED_SINCE=last_modified, + ) + assert resp.status_code == status.HTTP_200_OK + + +def test_arc_conditional_request_200_uses_db_value_not_stale_cache( + api_client_with_credentials, wwh_arc +): + """When the cache indicates a change (so the response will be a 200 either + way), the Last-Modified header must come from the DB fetch, not whatever + value happened to be cached.""" + stale_cached = Mock(spec=["pk", "modified"]) + stale_cached.__class__ = Arc + stale_cached.pk = wwh_arc.pk + stale_cached.modified = wwh_arc.modified - timedelta(seconds=50) + set_last_modified(stale_cached) + + since = http_date(int(wwh_arc.modified.timestamp()) - 100) + + resp = api_client_with_credentials.get( + reverse("api:arc-detail", kwargs={"pk": wwh_arc.pk}), + HTTP_IF_MODIFIED_SINCE=since, + ) + assert resp.status_code == status.HTTP_200_OK + assert parse_http_date(resp["Last-Modified"]) == int(wwh_arc.modified.timestamp()) + + +def test_arc_detail_returns_404_after_delete_not_304( + api_client_with_credentials, wwh_arc, django_capture_on_commit_callbacks +): + resp = api_client_with_credentials.get(reverse("api:arc-detail", kwargs={"pk": wwh_arc.pk})) + assert resp.status_code == status.HTTP_200_OK + last_modified = resp["Last-Modified"] + arc_pk = wwh_arc.pk + + # Cache invalidation runs on transaction.on_commit(); pytest-django's db + # fixture wraps the test in a transaction that's rolled back rather than + # committed, so the callback must be captured and executed explicitly to + # simulate what happens on a real (committing) delete. + with django_capture_on_commit_callbacks(execute=True): + wwh_arc.delete() + + resp = api_client_with_credentials.get( + reverse("api:arc-detail", kwargs={"pk": arc_pk}), + HTTP_IF_MODIFIED_SINCE=last_modified, + ) + assert resp.status_code == status.HTTP_404_NOT_FOUND + + +def test_series_detail_conditional_request_returns_200_after_issue_added( + api_client_with_credentials, fc_series, basic_issue +): + resp = api_client_with_credentials.get( + reverse("api:series-detail", kwargs={"pk": fc_series.pk}) + ) + assert resp.status_code == status.HTTP_200_OK + last_modified = _stale_since(resp) + + Issue.objects.create( + series=fc_series, + number="2", + slug="final-crisis-2", + cover_date=timezone.now().date(), + edited_by=basic_issue.edited_by, + created_by=basic_issue.created_by, + ) + + resp = api_client_with_credentials.get( + reverse("api:series-detail", kwargs={"pk": fc_series.pk}), + HTTP_IF_MODIFIED_SINCE=last_modified, + ) + assert resp.status_code == status.HTTP_200_OK + + +def test_series_issue_list_conditional_request_returns_200_after_issue_added( + api_client_with_credentials, fc_series, basic_issue +): + resp = api_client_with_credentials.get( + reverse("api:series-issue-list", kwargs={"pk": fc_series.pk}) + ) + assert resp.status_code == status.HTTP_200_OK + last_modified = _stale_since(resp) + + Issue.objects.create( + series=fc_series, + number="2", + slug="final-crisis-2", + cover_date=timezone.now().date(), + edited_by=basic_issue.edited_by, + created_by=basic_issue.created_by, + ) + + resp = api_client_with_credentials.get( + reverse("api:series-issue-list", kwargs={"pk": fc_series.pk}), + HTTP_IF_MODIFIED_SINCE=last_modified, + ) + assert resp.status_code == status.HTTP_200_OK + + +def test_arc_issue_list_conditional_request_returns_200_after_issue_arc_added( + api_client_with_credentials, wwh_arc, basic_issue +): + resp = api_client_with_credentials.get(reverse("api:arc-issue-list", kwargs={"pk": wwh_arc.pk})) + assert resp.status_code == status.HTTP_200_OK + last_modified = _stale_since(resp) + + basic_issue.arcs.add(wwh_arc) + + resp = api_client_with_credentials.get( + reverse("api:arc-issue-list", kwargs={"pk": wwh_arc.pk}), + HTTP_IF_MODIFIED_SINCE=last_modified, + ) + assert resp.status_code == status.HTTP_200_OK diff --git a/tests/comicsdb/test_cache.py b/tests/comicsdb/test_cache.py new file mode 100644 index 00000000..0e083919 --- /dev/null +++ b/tests/comicsdb/test_cache.py @@ -0,0 +1,85 @@ +from datetime import UTC, datetime +from unittest.mock import Mock, patch + +from comicsdb.cache import ( + delete_last_modified, + delete_last_modified_many, + get_last_modified, + last_modified_cache_key, + set_last_modified, +) +from comicsdb.models.arc import Arc +from comicsdb.models.character import Character + + +def _instance(model, pk, modified): + instance = Mock(spec=["pk", "modified"]) + instance.__class__ = model + instance.pk = pk + instance.modified = modified + return instance + + +def test_last_modified_cache_key_is_model_qualified(): + assert last_modified_cache_key(Arc, 42) == "modified:comicsdb.arc:42" + + +def test_arc_and_character_with_same_pk_do_not_collide(): + now = datetime(2024, 1, 1, tzinfo=UTC) + set_last_modified(_instance(Arc, 1, now)) + + assert get_last_modified(Character, 1) is None + assert get_last_modified(Arc, 1) == now + + +def test_get_last_modified_round_trips(): + now = datetime(2024, 6, 15, 12, 30, tzinfo=UTC) + set_last_modified(_instance(Arc, 7, now)) + + assert get_last_modified(Arc, 7) == now + + +def test_get_last_modified_missing_key_returns_none(): + assert get_last_modified(Arc, 999) is None + + +def test_set_last_modified_noop_without_modified(): + set_last_modified(_instance(Arc, 5, None)) + + assert get_last_modified(Arc, 5) is None + + +def test_delete_last_modified(): + set_last_modified(_instance(Arc, 3, datetime.now(tz=UTC))) + delete_last_modified(Arc, 3) + + assert get_last_modified(Arc, 3) is None + + +def test_delete_last_modified_many(): + now = datetime.now(tz=UTC) + set_last_modified(_instance(Arc, 1, now)) + set_last_modified(_instance(Arc, 2, now)) + + delete_last_modified_many(Arc, [1, 2]) + + assert get_last_modified(Arc, 1) is None + assert get_last_modified(Arc, 2) is None + + +def test_read_failure_degrades_to_none(): + with patch("comicsdb.cache.cache") as mock_cache: + mock_cache.get.side_effect = Exception("redis down") + assert get_last_modified(Arc, 1) is None + + +def test_write_failure_does_not_raise(): + with patch("comicsdb.cache.cache") as mock_cache: + mock_cache.set.side_effect = Exception("redis down") + set_last_modified(_instance(Arc, 1, datetime.now(tz=UTC))) + + +def test_delete_failure_does_not_raise(): + with patch("comicsdb.cache.cache") as mock_cache: + mock_cache.delete_many.side_effect = Exception("redis down") + delete_last_modified(Arc, 1) diff --git a/tests/comicsdb/test_signals.py b/tests/comicsdb/test_signals.py index 5b6dfd00..02264cc1 100644 --- a/tests/comicsdb/test_signals.py +++ b/tests/comicsdb/test_signals.py @@ -50,7 +50,7 @@ def test_update_related_modified_ignores_non_add_remove_actions(wwh_arc): wwh_arc.refresh_from_db() old_modified = wwh_arc.modified - update_related_modified(Arc, MagicMock(spec=Issue), "pre_add", {wwh_arc.pk}) + update_related_modified(Arc, "arcs", MagicMock(spec=Issue), "pre_add", {wwh_arc.pk}) wwh_arc.refresh_from_db() assert wwh_arc.modified == old_modified @@ -61,7 +61,7 @@ def test_update_related_modified_post_add_from_issue(wwh_arc): wwh_arc.refresh_from_db() old_modified = wwh_arc.modified - update_related_modified(Arc, MagicMock(spec=Issue), "post_add", {wwh_arc.pk}) + update_related_modified(Arc, "arcs", MagicMock(spec=Issue), "post_add", {wwh_arc.pk}) wwh_arc.refresh_from_db() assert wwh_arc.modified > old_modified @@ -72,7 +72,9 @@ def test_update_related_modified_post_remove_from_issue(superman): superman.refresh_from_db() old_modified = superman.modified - update_related_modified(Character, MagicMock(spec=Issue), "post_remove", {superman.pk}) + update_related_modified( + Character, "characters", MagicMock(spec=Issue), "post_remove", {superman.pk} + ) superman.refresh_from_db() assert superman.modified > old_modified @@ -83,7 +85,7 @@ def test_update_related_modified_from_parent_side(wwh_arc): wwh_arc.refresh_from_db() old_modified = wwh_arc.modified - update_related_modified(Arc, wwh_arc, "post_add", None) + update_related_modified(Arc, "arcs", wwh_arc, "post_add", None) wwh_arc.refresh_from_db() assert wwh_arc.modified > old_modified @@ -94,7 +96,7 @@ def test_update_related_modified_empty_pk_set_from_issue(wwh_arc): wwh_arc.refresh_from_db() old_modified = wwh_arc.modified - update_related_modified(Arc, MagicMock(spec=Issue), "post_add", set()) + update_related_modified(Arc, "arcs", MagicMock(spec=Issue), "post_add", set()) wwh_arc.refresh_from_db() assert wwh_arc.modified == old_modified @@ -105,24 +107,55 @@ def test_update_related_modified_post_clear_from_parent(wwh_arc): wwh_arc.refresh_from_db() old_modified = wwh_arc.modified - update_related_modified(Arc, wwh_arc, "post_clear", None) + update_related_modified(Arc, "arcs", wwh_arc, "post_clear", None) wwh_arc.refresh_from_db() assert wwh_arc.modified > old_modified -def test_update_related_modified_post_clear_from_issue_is_noop(wwh_arc): +def test_update_related_modified_post_clear_from_issue_without_snapshot_is_noop(wwh_arc): past = timezone.now() - timedelta(days=1) Arc.objects.filter(pk=wwh_arc.pk).update(modified=past) wwh_arc.refresh_from_db() old_modified = wwh_arc.modified - # When clearing from the issue side, pk_set is None so we cannot identify - # which parents were affected; the update is intentionally skipped. - update_related_modified(Arc, MagicMock(spec=Issue), "post_clear", None) + # Calling post_clear directly, without a preceding pre_clear snapshot, leaves + # no way to know which parents were affected, so it remains a no-op. + update_related_modified(Arc, "arcs", MagicMock(spec=Issue), "post_clear", None) wwh_arc.refresh_from_db() assert wwh_arc.modified == old_modified +def test_update_related_modified_pre_clear_then_post_clear_from_issue(basic_issue, wwh_arc): + basic_issue.arcs.add(wwh_arc) + + past = timezone.now() - timedelta(days=1) + Arc.objects.filter(pk=wwh_arc.pk).update(modified=past) + wwh_arc.refresh_from_db() + old_modified = wwh_arc.modified + + update_related_modified(Arc, "arcs", basic_issue, "pre_clear", None) + assert basic_issue._cleared_pks == {wwh_arc.pk} + + update_related_modified(Arc, "arcs", basic_issue, "post_clear", None) + assert not hasattr(basic_issue, "_cleared_pks") + + wwh_arc.refresh_from_db() + assert wwh_arc.modified > old_modified + + +def test_issue_arcs_clear_updates_arc_modified(basic_issue, wwh_arc): + basic_issue.arcs.add(wwh_arc) + + past = timezone.now() - timedelta(days=1) + Arc.objects.filter(pk=wwh_arc.pk).update(modified=past) + wwh_arc.refresh_from_db() + old_modified = wwh_arc.modified + + basic_issue.arcs.clear() + wwh_arc.refresh_from_db() + assert wwh_arc.modified > old_modified + + def test_arc_m2m_signal_updates_arc_modified(basic_issue, wwh_arc): past = timezone.now() - timedelta(days=1) Arc.objects.filter(pk=wwh_arc.pk).update(modified=past) diff --git a/tests/user_collection/test_api_collection.py b/tests/user_collection/test_api_collection.py index 457dae38..7532af1c 100644 --- a/tests/user_collection/test_api_collection.py +++ b/tests/user_collection/test_api_collection.py @@ -82,6 +82,24 @@ def test_get_invalid_collection_item(api_client_with_credentials): assert resp.status_code == status.HTTP_404_NOT_FOUND +def test_conditional_request_cannot_leak_other_users_item( + api_client, collection_user, other_collection_user, collection_item +): + """A cache warmed by the owner must not let another user 304 past the + per-user queryset filter.""" + api_client.force_authenticate(user=collection_user) + resp = api_client.get(reverse("api:collection-detail", kwargs={"pk": collection_item.pk})) + assert resp.status_code == status.HTTP_200_OK + last_modified = resp["Last-Modified"] + + api_client.force_authenticate(user=other_collection_user) + resp = api_client.get( + reverse("api:collection-detail", kwargs={"pk": collection_item.pk}), + HTTP_IF_MODIFIED_SINCE=last_modified, + ) + assert resp.status_code == status.HTTP_404_NOT_FOUND + + # Stats Endpoint Tests def test_unauthenticated_stats_requires_auth(api_client): """Unauthenticated users require authentication to view stats."""