rework etag

Author: jobselko

pulp_python/app/cache.py

@@ -1,5 +1,5 @@
 from django.core.exceptions import ObjectDoesNotExist
-from django.http import Http404, HttpResponseNotModified
+from django.http import Http404
 
 from pulpcore.plugin.cache import CacheKeys, SyncContentCache
 from pulpcore.plugin.util import cache_key, get_domain
@@ -14,33 +14,12 @@ class PythonApiCache(SyncContentCache):
     Cache for the Simple API.
 
     Adds Accept header to the cache key so HTML and JSON responses are cached separately.
-    Also handles ETag-based conditional requests (304 Not Modified).
     """
 
     def __init__(self, base_key=None):
         keys = (CacheKeys.path, CacheKeys.method, ACCEPT_HEADER_KEY)
         super().__init__(base_key=base_key, keys=keys)
 
-    def __call__(self, func):
-        original = super().__call__(func)
-
-        def with_conditional_get(*args, **kwargs):
-            response = original(*args, **kwargs)
-            etag = response.get("ETag")
-            if etag:
-                request = self.get_request_from_args(args)
-                if request and request.META.get("HTTP_IF_NONE_MATCH") == etag:
-                    not_modified = HttpResponseNotModified()
-                    not_modified["ETag"] = etag
-                    if cache_control := response.get("Cache-Control"):
-                        not_modified["Cache-Control"] = cache_control
-                    if x_pulp_cache := response.get("X-PULP-CACHE"):
-                        not_modified["X-PULP-CACHE"] = x_pulp_cache
-                    return not_modified
-            return response
-
-        return with_conditional_get
-
     def make_key(self, request):
         all_keys = {
             CacheKeys.path: request.path,

pulp_python/app/pypi/views.py

@@ -18,6 +18,8 @@
     HttpResponseNotFound,
 )
 from django.shortcuts import redirect
+from django.utils.decorators import method_decorator
+from django.views.decorators.http import condition
 from drf_spectacular.utils import extend_schema
 from dynaconf import settings
 from packaging.utils import canonicalize_name
@@ -67,6 +69,29 @@
 CACHE_CONTROL = "max-age=600, public"
 
 
+def _add_cache_control_to_304(func):
+    """Ensure 304 responses include Cache-Control."""
+
+    def wrapper(*args, **kwargs):
+        response = func(*args, **kwargs)
+        if response.status_code == 304:
+            response["Cache-Control"] = CACHE_CONTROL
+        return response
+
+    return wrapper
+
+
+def _etag_func(request, path, **kwargs):
+    """Compute unquoted ETag for the condition decorator. Returns None if no repo."""
+    distro = PyPIMixin.get_distribution(path)
+    try:
+        repo_ver = PyPIMixin.get_repository_version(distro)
+    except Http404:
+        return None
+    raw = f"{repo_ver.number}:{repo_ver.pulp_created.isoformat()}"
+    return hashlib.sha256(raw.encode()).hexdigest()[:16]
+
+
 class PyPISimpleHTMLRenderer(TemplateHTMLRenderer):
     media_type = PYPI_SIMPLE_V1_HTML
 
@@ -299,13 +324,9 @@ def get_provenance_url(self, package, version, filename):
             self.base_api_url, f"{base_path}/integrity/{package}/{version}/{filename}/provenance/"
         )
 
-    @staticmethod
-    def _make_etag(repo_version):
-        """Builds a quoted ETag from the repo version. Changes when content changes."""
-        raw = f"{repo_version.number}:{repo_version.pulp_created.isoformat()}"
-        return f'"{hashlib.sha256(raw.encode()).hexdigest()[:16]}"'
-
     @extend_schema(summary="Get index simple page")
+    @method_decorator(_add_cache_control_to_304)
+    @method_decorator(condition(etag_func=_etag_func))
     @PythonApiCache(base_key=find_base_path_cached)
     def list(self, request, path):
         """Gets the simple api html page for the index."""
@@ -319,11 +340,7 @@ def list(self, request, path):
             .iterator()
         )
         media_type = request.accepted_renderer.media_type
-        headers = {
-            "X-PyPI-Last-Serial": str(PYPI_SERIAL_CONSTANT),
-            "Cache-Control": CACHE_CONTROL,
-            "ETag": self._make_etag(repo_version),
-        }
+        headers = {"X-PyPI-Last-Serial": str(PYPI_SERIAL_CONSTANT), "Cache-Control": CACHE_CONTROL}
 
         if media_type == PYPI_SIMPLE_V1_JSON:
             index_data = write_simple_index_json(names)
@@ -368,17 +385,17 @@ def parse_package(release_package):
         }
 
     @extend_schema(operation_id="pypi_simple_package_read", summary="Get package simple page")
+    @method_decorator(_add_cache_control_to_304)
+    @method_decorator(condition(etag_func=_etag_func))
     @PythonApiCache(base_key=find_base_path_cached)
     def retrieve(self, request, path, package):
         """Retrieves the simple api html/json page for a package."""
         repo_ver, content = self.get_rvc()
         # Should I redirect if the normalized name is different?
         normalized = canonicalize_name(package)
         releases = {}
-        is_pull_through = False
         if self.distribution.remote:
             releases = self.pull_through_package_simple(normalized, path, self.distribution.remote)
-            is_pull_through = True
         elif self.should_redirect(repo_version=repo_ver):
             return redirect(urljoin(self.base_content_url, f"{path}/simple/{normalized}/"))
         if content is not None:
@@ -421,12 +438,7 @@ def retrieve(self, request, path, package):
             return HttpResponseNotFound(f"{normalized} does not exist.")
 
         media_type = request.accepted_renderer.media_type
-        headers = {
-            "X-PyPI-Last-Serial": str(PYPI_SERIAL_CONSTANT),
-            "Cache-Control": CACHE_CONTROL,
-        }
-        if not is_pull_through:
-            headers["ETag"] = self._make_etag(repo_ver)
+        headers = {"X-PyPI-Last-Serial": str(PYPI_SERIAL_CONSTANT), "Cache-Control": CACHE_CONTROL}
 
         if media_type == PYPI_SIMPLE_V1_JSON:
             detail_data = write_simple_detail_json(normalized, releases.values())

pulp_python/tests/functional/api/test_simple_cache.py

@@ -84,13 +84,18 @@ def test_simple_cache_etag_conditional_request(synced_distro):
     r1 = requests.get(url)
     assert r1.status_code == 200
     etag = r1.headers["ETag"]
+    cache_control = r1.headers["Cache-Control"]
 
     r2 = requests.get(url, headers={"If-None-Match": etag})
     assert r2.status_code == 304
     assert r2.headers["ETag"] == etag
+    assert r2.headers["Cache-Control"] == cache_control
+    assert "X-PULP-CACHE" not in r2.headers
     assert len(r2.content) == 0
 
     r3 = requests.get(url, headers={"If-None-Match": '"old"'})
     assert r3.status_code == 200
     assert r3.headers["ETag"] == etag
+    assert r3.headers["Cache-Control"] == cache_control
+    assert r3.headers["X-PULP-CACHE"] == "HIT"
     assert len(r3.content) > 0