diff --git a/src/documents/conditionals.py b/src/documents/conditionals.py index bb937cc93..ffb183e4a 100644 --- a/src/documents/conditionals.py +++ b/src/documents/conditionals.py @@ -1,9 +1,9 @@ from datetime import UTC from datetime import datetime -from typing import Any from django.conf import settings from django.core.cache import cache +from rest_framework.request import Request from documents.caching import CACHE_5_MINUTES from documents.caching import CACHE_50_MINUTES @@ -117,7 +117,7 @@ def preview_last_modified(request, pk: int) -> datetime | None: return doc.modified -def thumbnail_etag(request: Any, pk: int) -> str | None: +def thumbnail_etag(request: Request, pk: int) -> str | None: """ Thumbnails are version-dependent, so use the effective document checksum as the ETag to invalidate cache when the latest version changes. @@ -128,7 +128,7 @@ def thumbnail_etag(request: Any, pk: int) -> str | None: return doc.checksum -def thumbnail_last_modified(request: Any, pk: int) -> datetime | None: +def thumbnail_last_modified(request: Request, pk: int) -> datetime | None: """ Returns the filesystem last modified either from cache or from filesystem. Cache should be (slightly?) faster than filesystem diff --git a/src/documents/tests/test_version_conditionals.py b/src/documents/tests/test_version_conditionals.py index d26d7b03d..a2531c7e6 100644 --- a/src/documents/tests/test_version_conditionals.py +++ b/src/documents/tests/test_version_conditionals.py @@ -1,8 +1,12 @@ from datetime import timedelta from types import SimpleNamespace +from typing import TYPE_CHECKING +from typing import cast from unittest import mock +from django.db import connection from django.test import TestCase +from django.test.utils import CaptureQueriesContext from django.utils import timezone from documents.conditionals import metadata_etag @@ -13,6 +17,9 @@ from documents.models import Document from documents.tests.utils import DirectoriesMixin from documents.versioning import resolve_effective_document_by_pk +if TYPE_CHECKING: + from rest_framework.request import Request + class TestConditionals(DirectoriesMixin, TestCase): def test_metadata_etag_uses_latest_version_for_root_request(self) -> None: @@ -29,14 +36,18 @@ class TestConditionals(DirectoriesMixin, TestCase): mime_type="application/pdf", root_document=root, ) - request = SimpleNamespace(query_params={}) + request = cast("Request", SimpleNamespace(query_params={})) self.assertEqual( metadata_etag(request, root.id), f"{latest.checksum}:{latest.modified.isoformat()}", ) - self.assertEqual(preview_etag(request, root.id), latest.archive_checksum) - self.assertEqual(thumbnail_etag(request, root.id), latest.checksum) + # preview_etag/thumbnail_etag resolve the same (pk, request) and should + # reuse metadata_etag's cached resolution instead of re-querying. + with CaptureQueriesContext(connection) as ctx: + self.assertEqual(preview_etag(request, root.id), latest.archive_checksum) + self.assertEqual(thumbnail_etag(request, root.id), latest.checksum) + self.assertEqual(len(ctx.captured_queries), 0) def test_metadata_etag_changes_when_document_modified_changes(self) -> None: doc = Document.objects.create( @@ -44,15 +55,20 @@ class TestConditionals(DirectoriesMixin, TestCase): checksum="same-checksum", mime_type="application/pdf", ) - request = SimpleNamespace(query_params={}) - - original_etag = metadata_etag(request, doc.id) + # Each call simulates a separate incoming HTTP request, so it gets its + # own request object -- the per-request resolution cache must not leak + # a stale result across genuinely different requests. + original_etag = metadata_etag( + cast("Request", SimpleNamespace(query_params={})), + doc.id, + ) new_modified = timezone.now() + timedelta(seconds=5) Document.objects.filter(id=doc.id).update(modified=new_modified) - self.assertNotEqual(metadata_etag(request, doc.id), original_etag) + second_request = cast("Request", SimpleNamespace(query_params={})) + self.assertNotEqual(metadata_etag(second_request, doc.id), original_etag) self.assertEqual( - metadata_etag(request, doc.id), + metadata_etag(second_request, doc.id), f"{doc.checksum}:{new_modified.isoformat()}", ) @@ -76,9 +92,13 @@ class TestConditionals(DirectoriesMixin, TestCase): root_document=other_root, ) - invalid_request = SimpleNamespace(query_params={"version": "not-a-number"}) - unrelated_request = SimpleNamespace( - query_params={"version": str(other_version.id)}, + invalid_request = cast( + "Request", + SimpleNamespace(query_params={"version": "not-a-number"}), + ) + unrelated_request = cast( + "Request", + SimpleNamespace(query_params={"version": str(other_version.id)}), ) self.assertIsNone( @@ -105,7 +125,7 @@ class TestConditionals(DirectoriesMixin, TestCase): latest.thumbnail_path.parent.mkdir(parents=True, exist_ok=True) latest.thumbnail_path.write_bytes(b"thumb") - request = SimpleNamespace(query_params={}) + request = cast("Request", SimpleNamespace(query_params={})) with mock.patch( "documents.conditionals.get_thumbnail_modified_key", return_value="thumb-modified-key", diff --git a/src/documents/versioning.py b/src/documents/versioning.py index 844a5a136..d9e71674a 100644 --- a/src/documents/versioning.py +++ b/src/documents/versioning.py @@ -8,7 +8,7 @@ from typing import Any from documents.models import Document if TYPE_CHECKING: - from django.http import HttpRequest + from rest_framework.request import Request class VersionResolutionError(StrEnum): @@ -26,7 +26,7 @@ def _document_manager(*, include_deleted: bool) -> Any: return Document.global_objects if include_deleted else Document.objects -def get_request_version_param(request: HttpRequest) -> str | None: +def get_request_version_param(request: Request) -> str | None: if hasattr(request, "query_params"): return request.query_params.get("version") return None @@ -57,7 +57,7 @@ def get_latest_version_for_root( def resolve_requested_version_for_root( root_doc: Document, - request: Any, + request: Request, *, include_deleted: bool = False, ) -> VersionResolution: @@ -86,7 +86,7 @@ def resolve_requested_version_for_root( def resolve_effective_document( request_doc: Document, - request: Any, + request: Request, *, include_deleted: bool = False, ) -> VersionResolution: @@ -107,18 +107,41 @@ def resolve_effective_document( return VersionResolution(document=request_doc) +_EFFECTIVE_DOCUMENT_CACHE_ATTR = "_effective_document_resolution_cache" + + def resolve_effective_document_by_pk( pk: int, - request: Any, + request: Request, *, include_deleted: bool = False, ) -> VersionResolution: + # Django's `condition()` decorator (used for ETag/Last-Modified) invokes the + # etag_func and last_modified_func separately, and the view itself may resolve + # again -- all against the same request. Cache per-request so a single thumb/ + # metadata/preview request doesn't redo this resolution multiple times. + cache = getattr(request, _EFFECTIVE_DOCUMENT_CACHE_ATTR, None) + if cache is None: + cache = {} + setattr(request, _EFFECTIVE_DOCUMENT_CACHE_ATTR, cache) + + key = (pk, include_deleted) + if key in cache: + return cache[key] + manager = _document_manager(include_deleted=include_deleted) request_doc = manager.only("id", "root_document_id").filter(pk=pk).first() if request_doc is None: - return VersionResolution(document=None, error=VersionResolutionError.NOT_FOUND) - return resolve_effective_document( - request_doc, - request, - include_deleted=include_deleted, - ) + resolution = VersionResolution( + document=None, + error=VersionResolutionError.NOT_FOUND, + ) + else: + resolution = resolve_effective_document( + request_doc, + request, + include_deleted=include_deleted, + ) + + cache[key] = resolution + return resolution