fix(gamedata): planets refetched in one cached batch instead of one request per planet

backend/core/services/cache_manager.py

@@ -13,6 +13,8 @@
 from django.db import transaction
 from django.http import HttpRequest, HttpResponse
 from django.utils.cache import patch_cache_control, patch_vary_headers
+from django.utils.http import parse_etags
+from rest_framework.request import Request
 from rest_framework_csv.renderers import CSVRenderer
 
 logger = structlog.get_logger(__name__)
@@ -59,8 +61,11 @@ def key(
     ) -> str:
         prefix = cls._prefix(ns, scope)
         version = cache.get(f'{prefix}:ver') or 1
-        digest = blake2b(orjson.dumps([fmt, *parts], option=orjson.OPT_SORT_KEYS), digest_size=8).hexdigest()
-        return f'{prefix}:v{version}:{endpoint}:{digest}'
+        return f'{prefix}:v{version}:{endpoint}:{cls._digest(fmt, parts)}'
+
+    @staticmethod
+    def _digest(fmt: str, parts: tuple[str | int | UUID | None, ...]) -> str:
+        return blake2b(orjson.dumps([fmt, *parts], option=orjson.OPT_SORT_KEYS), digest_size=8).hexdigest()
 
     @classmethod
     def invalidate(cls, ns: CacheNamespace, scope: int | str | None = None) -> None:
@@ -91,24 +96,28 @@ def _rebuild(
             # the builder is slow or died, build ourselves
 
         try:
-            body = orjson.dumps(
-                build(),
-                default=lambda obj: (
-                    float(obj) if isinstance(obj, decimal.Decimal) else str(obj) if isinstance(obj, UUID) else None
-                ),
-            )
-            if fmt == 'csv':
-                # rendered from the json round trip, so values match the json payload
-                context = {'header': csv_header} if csv_header else {}
-                body = CSVRenderer().render(orjson.loads(body), renderer_context=context)
-            # weak: the gzip and the plain body share it
-            entry = (f'W/"{blake2b(body, digest_size=16).hexdigest()}"', gzip.compress(body, compresslevel=9, mtime=0))
+            entry = cls._encode(build(), fmt, csv_header)
             cache.set(key, entry, ttl)
             return entry
         finally:
             if owns_lock:
                 cache.delete(lock_key)
 
+    @staticmethod
+    def _encode(data: object, fmt: str = 'json', csv_header: list[str] | None = None) -> CacheEntry:
+        body = orjson.dumps(
+            data,
+            default=lambda obj: (
+                float(obj) if isinstance(obj, decimal.Decimal) else str(obj) if isinstance(obj, UUID) else None
+            ),
+        )
+        if fmt == 'csv':
+            # rendered from the json round trip, so values match the json payload
+            context = {'header': csv_header} if csv_header else {}
+            body = CSVRenderer().render(orjson.loads(body), renderer_context=context)
+        # weak: the gzip and the plain body share it
+        return f'W/"{blake2b(body, digest_size=16).hexdigest()}"', gzip.compress(body, compresslevel=9, mtime=0)
+
     @classmethod
     def respond(
         cls,
@@ -146,3 +155,50 @@ def respond(
         else:
             patch_cache_control(response, public=True, max_age=PUBLIC_MAX_AGE)
         return response
+
+    @classmethod
+    def respond_many(
+        cls,
+        request: HttpRequest | Request,
+        ns: CacheNamespace,
+        endpoint: str,
+        scopes: list[str],
+        build_many: Callable[[list[str]], dict[str, object]],
+    ) -> HttpResponse:
+        """
+        A JSON array of the per-scope entries of `endpoint`, in `scopes` order, sharing the entries `respond` builds
+        for each scope. `build_many` builds the missing ones in one go; scopes it leaves out are omitted, not cached.
+        """
+        if ns.private:
+            raise ValueError(f'{ns.name} is private, respond_many serves public entries only')
+
+        digest = cls._digest('json', ())
+        prefixes = {scope: cls._prefix(ns, scope) for scope in scopes}
+        versions = cache.get_many([f'{prefix}:ver' for prefix in prefixes.values()])
+        keys = {
+            scope: f'{prefix}:v{versions.get(f"{prefix}:ver") or 1}:{endpoint}:{digest}'
+            for scope, prefix in prefixes.items()
+        }
+        found: dict[str, CacheEntry] = cache.get_many(list(keys.values()))
+        entries = {scope: found[key] for scope, key in keys.items() if key in found}
+
+        if missing := [scope for scope in scopes if scope not in entries]:
+            built = {scope: cls._encode(data) for scope, data in build_many(missing).items()}
+            cache.set_many({keys[scope]: entry for scope, entry in built.items()}, ns.ttl)
+            entries |= built
+
+        members = [entries[scope] for scope in scopes if scope in entries]
+        etag = f'W/"{blake2b("".join(e for e, _ in members).encode(), digest_size=16).hexdigest()}"'
+        structlog.contextvars.bind_contextvars(cache_ns=ns.name, cache_hit=not missing)
+
+        # ConditionalGetMiddleware only answers GET and HEAD
+        if etag in parse_etags(request.headers.get('If-None-Match', '')):
+            response = HttpResponse(status=304)
+        else:
+            body = b'[' + b','.join(gzip.decompress(gzipped) for _, gzipped in members) + b']'
+            response = HttpResponse(body, content_type='application/json')  # GZipMiddleware compresses it
+
+        response['ETag'] = etag
+        response['X-Cache-Hit'] = '0' if missing else '1'
+        patch_cache_control(response, public=True, max_age=PUBLIC_MAX_AGE)
+        return response

backend/gamedata/api/serializer.py

@@ -235,6 +235,10 @@ class Meta:
 class PlanetIdsSerializer(serializers.ListSerializer):
     child = serializers.CharField(min_length=7, max_length=7)
 
+    def __init__(self, *args: object, **kwargs: object) -> None:
+        kwargs.setdefault('max_length', 200)
+        super().__init__(*args, **kwargs)
+
 
 class GameExchangeSerializer(serializers.ModelSerializer):
     ticker_id = serializers.CharField(read_only=True)

backend/gamedata/api/viewsets.py

@@ -177,8 +177,16 @@ def multiple(self, request: Request):
         serializer = PlanetIdsSerializer(data=request.data)
         serializer.is_valid(raise_exception=True)
 
-        result = GamePlanetSearchService.search_by_planet_natural_id(serializer.validated_data)
-        return Response(self.get_serializer(result, many=True).data)
+        def build_many(missing: list[str]) -> dict[str, object]:
+            planets = list(self.get_queryset().filter(planet_natural_id__in=missing))
+            return {
+                planet.planet_natural_id: data
+                for planet, data in zip(planets, self.get_serializer(planets, many=True).data, strict=True)
+            }
+
+        # the same entries as retrieve, so a planet is cached once for both endpoints
+        planet_natural_ids = list(dict.fromkeys(serializer.validated_data))
+        return CacheManager.respond_many(request, PLANET, 'retrieve', planet_natural_ids, build_many)
 
     @extend_schema(
         auth=[],

backend/gamedata/services/planet_search.py

@@ -110,13 +110,6 @@ def search(search_request: SearchRequestType) -> list[GamePlanet]:
 
         return data
 
-    @staticmethod
-    def search_by_planet_natural_id(planet_natural_ids: list[str]) -> list[GamePlanet]:
-        queryset = queryset_gameplanet()
-        queryset = queryset.filter(planet_natural_id__in=planet_natural_ids)
-
-        return list(queryset)
-
     @staticmethod
     def search_by_term(search_term: str) -> list[GamePlanet]:
         if not search_term:

backend/tests/gamedata/api/test_gamedata_viewsets.py

@@ -4,9 +4,14 @@
 from datetime import UTC, datetime, timedelta
 from unittest.mock import patch
 
+import orjson
 import pytest
+from core.services.cache_manager import CacheManager
+from django.db import connection
+from django.test.utils import CaptureQueriesContext
 from django.urls import reverse
 from django.utils import timezone
+from gamedata.gamedata_cache_manager import PLANET
 from gamedata.models.game_exchange import GameExchange
 from gamedata.models.game_planet import GamePlanet, GamePlanetCOGCProgramChoices
 from model_bakery import baker
@@ -197,13 +202,6 @@ def _search_payload(**overrides: object) -> dict[str, object]:
 
 @pytest.mark.usefixtures('locmem_cache')
 class TestGamePlanetViewSetMultiple:
-    def test_multiple_accepts_100_ids(self, api_client: APIClient) -> None:
-        ids = [f'AB-{i:03d}c' for i in range(100)]
-
-        response = api_client.post(reverse('data:planet-multiple'), data=ids, format='json')
-
-        assert response.status_code == 200
-
     def test_multiple_and_retrieve_never_share_an_entry(
         self, api_client: APIClient, planet_factory: Callable[..., GamePlanet]
     ) -> None:
@@ -216,9 +214,64 @@ def test_multiple_and_retrieve_never_share_an_entry(
         assert isinstance(api_client.get(detail_url).data, dict)
         assert isinstance(api_client.post(multiple_url, data=['OT-580b'], format='json').data, list)
 
-    def test_multiple_is_not_cached(self, api_client: APIClient) -> None:
-        response = api_client.post(reverse('data:planet-multiple'), data=['AB-001c'], format='json')
-        assert not response.has_header('X-Cache-Hit')
+    def test_multiple_returns_the_detail_bodies_in_request_order(
+        self, api_client: APIClient, planet_factory: Callable[..., GamePlanet]
+    ) -> None:
+        for planet_id in ('OT-580b', 'AB-001c'):
+            planet_factory(planet_natural_id=planet_id)
+
+        response = api_client.post(
+            reverse('data:planet-multiple'), data=['AB-001c', 'XX-000x', 'OT-580b', 'AB-001c'], format='json'
+        )
+
+        details = [
+            orjson.loads(api_client.get(reverse('data:planet-detail', kwargs={'planet_natural_id': p})).content)
+            for p in ('AB-001c', 'OT-580b')
+        ]
+        assert response.status_code == 200
+        assert orjson.loads(response.content) == details
+
+    def test_multiple_repeat_runs_no_queries_and_builds_only_new_planets(
+        self, api_client: APIClient, planet_factory: Callable[..., GamePlanet], django_assert_num_queries
+    ) -> None:
+        for planet_id in ('OT-580b', 'AB-001c'):
+            planet_factory(planet_natural_id=planet_id)
+        url = reverse('data:planet-multiple')
+        assert api_client.post(url, data=['OT-580b'], format='json')['X-Cache-Hit'] == '0'
+
+        with django_assert_num_queries(0):
+            response = api_client.post(url, data=['OT-580b'], format='json')
+        assert response['X-Cache-Hit'] == '1'
+
+        with CaptureQueriesContext(connection) as queries:
+            response = api_client.post(url, data=['OT-580b', 'AB-001c'], format='json')
+        assert [p['planet_natural_id'] for p in orjson.loads(response.content)] == ['OT-580b', 'AB-001c']
+        assert 'AB-001c' in queries[0]['sql']
+        assert not any('OT-580b' in q['sql'] for q in queries)
+
+    def test_multiple_serves_new_data_after_invalidation_and_answers_304(
+        self, api_client: APIClient, planet_factory: Callable[..., GamePlanet]
+    ) -> None:
+        planet = planet_factory(planet_natural_id='OT-580b', planet_name='Old')
+        url = reverse('data:planet-multiple')
+        old = api_client.post(url, data=['OT-580b'], format='json')
+        assert old['Cache-Control'] == 'public, max-age=60'
+
+        GamePlanet.objects.filter(pk=planet.pk).update(planet_name='New')
+        CacheManager.invalidate(PLANET, 'OT-580b')
+        new = api_client.post(url, data=['OT-580b'], format='json', HTTP_IF_NONE_MATCH=old['ETag'])
+
+        assert new.status_code == 200
+        assert orjson.loads(new.content)[0]['planet_name'] == 'New'
+        assert new['ETag'] != old['ETag']
+        unchanged = api_client.post(url, data=['OT-580b'], format='json', HTTP_IF_NONE_MATCH=new['ETag'])
+        assert unchanged.status_code == 304
+        assert unchanged.content == b''
+
+    @pytest.mark.parametrize(('count', 'status'), [(200, 200), (201, 400)])
+    def test_multiple_accepts_at_most_200_ids(self, api_client: APIClient, count: int, status: int) -> None:
+        ids = [f'AB-{i:03d}c' for i in range(count)]
+        assert api_client.post(reverse('data:planet-multiple'), data=ids, format='json').status_code == status
 
 
 @pytest.mark.usefixtures('locmem_cache')

backend/tests/gamedata/services/test_planet_search.py

@@ -9,7 +9,6 @@
 class TestGamePlanetSearchService:
     def test_search_by_term_and_id(self):
         baker.make('gamedata.GamePlanet', planet_natural_id='MORIA', planet_name='Deep')
-        assert len(GamePlanetSearchService.search_by_planet_natural_id(['MORIA'])) == 1
         assert len(GamePlanetSearchService.search_by_term('Deep')) == 1
         assert len(GamePlanetSearchService.search_by_term('')) == 0