From f65fb0306a05a56fd1fc821a98ce1aa3cf6f08af Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Fri, 16 Oct 2026 09:04:43 -0400 Subject: [PATCH 01/14] Expose resource attribution in the REST API Add a read-only `attribution` list (source label + URL) to the detail responses for arcs, characters, creators, imprints, issues, publishers, series, teams, and universes, so API consumers can credit where descriptive content came from. List responses are unchanged. - Add AttributionSerializer, returning `source` as its display label - Add Arc/Creator/Publisher read serializers for retrieve - Prefetch `attribution` on each retrieve queryset - Bump the parent's `modified` and cache version when an Attribution is saved or deleted, so cached detail responses and Last-Modified stay current - Document the field in api/README.md --- api/README.md | 23 ++++ api/v1_0/serializers/__init__.py | 9 +- api/v1_0/serializers/arc.py | 8 ++ api/v1_0/serializers/attribution.py | 11 ++ api/v1_0/serializers/character.py | 5 + api/v1_0/serializers/creator.py | 8 ++ api/v1_0/serializers/imprint.py | 5 + api/v1_0/serializers/issue.py | 3 + api/v1_0/serializers/publisher.py | 8 ++ api/v1_0/serializers/series.py | 5 + api/v1_0/serializers/team.py | 5 + api/v1_0/serializers/universe.py | 5 + api/views.py | 38 +++++- comicsdb/apps.py | 13 ++ comicsdb/signals.py | 16 +++ tests/comicsdb/test_api_attribution.py | 164 +++++++++++++++++++++++++ 16 files changed, 320 insertions(+), 6 deletions(-) create mode 100644 api/v1_0/serializers/attribution.py create mode 100644 tests/comicsdb/test_api_attribution.py diff --git a/api/README.md b/api/README.md index 7eaea37c..3f8104e9 100644 --- a/api/README.md +++ b/api/README.md @@ -116,6 +116,20 @@ Most resources follow a standard pattern: - **Create:** `POST /api/{resource}/` - Create new item (requires editor or admin) - **Update:** `PUT/PATCH /api/{resource}/{id}/` - Update item (requires editor or admin) +### Attribution + +Detail responses for arcs, characters, creators, imprints, issues, publishers, series, teams, and +universes include a read-only `attribution` list naming where descriptive content (such as `desc`) +was adapted from. If you reuse that content, credit these sources, since some licenses require it +(Wikipedia's CC BY-SA, for example). List responses don't include it. + +```json +"attribution": [ + {"source": "Wikipedia", "url": "https://en.wikipedia.org/wiki/Spider-Man"}, + {"source": "Marvel", "url": "https://www.marvel.com/characters/spider-man-peter-parker"} +] +``` + ### Nested Resources Some resources include nested actions: @@ -175,6 +189,7 @@ Story arcs that span multiple issues, often across different series. - `cv_id` - Comic Vine ID - `gcd_id` - Grand Comics Database ID - `resource_url` - Link to web UI + - `attribution` - Sources the descriptive content was adapted from, as a list of `{"source", "url"}` objects (empty if none). `source` is one of `Wikipedia`, `Marvel`, `DC`, or `Grand Comics Database` **Example:** @@ -232,6 +247,7 @@ Comic book characters appearing in issues. - `cv_id` - Comic Vine ID - `gcd_id` - Grand Comics Database ID - `resource_url` - Link to web UI + - `attribution` - Sources the descriptive content was adapted from, as a list of `{"source", "url"}` objects (empty if none). `source` is one of `Wikipedia`, `Marvel`, `DC`, or `Grand Comics Database` **Example:** @@ -286,6 +302,7 @@ Writers, artists, and other comic book creators. - `cv_id` - Comic Vine ID - `gcd_id` - Grand Comics Database ID - `resource_url` - Link to web UI + - `attribution` - Sources the descriptive content was adapted from, as a list of `{"source", "url"}` objects (empty if none). `source` is one of `Wikipedia`, `Marvel`, `DC`, or `Grand Comics Database` **Example:** @@ -426,6 +443,7 @@ The Issue endpoint supports the most comprehensive filtering options: - `cover_hash` - Perceptual image hash - `foc_date` - Final order cutoff date - `resource_url` - Link to web UI + - `attribution` - Sources the descriptive content was adapted from, as a list of `{"source", "url"}` objects (empty if none). `source` is one of `Wikipedia`, `Marvel`, `DC`, or `Grand Comics Database` **Cover Price (Write):** @@ -535,6 +553,7 @@ Comic book publishers. - `cv_id` - Comic Vine ID - `gcd_id` - Grand Comics Database ID - `resource_url` - Link to web UI + - `attribution` - Sources the descriptive content was adapted from, as a list of `{"source", "url"}` objects (empty if none). `source` is one of `Wikipedia`, `Marvel`, `DC`, or `Grand Comics Database` **Country (Write):** @@ -599,6 +618,7 @@ Publisher imprints (e.g., Vertigo, MAX). - `cv_id` - Comic Vine ID - `gcd_id` - Grand Comics Database ID - `resource_url` - Link to web UI + - `attribution` - Sources the descriptive content was adapted from, as a list of `{"source", "url"}` objects (empty if none). `source` is one of `Wikipedia`, `Marvel`, `DC`, or `Grand Comics Database` **Example:** @@ -708,6 +728,7 @@ Comic book series. - `genres` - Array of genre objects - `associated` - Associated series - `resource_url` - Link to web UI + - `attribution` - Sources the descriptive content was adapted from, as a list of `{"source", "url"}` objects (empty if none). `source` is one of `Wikipedia`, `Marvel`, `DC`, or `Grand Comics Database` **Series Status Choices:** @@ -813,6 +834,7 @@ Superhero teams and groups. - `cv_id` - Comic Vine ID - `gcd_id` - Grand Comics Database ID - `resource_url` - Link to web UI + - `attribution` - Sources the descriptive content was adapted from, as a list of `{"source", "url"}` objects (empty if none). `source` is one of `Wikipedia`, `Marvel`, `DC`, or `Grand Comics Database` **Example:** @@ -864,6 +886,7 @@ Comic book universes (e.g., Earth-616, Earth-1). - `desc` - Description - `image` - Universe image URL - `resource_url` - Link to web UI + - `attribution` - Sources the descriptive content was adapted from, as a list of `{"source", "url"}` objects (empty if none). `source` is one of `Wikipedia`, `Marvel`, `DC`, or `Grand Comics Database` **Example:** diff --git a/api/v1_0/serializers/__init__.py b/api/v1_0/serializers/__init__.py index f2554366..3b275b60 100644 --- a/api/v1_0/serializers/__init__.py +++ b/api/v1_0/serializers/__init__.py @@ -1,9 +1,11 @@ from api.v1_0.serializers.publisher import ( # noqa: I001 BasicPublisherSerializer, PublisherListSerializer, + PublisherReadSerializer, PublisherSerializer, ) -from api.v1_0.serializers.arc import ArcListSerializer, ArcSerializer +from api.v1_0.serializers.attribution import AttributionSerializer +from api.v1_0.serializers.arc import ArcListSerializer, ArcReadSerializer, ArcSerializer from api.v1_0.serializers.character import ( CharacterListSerializer, CharacterReadSerializer, @@ -11,6 +13,7 @@ ) from api.v1_0.serializers.creator import ( CreatorListSerializer, + CreatorReadSerializer, CreatorSerializer, CreditReadSerializer, CreditSerializer, @@ -65,8 +68,10 @@ __all__ = [ "ArcListSerializer", + "ArcReadSerializer", "ArcSerializer", "AssociatedSeriesSerializer", + "AttributionSerializer", "BasicImprintSerializer", "BasicPublisherSerializer", "CharacterListSerializer", @@ -77,6 +82,7 @@ "CollectionRatingUpdateSerializer", "CollectionReadSerializer", "CreatorListSerializer", + "CreatorReadSerializer", "CreatorSerializer", "CreditReadSerializer", "CreditSerializer", @@ -92,6 +98,7 @@ "MissingIssueSerializer", "MissingSeriesSerializer", "PublisherListSerializer", + "PublisherReadSerializer", "PublisherSerializer", "RatingSerializer", "ReadingListItemSerializer", diff --git a/api/v1_0/serializers/arc.py b/api/v1_0/serializers/arc.py index 16b80eaa..5035d938 100644 --- a/api/v1_0/serializers/arc.py +++ b/api/v1_0/serializers/arc.py @@ -1,5 +1,6 @@ from rest_framework import serializers +from api.v1_0.serializers.attribution import AttributionSerializer from comicsdb.models import Arc @@ -27,3 +28,10 @@ def update(self, instance: Arc, validated_data): class Meta: model = Arc fields = ("id", "name", "desc", "image", "cv_id", "gcd_id", "resource_url", "modified") + + +class ArcReadSerializer(ArcSerializer): + attribution = AttributionSerializer(many=True, read_only=True) + + class Meta(ArcSerializer.Meta): + fields = (*ArcSerializer.Meta.fields, "attribution") diff --git a/api/v1_0/serializers/attribution.py b/api/v1_0/serializers/attribution.py new file mode 100644 index 00000000..5e27160b --- /dev/null +++ b/api/v1_0/serializers/attribution.py @@ -0,0 +1,11 @@ +from rest_framework import serializers + +from comicsdb.models import Attribution + + +class AttributionSerializer(serializers.ModelSerializer): + source = serializers.CharField(source="get_source_display", read_only=True) + + class Meta: + model = Attribution + fields = ("source", "url") diff --git a/api/v1_0/serializers/character.py b/api/v1_0/serializers/character.py index ad18cc8d..dfa25f88 100644 --- a/api/v1_0/serializers/character.py +++ b/api/v1_0/serializers/character.py @@ -1,5 +1,6 @@ from rest_framework import serializers +from api.v1_0.serializers.attribution import AttributionSerializer from api.v1_0.serializers.creator import CreatorListSerializer from api.v1_0.serializers.team import TeamListSerializer from api.v1_0.serializers.universe import UniverseListSerializer @@ -77,3 +78,7 @@ class CharacterReadSerializer(CharacterSerializer): creators = CreatorListSerializer(many=True, read_only=True) teams = TeamListSerializer(many=True, read_only=True) universes = UniverseListSerializer(many=True, read_only=True) + attribution = AttributionSerializer(many=True, read_only=True) + + class Meta(CharacterSerializer.Meta): + fields = (*CharacterSerializer.Meta.fields, "attribution") diff --git a/api/v1_0/serializers/creator.py b/api/v1_0/serializers/creator.py index 1fe560f3..10a04daf 100644 --- a/api/v1_0/serializers/creator.py +++ b/api/v1_0/serializers/creator.py @@ -1,5 +1,6 @@ from rest_framework import serializers +from api.v1_0.serializers.attribution import AttributionSerializer from comicsdb.models import Creator, Credits, Role @@ -41,6 +42,13 @@ class Meta: ) +class CreatorReadSerializer(CreatorSerializer): + attribution = AttributionSerializer(many=True, read_only=True) + + class Meta(CreatorSerializer.Meta): + fields = (*CreatorSerializer.Meta.fields, "attribution") + + class RoleSerializer(serializers.ModelSerializer): class Meta: model = Role diff --git a/api/v1_0/serializers/imprint.py b/api/v1_0/serializers/imprint.py index f58eb00b..c02db2a6 100644 --- a/api/v1_0/serializers/imprint.py +++ b/api/v1_0/serializers/imprint.py @@ -1,6 +1,7 @@ from rest_framework import serializers from api.v1_0.serializers import BasicPublisherSerializer +from api.v1_0.serializers.attribution import AttributionSerializer from comicsdb.models import Imprint @@ -40,6 +41,10 @@ class Meta: class ImprintReadSerializer(ImprintSerializer): publisher = BasicPublisherSerializer(read_only=True) + attribution = AttributionSerializer(many=True, read_only=True) + + class Meta(ImprintSerializer.Meta): + fields = (*ImprintSerializer.Meta.fields, "attribution") class BasicImprintSerializer(serializers.ModelSerializer): diff --git a/api/v1_0/serializers/issue.py b/api/v1_0/serializers/issue.py index 1eb57573..b33bd187 100644 --- a/api/v1_0/serializers/issue.py +++ b/api/v1_0/serializers/issue.py @@ -8,6 +8,7 @@ from api.v1_0.serializers import CreditReadSerializer from api.v1_0.serializers.arc import ArcListSerializer +from api.v1_0.serializers.attribution import AttributionSerializer from api.v1_0.serializers.character import CharacterListSerializer from api.v1_0.serializers.genre import GenreSerializer from api.v1_0.serializers.imprint import BasicImprintSerializer @@ -331,6 +332,7 @@ class IssueReadSerializer(serializers.ModelSerializer): series = IssueSeriesSerializer(read_only=True) reprints = ReprintSerializer(many=True, read_only=True) rating = RatingSerializer(read_only=True) + attribution = AttributionSerializer(many=True, read_only=True) average_rating = serializers.DecimalField( max_digits=3, decimal_places=2, coerce_to_string=False, read_only=True ) @@ -384,4 +386,5 @@ class Meta: "gcd_id", "resource_url", "modified", + "attribution", ) diff --git a/api/v1_0/serializers/publisher.py b/api/v1_0/serializers/publisher.py index 67f4c41d..81c174a5 100644 --- a/api/v1_0/serializers/publisher.py +++ b/api/v1_0/serializers/publisher.py @@ -1,6 +1,7 @@ from django_countries.serializers import CountryFieldMixin from rest_framework import serializers +from api.v1_0.serializers.attribution import AttributionSerializer from comicsdb.models import Publisher @@ -42,6 +43,13 @@ class Meta: ) +class PublisherReadSerializer(PublisherSerializer): + attribution = AttributionSerializer(many=True, read_only=True) + + class Meta(PublisherSerializer.Meta): + fields = (*PublisherSerializer.Meta.fields, "attribution") + + class BasicPublisherSerializer(serializers.ModelSerializer): class Meta: model = Publisher diff --git a/api/v1_0/serializers/series.py b/api/v1_0/serializers/series.py index 3fe34e78..ffa028a0 100644 --- a/api/v1_0/serializers/series.py +++ b/api/v1_0/serializers/series.py @@ -1,5 +1,6 @@ from rest_framework import serializers +from api.v1_0.serializers.attribution import AttributionSerializer from api.v1_0.serializers.genre import GenreSerializer from api.v1_0.serializers.imprint import BasicImprintSerializer from api.v1_0.serializers.publisher import BasicPublisherSerializer @@ -116,3 +117,7 @@ class SeriesReadSerializer(SeriesSerializer): issue_count = serializers.IntegerField(source="num_issues", read_only=True) associated = AssociatedSeriesSerializer(many=True, read_only=True) genres = GenreSerializer(many=True, read_only=True) + attribution = AttributionSerializer(many=True, read_only=True) + + class Meta(SeriesSerializer.Meta): + fields = (*SeriesSerializer.Meta.fields, "attribution") diff --git a/api/v1_0/serializers/team.py b/api/v1_0/serializers/team.py index 318469dc..71bd79ad 100644 --- a/api/v1_0/serializers/team.py +++ b/api/v1_0/serializers/team.py @@ -1,5 +1,6 @@ from rest_framework import serializers +from api.v1_0.serializers.attribution import AttributionSerializer from api.v1_0.serializers.creator import CreatorListSerializer from api.v1_0.serializers.universe import UniverseListSerializer from comicsdb.models import Team @@ -67,3 +68,7 @@ class Meta: class TeamReadSerializer(TeamSerializer): creators = CreatorListSerializer(many=True, read_only=True) universes = UniverseListSerializer(many=True, read_only=True) + attribution = AttributionSerializer(many=True, read_only=True) + + class Meta(TeamSerializer.Meta): + fields = (*TeamSerializer.Meta.fields, "attribution") diff --git a/api/v1_0/serializers/universe.py b/api/v1_0/serializers/universe.py index 6343bcd6..50ad25ef 100644 --- a/api/v1_0/serializers/universe.py +++ b/api/v1_0/serializers/universe.py @@ -1,6 +1,7 @@ from rest_framework import serializers from api.v1_0.serializers import BasicPublisherSerializer +from api.v1_0.serializers.attribution import AttributionSerializer from comicsdb.models import Universe @@ -42,3 +43,7 @@ class Meta: class UniverseReadSerializer(UniverseSerializer): publisher = BasicPublisherSerializer(read_only=True) + attribution = AttributionSerializer(many=True, read_only=True) + + class Meta(UniverseSerializer.Meta): + fields = (*UniverseSerializer.Meta.fields, "attribution") diff --git a/api/views.py b/api/views.py index bd2b5065..aa638cb8 100644 --- a/api/views.py +++ b/api/views.py @@ -35,6 +35,7 @@ ) from api.v1_0.serializers import ( ArcListSerializer, + ArcReadSerializer, ArcSerializer, CharacterListSerializer, CharacterReadSerializer, @@ -44,6 +45,7 @@ CollectionRatingUpdateSerializer, CollectionReadSerializer, CreatorListSerializer, + CreatorReadSerializer, CreatorSerializer, CreditSerializer, ImprintListSerializer, @@ -55,6 +57,7 @@ MissingIssueSerializer, MissingSeriesSerializer, PublisherListSerializer, + PublisherReadSerializer, PublisherSerializer, ReadingListItemSerializer, ReadingListListSerializer, @@ -404,12 +407,20 @@ class ArcViewSet( # comment). A plain issue field edit can show stale here for up to # DETAIL_CACHE_TTL; that's the accepted tradeoff. + def get_queryset(self): + queryset = super().get_queryset() + if self.action == "retrieve": + queryset = queryset.prefetch_related("attribution") + return queryset + def get_serializer_class(self): match self.action: case "list": return ArcListSerializer case "issue_list": return IssueListSerializer + case "retrieve": + return ArcReadSerializer case _: return ArcSerializer @@ -461,7 +472,7 @@ class CharacterViewSet( def get_queryset(self): queryset = super().get_queryset() if self.action == "retrieve": - queryset = queryset.prefetch_related("creators", "teams", "universes") + queryset = queryset.prefetch_related("creators", "teams", "universes", "attribution") return queryset def get_serializer_class(self): @@ -497,10 +508,18 @@ class CreatorViewSet( parser_classes = (MultiPartParser, FormParser) cache_model_label = ModelLabel.CREATOR + def get_queryset(self): + queryset = super().get_queryset() + if self.action == "retrieve": + queryset = queryset.prefetch_related("attribution") + return queryset + def get_serializer_class(self): match self.action: case "list": return CreatorListSerializer + case "retrieve": + return CreatorReadSerializer case _: return CreatorSerializer @@ -560,7 +579,7 @@ class ImprintViewSet( def get_queryset(self): queryset = super().get_queryset() if self.action == "retrieve": - queryset = queryset.select_related("publisher") + queryset = queryset.select_related("publisher").prefetch_related("attribution") return queryset def get_serializer_class(self): @@ -651,6 +670,7 @@ def get_queryset(self): "teams", "universes", "variants", + "attribution", Prefetch( "credits_set", queryset=Credits.objects.order_by("creator__name") @@ -716,12 +736,20 @@ class PublisherViewSet( parser_classes = (MultiPartParser, FormParser) cache_model_label = ModelLabel.PUBLISHER + def get_queryset(self): + queryset = super().get_queryset() + if self.action == "retrieve": + queryset = queryset.prefetch_related("attribution") + return queryset + def get_serializer_class(self): match self.action: case "list": return PublisherListSerializer case "series_list": return SeriesListSerializer + case "retrieve": + return PublisherReadSerializer case _: return PublisherSerializer @@ -826,7 +854,7 @@ def get_queryset(self): case "retrieve": queryset = ( queryset.select_related("imprint") - .prefetch_related("genres", "associated") + .prefetch_related("genres", "associated", "attribution") .annotate(num_issues=Count("issues", distinct=True)) ) return queryset @@ -919,7 +947,7 @@ class TeamViewSet( def get_queryset(self): queryset = super().get_queryset() if self.action == "retrieve": - queryset = queryset.prefetch_related("creators", "universes") + queryset = queryset.prefetch_related("creators", "universes", "attribution") return queryset def get_serializer_class(self): @@ -961,7 +989,7 @@ class UniverseViewSet( def get_queryset(self): queryset = super().get_queryset() if self.action == "retrieve": - queryset = queryset.select_related("publisher") + queryset = queryset.select_related("publisher").prefetch_related("attribution") return queryset def get_serializer_class(self): diff --git a/comicsdb/apps.py b/comicsdb/apps.py index 412bdab7..1426e211 100644 --- a/comicsdb/apps.py +++ b/comicsdb/apps.py @@ -20,6 +20,7 @@ update_issue_modified_on_reprint_change, update_issue_modified_on_universe_change, update_issue_modified_on_variant_change, + update_parent_modified_on_attribution_change, update_series_modified_on_associated_change, update_series_modified_on_genre_change, update_series_modified_on_issue_delete, @@ -142,6 +143,18 @@ def ready(self): dispatch_uid="post_delete_variant_issue_modified", ) + attribution = self.get_model("Attribution") + post_save.connect( + update_parent_modified_on_attribution_change, + sender=attribution, + dispatch_uid="post_save_attribution_parent_modified", + ) + post_delete.connect( + update_parent_modified_on_attribution_change, + sender=attribution, + dispatch_uid="post_delete_attribution_parent_modified", + ) + credits_ = self.get_model("Credits") pre_delete.connect(pre_delete_credit, sender=credits_, dispatch_uid="pre_delete_credits") post_save.connect( diff --git a/comicsdb/signals.py b/comicsdb/signals.py index 95e562c1..98bb3d6e 100644 --- a/comicsdb/signals.py +++ b/comicsdb/signals.py @@ -269,3 +269,19 @@ def bump_cache_on_name_change(label, sender, instance, **kwargs): pre_save flag is missing, so an unexpected path errs toward freshness.""" if instance.__dict__.pop("_cache_name_changed", True): bump_model_version(label) + + +def update_parent_modified_on_attribution_change(sender, instance, **kwargs): + """Attribution is a generic relation, so saving or deleting one doesn't + touch its parent row. Bump the parent's `modified` explicitly so its + cached detail response (which embeds `attribution`) self-invalidates, + and its cache version, since its list response embeds `modified`.""" + model = instance.content_type.model_class() + if model is None: + return + model.objects.filter(pk=instance.object_id).update(modified=timezone.now()) + try: + label = ModelLabel(model._meta.model_name) + except ValueError: + return + bump_model_version(label) diff --git a/tests/comicsdb/test_api_attribution.py b/tests/comicsdb/test_api_attribution.py new file mode 100644 index 00000000..d7eb273f --- /dev/null +++ b/tests/comicsdb/test_api_attribution.py @@ -0,0 +1,164 @@ +import uuid +from datetime import timedelta +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 django.utils import timezone +from rest_framework import status + +from api.cache import ModelLabel, get_model_version +from comicsdb.models import Attribution, Character + +# (fixture name, API basename) for every resource whose detail response embeds attribution. +ATTRIBUTED_RESOURCES = [ + ("wwh_arc", "arc"), + ("batman", "character"), + ("john_byrne", "creator"), + ("vertigo_imprint", "imprint"), + ("basic_issue", "issue"), + ("dc_comics", "publisher"), + ("fc_series", "series"), + ("teen_titans", "team"), + ("earth_2_universe", "universe"), +] + +WIKI_URL = "https://en.wikipedia.org/wiki/Example" +MARVEL_URL = "https://www.marvel.com/characters/example" + + +@pytest.fixture +def local_cache(): + """Isolate the response cache from concurrent xdist workers sharing Redis.""" + test_cache = LocMemCache(f"test-api-attribution-{uuid.uuid4()}", {}) + with patch("api.views.cache", test_cache), patch("api.cache.cache", test_cache): + yield test_cache + + +@pytest.mark.parametrize(("fixture_name", "basename"), ATTRIBUTED_RESOURCES) +def test_detail_attribution_is_empty_list_by_default( + request, api_client_with_credentials, local_cache, fixture_name, basename +): + obj = request.getfixturevalue(fixture_name) + resp = api_client_with_credentials.get(reverse(f"api:{basename}-detail", args=(obj.pk,))) + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["attribution"] == [] + + +@pytest.mark.parametrize(("fixture_name", "basename"), ATTRIBUTED_RESOURCES) +def test_detail_attribution_returns_source_label_and_url( + request, api_client_with_credentials, local_cache, fixture_name, basename +): + obj = request.getfixturevalue(fixture_name) + obj.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) + obj.attribution.create(source=Attribution.Source.MARVEL, url=MARVEL_URL) + + resp = api_client_with_credentials.get(reverse(f"api:{basename}-detail", args=(obj.pk,))) + assert resp.status_code == status.HTTP_200_OK + assert sorted(resp.json()["attribution"], key=lambda a: a["source"]) == [ + {"source": "Marvel", "url": MARVEL_URL}, + {"source": "Wikipedia", "url": WIKI_URL}, + ] + + +@pytest.mark.parametrize(("fixture_name", "basename"), ATTRIBUTED_RESOURCES) +def test_list_does_not_include_attribution( + request, api_client_with_credentials, local_cache, fixture_name, basename +): + obj = request.getfixturevalue(fixture_name) + obj.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) + + resp = api_client_with_credentials.get(reverse(f"api:{basename}-list")) + assert resp.status_code == status.HTTP_200_OK + assert "attribution" not in resp.json()["results"][0] + + +@pytest.mark.parametrize(("fixture_name", "basename"), ATTRIBUTED_RESOURCES) +def test_detail_attribution_query_count_is_constant( + request, api_client_with_credentials, local_cache, fixture_name, basename +): + obj = request.getfixturevalue(fixture_name) + url = reverse(f"api:{basename}-detail", args=(obj.pk,)) + obj.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) + + local_cache.clear() + with CaptureQueriesContext(connection) as one: + api_client_with_credentials.get(url) + + obj.attribution.create(source=Attribution.Source.MARVEL, url=MARVEL_URL) + obj.attribution.create(source=Attribution.Source.GCD, url="https://www.comics.org/x/") + + local_cache.clear() + with CaptureQueriesContext(connection) as three: + resp = api_client_with_credentials.get(url) + + assert len(resp.json()["attribution"]) == 3 + assert len(three) == len(one) + + +def test_attribution_add_busts_detail_cache(api_client_with_credentials, batman, local_cache): + url = reverse("api:character-detail", args=(batman.pk,)) + assert api_client_with_credentials.get(url).json()["attribution"] == [] + + batman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) + + resp = api_client_with_credentials.get(url) + assert resp.json()["attribution"] == [{"source": "Wikipedia", "url": WIKI_URL}] + + +def test_attribution_edit_busts_detail_cache(api_client_with_credentials, batman, local_cache): + attribution = batman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) + url = reverse("api:character-detail", args=(batman.pk,)) + assert api_client_with_credentials.get(url).json()["attribution"][0]["url"] == WIKI_URL + + attribution.source = Attribution.Source.MARVEL + attribution.url = MARVEL_URL + attribution.save() + + resp = api_client_with_credentials.get(url) + assert resp.json()["attribution"] == [{"source": "Marvel", "url": MARVEL_URL}] + + +def test_attribution_delete_busts_detail_cache(api_client_with_credentials, batman, local_cache): + attribution = batman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) + url = reverse("api:character-detail", args=(batman.pk,)) + assert len(api_client_with_credentials.get(url).json()["attribution"]) == 1 + + attribution.delete() + + assert api_client_with_credentials.get(url).json()["attribution"] == [] + + +def test_attribution_change_updates_last_modified(api_client_with_credentials, batman, local_cache): + # Last-Modified has one-second resolution; backdate so the bump is visible. + Character.objects.filter(pk=batman.pk).update(modified=timezone.now() - timedelta(days=1)) + url = reverse("api:character-detail", args=(batman.pk,)) + last_modified = api_client_with_credentials.get(url)["Last-Modified"] + stale = api_client_with_credentials.get(url, HTTP_IF_MODIFIED_SINCE=last_modified) + assert stale.status_code == status.HTTP_304_NOT_MODIFIED + + batman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) + + resp = api_client_with_credentials.get(url, HTTP_IF_MODIFIED_SINCE=last_modified) + assert resp.status_code == status.HTTP_200_OK + assert len(resp.json()["attribution"]) == 1 + + +def test_attribution_change_bumps_parent_modified_and_cache_version(batman, local_cache): + past = timezone.now() - timedelta(days=1) + Character.objects.filter(pk=batman.pk).update(modified=past) + before = get_model_version(ModelLabel.CHARACTER) + + attribution = batman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) + batman.refresh_from_db() + assert batman.modified > past + assert get_model_version(ModelLabel.CHARACTER) == before + 1 + + Character.objects.filter(pk=batman.pk).update(modified=past) + attribution.delete() + batman.refresh_from_db() + assert batman.modified > past + assert get_model_version(ModelLabel.CHARACTER) == before + 2 From b72d0cb4619b9664c843db643f243d96ea19861c Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Fri, 16 Oct 2026 09:09:06 -0400 Subject: [PATCH 02/14] Strengthen attribution prefetch test Replace the endpoint query-count comparison, which passed with or without the prefetch, with a check that the retrieve queryset returns objects whose attribution is already loaded (zero queries on access). --- tests/comicsdb/test_api_attribution.py | 34 ++++++++++++-------------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/tests/comicsdb/test_api_attribution.py b/tests/comicsdb/test_api_attribution.py index d7eb273f..6f94bb17 100644 --- a/tests/comicsdb/test_api_attribution.py +++ b/tests/comicsdb/test_api_attribution.py @@ -4,11 +4,11 @@ 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 django.urls import resolve, reverse from django.utils import timezone from rest_framework import status +from rest_framework.request import Request +from rest_framework.test import APIRequestFactory from api.cache import ModelLabel, get_model_version from comicsdb.models import Attribution, Character @@ -77,26 +77,24 @@ def test_list_does_not_include_attribution( @pytest.mark.parametrize(("fixture_name", "basename"), ATTRIBUTED_RESOURCES) -def test_detail_attribution_query_count_is_constant( - request, api_client_with_credentials, local_cache, fixture_name, basename +def test_retrieve_queryset_prefetches_attribution( + db, request, django_assert_num_queries, fixture_name, basename ): + """A detail response serializes a single object, so a missing prefetch + still costs only one attribution query -- comparing query counts over + the endpoint can't catch it. Check the retrieve queryset directly + instead: the object it returns must already have attribution loaded.""" obj = request.getfixturevalue(fixture_name) - url = reverse(f"api:{basename}-detail", args=(obj.pk,)) obj.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) - local_cache.clear() - with CaptureQueriesContext(connection) as one: - api_client_with_credentials.get(url) - - obj.attribution.create(source=Attribution.Source.MARVEL, url=MARVEL_URL) - obj.attribution.create(source=Attribution.Source.GCD, url="https://www.comics.org/x/") - - local_cache.clear() - with CaptureQueriesContext(connection) as three: - resp = api_client_with_credentials.get(url) + view = resolve(reverse(f"api:{basename}-detail", args=(obj.pk,))).func.cls() + view.action = "retrieve" + view.request = Request(APIRequestFactory().get("/")) + view.kwargs = {"pk": obj.pk} + fetched = view.get_queryset().get(pk=obj.pk) - assert len(resp.json()["attribution"]) == 3 - assert len(three) == len(one) + with django_assert_num_queries(0): + assert [a.url for a in fetched.attribution.all()] == [WIKI_URL] def test_attribution_add_busts_detail_cache(api_client_with_credentials, batman, local_cache): From 411778c9d82bf4c1af8d496f2f48add3076e9e5a Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Fri, 16 Oct 2026 09:29:26 -0400 Subject: [PATCH 03/14] Stabilize attribution order and untranslated source names Add an id tiebreaker to Attribution.Meta.ordering so an object's attributions come back in creation order. Before, Postgres could return them in any order, so the same data could serialize differently after a cache miss. Serialize `source` from a fixed map of English names instead of the translatable choice label. The detail cache key doesn't include the language, so a translated label could be cached and then served to clients in other languages. The values are the ones the README already documents, and the OpenAPI schema now lists them as an enum. --- api/v1_0/serializers/attribution.py | 16 +++++++++++++++- comicsdb/migrations/0060_attribution_ordering.py | 16 ++++++++++++++++ comicsdb/models/attribution.py | 2 +- tests/comicsdb/test_api_attribution.py | 10 ++++++++-- 4 files changed, 40 insertions(+), 4 deletions(-) create mode 100644 comicsdb/migrations/0060_attribution_ordering.py diff --git a/api/v1_0/serializers/attribution.py b/api/v1_0/serializers/attribution.py index 5e27160b..060c137c 100644 --- a/api/v1_0/serializers/attribution.py +++ b/api/v1_0/serializers/attribution.py @@ -1,11 +1,25 @@ +from drf_spectacular.utils import extend_schema_field from rest_framework import serializers from comicsdb.models import Attribution +# Fixed, untranslated names so API output doesn't vary with the request's language (and can be +# cached per object), unlike the model's translatable choice labels. +SOURCE_NAMES = { + Attribution.Source.MARVEL: "Marvel", + Attribution.Source.WIKIPEDIA: "Wikipedia", + Attribution.Source.GCD: "Grand Comics Database", + Attribution.Source.DC: "DC", +} + class AttributionSerializer(serializers.ModelSerializer): - source = serializers.CharField(source="get_source_display", read_only=True) + source = serializers.SerializerMethodField() class Meta: model = Attribution fields = ("source", "url") + + @extend_schema_field(serializers.ChoiceField(choices=list(SOURCE_NAMES.values()))) + def get_source(self, obj: Attribution) -> str: + return SOURCE_NAMES[obj.source] diff --git a/comicsdb/migrations/0060_attribution_ordering.py b/comicsdb/migrations/0060_attribution_ordering.py new file mode 100644 index 00000000..8fed06a2 --- /dev/null +++ b/comicsdb/migrations/0060_attribution_ordering.py @@ -0,0 +1,16 @@ +# Generated by Django 6.1.2 on 2026-10-16 13:18 + +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("comicsdb", "0059_issue_desc_is_solicitation"), + ] + + operations = [ + migrations.AlterModelOptions( + name="attribution", + options={"ordering": ["content_type", "object_id", "id"]}, + ), + ] diff --git a/comicsdb/models/attribution.py b/comicsdb/models/attribution.py index 22606763..9b91a269 100644 --- a/comicsdb/models/attribution.py +++ b/comicsdb/models/attribution.py @@ -20,7 +20,7 @@ class Source(models.TextChoices): class Meta: indexes = [models.Index(fields=["content_type", "object_id"], name="ct_obj_id_idx")] - ordering = ["content_type", "object_id"] + ordering = ["content_type", "object_id", "id"] def __str__(self) -> str: return f"{self.source} Attribution" diff --git a/tests/comicsdb/test_api_attribution.py b/tests/comicsdb/test_api_attribution.py index 6f94bb17..703be1e7 100644 --- a/tests/comicsdb/test_api_attribution.py +++ b/tests/comicsdb/test_api_attribution.py @@ -11,6 +11,7 @@ from rest_framework.test import APIRequestFactory from api.cache import ModelLabel, get_model_version +from api.v1_0.serializers.attribution import SOURCE_NAMES from comicsdb.models import Attribution, Character # (fixture name, API basename) for every resource whose detail response embeds attribution. @@ -58,12 +59,17 @@ def test_detail_attribution_returns_source_label_and_url( resp = api_client_with_credentials.get(reverse(f"api:{basename}-detail", args=(obj.pk,))) assert resp.status_code == status.HTTP_200_OK - assert sorted(resp.json()["attribution"], key=lambda a: a["source"]) == [ - {"source": "Marvel", "url": MARVEL_URL}, + # Ordered by creation within one object, so the response is stable across cache misses. + assert resp.json()["attribution"] == [ {"source": "Wikipedia", "url": WIKI_URL}, + {"source": "Marvel", "url": MARVEL_URL}, ] +def test_every_attribution_source_has_an_api_name(): + assert set(SOURCE_NAMES) == set(Attribution.Source) + + @pytest.mark.parametrize(("fixture_name", "basename"), ATTRIBUTED_RESOURCES) def test_list_does_not_include_attribution( request, api_client_with_credentials, local_cache, fixture_name, basename From b47b2c213f9ea79e04c9c225542e335496212469 Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Fri, 16 Oct 2026 09:32:36 -0400 Subject: [PATCH 04/14] Tighten attribution parent-bump signal handling - Look up the parent model via the cached ContentType.objects.get_for_id() instead of the content_type FK, avoiding a query per save/delete. - Skip the post_delete bump when the delete cascades from the parent (detected via the signal's `origin`), avoiding no-op UPDATEs and cache bumps when deleting series, issues, etc. - Ignore raw (loaddata) saves, and record the previous parent in pre_save so reassigning an attribution also bumps the old parent. --- comicsdb/apps.py | 13 ++++-- comicsdb/signals.py | 44 +++++++++++++++++-- tests/comicsdb/test_api_attribution.py | 58 ++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 6 deletions(-) diff --git a/comicsdb/apps.py b/comicsdb/apps.py index 1426e211..f2863640 100644 --- a/comicsdb/apps.py +++ b/comicsdb/apps.py @@ -9,6 +9,7 @@ bump_cache_on_name_change, pre_delete_credit, pre_delete_image, + track_attribution_parent, track_name_change, update_arc_modified, update_character_modified, @@ -20,7 +21,8 @@ update_issue_modified_on_reprint_change, update_issue_modified_on_universe_change, update_issue_modified_on_variant_change, - update_parent_modified_on_attribution_change, + update_parent_modified_on_attribution_delete, + update_parent_modified_on_attribution_save, update_series_modified_on_associated_change, update_series_modified_on_genre_change, update_series_modified_on_issue_delete, @@ -144,13 +146,18 @@ def ready(self): ) attribution = self.get_model("Attribution") + pre_save.connect( + track_attribution_parent, + sender=attribution, + dispatch_uid="pre_save_attribution_track_parent", + ) post_save.connect( - update_parent_modified_on_attribution_change, + update_parent_modified_on_attribution_save, sender=attribution, dispatch_uid="post_save_attribution_parent_modified", ) post_delete.connect( - update_parent_modified_on_attribution_change, + update_parent_modified_on_attribution_delete, sender=attribution, dispatch_uid="post_delete_attribution_parent_modified", ) diff --git a/comicsdb/signals.py b/comicsdb/signals.py index 98bb3d6e..4ab394b5 100644 --- a/comicsdb/signals.py +++ b/comicsdb/signals.py @@ -1,5 +1,6 @@ import logging +from django.db.models import QuerySet from django.utils import timezone from sorl.thumbnail import delete @@ -271,17 +272,54 @@ def bump_cache_on_name_change(label, sender, instance, **kwargs): bump_model_version(label) -def update_parent_modified_on_attribution_change(sender, instance, **kwargs): +def _touch_attribution_parent(content_type_id, object_id): """Attribution is a generic relation, so saving or deleting one doesn't touch its parent row. Bump the parent's `modified` explicitly so its cached detail response (which embeds `attribution`) self-invalidates, and its cache version, since its list response embeds `modified`.""" - model = instance.content_type.model_class() + from django.contrib.contenttypes.models import ContentType # noqa: PLC0415 + + # get_for_id() is served from ContentType's in-process cache. + model = ContentType.objects.get_for_id(content_type_id).model_class() if model is None: return - model.objects.filter(pk=instance.object_id).update(modified=timezone.now()) + model.objects.filter(pk=object_id).update(modified=timezone.now()) try: label = ModelLabel(model._meta.model_name) except ValueError: return bump_model_version(label) + + +def track_attribution_parent(sender, instance, raw=False, **kwargs): + """pre_save receiver recording the parent an existing attribution points + at before this save, so update_parent_modified_on_attribution_save() can + also bump the old parent when the attribution is reassigned.""" + instance._previous_parent = None + if raw or instance._state.adding: + return + instance._previous_parent = ( + sender.objects.filter(pk=instance.pk).values_list("content_type_id", "object_id").first() + ) + + +def update_parent_modified_on_attribution_save(sender, instance, raw=False, **kwargs): + # Fixture loading may run before the ContentType/parent rows exist. + if raw: + return + current = (instance.content_type_id, instance.object_id) + _touch_attribution_parent(*current) + previous = instance.__dict__.pop("_previous_parent", None) + if previous is not None and previous != current: + _touch_attribution_parent(*previous) + + +def update_parent_modified_on_attribution_delete(sender, instance, origin=None, **kwargs): + # Attributions are only deleted on their own (instance or queryset + # delete) or by cascade when their parent is deleted. In the cascade + # case the parent is going away too, so there's nothing to bump. + deleted_directly = isinstance(origin, sender) or ( + isinstance(origin, QuerySet) and origin.model is sender + ) + if deleted_directly: + _touch_attribution_parent(instance.content_type_id, instance.object_id) diff --git a/tests/comicsdb/test_api_attribution.py b/tests/comicsdb/test_api_attribution.py index 703be1e7..442cc3bc 100644 --- a/tests/comicsdb/test_api_attribution.py +++ b/tests/comicsdb/test_api_attribution.py @@ -4,6 +4,8 @@ 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 resolve, reverse from django.utils import timezone from rest_framework import status @@ -166,3 +168,59 @@ def test_attribution_change_bumps_parent_modified_and_cache_version(batman, loca batman.refresh_from_db() assert batman.modified > past assert get_model_version(ModelLabel.CHARACTER) == before + 2 + + +def test_attribution_reassignment_bumps_old_and_new_parent(batman, superman, local_cache): + past = timezone.now() - timedelta(days=1) + attribution = batman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) + Character.objects.filter(pk__in=(batman.pk, superman.pk)).update(modified=past) + + attribution.object_id = superman.pk + attribution.save() + + batman.refresh_from_db() + superman.refresh_from_db() + assert batman.modified > past + assert superman.modified > past + + +def test_attribution_save_does_not_query_content_type(batman, local_cache): + attribution = batman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) + attribution = Attribution.objects.get(pk=attribution.pk) # fresh instance, no cached FK + + with CaptureQueriesContext(connection) as ctx: + attribution.save() + attribution.delete() + + assert not [q for q in ctx.captured_queries if "django_content_type" in q["sql"]] + + +def test_attribution_raw_save_skips_parent_bump(batman, local_cache): + attribution = Attribution( + source=Attribution.Source.WIKIPEDIA, url=WIKI_URL, content_object=batman + ) + with patch("comicsdb.signals._touch_attribution_parent") as touch: + attribution.save_base(raw=True) + touch.assert_not_called() + + +def test_parent_delete_skips_attribution_parent_bump(batman, local_cache): + batman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) + batman.attribution.create(source=Attribution.Source.MARVEL, url=MARVEL_URL) + + with patch("comicsdb.signals._touch_attribution_parent") as touch: + batman.delete() + + touch.assert_not_called() + assert not Attribution.objects.exists() + + +def test_attribution_queryset_delete_bumps_parent(batman, local_cache): + batman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) + past = timezone.now() - timedelta(days=1) + Character.objects.filter(pk=batman.pk).update(modified=past) + + batman.attribution.all().delete() + + batman.refresh_from_db() + assert batman.modified > past From 7b7d5b963796c25a1b798cdf746b59ad146b2fa8 Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Fri, 16 Oct 2026 09:45:16 -0400 Subject: [PATCH 05/14] Reduce redundant attribution parent bumps and dedupe API prefetch - Skip the parent bump for attribution parents without a `modified` field, instead of raising FieldError. - Add attribution_parent_saved_separately() and use it around the attribution formset saves in the create/update views, whose own parent save already bumps `modified` and the cache version. Move the parent save inside the transaction in the update views so it commits together with the attribution changes. - Add AttributionRetrieveMixin to prefetch `attribution` on retrieve, replacing the per-viewset prefetches. --- api/views.py | 55 ++++++++++++++------------ comicsdb/signals.py | 32 ++++++++++++++- comicsdb/views/issue.py | 12 ++++-- comicsdb/views/mixins.py | 13 ++++-- tests/comicsdb/test_api_attribution.py | 41 ++++++++++++++++++- 5 files changed, 119 insertions(+), 34 deletions(-) diff --git a/api/views.py b/api/views.py index aa638cb8..fa13d0e6 100644 --- a/api/views.py +++ b/api/views.py @@ -198,6 +198,17 @@ def get_object_modified(self): return self._cached_object_modified +class AttributionRetrieveMixin: + """For viewsets whose retrieve serializer embeds `attribution`: prefetch + it on retrieve. Concrete get_queryset() overrides must call super().""" + + def get_queryset(self): + queryset = super().get_queryset() + if self.action == "retrieve": + queryset = queryset.prefetch_related("attribution") + return queryset + + 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 @@ -378,6 +389,7 @@ class ArcViewSet( UserTrackingMixin, IssueListMixin, mixins.CreateModelMixin, + AttributionRetrieveMixin, ConditionalRetrieveModelMixin, CachedListModelMixin, mixins.UpdateModelMixin, @@ -407,12 +419,6 @@ class ArcViewSet( # comment). A plain issue field edit can show stale here for up to # DETAIL_CACHE_TTL; that's the accepted tradeoff. - def get_queryset(self): - queryset = super().get_queryset() - if self.action == "retrieve": - queryset = queryset.prefetch_related("attribution") - return queryset - def get_serializer_class(self): match self.action: case "list": @@ -429,6 +435,7 @@ class CharacterViewSet( UserTrackingMixin, IssueListMixin, mixins.CreateModelMixin, + AttributionRetrieveMixin, ConditionalRetrieveModelMixin, CachedListModelMixin, mixins.UpdateModelMixin, @@ -472,7 +479,7 @@ class CharacterViewSet( def get_queryset(self): queryset = super().get_queryset() if self.action == "retrieve": - queryset = queryset.prefetch_related("creators", "teams", "universes", "attribution") + queryset = queryset.prefetch_related("creators", "teams", "universes") return queryset def get_serializer_class(self): @@ -490,6 +497,7 @@ def get_serializer_class(self): class CreatorViewSet( UserTrackingMixin, mixins.CreateModelMixin, + AttributionRetrieveMixin, ConditionalRetrieveModelMixin, CachedListModelMixin, mixins.UpdateModelMixin, @@ -508,12 +516,6 @@ class CreatorViewSet( parser_classes = (MultiPartParser, FormParser) cache_model_label = ModelLabel.CREATOR - def get_queryset(self): - queryset = super().get_queryset() - if self.action == "retrieve": - queryset = queryset.prefetch_related("attribution") - return queryset - def get_serializer_class(self): match self.action: case "list": @@ -549,6 +551,7 @@ def create(self, request, *args, **kwargs) -> Response: class ImprintViewSet( UserTrackingMixin, mixins.CreateModelMixin, + AttributionRetrieveMixin, ConditionalRetrieveModelMixin, CachedListModelMixin, mixins.UpdateModelMixin, @@ -579,7 +582,7 @@ class ImprintViewSet( def get_queryset(self): queryset = super().get_queryset() if self.action == "retrieve": - queryset = queryset.select_related("publisher").prefetch_related("attribution") + queryset = queryset.select_related("publisher") return queryset def get_serializer_class(self): @@ -595,6 +598,7 @@ def get_serializer_class(self): class IssueViewSet( UserTrackingMixin, mixins.CreateModelMixin, + AttributionRetrieveMixin, ConditionalRetrieveModelMixin, CachedListModelMixin, mixins.UpdateModelMixin, @@ -654,9 +658,11 @@ def get_modified_queryset(self): def get_queryset(self): if self.action == "list": - return Issue.objects.select_related("series", "series__series_type") + return super().get_queryset().select_related("series", "series__series_type") return ( - Issue.objects.select_related( + super() + .get_queryset() + .select_related( "series", "series__series_type", "series__publisher", @@ -670,7 +676,6 @@ def get_queryset(self): "teams", "universes", "variants", - "attribution", Prefetch( "credits_set", queryset=Credits.objects.order_by("creator__name") @@ -712,6 +717,7 @@ def get_serializer_class(self): class PublisherViewSet( UserTrackingMixin, mixins.CreateModelMixin, + AttributionRetrieveMixin, ConditionalRetrieveModelMixin, CachedListModelMixin, mixins.UpdateModelMixin, @@ -736,12 +742,6 @@ class PublisherViewSet( parser_classes = (MultiPartParser, FormParser) cache_model_label = ModelLabel.PUBLISHER - def get_queryset(self): - queryset = super().get_queryset() - if self.action == "retrieve": - queryset = queryset.prefetch_related("attribution") - return queryset - def get_serializer_class(self): match self.action: case "list": @@ -809,6 +809,7 @@ class SeriesViewSet( UserTrackingMixin, IssueListMixin, mixins.CreateModelMixin, + AttributionRetrieveMixin, ConditionalRetrieveModelMixin, CachedListModelMixin, mixins.UpdateModelMixin, @@ -854,7 +855,7 @@ def get_queryset(self): case "retrieve": queryset = ( queryset.select_related("imprint") - .prefetch_related("genres", "associated", "attribution") + .prefetch_related("genres", "associated") .annotate(num_issues=Count("issues", distinct=True)) ) return queryset @@ -905,6 +906,7 @@ class TeamViewSet( UserTrackingMixin, IssueListMixin, mixins.CreateModelMixin, + AttributionRetrieveMixin, ConditionalRetrieveModelMixin, CachedListModelMixin, mixins.UpdateModelMixin, @@ -947,7 +949,7 @@ class TeamViewSet( def get_queryset(self): queryset = super().get_queryset() if self.action == "retrieve": - queryset = queryset.prefetch_related("creators", "universes", "attribution") + queryset = queryset.prefetch_related("creators", "universes") return queryset def get_serializer_class(self): @@ -965,6 +967,7 @@ def get_serializer_class(self): class UniverseViewSet( UserTrackingMixin, mixins.CreateModelMixin, + AttributionRetrieveMixin, ConditionalRetrieveModelMixin, CachedListModelMixin, mixins.UpdateModelMixin, @@ -989,7 +992,7 @@ class UniverseViewSet( def get_queryset(self): queryset = super().get_queryset() if self.action == "retrieve": - queryset = queryset.select_related("publisher").prefetch_related("attribution") + queryset = queryset.select_related("publisher") return queryset def get_serializer_class(self): diff --git a/comicsdb/signals.py b/comicsdb/signals.py index 4ab394b5..fc218a7b 100644 --- a/comicsdb/signals.py +++ b/comicsdb/signals.py @@ -1,5 +1,8 @@ import logging +from contextlib import contextmanager +from contextvars import ContextVar +from django.core.exceptions import FieldDoesNotExist from django.db.models import QuerySet from django.utils import timezone from sorl.thumbnail import delete @@ -272,6 +275,26 @@ def bump_cache_on_name_change(label, sender, instance, **kwargs): bump_model_version(label) +_attribution_parent_saved_separately = ContextVar( + "attribution_parent_saved_separately", default=False +) + + +@contextmanager +def attribution_parent_saved_separately(): + """Skip the attribution signals' parent bump inside this block. + + For edit views that save the parent itself in the same transaction as + its attribution formset: that save already updates `modified` (auto_now) + and bumps the parent's cache version, so bumping it again per + attribution row would only duplicate the work.""" + token = _attribution_parent_saved_separately.set(True) + try: + yield + finally: + _attribution_parent_saved_separately.reset(token) + + def _touch_attribution_parent(content_type_id, object_id): """Attribution is a generic relation, so saving or deleting one doesn't touch its parent row. Bump the parent's `modified` explicitly so its @@ -279,10 +302,17 @@ def _touch_attribution_parent(content_type_id, object_id): and its cache version, since its list response embeds `modified`.""" from django.contrib.contenttypes.models import ContentType # noqa: PLC0415 + if _attribution_parent_saved_separately.get(): + return # get_for_id() is served from ContentType's in-process cache. model = ContentType.objects.get_for_id(content_type_id).model_class() if model is None: return + # The generic FK accepts any model; only bump ones that track `modified`. + try: + model._meta.get_field("modified") + except FieldDoesNotExist: + return model.objects.filter(pk=object_id).update(modified=timezone.now()) try: label = ModelLabel(model._meta.model_name) @@ -296,7 +326,7 @@ def track_attribution_parent(sender, instance, raw=False, **kwargs): at before this save, so update_parent_modified_on_attribution_save() can also bump the old parent when the attribution is reassigned.""" instance._previous_parent = None - if raw or instance._state.adding: + if raw or instance._state.adding or _attribution_parent_saved_separately.get(): return instance._previous_parent = ( sender.objects.filter(pk=instance.pk).values_list("content_type_id", "object_id").first() diff --git a/comicsdb/views/issue.py b/comicsdb/views/issue.py index 7929a0d2..4ff07357 100644 --- a/comicsdb/views/issue.py +++ b/comicsdb/views/issue.py @@ -25,6 +25,7 @@ from comicsdb.models.attribution import Attribution from comicsdb.models.series import SeriesType from comicsdb.models.variant import Variant +from comicsdb.signals import attribution_parent_saved_separately from comicsdb.views.constants import DETAIL_PAGINATE_BY, PAGINATE_BY from comicsdb.views.history import HistoryListView from comicsdb.views.issue_list_helpers import ( @@ -281,7 +282,9 @@ def form_valid(self, form): variants_form.instance = self.object variants_form.save() attribution_form.instance = self.object - attribution_form.save() + # The issue was just saved, so there's nothing for the rows to bump. + with attribution_parent_saved_separately(): + attribution_form.save() LOGGER.info( "Issue: %s #%s was created by %s", @@ -362,7 +365,8 @@ def form_valid(self, form): attribution_form.instance = self.object credits_form.save() variants_form.save() - attribution_form.save() + with attribution_parent_saved_separately(): + attribution_form.save() else: return super().form_invalid(form) @@ -372,7 +376,9 @@ def form_valid(self, form): form.instance.number, self.request.user, ) - return super().form_valid(form) + # Saves the issue, whose `modified`/cache bump also covers the + # attribution changes -- so it must commit with them. + return super().form_valid(form) class IssueFormsetAddRowView(LoginRequiredMixin, View): diff --git a/comicsdb/views/mixins.py b/comicsdb/views/mixins.py index dd4a4d28..9924e6f4 100644 --- a/comicsdb/views/mixins.py +++ b/comicsdb/views/mixins.py @@ -16,6 +16,7 @@ from comicsdb.forms.attribution import AttributionFormSet from comicsdb.forms.issue import MINIMUM_YEAR from comicsdb.models.attribution import Attribution +from comicsdb.signals import attribution_parent_saved_separately from comicsdb.views.constants import DETAIL_PAGINATE_BY LOGGER = logging.getLogger(__name__) @@ -69,7 +70,9 @@ def form_valid(self, form): if attribution_form.is_valid(): self.object = form.save() attribution_form.instance = self.object - attribution_form.save() + # The parent was just saved, so there's nothing for the rows to bump. + with attribution_parent_saved_separately(): + attribution_form.save() else: return super().form_invalid(form) @@ -146,12 +149,16 @@ def form_valid(self, form): if attribution_form.is_valid(): self.object = form.save(commit=False) attribution_form.instance = self.object - attribution_form.save() + with attribution_parent_saved_separately(): + attribution_form.save() else: return super().form_invalid(form) + # Saves the parent, whose `modified`/cache bump also covers the + # attribution changes -- so it must commit with them. + response = super().form_valid(form) LOGGER.info(self.get_log_message(), form.instance.name, self.request.user) - return super().form_valid(form) + return response class NavigationMixin: diff --git a/tests/comicsdb/test_api_attribution.py b/tests/comicsdb/test_api_attribution.py index 442cc3bc..f1f156a9 100644 --- a/tests/comicsdb/test_api_attribution.py +++ b/tests/comicsdb/test_api_attribution.py @@ -14,7 +14,7 @@ from api.cache import ModelLabel, get_model_version from api.v1_0.serializers.attribution import SOURCE_NAMES -from comicsdb.models import Attribution, Character +from comicsdb.models import Attribution, Character, Rating # (fixture name, API basename) for every resource whose detail response embeds attribution. ATTRIBUTED_RESOURCES = [ @@ -224,3 +224,42 @@ def test_attribution_queryset_delete_bumps_parent(batman, local_cache): batman.refresh_from_db() assert batman.modified > past + + +def test_attribution_on_model_without_modified_saves(db, local_cache): + rating = Rating.objects.create(name="Test Rating") + + attribution = Attribution.objects.create( + source=Attribution.Source.WIKIPEDIA, url=WIKI_URL, content_object=rating + ) + attribution.delete() + + +def test_edit_view_writes_parent_row_once(auto_login_user, dc_comics, local_cache): + """The edit view saves the parent itself, so the attribution signal + mustn't also UPDATE it once per formset row.""" + client, _ = auto_login_user() + data = { + "name": dc_comics.name, + "country": "US", + "attribution-TOTAL_FORMS": 2, + "attribution-INITIAL_FORMS": 0, + "attribution-0-source": "W", + "attribution-0-url": WIKI_URL, + "attribution-1-source": "D", + "attribution-1-url": "https://www.dc.com/example", + } + past = timezone.now() - timedelta(days=1) + type(dc_comics).objects.filter(pk=dc_comics.pk).update(modified=past) + + with CaptureQueriesContext(connection) as ctx: + resp = client.post(reverse("publisher:update", kwargs={"slug": dc_comics.slug}), data) + + assert resp.status_code == status.HTTP_302_FOUND + assert dc_comics.attribution.count() == 2 + parent_updates = [ + q for q in ctx.captured_queries if q["sql"].startswith('UPDATE "comicsdb_publisher"') + ] + assert len(parent_updates) == 1 + dc_comics.refresh_from_db() + assert dc_comics.modified > past From a29201891cdac3c7ddb8de2ee2545b20c73d2008 Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Fri, 16 Oct 2026 09:47:19 -0400 Subject: [PATCH 06/14] Remove unused noqa --- tests/users/test_views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/users/test_views.py b/tests/users/test_views.py index ba9cf571..f788b750 100644 --- a/tests/users/test_views.py +++ b/tests/users/test_views.py @@ -29,7 +29,7 @@ def test_view_url_exists_at_desired_location(auto_login_user, url): assert resp.status_code == HTML_OK_CODE -# @pytest.mark.parametrize("url", ["/accounts/update", "/accounts/password", "/accounts/signup"]) # noqa: E501 +# @pytest.mark.parametrize("url", ["/accounts/update", "/accounts/password", "/accounts/signup"]) # def test_view_url_exists_at_desired_location_redirected(auto_login_user, url): # client, _ = auto_login_user() # resp = client.get(url) From 62289b687e7926771de068c5e2568d368f21c876 Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Fri, 16 Oct 2026 09:51:36 -0400 Subject: [PATCH 07/14] Avoid double saves in create views and tidy attribution edge cases - Redirect directly from AttributionCreateMixin and IssueCreate instead of calling super().form_valid(), which re-ran form.save() and wrote a second history record for every new object. - Bump each parent at most once per attribution queryset delete. - Name the attribution source enum AttributionSourceEnum in the OpenAPI schema rather than the generic SourceEnum. --- api/v1_0/serializers/attribution.py | 4 ++- comicsdb/signals.py | 14 +++++---- comicsdb/views/issue.py | 3 +- comicsdb/views/mixins.py | 4 ++- metron/settings.py | 1 + tests/comicsdb/test_api_attribution.py | 41 +++++++++++++++++++++++++- tests/comicsdb/test_issue_views.py | 23 +++++++++++++++ 7 files changed, 81 insertions(+), 9 deletions(-) diff --git a/api/v1_0/serializers/attribution.py b/api/v1_0/serializers/attribution.py index 060c137c..488fe12b 100644 --- a/api/v1_0/serializers/attribution.py +++ b/api/v1_0/serializers/attribution.py @@ -11,6 +11,8 @@ Attribution.Source.GCD: "Grand Comics Database", Attribution.Source.DC: "DC", } +# Referenced by SPECTACULAR_SETTINGS["ENUM_NAME_OVERRIDES"]. +SOURCE_CHOICES = list(SOURCE_NAMES.values()) class AttributionSerializer(serializers.ModelSerializer): @@ -20,6 +22,6 @@ class Meta: model = Attribution fields = ("source", "url") - @extend_schema_field(serializers.ChoiceField(choices=list(SOURCE_NAMES.values()))) + @extend_schema_field(serializers.ChoiceField(choices=SOURCE_CHOICES)) def get_source(self, obj: Attribution) -> str: return SOURCE_NAMES[obj.source] diff --git a/comicsdb/signals.py b/comicsdb/signals.py index fc218a7b..f32cc9cf 100644 --- a/comicsdb/signals.py +++ b/comicsdb/signals.py @@ -348,8 +348,12 @@ def update_parent_modified_on_attribution_delete(sender, instance, origin=None, # Attributions are only deleted on their own (instance or queryset # delete) or by cascade when their parent is deleted. In the cascade # case the parent is going away too, so there's nothing to bump. - deleted_directly = isinstance(origin, sender) or ( - isinstance(origin, QuerySet) and origin.model is sender - ) - if deleted_directly: - _touch_attribution_parent(instance.content_type_id, instance.object_id) + parent = (instance.content_type_id, instance.object_id) + if isinstance(origin, sender): + _touch_attribution_parent(*parent) + elif isinstance(origin, QuerySet) and origin.model is sender: + # A queryset delete sends post_delete per row; bump each parent once. + touched = origin.__dict__.setdefault("_touched_attribution_parents", set()) + if parent not in touched: + touched.add(parent) + _touch_attribution_parent(*parent) diff --git a/comicsdb/views/issue.py b/comicsdb/views/issue.py index 4ff07357..3a7b60ab 100644 --- a/comicsdb/views/issue.py +++ b/comicsdb/views/issue.py @@ -299,7 +299,8 @@ def form_valid(self, form): "It may have been created by another user.", ) return self.form_invalid(form) - return super().form_valid(form) + # Not super().form_valid(): it would call form.save() a second time. + return HttpResponseRedirect(self.get_success_url()) class IssueUpdate(LoginRequiredMixin, UpdateView): diff --git a/comicsdb/views/mixins.py b/comicsdb/views/mixins.py index 9924e6f4..46f88a77 100644 --- a/comicsdb/views/mixins.py +++ b/comicsdb/views/mixins.py @@ -8,6 +8,7 @@ from django.db import transaction from django.db.models import Q +from django.http import HttpResponseRedirect from django.shortcuts import get_object_or_404, render from django.urls import reverse from django.views import View @@ -77,7 +78,8 @@ def form_valid(self, form): return super().form_invalid(form) LOGGER.info(self.get_log_message(), form.instance.name, self.request.user) - return super().form_valid(form) + # Not super().form_valid(): it would call form.save() a second time. + return HttpResponseRedirect(self.get_success_url()) class AttributionUpdateMixin: diff --git a/metron/settings.py b/metron/settings.py index 451ecb2c..c71971fc 100644 --- a/metron/settings.py +++ b/metron/settings.py @@ -344,6 +344,7 @@ "url": "https://creativecommons.org/licenses/by-sa/4.0/", }, "ENUM_NAME_OVERRIDES": { + "AttributionSourceEnum": "api.v1_0.serializers.attribution.SOURCE_CHOICES", "CurrencyEnum": "metron.choices.CURRENCY_CHOICES", "DesiredGradeEnum": "user_collection.models.GRADE_CHOICES", }, diff --git a/tests/comicsdb/test_api_attribution.py b/tests/comicsdb/test_api_attribution.py index f1f156a9..20129ec6 100644 --- a/tests/comicsdb/test_api_attribution.py +++ b/tests/comicsdb/test_api_attribution.py @@ -8,13 +8,14 @@ from django.test.utils import CaptureQueriesContext from django.urls import resolve, reverse from django.utils import timezone +from drf_spectacular.generators import SchemaGenerator from rest_framework import status from rest_framework.request import Request from rest_framework.test import APIRequestFactory from api.cache import ModelLabel, get_model_version from api.v1_0.serializers.attribution import SOURCE_NAMES -from comicsdb.models import Attribution, Character, Rating +from comicsdb.models import Attribution, Character, Publisher, Rating # (fixture name, API basename) for every resource whose detail response embeds attribution. ATTRIBUTED_RESOURCES = [ @@ -263,3 +264,41 @@ def test_edit_view_writes_parent_row_once(auto_login_user, dc_comics, local_cach assert len(parent_updates) == 1 dc_comics.refresh_from_db() assert dc_comics.modified > past + + +def test_create_view_saves_parent_once(auto_login_user, local_cache): + client, _ = auto_login_user() + data = { + "name": "Ahoy Comics", + "country": "US", + "comicsdb-attribution-content_type-object_id-TOTAL_FORMS": 1, + "comicsdb-attribution-content_type-object_id-INITIAL_FORMS": 0, + "comicsdb-attribution-content_type-object_id-0-source": "W", + "comicsdb-attribution-content_type-object_id-0-url": WIKI_URL, + } + + resp = client.post(reverse("publisher:create"), data=data) + + assert resp.status_code == status.HTTP_302_FOUND + ahoy = Publisher.objects.get(name="Ahoy Comics") + assert resp.url == ahoy.get_absolute_url() + assert ahoy.attribution.count() == 1 + # One history row per save. + assert ahoy.history.count() == 1 + + +def test_attribution_queryset_delete_bumps_each_parent_once(batman, superman, local_cache): + batman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) + batman.attribution.create(source=Attribution.Source.MARVEL, url=MARVEL_URL) + superman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) + + with patch("comicsdb.signals._touch_attribution_parent") as touch: + Attribution.objects.all().delete() + + assert touch.call_count == 2 + + +def test_schema_names_attribution_source_enum(db): + schemas = SchemaGenerator().get_schema(request=None, public=True)["components"]["schemas"] + assert schemas["AttributionSourceEnum"]["enum"] == list(SOURCE_NAMES.values()) + assert "SourceEnum" not in schemas diff --git a/tests/comicsdb/test_issue_views.py b/tests/comicsdb/test_issue_views.py index cf94755d..f4040f69 100644 --- a/tests/comicsdb/test_issue_views.py +++ b/tests/comicsdb/test_issue_views.py @@ -2199,6 +2199,29 @@ def test_issue_create_checks_variant_upc_against_cover_date( assert b'href="#section-variants"' in resp.content +def test_issue_create_saves_issue_once(auto_login_user, fc_series): + client, _ = auto_login_user() + data = { + "series": fc_series.pk, + "number": "1", + "cover_date": "2023-01-01", + "rating": Rating.objects.create(name="Test Rating").pk, + **{ + f"{prefix}-{field}": "0" + for prefix in ("credits", "variants", "attribution") + for field in ("TOTAL_FORMS", "INITIAL_FORMS") + }, + } + + resp = client.post(reverse("issue:create"), data=data) + + issue = Issue.objects.get(series=fc_series, number="1") + assert resp.status_code == HTML_REDIRECT_CODE + assert resp.url == issue.get_absolute_url() + # One history row per save. + assert issue.history.count() == 1 + + def test_issue_detail_shows_solicitation_tag(solicited_issue, auto_login_user): client, _ = auto_login_user() resp = client.get(reverse("issue:detail", kwargs={"slug": solicited_issue.slug})) From 6cd4b1a17337f20044ddc0feb8025b0faf6a773e Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Fri, 16 Oct 2026 10:09:42 -0400 Subject: [PATCH 08/14] Remove unused move_pub_to_imprint management command --- .../commands/move_pub_to_imprint.py | 131 ------------------ 1 file changed, 131 deletions(-) delete mode 100644 comicsdb/management/commands/move_pub_to_imprint.py diff --git a/comicsdb/management/commands/move_pub_to_imprint.py b/comicsdb/management/commands/move_pub_to_imprint.py deleted file mode 100644 index cacddaec..00000000 --- a/comicsdb/management/commands/move_pub_to_imprint.py +++ /dev/null @@ -1,131 +0,0 @@ -from typing import TYPE_CHECKING - -from django.contrib.contenttypes.models import ContentType -from django.core.management import BaseCommand - -if TYPE_CHECKING: - from django.db.models import QuerySet - -from comicsdb.models import Attribution, Imprint, Publisher, Series - - -class Command(BaseCommand): - help = "Moves publisher to imprint" - - def _delete_original(self, orig_pub: Publisher) -> None: - """ - Deletes the original publisher. - - Args: - orig_pub: The original Publisher object to be deleted. - - Returns: - None - """ - name = orig_pub.name - num, _ = orig_pub.delete() - if num > 0: - self.stdout.write(self.style.SUCCESS(f"Deleted Publisher: '{name}'")) - else: - self.stdout.write(self.style.ERROR(f"Failed to Delete Publisher: '{name}'")) - - def _update_attribution(self, orig_pub: Publisher, imprint: Imprint) -> None: - """ - Updates the attribution of the original publisher to the new imprint. - - Args: - orig_pub: The original Publisher object whose attribution needs to be updated. - imprint: The new Imprint object to which the attribution is updated. - - Returns: - None - """ - imprint_type = ContentType.objects.get_for_model(Imprint) - orig_attributions: QuerySet[Attribution] = orig_pub.attribution.all() - for attribution in orig_attributions: - attribution.content_type = imprint_type - attribution.object_id = imprint.pk - - res = Attribution.objects.bulk_update(orig_attributions, ["content_type", "object_id"]) - self.stdout.write( - self.style.SUCCESS(f"Moved {res} attribution records from the publisher to imprint") - ) - - def _create_imprint(self, original: Publisher, parent: Publisher) -> Imprint: - """ - Creates a new Imprint based on the information from the original Publisher. - - Args: - original: The original Publisher object from which the Imprint is created. - parent: The parent Publisher object for the new Imprint. - - Returns: - The newly created Imprint object. - """ - # TODO: Add image from original publisher - imprint, create = Imprint.objects.get_or_create( - name=original.name, - slug=original.slug, - desc=original.desc, - cv_id=original.cv_id, - publisher=parent, - founded=original.founded, - ) - if create: - self.stdout.write(self.style.SUCCESS(f"Added Imprint: {imprint}")) - else: - self.stdout.write(self.style.WARNING(f"Retrieved Existing Imprint: {imprint}")) - return imprint - - def _update_series(self, imprint: Imprint, series_qs) -> None: - """ - Updates the series objects with the new imprint and publisher. - - Args: - imprint: The new Imprint object to be assigned to the series. - series_qs: QuerySet of series objects to be updated. - - Returns: - None - """ - for series in series_qs: - series.imprint = imprint - series.publisher = imprint.publisher - changed = Series.objects.bulk_update(series_qs, ["imprint", "publisher"]) - self.stdout.write(self.style.SUCCESS(f"Updated {changed} series")) - - def add_arguments(self, parser): - """ - Adds command line arguments for the move_pub_to_imprint management command. - - Args: - parser: The parser object to which the arguments are added. - - Returns: - None - """ - parser.add_argument("--publisher", type=int, required=True) - parser.add_argument("--parent", type=int, required=True) - - def handle(self, *args, **options): - """ - Handles the process of moving a publisher to a new imprint by creating the imprint, - updating series and attribution, and deleting the original publisher. - - Args: - *args: Variable length argument list. - **options: Keyword arguments. - - Returns: - None - """ - - orig_pub = Publisher.objects.get(pk=options["publisher"]) - parent_pub = Publisher.objects.get(pk=options["parent"]) - - new_imprint = self._create_imprint(orig_pub, parent_pub) - series_qs = orig_pub.series.all() - self._update_series(new_imprint, series_qs) - self._update_attribution(orig_pub, new_imprint) - # remove the original publisher - self._delete_original(orig_pub) From 936055f983300f9fbf7284df9829a0acd0f06e4b Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Fri, 16 Oct 2026 10:12:20 -0400 Subject: [PATCH 09/14] Drop attribution prefetch mixin and parent-bump suppression - Remove AttributionRetrieveMixin: retrieve serializes a single object, so prefetching attribution saved no queries. - Revert attribution_parent_saved_separately(): it saved one cheap UPDATE per formset row but every save path had to remember to use it (the admin inlines didn't), and forgetting was silent. - Fall back to the stored source code in AttributionSerializer instead of raising KeyError on a value outside SOURCE_NAMES. --- api/v1_0/serializers/attribution.py | 3 +- api/views.py | 26 +--------- comicsdb/signals.py | 26 +--------- comicsdb/views/issue.py | 12 ++--- comicsdb/views/mixins.py | 13 ++--- tests/comicsdb/test_api_attribution.py | 68 ++++++-------------------- 6 files changed, 25 insertions(+), 123 deletions(-) diff --git a/api/v1_0/serializers/attribution.py b/api/v1_0/serializers/attribution.py index 488fe12b..caf77b73 100644 --- a/api/v1_0/serializers/attribution.py +++ b/api/v1_0/serializers/attribution.py @@ -24,4 +24,5 @@ class Meta: @extend_schema_field(serializers.ChoiceField(choices=SOURCE_CHOICES)) def get_source(self, obj: Attribution) -> str: - return SOURCE_NAMES[obj.source] + # Fall back to the stored code rather than 500 on a value outside the choices. + return SOURCE_NAMES.get(obj.source, obj.source) diff --git a/api/views.py b/api/views.py index fa13d0e6..a5a42a24 100644 --- a/api/views.py +++ b/api/views.py @@ -198,17 +198,6 @@ def get_object_modified(self): return self._cached_object_modified -class AttributionRetrieveMixin: - """For viewsets whose retrieve serializer embeds `attribution`: prefetch - it on retrieve. Concrete get_queryset() overrides must call super().""" - - def get_queryset(self): - queryset = super().get_queryset() - if self.action == "retrieve": - queryset = queryset.prefetch_related("attribution") - return queryset - - 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 @@ -389,7 +378,6 @@ class ArcViewSet( UserTrackingMixin, IssueListMixin, mixins.CreateModelMixin, - AttributionRetrieveMixin, ConditionalRetrieveModelMixin, CachedListModelMixin, mixins.UpdateModelMixin, @@ -435,7 +423,6 @@ class CharacterViewSet( UserTrackingMixin, IssueListMixin, mixins.CreateModelMixin, - AttributionRetrieveMixin, ConditionalRetrieveModelMixin, CachedListModelMixin, mixins.UpdateModelMixin, @@ -497,7 +484,6 @@ def get_serializer_class(self): class CreatorViewSet( UserTrackingMixin, mixins.CreateModelMixin, - AttributionRetrieveMixin, ConditionalRetrieveModelMixin, CachedListModelMixin, mixins.UpdateModelMixin, @@ -551,7 +537,6 @@ def create(self, request, *args, **kwargs) -> Response: class ImprintViewSet( UserTrackingMixin, mixins.CreateModelMixin, - AttributionRetrieveMixin, ConditionalRetrieveModelMixin, CachedListModelMixin, mixins.UpdateModelMixin, @@ -598,7 +583,6 @@ def get_serializer_class(self): class IssueViewSet( UserTrackingMixin, mixins.CreateModelMixin, - AttributionRetrieveMixin, ConditionalRetrieveModelMixin, CachedListModelMixin, mixins.UpdateModelMixin, @@ -658,11 +642,9 @@ def get_modified_queryset(self): def get_queryset(self): if self.action == "list": - return super().get_queryset().select_related("series", "series__series_type") + return Issue.objects.select_related("series", "series__series_type") return ( - super() - .get_queryset() - .select_related( + Issue.objects.select_related( "series", "series__series_type", "series__publisher", @@ -717,7 +699,6 @@ def get_serializer_class(self): class PublisherViewSet( UserTrackingMixin, mixins.CreateModelMixin, - AttributionRetrieveMixin, ConditionalRetrieveModelMixin, CachedListModelMixin, mixins.UpdateModelMixin, @@ -809,7 +790,6 @@ class SeriesViewSet( UserTrackingMixin, IssueListMixin, mixins.CreateModelMixin, - AttributionRetrieveMixin, ConditionalRetrieveModelMixin, CachedListModelMixin, mixins.UpdateModelMixin, @@ -906,7 +886,6 @@ class TeamViewSet( UserTrackingMixin, IssueListMixin, mixins.CreateModelMixin, - AttributionRetrieveMixin, ConditionalRetrieveModelMixin, CachedListModelMixin, mixins.UpdateModelMixin, @@ -967,7 +946,6 @@ def get_serializer_class(self): class UniverseViewSet( UserTrackingMixin, mixins.CreateModelMixin, - AttributionRetrieveMixin, ConditionalRetrieveModelMixin, CachedListModelMixin, mixins.UpdateModelMixin, diff --git a/comicsdb/signals.py b/comicsdb/signals.py index f32cc9cf..faa6da13 100644 --- a/comicsdb/signals.py +++ b/comicsdb/signals.py @@ -1,6 +1,4 @@ import logging -from contextlib import contextmanager -from contextvars import ContextVar from django.core.exceptions import FieldDoesNotExist from django.db.models import QuerySet @@ -275,26 +273,6 @@ def bump_cache_on_name_change(label, sender, instance, **kwargs): bump_model_version(label) -_attribution_parent_saved_separately = ContextVar( - "attribution_parent_saved_separately", default=False -) - - -@contextmanager -def attribution_parent_saved_separately(): - """Skip the attribution signals' parent bump inside this block. - - For edit views that save the parent itself in the same transaction as - its attribution formset: that save already updates `modified` (auto_now) - and bumps the parent's cache version, so bumping it again per - attribution row would only duplicate the work.""" - token = _attribution_parent_saved_separately.set(True) - try: - yield - finally: - _attribution_parent_saved_separately.reset(token) - - def _touch_attribution_parent(content_type_id, object_id): """Attribution is a generic relation, so saving or deleting one doesn't touch its parent row. Bump the parent's `modified` explicitly so its @@ -302,8 +280,6 @@ def _touch_attribution_parent(content_type_id, object_id): and its cache version, since its list response embeds `modified`.""" from django.contrib.contenttypes.models import ContentType # noqa: PLC0415 - if _attribution_parent_saved_separately.get(): - return # get_for_id() is served from ContentType's in-process cache. model = ContentType.objects.get_for_id(content_type_id).model_class() if model is None: @@ -326,7 +302,7 @@ def track_attribution_parent(sender, instance, raw=False, **kwargs): at before this save, so update_parent_modified_on_attribution_save() can also bump the old parent when the attribution is reassigned.""" instance._previous_parent = None - if raw or instance._state.adding or _attribution_parent_saved_separately.get(): + if raw or instance._state.adding: return instance._previous_parent = ( sender.objects.filter(pk=instance.pk).values_list("content_type_id", "object_id").first() diff --git a/comicsdb/views/issue.py b/comicsdb/views/issue.py index 3a7b60ab..0da95e8f 100644 --- a/comicsdb/views/issue.py +++ b/comicsdb/views/issue.py @@ -25,7 +25,6 @@ from comicsdb.models.attribution import Attribution from comicsdb.models.series import SeriesType from comicsdb.models.variant import Variant -from comicsdb.signals import attribution_parent_saved_separately from comicsdb.views.constants import DETAIL_PAGINATE_BY, PAGINATE_BY from comicsdb.views.history import HistoryListView from comicsdb.views.issue_list_helpers import ( @@ -282,9 +281,7 @@ def form_valid(self, form): variants_form.instance = self.object variants_form.save() attribution_form.instance = self.object - # The issue was just saved, so there's nothing for the rows to bump. - with attribution_parent_saved_separately(): - attribution_form.save() + attribution_form.save() LOGGER.info( "Issue: %s #%s was created by %s", @@ -366,8 +363,7 @@ def form_valid(self, form): attribution_form.instance = self.object credits_form.save() variants_form.save() - with attribution_parent_saved_separately(): - attribution_form.save() + attribution_form.save() else: return super().form_invalid(form) @@ -377,9 +373,7 @@ def form_valid(self, form): form.instance.number, self.request.user, ) - # Saves the issue, whose `modified`/cache bump also covers the - # attribution changes -- so it must commit with them. - return super().form_valid(form) + return super().form_valid(form) class IssueFormsetAddRowView(LoginRequiredMixin, View): diff --git a/comicsdb/views/mixins.py b/comicsdb/views/mixins.py index 46f88a77..92b333cd 100644 --- a/comicsdb/views/mixins.py +++ b/comicsdb/views/mixins.py @@ -17,7 +17,6 @@ from comicsdb.forms.attribution import AttributionFormSet from comicsdb.forms.issue import MINIMUM_YEAR from comicsdb.models.attribution import Attribution -from comicsdb.signals import attribution_parent_saved_separately from comicsdb.views.constants import DETAIL_PAGINATE_BY LOGGER = logging.getLogger(__name__) @@ -71,9 +70,7 @@ def form_valid(self, form): if attribution_form.is_valid(): self.object = form.save() attribution_form.instance = self.object - # The parent was just saved, so there's nothing for the rows to bump. - with attribution_parent_saved_separately(): - attribution_form.save() + attribution_form.save() else: return super().form_invalid(form) @@ -151,16 +148,12 @@ def form_valid(self, form): if attribution_form.is_valid(): self.object = form.save(commit=False) attribution_form.instance = self.object - with attribution_parent_saved_separately(): - attribution_form.save() + attribution_form.save() else: return super().form_invalid(form) - # Saves the parent, whose `modified`/cache bump also covers the - # attribution changes -- so it must commit with them. - response = super().form_valid(form) LOGGER.info(self.get_log_message(), form.instance.name, self.request.user) - return response + return super().form_valid(form) class NavigationMixin: diff --git a/tests/comicsdb/test_api_attribution.py b/tests/comicsdb/test_api_attribution.py index 20129ec6..3299c01c 100644 --- a/tests/comicsdb/test_api_attribution.py +++ b/tests/comicsdb/test_api_attribution.py @@ -6,12 +6,10 @@ from django.core.cache.backends.locmem import LocMemCache from django.db import connection from django.test.utils import CaptureQueriesContext -from django.urls import resolve, reverse +from django.urls import reverse from django.utils import timezone from drf_spectacular.generators import SchemaGenerator from rest_framework import status -from rest_framework.request import Request -from rest_framework.test import APIRequestFactory from api.cache import ModelLabel, get_model_version from api.v1_0.serializers.attribution import SOURCE_NAMES @@ -85,27 +83,6 @@ def test_list_does_not_include_attribution( assert "attribution" not in resp.json()["results"][0] -@pytest.mark.parametrize(("fixture_name", "basename"), ATTRIBUTED_RESOURCES) -def test_retrieve_queryset_prefetches_attribution( - db, request, django_assert_num_queries, fixture_name, basename -): - """A detail response serializes a single object, so a missing prefetch - still costs only one attribution query -- comparing query counts over - the endpoint can't catch it. Check the retrieve queryset directly - instead: the object it returns must already have attribution loaded.""" - obj = request.getfixturevalue(fixture_name) - obj.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) - - view = resolve(reverse(f"api:{basename}-detail", args=(obj.pk,))).func.cls() - view.action = "retrieve" - view.request = Request(APIRequestFactory().get("/")) - view.kwargs = {"pk": obj.pk} - fetched = view.get_queryset().get(pk=obj.pk) - - with django_assert_num_queries(0): - assert [a.url for a in fetched.attribution.all()] == [WIKI_URL] - - def test_attribution_add_busts_detail_cache(api_client_with_credentials, batman, local_cache): url = reverse("api:character-detail", args=(batman.pk,)) assert api_client_with_credentials.get(url).json()["attribution"] == [] @@ -236,36 +213,6 @@ def test_attribution_on_model_without_modified_saves(db, local_cache): attribution.delete() -def test_edit_view_writes_parent_row_once(auto_login_user, dc_comics, local_cache): - """The edit view saves the parent itself, so the attribution signal - mustn't also UPDATE it once per formset row.""" - client, _ = auto_login_user() - data = { - "name": dc_comics.name, - "country": "US", - "attribution-TOTAL_FORMS": 2, - "attribution-INITIAL_FORMS": 0, - "attribution-0-source": "W", - "attribution-0-url": WIKI_URL, - "attribution-1-source": "D", - "attribution-1-url": "https://www.dc.com/example", - } - past = timezone.now() - timedelta(days=1) - type(dc_comics).objects.filter(pk=dc_comics.pk).update(modified=past) - - with CaptureQueriesContext(connection) as ctx: - resp = client.post(reverse("publisher:update", kwargs={"slug": dc_comics.slug}), data) - - assert resp.status_code == status.HTTP_302_FOUND - assert dc_comics.attribution.count() == 2 - parent_updates = [ - q for q in ctx.captured_queries if q["sql"].startswith('UPDATE "comicsdb_publisher"') - ] - assert len(parent_updates) == 1 - dc_comics.refresh_from_db() - assert dc_comics.modified > past - - def test_create_view_saves_parent_once(auto_login_user, local_cache): client, _ = auto_login_user() data = { @@ -302,3 +249,16 @@ def test_schema_names_attribution_source_enum(db): schemas = SchemaGenerator().get_schema(request=None, public=True)["components"]["schemas"] assert schemas["AttributionSourceEnum"]["enum"] == list(SOURCE_NAMES.values()) assert "SourceEnum" not in schemas + + +def test_unknown_source_code_falls_back_to_stored_value( + api_client_with_credentials, batman, local_cache +): + # The model only enforces choices in forms, so a raw write can store anything. + attribution = batman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) + Attribution.objects.filter(pk=attribution.pk).update(source="X") + + resp = api_client_with_credentials.get(reverse("api:character-detail", args=(batman.pk,))) + + assert resp.status_code == status.HTTP_200_OK + assert resp.json()["attribution"] == [{"source": "X", "url": WIKI_URL}] From 6744bd22adc4611d763fc14146c6e11d186b61b0 Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Fri, 16 Oct 2026 10:16:39 -0400 Subject: [PATCH 10/14] Track attribution's parent at load instead of re-querying in pre_save Record (content_type_id, object_id) in Attribution.from_db() and after each save, and compare against it in post_save to detect reassignment. This replaces the pre_save handler, which ran an extra SELECT before every update of an existing attribution. --- comicsdb/apps.py | 6 ---- comicsdb/models/attribution.py | 12 +++++++ comicsdb/signals.py | 17 +++------ tests/comicsdb/test_api_attribution.py | 50 ++++++++++++++++++++++++++ 4 files changed, 66 insertions(+), 19 deletions(-) diff --git a/comicsdb/apps.py b/comicsdb/apps.py index f2863640..f5f4fd4e 100644 --- a/comicsdb/apps.py +++ b/comicsdb/apps.py @@ -9,7 +9,6 @@ bump_cache_on_name_change, pre_delete_credit, pre_delete_image, - track_attribution_parent, track_name_change, update_arc_modified, update_character_modified, @@ -146,11 +145,6 @@ def ready(self): ) attribution = self.get_model("Attribution") - pre_save.connect( - track_attribution_parent, - sender=attribution, - dispatch_uid="pre_save_attribution_track_parent", - ) post_save.connect( update_parent_modified_on_attribution_save, sender=attribution, diff --git a/comicsdb/models/attribution.py b/comicsdb/models/attribution.py index 9b91a269..4a9081d2 100644 --- a/comicsdb/models/attribution.py +++ b/comicsdb/models/attribution.py @@ -18,9 +18,21 @@ class Source(models.TextChoices): object_id = models.PositiveIntegerField() content_object = GenericForeignKey("content_type", "object_id") + #: (content_type_id, object_id) as last loaded or saved, so the post_save + #: signal can bump the old parent when an attribution is reassigned. + #: None for an unsaved instance, or one loaded with either field deferred. + loaded_parent: tuple[int, int] | None = None + class Meta: indexes = [models.Index(fields=["content_type", "object_id"], name="ct_obj_id_idx")] ordering = ["content_type", "object_id", "id"] def __str__(self) -> str: return f"{self.source} Attribution" + + @classmethod + def from_db(cls, db, field_names, values, *, fetch_mode=None): + instance = super().from_db(db, field_names, values, fetch_mode=fetch_mode) + if "content_type_id" in instance.__dict__ and "object_id" in instance.__dict__: + instance.loaded_parent = (instance.content_type_id, instance.object_id) + return instance diff --git a/comicsdb/signals.py b/comicsdb/signals.py index faa6da13..af8486e3 100644 --- a/comicsdb/signals.py +++ b/comicsdb/signals.py @@ -297,27 +297,18 @@ def _touch_attribution_parent(content_type_id, object_id): bump_model_version(label) -def track_attribution_parent(sender, instance, raw=False, **kwargs): - """pre_save receiver recording the parent an existing attribution points - at before this save, so update_parent_modified_on_attribution_save() can - also bump the old parent when the attribution is reassigned.""" - instance._previous_parent = None - if raw or instance._state.adding: - return - instance._previous_parent = ( - sender.objects.filter(pk=instance.pk).values_list("content_type_id", "object_id").first() - ) - - def update_parent_modified_on_attribution_save(sender, instance, raw=False, **kwargs): # Fixture loading may run before the ContentType/parent rows exist. if raw: return current = (instance.content_type_id, instance.object_id) _touch_attribution_parent(*current) - previous = instance.__dict__.pop("_previous_parent", None) + # Also bump the old parent if the attribution was reassigned (see + # Attribution.from_db()), then track the new one for the next save. + previous = instance.loaded_parent if previous is not None and previous != current: _touch_attribution_parent(*previous) + instance.loaded_parent = current def update_parent_modified_on_attribution_delete(sender, instance, origin=None, **kwargs): diff --git a/tests/comicsdb/test_api_attribution.py b/tests/comicsdb/test_api_attribution.py index 3299c01c..47ee047f 100644 --- a/tests/comicsdb/test_api_attribution.py +++ b/tests/comicsdb/test_api_attribution.py @@ -162,6 +162,56 @@ def test_attribution_reassignment_bumps_old_and_new_parent(batman, superman, loc assert superman.modified > past +def test_attribution_reassignment_of_loaded_instance_bumps_old_parent( + batman, superman, local_cache +): + created = batman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) + attribution = Attribution.objects.get(pk=created.pk) + past = timezone.now() - timedelta(days=1) + Character.objects.filter(pk__in=(batman.pk, superman.pk)).update(modified=past) + + attribution.object_id = superman.pk + attribution.save() + batman.refresh_from_db() + assert batman.modified > past + + # The instance now tracks superman, so moving it back bumps superman. + Character.objects.filter(pk=superman.pk).update(modified=past) + attribution.object_id = batman.pk + attribution.save() + superman.refresh_from_db() + assert superman.modified > past + + +def test_attribution_save_does_not_reread_its_row(batman, local_cache): + created = batman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) + attribution = Attribution.objects.get(pk=created.pk) + attribution.url = MARVEL_URL + + with CaptureQueriesContext(connection) as ctx: + attribution.save() + + assert not [ + q + for q in ctx.captured_queries + if q["sql"].startswith("SELECT") and "comicsdb_attribution" in q["sql"] + ] + + +def test_attribution_save_with_deferred_parent_fields(batman, local_cache): + created = batman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) + attribution = Attribution.objects.only("url").get(pk=created.pk) + assert attribution.loaded_parent is None + past = timezone.now() - timedelta(days=1) + Character.objects.filter(pk=batman.pk).update(modified=past) + + attribution.url = MARVEL_URL + attribution.save() + + batman.refresh_from_db() + assert batman.modified > past + + def test_attribution_save_does_not_query_content_type(batman, local_cache): attribution = batman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) attribution = Attribution.objects.get(pk=attribution.pk) # fresh instance, no cached FK From e5407acb83f015106dc17e38e28ec9f89ff54143 Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Fri, 16 Oct 2026 10:23:06 -0400 Subject: [PATCH 11/14] Save parent inside the transaction in attribution update views AttributionUpdateMixin.form_valid() only called form.save(commit=False) inside transaction.atomic() and left the real save to super().form_valid(), which ran after the block closed. If that save failed, the attribution rows and the parent `modified` bump stayed committed, but the parent edit was lost. Save the form, including its M2M fields, inside the transaction and redirect directly, the same way AttributionCreateMixin does. --- comicsdb/views/mixins.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/comicsdb/views/mixins.py b/comicsdb/views/mixins.py index 92b333cd..b6fd3ba0 100644 --- a/comicsdb/views/mixins.py +++ b/comicsdb/views/mixins.py @@ -146,14 +146,16 @@ def form_valid(self, form): with transaction.atomic(): form.instance.edited_by = self.request.user if attribution_form.is_valid(): - self.object = form.save(commit=False) + self.object = form.save() attribution_form.instance = self.object attribution_form.save() else: return super().form_invalid(form) LOGGER.info(self.get_log_message(), form.instance.name, self.request.user) - return super().form_valid(form) + # Not super().form_valid(): it would call form.save() a second time, outside the + # transaction. + return HttpResponseRedirect(self.get_success_url()) class NavigationMixin: From 816ec86ff175440d0e14d3e431e071620f1aa6e3 Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Fri, 16 Oct 2026 10:28:26 -0400 Subject: [PATCH 12/14] Drop attribution parent reassignment tracking Nothing reassigns an attribution to a different parent: the formset and admin inline fix the parent, and move_pub_to_imprint, the only code that moved attributions, has been removed. Remove Attribution.from_db() and loaded_parent, and only bump the current parent on save. --- comicsdb/models/attribution.py | 12 ------- comicsdb/signals.py | 11 ++---- tests/comicsdb/test_api_attribution.py | 49 -------------------------- 3 files changed, 3 insertions(+), 69 deletions(-) diff --git a/comicsdb/models/attribution.py b/comicsdb/models/attribution.py index 4a9081d2..9b91a269 100644 --- a/comicsdb/models/attribution.py +++ b/comicsdb/models/attribution.py @@ -18,21 +18,9 @@ class Source(models.TextChoices): object_id = models.PositiveIntegerField() content_object = GenericForeignKey("content_type", "object_id") - #: (content_type_id, object_id) as last loaded or saved, so the post_save - #: signal can bump the old parent when an attribution is reassigned. - #: None for an unsaved instance, or one loaded with either field deferred. - loaded_parent: tuple[int, int] | None = None - class Meta: indexes = [models.Index(fields=["content_type", "object_id"], name="ct_obj_id_idx")] ordering = ["content_type", "object_id", "id"] def __str__(self) -> str: return f"{self.source} Attribution" - - @classmethod - def from_db(cls, db, field_names, values, *, fetch_mode=None): - instance = super().from_db(db, field_names, values, fetch_mode=fetch_mode) - if "content_type_id" in instance.__dict__ and "object_id" in instance.__dict__: - instance.loaded_parent = (instance.content_type_id, instance.object_id) - return instance diff --git a/comicsdb/signals.py b/comicsdb/signals.py index af8486e3..c68fc4d9 100644 --- a/comicsdb/signals.py +++ b/comicsdb/signals.py @@ -301,14 +301,9 @@ def update_parent_modified_on_attribution_save(sender, instance, raw=False, **kw # Fixture loading may run before the ContentType/parent rows exist. if raw: return - current = (instance.content_type_id, instance.object_id) - _touch_attribution_parent(*current) - # Also bump the old parent if the attribution was reassigned (see - # Attribution.from_db()), then track the new one for the next save. - previous = instance.loaded_parent - if previous is not None and previous != current: - _touch_attribution_parent(*previous) - instance.loaded_parent = current + # Only the current parent is bumped: nothing reassigns an attribution to a + # different parent, so code that does must bump the old parent itself. + _touch_attribution_parent(instance.content_type_id, instance.object_id) def update_parent_modified_on_attribution_delete(sender, instance, origin=None, **kwargs): diff --git a/tests/comicsdb/test_api_attribution.py b/tests/comicsdb/test_api_attribution.py index 47ee047f..383b1bf8 100644 --- a/tests/comicsdb/test_api_attribution.py +++ b/tests/comicsdb/test_api_attribution.py @@ -148,41 +148,6 @@ def test_attribution_change_bumps_parent_modified_and_cache_version(batman, loca assert get_model_version(ModelLabel.CHARACTER) == before + 2 -def test_attribution_reassignment_bumps_old_and_new_parent(batman, superman, local_cache): - past = timezone.now() - timedelta(days=1) - attribution = batman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) - Character.objects.filter(pk__in=(batman.pk, superman.pk)).update(modified=past) - - attribution.object_id = superman.pk - attribution.save() - - batman.refresh_from_db() - superman.refresh_from_db() - assert batman.modified > past - assert superman.modified > past - - -def test_attribution_reassignment_of_loaded_instance_bumps_old_parent( - batman, superman, local_cache -): - created = batman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) - attribution = Attribution.objects.get(pk=created.pk) - past = timezone.now() - timedelta(days=1) - Character.objects.filter(pk__in=(batman.pk, superman.pk)).update(modified=past) - - attribution.object_id = superman.pk - attribution.save() - batman.refresh_from_db() - assert batman.modified > past - - # The instance now tracks superman, so moving it back bumps superman. - Character.objects.filter(pk=superman.pk).update(modified=past) - attribution.object_id = batman.pk - attribution.save() - superman.refresh_from_db() - assert superman.modified > past - - def test_attribution_save_does_not_reread_its_row(batman, local_cache): created = batman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) attribution = Attribution.objects.get(pk=created.pk) @@ -198,20 +163,6 @@ def test_attribution_save_does_not_reread_its_row(batman, local_cache): ] -def test_attribution_save_with_deferred_parent_fields(batman, local_cache): - created = batman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) - attribution = Attribution.objects.only("url").get(pk=created.pk) - assert attribution.loaded_parent is None - past = timezone.now() - timedelta(days=1) - Character.objects.filter(pk=batman.pk).update(modified=past) - - attribution.url = MARVEL_URL - attribution.save() - - batman.refresh_from_db() - assert batman.modified > past - - def test_attribution_save_does_not_query_content_type(batman, local_cache): attribution = batman.attribution.create(source=Attribution.Source.WIKIPEDIA, url=WIKI_URL) attribution = Attribution.objects.get(pk=attribution.pk) # fresh instance, no cached FK From 827052d9bc8ae1385fb57a3f762f3377aae4566f Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Fri, 16 Oct 2026 10:34:33 -0400 Subject: [PATCH 13/14] Add API changelog entry for version 1.12 Record the new attribution field on detail endpoints, and the cover_date_range filter from #628, which never got an entry. --- api/README.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/api/README.md b/api/README.md index 3f8104e9..dc88ad92 100644 --- a/api/README.md +++ b/api/README.md @@ -2571,6 +2571,11 @@ For questions, issues, or feature requests: ## Changelog +### Version 1.12 + +- Added read-only `attribution` field to the Arc, Character, Creator, Imprint, Issue, Publisher, Series, Team, and Universe detail endpoints: a list of `{"source", "url"}` objects naming where descriptive content was adapted from +- Added `cover_date_range_after` and `cover_date_range_before` filters to the Issue endpoint + ### Version 1.11 - Added `publisher` (object with `id`/`name`) and `series_type` (object with `id`/`name`) fields to the Series list endpoint From 10d103c206b1eb2310b1c7c34e3a161e0a498dcb Mon Sep 17 00:00:00 2001 From: Brian Pepple Date: Fri, 16 Oct 2026 10:44:59 -0400 Subject: [PATCH 14/14] Version detail cache keys by response shape Detail keys depend only on pk and `modified`, so responses cached before a shape change, such as the new `attribution` field, were served for up to DETAIL_CACHE_TTL after deploy. Add DETAIL_CACHE_SCHEMA_VERSION to the key so bumping it orphans entries in the old shape. --- api/cache.py | 11 ++++++++++- tests/api/test_audit_response_cache.py | 5 +++++ tests/comicsdb/test_api_response_caching.py | 16 +++++++++++++++- 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/api/cache.py b/api/cache.py index 9410b9d6..a4fabea3 100644 --- a/api/cache.py +++ b/api/cache.py @@ -23,6 +23,10 @@ DETAIL_CACHE_TTL = 60 * 60 * 24 * 3 # 3 days; live keys self-invalidate on write. LIST_CACHE_TTL = 60 * 2 # 2min; bounds staleness from nested-object changes we don't chase. +# Bump when a detail response's shape changes (e.g. a field is added), so entries cached in the +# old shape aren't served for up to DETAIL_CACHE_TTL after deploy. List keys don't need this: +# LIST_CACHE_TTL is short enough to ride out. +DETAIL_CACHE_SCHEMA_VERSION = 2 _VERSION_KEY_PREFIX = "cachever" @@ -112,7 +116,12 @@ def detail_cache_key( `request` supplies origin and query params -- see `_request_digest()`. """ - key = f"api:detail:{model_label}:{action}:{pk}:{modified.timestamp()}" + # The schema version goes after the model label, which audit_response_cache + # reads as the key's category. + key = ( + f"api:detail:{model_label}:v{DETAIL_CACHE_SCHEMA_VERSION}:{action}:{pk}:" + f"{modified.timestamp()}" + ) if dependent_labels: version_map = get_model_versions(dependent_labels) versions = "-".join(str(version_map[lbl]) for lbl in dependent_labels) diff --git a/tests/api/test_audit_response_cache.py b/tests/api/test_audit_response_cache.py index 37b405cc..07aeafcc 100644 --- a/tests/api/test_audit_response_cache.py +++ b/tests/api/test_audit_response_cache.py @@ -20,6 +20,11 @@ def test_categorize_detail_key_with_dependent_labels_still_extracts_model(): assert Command._categorize(key) == "api:detail:series" +def test_categorize_detail_key_with_schema_version_still_extracts_model(): + key = ":1:api:detail:arc:v2:retrieve:7:1704085200.0:abcdef0123456789" + assert Command._categorize(key) == "api:detail:arc" + + def test_categorize_list_key_extracts_model_label(): key = ":1:api:list:arc::4-2:abcdef0123456789" assert Command._categorize(key) == "api:list:arc" diff --git a/tests/comicsdb/test_api_response_caching.py b/tests/comicsdb/test_api_response_caching.py index ba967adb..f9d62113 100644 --- a/tests/comicsdb/test_api_response_caching.py +++ b/tests/comicsdb/test_api_response_caching.py @@ -11,7 +11,7 @@ from rest_framework.pagination import PageNumberPagination from rest_framework.request import Request -from api import views as api_views +from api import cache as api_cache, views as api_views from api.cache import ModelLabel, get_model_version from api.views import CollectionViewSet, PullListViewSet, WishListViewSet from comicsdb.models import Credits, Issue, Variant @@ -839,6 +839,20 @@ def test_arc_retrieve_does_not_poison_issue_list_cache( assert body["results"][0]["number"] == "1" +def test_detail_schema_version_bump_orphans_cached_responses( + api_client_with_credentials, fc_arc, monkeypatch, local_cache +): + """Entries cached before a detail response's shape changed must not be + served after DETAIL_CACHE_SCHEMA_VERSION is bumped.""" + url = reverse("api:arc-detail", kwargs={"pk": fc_arc.pk}) + assert api_client_with_credentials.get(url)["X-Cache"] == "MISS" + assert api_client_with_credentials.get(url)["X-Cache"] == "HIT" + + monkeypatch.setattr(api_cache, "DETAIL_CACHE_SCHEMA_VERSION", 3) + + assert api_client_with_credentials.get(url)["X-Cache"] == "MISS" + + def test_arc_issue_list_pagination_is_not_poisoned_across_pages( api_client_with_credentials, issue_with_arc, fc_arc, fc_series, monkeypatch, local_cache ):