diff --git a/backend/core/services/cache_manager.py b/backend/core/services/cache_manager.py index ff0152f..b0d5774 100644 --- a/backend/core/services/cache_manager.py +++ b/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 diff --git a/backend/gamedata/api/serializer.py b/backend/gamedata/api/serializer.py index 294af93..58a1a42 100644 --- a/backend/gamedata/api/serializer.py +++ b/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) diff --git a/backend/gamedata/api/viewsets.py b/backend/gamedata/api/viewsets.py index d23d658..734d549 100644 --- a/backend/gamedata/api/viewsets.py +++ b/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=[], diff --git a/backend/gamedata/services/planet_search.py b/backend/gamedata/services/planet_search.py index f9427d0..369ef77 100644 --- a/backend/gamedata/services/planet_search.py +++ b/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: diff --git a/backend/tests/gamedata/api/test_gamedata_viewsets.py b/backend/tests/gamedata/api/test_gamedata_viewsets.py index 79d1bc9..671f35f 100644 --- a/backend/tests/gamedata/api/test_gamedata_viewsets.py +++ b/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') diff --git a/backend/tests/gamedata/services/test_planet_search.py b/backend/tests/gamedata/services/test_planet_search.py index b9114d4..36f9aae 100644 --- a/backend/tests/gamedata/services/test_planet_search.py +++ b/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