From c1a424c5421aac8de9686d14b7cb0e87305f5584 Mon Sep 17 00:00:00 2001
From: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
Date: Tue, 25 Aug 2026 13:02:40 +0200
Subject: [PATCH 1/2] test: add functional tests for conditional cache
Signed-off-by: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
---
CHANGES/7929.feature | 1 +
pulpcore/app/models/publication.py | 22 +-
pulpcore/cache/cache.py | 113 ++++-
pulpcore/content/handler.py | 213 ++++++---
pulpcore/responses.py | 53 ++-
.../test_content_if_modified_since.py | 433 ++++++++++++++++++
pulpcore/tests/unit/content/test_handler.py | 236 +++++++++-
.../unit/models/test_publication_retention.py | 6 +
pulpcore/tests/unit/test_cache.py | 192 ++++++++
pulpcore/tests/unit/test_responses.py | 113 +++++
10 files changed, 1288 insertions(+), 94 deletions(-)
create mode 100644 CHANGES/7929.feature
create mode 100644 pulpcore/tests/functional/api/using_plugin/test_content_if_modified_since.py
create mode 100644 pulpcore/tests/unit/test_responses.py
diff --git a/CHANGES/7929.feature b/CHANGES/7929.feature
new file mode 100644
index 00000000000..b2098c9a935
--- /dev/null
+++ b/CHANGES/7929.feature
@@ -0,0 +1 @@
+Added `Last-Modified` / `If-Modified-Since` (`304 Not Modified`) and `Cache-Control: public, max-age=0, must-revalidate` on content-app artifact responses (filesystem and `ArtifactResponse`; not object-storage 302s) so edge caches can revalidate after ContentGuard without re-fetching the body.
diff --git a/pulpcore/app/models/publication.py b/pulpcore/app/models/publication.py
index 734fbb4c11c..fe5474eddb6 100644
--- a/pulpcore/app/models/publication.py
+++ b/pulpcore/app/models/publication.py
@@ -794,14 +794,26 @@ def get_fallback_ca(self, path):
"""
Return a ContentArtifact for path from the grace-period publication history, or None.
+ See :meth:`get_fallback` for the publication that contained the unit.
+ """
+ ca, _publication = self.get_fallback(path)
+ return ca
+
+ def get_fallback(self, path):
+ """
+ Return ``(ContentArtifact, Publication)`` from grace-period history, or ``(None, None)``.
+
Iterates DistributedPublication records for this distribution from newest to oldest,
trying each publication until the path is found. Handles both pass-through and
non-pass-through (PublishedArtifact) publications.
- Returns None immediately when DISTRIBUTED_PUBLICATION_RETENTION_PERIOD is 0.
+ Returns ``(None, None)`` immediately when DISTRIBUTED_PUBLICATION_RETENTION_PERIOD is 0.
+ The publication is the one that still contains the unit, which may be a superseded
+ version — callers that need ``RepositoryContent.pulp_created`` must use that publication's
+ repository version, not the distribution's current one.
"""
if not retain_distributed_pub_enabled():
- return None
+ return None, None
recent_dp = (
DistributedPublication.get_non_expired()
.filter(distribution=self)
@@ -817,7 +829,7 @@ def get_fallback_ca(self, path):
.first()
)
if ca is not None:
- return ca
+ return ca, pub
else:
pa = (
pub.published_artifact.select_related(
@@ -828,8 +840,8 @@ def get_fallback_ca(self, path):
.first()
)
if pa is not None:
- return pa.content_artifact
- return None
+ return pa.content_artifact, pub
+ return None, None
@hook(BEFORE_CREATE)
def _set_default_content_guard(self):
diff --git a/pulpcore/cache/cache.py b/pulpcore/cache/cache.py
index 6fcf470e14a..49f42fdf20d 100644
--- a/pulpcore/cache/cache.py
+++ b/pulpcore/cache/cache.py
@@ -4,10 +4,11 @@
from functools import wraps
from aiohttp.web import FileResponse, HTTPSuccessful, Request, Response, StreamResponse
-from aiohttp.web_exceptions import HTTPFound
+from aiohttp.web_exceptions import HTTPFound, HTTPNotModified
from django.conf import settings
from django.http import FileResponse as ApiFileResponse
from django.http import HttpResponse, HttpResponseRedirect
+from django.utils.http import parse_http_date_safe
from redis import ConnectionError
from redis.asyncio import ConnectionError as AConnectionError
from rest_framework.request import Request as ApiRequest
@@ -18,7 +19,7 @@
get_redis_connection,
)
from pulpcore.metrics import artifacts_size_counter
-from pulpcore.responses import ArtifactResponse
+from pulpcore.responses import ArtifactResponse, PulpFileResponse
DEFAULT_EXPIRES_TTL = settings.CACHE_SETTINGS["EXPIRES_TTL"]
@@ -306,7 +307,7 @@ class AsyncContentCache(AsyncCache):
"""Cache object meant to be used for the content app"""
RESPONSE_TYPES = {
- "FileResponse": FileResponse,
+ "FileResponse": PulpFileResponse,
"ArtifactResponse": ArtifactResponse,
"Response": Response,
"Redirect": HTTPFound,
@@ -349,32 +350,93 @@ async def cached_function(*args, **kwargs):
if self.auth:
await self.auth(request, self, bk)
key = self.make_key(request)
+
# Check cache
- response = await self.make_response(key, bk)
- if response is None:
- # Cache miss, create new entry
- response = await self.make_entry(
- key, bk, func, args, kwargs, self.default_expires_ttl
+ entry = await self.get_entry(key, bk)
+ if entry is not None:
+ # Cache hit. Authorization has already run. If the client's If-Modified-Since
+ # covers the stored last_modified, answer a bodyless 304 without reconstructing
+ # the full response. Fall back to the header for entries cached before this field.
+ last_modified = entry.get("last_modified") or entry.get("headers", {}).get(
+ "Last-Modified"
)
- elif size := response.headers.get("X-PULP-ARTIFACT-SIZE"):
- artifacts_size_counter.add(size)
-
+ if self._not_modified(request, last_modified):
+ headers = dict(entry.get("headers") or {})
+ headers["X-PULP-CACHE"] = "HIT"
+ raise self._make_not_modified(headers, last_modified)
+ response = self.build_response(entry)
+ if size := response.headers.get("X-PULP-ARTIFACT-SIZE"):
+ artifacts_size_counter.add(size)
+ return response
+
+ # Cache miss: build and cache the full response (a 304 is never stored). Still answer
+ # a matching conditional request with a 304 from the fresh response's Last-Modified,
+ # but never after a stream has already started writing.
+ response = await self.make_entry(key, bk, func, args, kwargs, self.default_expires_ttl)
+ if getattr(response, "prepared", False):
+ return response
+ last_modified = response.headers.get("Last-Modified")
+ if self._not_modified(request, last_modified):
+ raise self._make_not_modified(response.headers, last_modified)
return response
return cached_function
+ @staticmethod
+ def _not_modified(request, last_modified):
+ """True when the request's If-Modified-Since covers the given Last-Modified value.
+
+ Ignore If-Modified-Since when If-None-Match is present, or when it is later than
+ the server clock.
+ """
+ if not last_modified:
+ return False
+ if request.headers.get("If-None-Match"):
+ return False
+ if_modified_since = parse_http_date_safe(request.headers.get("If-Modified-Since", ""))
+ if if_modified_since is None or if_modified_since > time.time():
+ return False
+ lm_epoch = parse_http_date_safe(last_modified)
+ return lm_epoch is not None and lm_epoch <= if_modified_since
+
+ @staticmethod
+ def _make_not_modified(source_headers, last_modified):
+ """Build a bodyless 304 echoing Last-Modified and any caching metadata already present."""
+ headers = {"Last-Modified": last_modified}
+ for name in ("Cache-Control", "X-PULP-CACHE"):
+ if value := source_headers.get(name):
+ headers[name] = value
+ return HTTPNotModified(headers=headers)
+
def get_request_from_args(self, args):
"""Finds the request object from list of args"""
for arg in args:
if isinstance(arg, Request):
return arg
- async def make_response(self, key, base_key):
- """Tries to find the cached entry and turn it into a proper response"""
+ async def get_entry(self, key, base_key):
+ """Return the cached entry dict for ``key`` (deleting stale/invalid rows), or None."""
entry = await self.get(key, base_key)
if not entry:
return None
entry = json.loads(entry)
+ response_type = entry.get("type")
+ # None means "doesn't expire", unset/absent means "already expired".
+ expires = entry.get("expires", -1)
+ if (not response_type or response_type not in self.RESPONSE_TYPES) or (
+ expires and expires < time.time()
+ ):
+ # Bad entry, delete from cache
+ await self.delete(key, base_key)
+ return None
+ return entry
+
+ def build_response(self, entry):
+ """Turn a cached entry dict into a proper response object (marked as a cache HIT)."""
+ entry = dict(entry) # do not mutate the caller's dict
+ entry.pop("expires", None)
+ entry.pop("last_modified", None)
+ response_type = entry.pop("type")
if binary := entry.pop("body", None):
# raw binary data were translated to their hexadecimal representation and saved in
@@ -383,23 +445,24 @@ async def make_response(self, key, base_key):
# https://docs.aiohttp.org/en/stable/web_reference.html#response
entry["body"] = bytes.fromhex(binary)
- response_type = entry.pop("type", None)
- # None means "doesn't expire", unset means "already expired".
- expires = entry.pop("expires", -1)
- if (not response_type or response_type not in self.RESPONSE_TYPES) or (
- expires and expires < time.time()
- ):
- # Bad entry, delete from cache
- await self.delete(key, base_key)
- return None
response = self.RESPONSE_TYPES[response_type](**entry)
response.headers.update({"X-PULP-CACHE": "HIT"})
return response
+ async def make_response(self, key, base_key):
+ """Tries to find the cached entry and turn it into a proper response"""
+ entry = await self.get_entry(key, base_key)
+ if entry is None:
+ return None
+ return self.build_response(entry)
+
async def make_entry(self, key, base_key, handler, args, kwargs, expires=DEFAULT_EXPIRES_TTL):
"""Gets the response for the request and try to turn it into a cacheable entry"""
try:
response = await handler(*args, **kwargs)
+ except HTTPNotModified:
+ # HTTPNotModified is HTTPSuccessful; do not swallow it into a cached entry.
+ raise
except (HTTPSuccessful, HTTPFound) as e:
response = e
@@ -408,7 +471,13 @@ async def make_entry(self, key, base_key, handler, args, kwargs, expires=DEFAULT
if hasattr(response, "future_response"):
response = response.future_response
+ if getattr(response, "status", None) == 304:
+ return original_response
+
entry = {"headers": dict(response.headers), "status": response.status}
+ if last_modified := response.headers.get("Last-Modified"):
+ # Stored alongside headers so a cache hit can 304 without reconstructing the response.
+ entry["last_modified"] = last_modified
if expires is not None:
# Redis TTL is not sufficient: https://github.com/pulp/pulpcore/issues/4845
entry["expires"] = expires + time.time()
diff --git a/pulpcore/content/handler.py b/pulpcore/content/handler.py
index 77c5cfea43e..4b12af1ccc4 100644
--- a/pulpcore/content/handler.py
+++ b/pulpcore/content/handler.py
@@ -10,7 +10,7 @@
import django
from aiohttp.client_exceptions import ClientConnectionError, ClientResponseError
-from aiohttp.web import FileResponse, HTTPOk, StreamResponse
+from aiohttp.web import HTTPOk, StreamResponse
from aiohttp.web_exceptions import (
HTTPError,
HTTPForbidden,
@@ -21,11 +21,12 @@
)
from asgiref.sync import sync_to_async
from django.utils import timezone
+from django.utils.http import http_date
from multidict import CIMultiDict
from yarl import URL
from pulpcore.constants import CHECKPOINT_TS_FORMAT, STORAGE_RESPONSE_MAP
-from pulpcore.responses import ArtifactResponse
+from pulpcore.responses import ArtifactResponse, PulpFileResponse
os.environ.setdefault("DJANGO_SETTINGS_MODULE", "pulpcore.app.settings")
django.setup()
@@ -524,6 +525,10 @@ def response_headers(path, distribution=None):
if content_type:
headers["Content-Type"] = content_type
+ # Tell edge caches to revalidate on every use. Combined with Last-Modified below this
+ # lets them confirm freshness with a lightweight If-Modified-Since instead of re-fetching.
+ headers["Cache-Control"] = "public, max-age=0, must-revalidate"
+
# Let plugin-Distribution set headers for this path if it wants.
if distribution:
headers.update(distribution.content_headers_for(path))
@@ -720,14 +725,16 @@ async def _match_and_stream(self, path, request):
content_handler_result = await sync_to_async(distro.content_handler)(original_rel_path)
if content_handler_result is not None:
if isinstance(content_handler_result, ContentArtifact):
- if content_handler_result.artifact:
- return await self._serve_content_artifact(
- content_handler_result, headers, request
- )
- else:
- return await self._stream_content_artifact(
- request, StreamResponse(headers=headers), content_handler_result
- )
+ ch_repository, ch_repo_version, ch_publication = await sync_to_async(
+ distro.get_repository_publication_and_version
+ )()
+ return await self._serve_ca(
+ content_handler_result,
+ headers,
+ request,
+ publication=ch_publication,
+ repository_version=ch_repo_version,
+ )
else:
# the result is a response so just return it
return content_handler_result
@@ -784,12 +791,7 @@ async def _match_and_stream(self, path, request):
except ObjectDoesNotExist:
pass
else:
- if ca.artifact:
- return await self._serve_content_artifact(ca, headers, request)
- else:
- return await self._stream_content_artifact(
- request, StreamResponse(headers=headers), ca
- )
+ return await self._serve_ca(ca, headers, request, publication=publication)
# pass-through
if publication.pass_through:
@@ -813,23 +815,13 @@ async def _match_and_stream(self, path, request):
except ObjectDoesNotExist:
pass
else:
- if ca.artifact:
- return await self._serve_content_artifact(ca, headers, request)
- else:
- return await self._stream_content_artifact(
- request, StreamResponse(headers=headers), ca
- )
+ return await self._serve_ca(ca, headers, request, publication=publication)
# Grace-period fallback: serve from a recently-superseded publication
if distro.SERVE_FROM_PUBLICATION:
- ca = await sync_to_async(distro.get_fallback_ca)(original_rel_path)
+ ca, fallback_publication = await sync_to_async(distro.get_fallback)(original_rel_path)
if ca is not None:
- if ca.artifact:
- return await self._serve_content_artifact(ca, headers, request)
- else:
- return await self._stream_content_artifact(
- request, StreamResponse(headers=headers), ca
- )
+ return await self._serve_ca(ca, headers, request, publication=fallback_publication)
if repo_version and not publication and not distro.SERVE_FROM_PUBLICATION:
# Look for index.html or list the directory
@@ -871,12 +863,7 @@ async def _match_and_stream(self, path, request):
except ObjectDoesNotExist:
pass
else:
- if ca.artifact:
- return await self._serve_content_artifact(ca, headers, request)
- else:
- return await self._stream_content_artifact(
- request, StreamResponse(headers=headers), ca
- )
+ return await self._serve_ca(ca, headers, request, repository_version=repo_version)
# If we haven't found a match yet, try to use pull-through caching with remote
if distro.remote:
@@ -893,13 +880,10 @@ async def _match_and_stream(self, path, request):
# Try to add content to repository if present & supported
if repository and repository.PULL_THROUGH_SUPPORTED:
await repository.async_pull_through_add_content(ca)
- # Try to stream the ContentArtifact if already created
- if ca.artifact:
- return await self._serve_content_artifact(ca, headers, request)
- else:
- return await self._stream_content_artifact(
- request, StreamResponse(headers=headers), ca
- )
+ # Serve the ContentArtifact if already created (streams if not yet saved)
+ return await self._serve_ca(
+ ca, headers, request, repository_version=repo_version
+ )
else:
# Try to stream the RemoteArtifact and potentially save it as a new Content unit
save_artifact = (
@@ -1090,6 +1074,81 @@ def _save_artifact(self, download_result, remote_artifact, request=None):
ret.update({ca.relative_path: ca for ca in cas})
return ret
+ async def _content_last_modified(
+ self, content_artifact, *, repository_version=None, publication=None
+ ):
+ """
+ Return when the content unit was added to the repository being served, or None.
+
+ Uses ``RepositoryContent.pulp_created`` (the time the unit joined the served repository
+ version), which is the value the content app exposes as ``Last-Modified``. Returns None
+ when no repository version is available or the unit has no membership row (e.g. publish-
+ generated metadata), in which case no ``Last-Modified`` header is set.
+ """
+
+ def _get():
+ repo_version = repository_version
+ if repo_version is None and publication is not None:
+ repo_version = publication.repository_version
+ if repo_version is None:
+ return None
+ return (
+ repo_version._content_relationships()
+ .filter(content_id=content_artifact.content_id)
+ .order_by("-pulp_created")
+ .values_list("pulp_created", flat=True)
+ .first()
+ )
+
+ return await sync_to_async(_get)()
+
+ @staticmethod
+ def _last_modified_http_date(last_modified):
+ """Format a datetime as an HTTP ``Last-Modified`` value, or None."""
+ if last_modified is None:
+ return None
+ return http_date(last_modified.timestamp())
+
+ @staticmethod
+ def _strip_cache_control(headers):
+ """Drop Cache-Control so a response cannot be stored as a shared public copy."""
+ headers.pop("Cache-Control", None)
+ return headers
+
+ @staticmethod
+ def _maybe_not_modified(request, headers, last_modified_header, *, raise_304=True):
+ """Return True when If-Modified-Since covers Last-Modified; optionally raise 304."""
+ if not AsyncContentCache._not_modified(request, last_modified_header):
+ return False
+ if raise_304:
+ raise AsyncContentCache._make_not_modified(headers, last_modified_header)
+ return True
+
+ async def _serve_ca(self, ca, headers, request, *, publication=None, repository_version=None):
+ """Serve a ContentArtifact, attaching ``Last-Modified`` from pulp_created.
+
+ Looks up when the unit joined the served repository version. Saved artifacts get that
+ timestamp in ``_serve_content_artifact`` (after the redirect check, so object-storage
+ 302s stay unmodified). On-demand units without a local artifact 304 before the remote
+ fetch when If-Modified-Since covers that timestamp.
+ """
+ last_modified = await self._content_last_modified(
+ ca, publication=publication, repository_version=repository_version
+ )
+ if ca.artifact:
+ # Last-Modified is applied in `_serve_content_artifact` after the redirect check so
+ # object-storage 302s do not advertise a Pulp validator they cannot honor.
+ return await self._serve_content_artifact(
+ ca, headers, request, last_modified=last_modified
+ )
+ last_modified_header = self._last_modified_http_date(last_modified)
+ if last_modified_header:
+ headers["Last-Modified"] = last_modified_header
+ # 304 before opening the remote, including when the cache is on: streams are not
+ # stored as cacheable file responses, and a started StreamResponse cannot become 304.
+ self._maybe_not_modified(request, headers, last_modified_header)
+ return await self._stream_content_artifact(request, StreamResponse(headers=headers), ca)
+
def _build_response_from_content_artifact(self, content_artifact, headers, request):
"""Helper method to build the correct response to serve a ContentArtifact."""
@@ -1118,27 +1177,33 @@ def _build_url(**kwargs):
storage = domain.get_storage()
headers["X-PULP-ARTIFACT-SIZE"] = str(artifact_file.size)
+ def _object_storage_redirect(url):
+ # Presigned Locations must not be stored by shared caches.
+ return HTTPFound(url, headers=self._strip_cache_control(CIMultiDict(headers)))
+
if domain.storage_class == "pulpcore.app.models.storage.FileSystem":
path = storage.path(artifact_name)
if not os.path.exists(path):
raise Exception(_("Expected path '{}' is not found").format(path))
- return FileResponse(path, headers=headers)
+ return PulpFileResponse(path, headers=headers)
elif not domain.redirect_to_object_storage:
return ArtifactResponse(content_artifact.artifact, headers=headers)
elif domain.storage_class in (
"storages.backends.s3boto3.S3Boto3Storage",
"storages.backends.s3.S3Storage",
):
- return HTTPFound(_build_url(http_method=request.method), headers=headers)
+ return _object_storage_redirect(_build_url(http_method=request.method))
elif domain.storage_class in (
"storages.backends.azure_storage.AzureStorage",
"storages.backends.gcloud.GoogleCloudStorage",
):
- return HTTPFound(_build_url(), headers=headers)
+ return _object_storage_redirect(_build_url())
else:
raise NotImplementedError()
- async def _serve_content_artifact(self, content_artifact, headers, request):
+ async def _serve_content_artifact(
+ self, content_artifact, headers, request, *, last_modified=None
+ ):
"""
Handle response for a Content Artifact with the file present.
@@ -1150,6 +1215,8 @@ async def _serve_content_artifact(self, content_artifact, headers, request):
respond with.
headers (dict): A dictionary of response headers.
request(aiohttp.web.Request) The request to prepare a response for.
+ last_modified (datetime): When the content was added to the served repository, used
+ for the ``Last-Modified`` header and ``If-Modified-Since`` handling. May be None.
Raises:
[aiohttp.web_exceptions.HTTPFound][]: When we need to redirect to the file
@@ -1161,26 +1228,44 @@ async def _serve_content_artifact(self, content_artifact, headers, request):
"""
artifact_file = content_artifact.artifact.file
content_length = artifact_file.size
-
- try:
- range_start, range_stop = request.http_range.start, request.http_range.stop
- if range_start or range_stop:
- if range_stop and artifact_file.size and range_stop > artifact_file.size:
- start = 0 if range_start is None else range_start
- content_length = artifact_file.size - start
- elif range_stop:
- content_length = range_stop - range_start
- except ValueError:
- size = artifact_file.size or "*"
- raise HTTPRequestRangeNotSatisfiable(headers={"Content-Range": f"bytes */{size}"})
-
- artifacts_size_counter.add(content_length)
+ last_modified_header = self._last_modified_http_date(last_modified)
response = self._build_response_from_content_artifact(content_artifact, headers, request)
if isinstance(response, HTTPFound):
+ # Redirect (object-storage) responses are left without a Pulp validator. Presigned
+ # Locations must not be stored by shared caches.
+ self._strip_cache_control(response.headers)
+ artifacts_size_counter.add(content_length)
raise response
- else:
- return response
+
+ if last_modified_header is not None:
+ response.headers["Last-Modified"] = last_modified_header
+
+ # If-Modified-Since is checked as if Range were not present. A matching
+ # If-Modified-Since must 304, not 416. When the cache is on, skip the handler 304 so
+ # Redis can store a 200.
+ would_304 = self._maybe_not_modified(
+ request,
+ response.headers,
+ response.headers.get("Last-Modified"),
+ raise_304=not settings.CACHE_ENABLED,
+ )
+
+ if not would_304:
+ try:
+ range_start, range_stop = request.http_range.start, request.http_range.stop
+ if range_start or range_stop:
+ if range_stop and artifact_file.size and range_stop > artifact_file.size:
+ start = 0 if range_start is None else range_start
+ content_length = artifact_file.size - start
+ elif range_stop:
+ content_length = range_stop - range_start
+ except ValueError:
+ size = artifact_file.size or "*"
+ raise HTTPRequestRangeNotSatisfiable(headers={"Content-Range": f"bytes */{size}"})
+
+ artifacts_size_counter.add(content_length)
+ return response
async def _stream_remote_artifact(
self, request, response, remote_artifact, save_artifact, repository=None
@@ -1212,8 +1297,8 @@ async def _stream_remote_artifact(
)
)
- # According to RFC7233 if a server cannot satisfy a Range request, the response needs to
- # contain a Content-Range header with an unsatisfied-range value.
+ # If a Range cannot be satisfied, the response needs a Content-Range header with an
+ # unsatisfied-range value.
try:
range_start, range_stop = request.http_range.start, request.http_range.stop
size = remote_artifact.size
diff --git a/pulpcore/responses.py b/pulpcore/responses.py
index 1b1fac62a0d..25086f2e872 100644
--- a/pulpcore/responses.py
+++ b/pulpcore/responses.py
@@ -1,7 +1,7 @@
import asyncio
from aiohttp import hdrs
-from aiohttp.web import StreamResponse
+from aiohttp.web import FileResponse, StreamResponse
from aiohttp.web_exceptions import (
HTTPPartialContent,
HTTPRequestRangeNotSatisfiable,
@@ -9,6 +9,57 @@
from pulpcore.app.models import Artifact
+# aiohttp reads these as ``@reify`` properties off ``request._cache`` (private, present through
+# aiohttp 3.10–3.14). Seeding them to None suppresses only the headers that would 304 against file
+# mtime; If-Range / If-Match / If-Unmodified-Since stay intact so Range requests cannot return a
+# corrupt 206.
+_MTIME_304_REIFY_KEYS = ("if_modified_since", "if_none_match")
+
+
+def _suppress_mtime_conditionals(request):
+ """Neutralize aiohttp's native mtime-based If-Modified-Since/ETag 304 handling for a request."""
+ for key in _MTIME_304_REIFY_KEYS:
+ request._cache[key] = None
+
+
+class PulpFileResponse(FileResponse):
+ """A FileResponse that lets the content app own the ``Last-Modified`` validator.
+
+ aiohttp's ``FileResponse`` overwrites ``Last-Modified`` with the file's mtime and runs its own
+ ``If-Modified-Since``/``ETag`` handling against that mtime. The content app instead uses
+ ``RepositoryContent.pulp_created`` (or omits the header) and answers conditional requests
+ itself, so this class never advertises filesystem mtime as a validator.
+ """
+
+ async def prepare(self, request):
+ # aiohttp < 3.11 runs the mtime-304 check inline in prepare(); 3.11+ moved it into
+ # _make_response() (which prepare() calls). Seed here so it is neutralized on every
+ # supported aiohttp version, not just those that expose _make_response().
+ _suppress_mtime_conditionals(request)
+ return await super().prepare(request)
+
+ def _make_response(self, request, accept_encoding):
+ _suppress_mtime_conditionals(request)
+ return super()._make_response(request, accept_encoding)
+
+ @property
+ def last_modified(self):
+ return FileResponse.last_modified.fget(self)
+
+ @last_modified.setter
+ def last_modified(self, value):
+ # Never replace a handler Last-Modified, and never advertise the file mtime.
+ return
+
+ @property
+ def etag(self):
+ return FileResponse.etag.fget(self)
+
+ @etag.setter
+ def etag(self, value):
+ # mtime-based ETags would disagree with RepositoryContent.pulp_created as Last-Modified.
+ return
+
class ArtifactResponse(StreamResponse):
"""A response object can be used to send artifacts."""
diff --git a/pulpcore/tests/functional/api/using_plugin/test_content_if_modified_since.py b/pulpcore/tests/functional/api/using_plugin/test_content_if_modified_since.py
new file mode 100644
index 00000000000..9046d41e4ed
--- /dev/null
+++ b/pulpcore/tests/functional/api/using_plugin/test_content_if_modified_since.py
@@ -0,0 +1,433 @@
+"""Tests for If-Modified-Since / 304 Not Modified on the content app."""
+
+from base64 import b64encode
+from time import sleep, time
+from urllib.parse import urljoin
+from uuid import uuid4
+
+import pytest
+import requests
+from django.utils.http import http_date, parse_http_date
+
+from pulpcore.client.pulp_file import FileRepositorySyncURL, PatchedfileFileDistribution
+from pulpcore.content.handler import Handler
+
+CACHE_CONTROL = Handler.response_headers("1.iso")["Cache-Control"]
+ONE_DAY_SECONDS = 86400
+
+
+def _get(url, headers=None):
+ """GET the content-app response without following object-storage redirects."""
+ return requests.get(url, headers=headers, allow_redirects=False)
+
+
+def _assert_artifact_200(response):
+ assert response.status_code == 200
+ assert response.content
+ assert response.headers.get("Cache-Control") == CACHE_CONTROL
+ assert response.headers.get("Last-Modified")
+
+
+def _get_and_assert_last_modified(url, headers=None):
+ """GET the artifact, assert a full 200, and return (response, Last-Modified value)."""
+ response = _get(url, headers=headers)
+ _assert_artifact_200(response)
+ return response, response.headers["Last-Modified"]
+
+
+def _assert_304(response, last_modified):
+ assert response.status_code == 304
+ assert response.content == b""
+ assert response.headers.get("Last-Modified") == last_modified
+ assert response.headers.get("Cache-Control") == CACHE_CONTROL
+
+
+@pytest.fixture
+def redis_required(redis_status):
+ """Skip when the content cache (Redis) is not reachable."""
+ if not redis_status:
+ pytest.skip("Could not connect to the Redis server")
+
+
+@pytest.fixture
+def inline_storage(pulp_settings):
+ """Skip when the instance redirects to object storage instead of serving bytes inline.
+
+ The 304 path applies to filesystem/ArtifactResponse serving; object-storage 302s are not
+ 304'd by design.
+ """
+ backend = pulp_settings.STORAGES["default"]["BACKEND"]
+ redirects = (
+ backend != "pulpcore.app.models.storage.FileSystem"
+ and pulp_settings.REDIRECT_TO_OBJECT_STORAGE
+ )
+ if redirects:
+ pytest.skip("object-storage redirects are not 304'd by design")
+
+
+@pytest.fixture
+def object_storage_redirects(pulp_settings):
+ """Skip unless the instance redirects to object storage (302)."""
+ backend = pulp_settings.STORAGES["default"]["BACKEND"]
+ if (
+ backend == "pulpcore.app.models.storage.FileSystem"
+ or not pulp_settings.REDIRECT_TO_OBJECT_STORAGE
+ ):
+ pytest.skip("not using object-storage redirects")
+
+
+@pytest.fixture
+def published_file_distribution(
+ file_repo_with_auto_publish,
+ file_remote_factory,
+ file_bindings,
+ file_distribution_factory,
+ distribution_base_url,
+ monitor_task,
+ basic_manifest_path,
+):
+ """Immediate-sync a 3-file repo, distribute it, and return (repo, distro, base_url)."""
+ remote = file_remote_factory(manifest_path=basic_manifest_path, policy="immediate")
+ body = FileRepositorySyncURL(remote=remote.pulp_href)
+ monitor_task(
+ file_bindings.RepositoriesFileApi.sync(file_repo_with_auto_publish.pulp_href, body).task
+ )
+ repo = file_bindings.RepositoriesFileApi.read(file_repo_with_auto_publish.pulp_href)
+ distro = file_distribution_factory(repository=repo.pulp_href)
+ return repo, distro, distribution_base_url(distro.base_url)
+
+
+@pytest.mark.parallel
+def test_artifact_get_sets_last_modified_and_cache_control(
+ published_file_distribution, inline_storage, redis_status
+):
+ """A plain 200 carries the Last-Modified validator and revalidate Cache-Control."""
+ _repo, _distro, base_url = published_file_distribution
+ url = urljoin(base_url, "1.iso")
+
+ response = _get(url)
+ _assert_artifact_200(response)
+ last_modified = response.headers["Last-Modified"]
+
+ if redis_status:
+ assert response.headers.get("X-PULP-CACHE") == "MISS"
+ cached = _get(url)
+ _assert_artifact_200(cached)
+ assert cached.headers.get("X-PULP-CACHE") == "HIT"
+ assert cached.headers["Last-Modified"] == last_modified
+
+
+@pytest.mark.parallel
+def test_matching_if_modified_since_returns_304(
+ published_file_distribution, inline_storage, redis_status
+):
+ """An If-Modified-Since at or after the validator gets a bodyless 304."""
+ _repo, _distro, base_url = published_file_distribution
+ url = urljoin(base_url, "1.iso")
+
+ _first, last_modified = _get_and_assert_last_modified(url)
+
+ matched = _get(url, headers={"If-Modified-Since": last_modified})
+ _assert_304(matched, last_modified)
+
+ sleep(2)
+ later_response = _get(url, headers={"If-Modified-Since": http_date()})
+ _assert_304(later_response, last_modified)
+
+ if redis_status:
+ assert matched.headers.get("X-PULP-CACHE") == "HIT"
+ warm = _get(url)
+ _assert_artifact_200(warm)
+ assert warm.headers.get("X-PULP-CACHE") == "HIT"
+ assert warm.content
+
+
+@pytest.mark.parallel
+def test_stale_garbage_and_future_if_modified_since_return_200(
+ published_file_distribution, inline_storage
+):
+ """Older, unparseable, or future If-Modified-Since values fall back to a full 200."""
+ _repo, _distro, base_url = published_file_distribution
+ url = urljoin(base_url, "1.iso")
+
+ _first, last_modified = _get_and_assert_last_modified(url)
+ last_modified_epoch = parse_http_date(last_modified)
+
+ stale = http_date(last_modified_epoch - ONE_DAY_SECONDS)
+ future = http_date(time() + ONE_DAY_SECONDS)
+ for if_modified_since in (stale, "not a date", future):
+ response = _get(url, headers={"If-Modified-Since": if_modified_since})
+ _assert_artifact_200(response)
+ assert response.headers["Last-Modified"] == last_modified
+
+
+@pytest.mark.parallel
+def test_last_modified_is_membership_not_version_time(
+ file_bindings,
+ file_repository_factory,
+ file_content_unit_with_name_factory,
+ file_distribution_factory,
+ distribution_base_url,
+ monitor_task,
+ inline_storage,
+):
+ """Last-Modified is the served unit's membership time, not the repo version's time."""
+ repo = file_repository_factory(autopublish=True)
+ content_a = file_content_unit_with_name_factory(f"{uuid4()}.iso")
+ content_b = file_content_unit_with_name_factory(f"{uuid4()}.iso")
+
+ monitor_task(
+ file_bindings.RepositoriesFileApi.modify(
+ repo.pulp_href, {"add_content_units": [content_a.pulp_href]}
+ ).task
+ )
+ repo = file_bindings.RepositoriesFileApi.read(repo.pulp_href)
+ version_1 = file_bindings.RepositoriesFileVersionsApi.read(repo.latest_version_href)
+
+ sleep(2)
+
+ monitor_task(
+ file_bindings.RepositoriesFileApi.modify(
+ repo.pulp_href, {"add_content_units": [content_b.pulp_href]}
+ ).task
+ )
+ repo = file_bindings.RepositoriesFileApi.read(repo.pulp_href)
+ version_2 = file_bindings.RepositoriesFileVersionsApi.read(repo.latest_version_href)
+
+ distro = file_distribution_factory(repository=repo.pulp_href)
+ url = urljoin(distribution_base_url(distro.base_url), content_a.relative_path)
+ response = _get(url)
+ _assert_artifact_200(response)
+
+ last_modified = parse_http_date(response.headers["Last-Modified"])
+ v1_created = version_1.pulp_created.timestamp()
+ v2_created = version_2.pulp_created.timestamp()
+
+ # content_a joined the repo in version 1, so its validator predates version 2 entirely...
+ assert last_modified < v2_created
+ # ...and sits at version 1's creation (within HTTP-date's 1-second resolution), not later.
+ assert last_modified <= v1_created + 1
+
+
+@pytest.mark.parallel
+def test_last_modified_is_membership_not_content_created(
+ file_bindings,
+ file_repository_factory,
+ file_content_unit_with_name_factory,
+ file_distribution_factory,
+ distribution_base_url,
+ monitor_task,
+ inline_storage,
+):
+ """One unit in two repos yields per-repo validators, not the unit's created time."""
+ content = file_content_unit_with_name_factory(f"{uuid4()}.iso")
+ repo_a = file_repository_factory(autopublish=True)
+ monitor_task(
+ file_bindings.RepositoriesFileApi.modify(
+ repo_a.pulp_href, {"add_content_units": [content.pulp_href]}
+ ).task
+ )
+ distro_a = file_distribution_factory(repository=repo_a.pulp_href)
+ url_a = urljoin(distribution_base_url(distro_a.base_url), content.relative_path)
+ response_a = _get(url_a)
+ _assert_artifact_200(response_a)
+
+ sleep(2)
+
+ repo_b = file_repository_factory(autopublish=True)
+ monitor_task(
+ file_bindings.RepositoriesFileApi.modify(
+ repo_b.pulp_href, {"add_content_units": [content.pulp_href]}
+ ).task
+ )
+ distro_b = file_distribution_factory(repository=repo_b.pulp_href)
+ url_b = urljoin(distribution_base_url(distro_b.base_url), content.relative_path)
+ response_b = _get(url_b)
+ _assert_artifact_200(response_b)
+
+ last_modified_a = parse_http_date(response_a.headers["Last-Modified"])
+ last_modified_b = parse_http_date(response_b.headers["Last-Modified"])
+ content_created = content.pulp_created.timestamp()
+
+ assert last_modified_b > last_modified_a
+ assert last_modified_b > content_created
+
+
+@pytest.mark.parallel
+def test_content_guard_still_runs_before_304(
+ published_file_distribution,
+ inline_storage,
+ pulpcore_bindings,
+ file_bindings,
+ gen_object_with_cleanup,
+ monitor_task,
+):
+ """Authorization runs on every request: a conditional GET is 403'd before any 304."""
+ _repo, distro, base_url = published_file_distribution
+ url = urljoin(base_url, "1.iso")
+ guard = gen_object_with_cleanup(
+ pulpcore_bindings.ContentguardsHeaderApi,
+ {
+ "name": str(uuid4()),
+ "header_name": "x-header",
+ "header_value": "123456",
+ },
+ )
+ body = PatchedfileFileDistribution(content_guard=guard.pulp_href)
+ monitor_task(file_bindings.DistributionsFileApi.partial_update(distro.pulp_href, body).task)
+
+ auth_headers = {"x-header": b64encode(b"123456").decode("ascii")}
+
+ denied = _get(url, headers={"If-Modified-Since": http_date()})
+ assert denied.status_code == 403
+
+ authorized = _get(url, headers=auth_headers)
+ _assert_artifact_200(authorized)
+ last_modified = authorized.headers["Last-Modified"]
+
+ revalidated = _get(url, headers={**auth_headers, "If-Modified-Since": last_modified})
+ _assert_304(revalidated, last_modified)
+
+
+@pytest.mark.parallel
+def test_published_metadata_has_no_last_modified(published_file_distribution, inline_storage):
+ """Publish-generated metadata (PULP_MANIFEST) has no membership row, so no validator."""
+ _repo, _distro, base_url = published_file_distribution
+ url = urljoin(base_url, "PULP_MANIFEST")
+
+ response = _get(url)
+ assert response.status_code == 200
+ assert response.content
+ assert "Last-Modified" not in response.headers
+
+ again = _get(url, headers={"If-Modified-Since": http_date()})
+ assert again.status_code == 200
+ assert again.content
+ assert "Last-Modified" not in again.headers
+
+
+@pytest.mark.parallel
+def test_on_demand_304_does_not_fetch_remote(
+ file_repo_with_auto_publish,
+ generate_server_and_remote,
+ file_bindings,
+ file_distribution_factory,
+ distribution_base_url,
+ monitor_task,
+ basic_manifest_path,
+ inline_storage,
+):
+ """An on-demand 304 is answered from the validator without touching the remote."""
+ server, remote = generate_server_and_remote(
+ manifest_path=basic_manifest_path, policy="on_demand"
+ )
+ body = FileRepositorySyncURL(remote=remote.pulp_href)
+ monitor_task(
+ file_bindings.RepositoriesFileApi.sync(file_repo_with_auto_publish.pulp_href, body).task
+ )
+ repo = file_bindings.RepositoriesFileApi.read(file_repo_with_auto_publish.pulp_href)
+ distro = file_distribution_factory(repository=repo.pulp_href)
+ url = urljoin(distribution_base_url(distro.base_url), "1.iso")
+
+ def iso_fetches():
+ return [r for r in server.requests_record if "1.iso" in r.raw_path]
+
+ assert iso_fetches() == []
+
+ sleep(2)
+ response = _get(url, headers={"If-Modified-Since": http_date()})
+ assert response.status_code == 304
+ assert response.content == b""
+ assert response.headers.get("Last-Modified")
+ assert response.headers.get("Cache-Control") == CACHE_CONTROL
+ assert iso_fetches() == []
+
+
+@pytest.mark.parallel
+def test_matching_if_modified_since_beats_range(published_file_distribution, inline_storage):
+ """A matching If-Modified-Since wins over a Range header: 304, not 206/416."""
+ _repo, _distro, base_url = published_file_distribution
+ url = urljoin(base_url, "1.iso")
+
+ _first, last_modified = _get_and_assert_last_modified(url)
+
+ response = _get(url, headers={"If-Modified-Since": last_modified, "Range": "bytes=0-0"})
+ _assert_304(response, last_modified)
+
+
+@pytest.mark.parallel
+def test_object_storage_redirect_is_not_304(published_file_distribution, object_storage_redirects):
+ """Object-storage 302s carry no validator and never 304, even with If-Modified-Since."""
+ _repo, _distro, base_url = published_file_distribution
+ url = urljoin(base_url, "1.iso")
+
+ response = _get(url)
+ assert response.status_code == 302
+ assert "Last-Modified" not in response.headers
+ assert "Cache-Control" not in response.headers
+
+ again = _get(url, headers={"If-Modified-Since": http_date()})
+ assert again.status_code == 302
+ assert "Last-Modified" not in again.headers
+
+
+@pytest.mark.parallel
+def test_cold_cache_miss_with_matching_ims_304s_but_caches_200(
+ published_file_distribution, inline_storage, redis_required
+):
+ """A conditional first request 304s from the cache miss yet still stores a full 200.
+
+ The handler defers its own 304 to Redis when the cache is on, so make_entry builds and caches
+ the 200 while the miss path answers a bodyless 304. A plain follow-up must then be a full
+ cache HIT, proving the 304 was never stored as an empty entry.
+ """
+ _repo, _distro, base_url = published_file_distribution
+ url = urljoin(base_url, "1.iso")
+
+ # First-ever request for this URL is conditional (cold cache).
+ first = _get(url, headers={"If-Modified-Since": http_date()})
+ assert first.status_code == 304
+ assert first.content == b""
+ last_modified = first.headers.get("Last-Modified")
+ assert last_modified
+ assert first.headers.get("Cache-Control") == CACHE_CONTROL
+
+ # The miss stored a full 200, so a plain GET is a cache HIT with the same validator.
+ warm = _get(url)
+ _assert_artifact_200(warm)
+ assert warm.headers.get("X-PULP-CACHE") == "HIT"
+ assert warm.headers["Last-Modified"] == last_modified
+
+
+@pytest.mark.parallel
+def test_if_none_match_suppresses_304(published_file_distribution, inline_storage):
+ """If-None-Match disables If-Modified-Since handling, so a match still returns a full 200."""
+ _repo, _distro, base_url = published_file_distribution
+ url = urljoin(base_url, "1.iso")
+
+ _first, last_modified = _get_and_assert_last_modified(url)
+
+ response = _get(
+ url, headers={"If-Modified-Since": last_modified, "If-None-Match": '"anything"'}
+ )
+ _assert_artifact_200(response)
+ assert response.headers["Last-Modified"] == last_modified
+
+
+@pytest.mark.parallel
+def test_head_request_revalidates_with_304(published_file_distribution, inline_storage):
+ """A conditional HEAD (as CDNs issue) sets the validator on 200 and 304s on revalidation."""
+ _repo, _distro, base_url = published_file_distribution
+ url = urljoin(base_url, "1.iso")
+
+ initial = requests.head(url, allow_redirects=False)
+ assert initial.status_code == 200
+ last_modified = initial.headers["Last-Modified"]
+ assert initial.headers.get("Cache-Control") == CACHE_CONTROL
+
+ revalidated = requests.head(
+ url, allow_redirects=False, headers={"If-Modified-Since": last_modified}
+ )
+ assert revalidated.status_code == 304
+ assert revalidated.headers.get("Last-Modified") == last_modified
+ assert revalidated.headers.get("Cache-Control") == CACHE_CONTROL
diff --git a/pulpcore/tests/unit/content/test_handler.py b/pulpcore/tests/unit/content/test_handler.py
index 045efdad6cd..bf17b912743 100644
--- a/pulpcore/tests/unit/content/test_handler.py
+++ b/pulpcore/tests/unit/content/test_handler.py
@@ -1,11 +1,19 @@
import uuid
-from datetime import timedelta
+from datetime import datetime, timedelta
+from datetime import timezone as dt_timezone
from unittest.mock import AsyncMock, Mock
import pytest
import pytest_asyncio
-from aiohttp.web_exceptions import HTTPMovedPermanently
+from aiohttp.web_exceptions import (
+ HTTPFound,
+ HTTPMovedPermanently,
+ HTTPNotModified,
+)
+from asgiref.sync import sync_to_async
from django.db import IntegrityError
+from django.test import override_settings
+from django.utils.http import http_date
from django_guid import clear_guid, set_guid
from pulpcore.app.models import AppStatus
@@ -198,6 +206,18 @@ async def create_distribution(remote, repository=None):
)
+async def _add_content_to_new_version(repo, content):
+ """Add ``content`` to a new complete version of ``repo`` and return that version."""
+ repo.CONTENT_TYPES = [Content]
+
+ def _add():
+ with repo.new_version() as version:
+ version.add_content(Content.objects.filter(pk=content.pk))
+ return repo.latest_version()
+
+ return await sync_to_async(_add)()
+
+
@pytest.mark.asyncio
@pytest.mark.django_db
async def test_pull_through_remote_artifact_exists(request123, tmp_path):
@@ -586,6 +606,218 @@ def test_render_html_normal_name():
assert 'simple-dir/' in html
+_LAST_MODIFIED = datetime(2020, 1, 1, tzinfo=dt_timezone.utc)
+_LAST_MODIFIED_HTTP = http_date(_LAST_MODIFIED.timestamp())
+_IF_MODIFIED_SINCE_AFTER = http_date(datetime(2021, 1, 1, tzinfo=dt_timezone.utc).timestamp())
+_IF_MODIFIED_SINCE_BEFORE = http_date(datetime(2019, 1, 1, tzinfo=dt_timezone.utc).timestamp())
+_CACHE_CONTROL = "public, max-age=0, must-revalidate"
+
+
+class _UnsatisfiableRange:
+ @property
+ def start(self):
+ raise ValueError()
+
+ stop = None
+
+
+def _request(*, if_modified_since=None, http_range=None):
+ return Mock(
+ method="GET",
+ http_range=http_range if http_range is not None else Mock(start=None, stop=None),
+ headers={"If-Modified-Since": if_modified_since} if if_modified_since else {},
+ )
+
+
+def _ca(*, artifact=True):
+ ca = Mock()
+ ca.relative_path = "file.iso"
+ if artifact:
+ ca.artifact.file.size = 7
+ ca.artifact.file.name = "artifacts/obj"
+ else:
+ ca.artifact = None
+ return ca
+
+
+def _handler_with_built_response(monkeypatch, built=None):
+ handler = Handler()
+ ca = _ca()
+ if built is None:
+ built = Mock(headers={"Cache-Control": _CACHE_CONTROL}, status=200)
+ monkeypatch.setattr(handler, "_build_response_from_content_artifact", Mock(return_value=built))
+ return handler, ca, built
+
+
+def _membership_pulp_created(version, content):
+ return (
+ version._content_relationships()
+ .filter(content_id=content.pk)
+ .values_list("pulp_created", flat=True)
+ .get()
+ )
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize(
+ "if_modified_since, last_modified, cache_enabled, expect_304",
+ [
+ (None, _LAST_MODIFIED, False, False),
+ (_IF_MODIFIED_SINCE_AFTER, _LAST_MODIFIED, False, True),
+ (_IF_MODIFIED_SINCE_BEFORE, _LAST_MODIFIED, False, False),
+ (_IF_MODIFIED_SINCE_AFTER, None, False, False),
+ (_IF_MODIFIED_SINCE_AFTER, _LAST_MODIFIED, True, False),
+ ],
+ ids=["no-if-modified-since", "fresh", "stale", "no-timestamp", "cache-on"],
+)
+async def test_serve_content_artifact_if_modified_since(
+ monkeypatch, if_modified_since, last_modified, cache_enabled, expect_304
+):
+ """Filesystem responses stamp Last-Modified.
+
+ A matching If-Modified-Since is 304 unless the cache is on.
+ """
+ handler, ca, built = _handler_with_built_response(monkeypatch)
+
+ with override_settings(CACHE_ENABLED=cache_enabled):
+ if expect_304:
+ with pytest.raises(HTTPNotModified) as exc:
+ await handler._serve_content_artifact(
+ ca,
+ {},
+ _request(if_modified_since=if_modified_since),
+ last_modified=last_modified,
+ )
+ assert exc.value.headers["Last-Modified"] == _LAST_MODIFIED_HTTP
+ assert exc.value.headers["Cache-Control"] == _CACHE_CONTROL
+ else:
+ response = await handler._serve_content_artifact(
+ ca,
+ {},
+ _request(if_modified_since=if_modified_since),
+ last_modified=last_modified,
+ )
+ assert response is built
+ if last_modified is None:
+ assert "Last-Modified" not in response.headers
+ else:
+ assert response.headers["Last-Modified"] == _LAST_MODIFIED_HTTP
+
+
+def test_response_headers_sets_cache_control():
+ """All content responses instruct edge caches to revalidate on every use."""
+ headers = Handler.response_headers("path/to/file.iso")
+ assert headers["Cache-Control"] == _CACHE_CONTROL
+
+
+@pytest.mark.asyncio
+async def test_serve_content_artifact_redirect_is_not_304(monkeypatch):
+ """Object-storage 302s never get a Pulp Last-Modified.
+
+ A matching If-Modified-Since must not 304.
+ """
+ redirect = HTTPFound(
+ "http://example.test/redirect",
+ headers={"Cache-Control": _CACHE_CONTROL},
+ )
+ handler, ca, _built = _handler_with_built_response(monkeypatch, built=redirect)
+
+ with override_settings(CACHE_ENABLED=False):
+ with pytest.raises(HTTPFound) as exc:
+ await handler._serve_content_artifact(
+ ca,
+ {},
+ _request(if_modified_since=_IF_MODIFIED_SINCE_AFTER),
+ last_modified=_LAST_MODIFIED,
+ )
+
+ assert "Last-Modified" not in exc.value.headers
+ assert "Cache-Control" not in exc.value.headers
+
+
+@pytest.mark.asyncio
+async def test_serve_content_artifact_304_beats_unsatisfiable_range(monkeypatch):
+ """A matching If-Modified-Since 304s even when Range would otherwise be 416."""
+ handler, ca, _built = _handler_with_built_response(monkeypatch)
+ request = _request(if_modified_since=_IF_MODIFIED_SINCE_AFTER, http_range=_UnsatisfiableRange())
+
+ with override_settings(CACHE_ENABLED=False):
+ with pytest.raises(HTTPNotModified) as exc:
+ await handler._serve_content_artifact(ca, {}, request, last_modified=_LAST_MODIFIED)
+
+ assert exc.value.status == 304
+ assert exc.value.headers["Last-Modified"] == _LAST_MODIFIED_HTTP
+
+
+@pytest.mark.asyncio
+async def test_on_demand_conditional_before_stream(monkeypatch):
+ """On-demand units 304 before the remote fetch; otherwise the stream carries Last-Modified."""
+ handler = Handler()
+ ca = _ca(artifact=False)
+ monkeypatch.setattr(handler, "_content_last_modified", AsyncMock(return_value=_LAST_MODIFIED))
+ handler._stream_content_artifact = AsyncMock(return_value="streamed")
+
+ with pytest.raises(HTTPNotModified) as exc:
+ await handler._serve_ca(
+ ca,
+ {"Cache-Control": _CACHE_CONTROL},
+ Mock(headers={"If-Modified-Since": _LAST_MODIFIED_HTTP}),
+ repository_version="rv",
+ )
+ handler._stream_content_artifact.assert_not_awaited()
+ assert exc.value.headers["Last-Modified"] == _LAST_MODIFIED_HTTP
+ assert exc.value.headers["Cache-Control"] == _CACHE_CONTROL
+
+ result = await handler._serve_ca(ca, {}, Mock(headers={}), repository_version="rv")
+ assert result == "streamed"
+ _, stream_response, stream_ca = handler._stream_content_artifact.call_args.args
+ assert stream_ca is ca
+ assert stream_response.headers["Last-Modified"] == _LAST_MODIFIED_HTTP
+
+
+@pytest.mark.asyncio
+@pytest.mark.django_db
+async def test_content_last_modified_from_repository_membership():
+ """Last-Modified is RepositoryContent.pulp_created for the served version, else omitted."""
+ repo = await create_repository()
+ content = await create_content()
+ other = await create_content()
+ publication = None
+ try:
+ ca = await create_content_artifact(content)
+ handler = Handler()
+ assert await handler._content_last_modified(ca) is None
+
+ v1 = await _add_content_to_new_version(repo, content)
+ expected = await sync_to_async(_membership_pulp_created)(v1, content)
+ assert await handler._content_last_modified(ca, repository_version=v1) == expected
+
+ publication = await sync_to_async(Publication.objects.create)(repository_version=v1)
+ assert await handler._content_last_modified(ca, publication=publication) == expected
+
+ def _add_other():
+ with repo.new_version() as version:
+ version.add_content(Content.objects.filter(pk=other.pk))
+ return repo.latest_version()
+
+ v2 = await sync_to_async(_add_other)()
+ assert await handler._content_last_modified(ca, repository_version=v2) == expected
+
+ def _remove():
+ with repo.new_version() as version:
+ version.remove_content(Content.objects.filter(pk=content.pk))
+ return repo.latest_version()
+
+ v3 = await sync_to_async(_remove)()
+ assert await handler._content_last_modified(ca, repository_version=v3) is None
+ finally:
+ if publication is not None:
+ await publication.adelete()
+ await repo.adelete()
+ await content.adelete()
+ await other.adelete()
+
+
@pytest.mark.asyncio
@pytest.mark.django_db
async def test_async_pull_through_add(ca1, monkeypatch, app_status):
diff --git a/pulpcore/tests/unit/models/test_publication_retention.py b/pulpcore/tests/unit/models/test_publication_retention.py
index ce9b7470ea4..b5cc0404149 100644
--- a/pulpcore/tests/unit/models/test_publication_retention.py
+++ b/pulpcore/tests/unit/models/test_publication_retention.py
@@ -348,6 +348,9 @@ def test_returns_ca_when_content_in_publication(self, version_with_content, expe
pub_with_a = pub_factory(version_with_content, pass_through=True)
dist = dist_factory(pub=pub_with_a)
assert dist.get_fallback_ca(self.content_path) == expected_ca
+ ca, publication = dist.get_fallback(self.content_path)
+ assert ca == expected_ca
+ assert publication.pk == pub_with_a.pk
def test_returns_none_when_content_not_in_publication(self, version_without_content):
"""Returns None when the served publication does not contain the content."""
@@ -379,6 +382,9 @@ def test_returns_ca_when_content_only_in_superseded_publication(
pub_without_a = pub_factory(version_without_content, pass_through=True)
update_dist(dist, pub=pub_without_a)
assert dist.get_fallback_ca(self.content_path) == expected_ca
+ ca, publication = dist.get_fallback(self.content_path)
+ assert ca == expected_ca
+ assert publication.pk == pub_with_a.pk
def test_returns_none_when_repository_unset(self, version_with_content, expected_ca):
"""Returns None once the distribution's repository is cleared."""
diff --git a/pulpcore/tests/unit/test_cache.py b/pulpcore/tests/unit/test_cache.py
index 6da69732e07..3cb01f9110f 100644
--- a/pulpcore/tests/unit/test_cache.py
+++ b/pulpcore/tests/unit/test_cache.py
@@ -1,9 +1,17 @@
+import json
from time import sleep
+from time import time as now
+from unittest.mock import AsyncMock, Mock
import pytest
+from aiohttp.web import Response
+from aiohttp.web_exceptions import HTTPNotModified
+from django.test import override_settings
+from django.utils.http import http_date
import pulpcore.app.redis_connection
from pulpcore.cache import Cache
+from pulpcore.cache.cache import AsyncContentCache
@pytest.fixture
@@ -107,3 +115,187 @@ def test_clear(pulp_redisdb):
cache.redis.flushdb()
for key, _, base_key in tuples:
assert not cache.exists(key, base_key=base_key)
+
+
+def _request_with_if_modified_since(value):
+ return Mock(headers={"If-Modified-Since": value} if value else {})
+
+
+_LM = http_date(1_000_000_000)
+
+
+def test_async_content_cache_not_modified():
+ """If-Modified-Since is compared to Last-Modified at second resolution."""
+ newer = http_date(1_000_000_060)
+ older = http_date(999_999_940)
+ future = http_date(now() + 86400)
+ inm = Mock(headers={"If-Modified-Since": _LM, "If-None-Match": '"abc"'})
+
+ assert AsyncContentCache._not_modified(_request_with_if_modified_since(_LM), _LM) is True
+ assert AsyncContentCache._not_modified(_request_with_if_modified_since(newer), _LM) is True
+ assert AsyncContentCache._not_modified(_request_with_if_modified_since(older), _LM) is False
+ assert AsyncContentCache._not_modified(_request_with_if_modified_since(None), _LM) is False
+ assert AsyncContentCache._not_modified(_request_with_if_modified_since(_LM), None) is False
+ assert AsyncContentCache._not_modified(_request_with_if_modified_since("garbage"), _LM) is False
+ assert AsyncContentCache._not_modified(inm, _LM) is False
+ assert AsyncContentCache._not_modified(_request_with_if_modified_since(future), _LM) is False
+
+
+def test_async_content_cache_make_not_modified_echoes_metadata():
+ """The 304 carries only validator/caching metadata already present on the source."""
+ source = {
+ "Cache-Control": "public, max-age=0, must-revalidate",
+ "Content-Length": "1024",
+ "X-PULP-CACHE": "HIT",
+ }
+
+ exc = AsyncContentCache._make_not_modified(source, _LM)
+
+ assert isinstance(exc, HTTPNotModified)
+ assert exc.headers["Last-Modified"] == _LM
+ assert exc.headers["Cache-Control"] == "public, max-age=0, must-revalidate"
+ assert exc.headers["X-PULP-CACHE"] == "HIT"
+ assert "Content-Length" not in exc.headers
+
+ bare = AsyncContentCache._make_not_modified({}, _LM)
+ assert "X-PULP-CACHE" not in bare.headers
+ assert "Cache-Control" not in bare.headers
+
+
+def test_async_content_cache_build_response_pops_last_modified():
+ """build_response must not pass the stored last_modified field to the response constructor."""
+ cache = AsyncContentCache.__new__(AsyncContentCache)
+ entry = {
+ "type": "Response",
+ "status": 200,
+ "headers": {"Last-Modified": _LM},
+ "last_modified": _LM,
+ "body": b"hello".hex(),
+ }
+
+ response = cache.build_response(entry)
+
+ assert response.status == 200
+ assert response.body == b"hello"
+ assert response.headers["Last-Modified"] == _LM
+ assert response.headers["X-PULP-CACHE"] == "HIT"
+
+
+def _entry(*, store_field=True):
+ entry = {
+ "type": "Response",
+ "status": 200,
+ "headers": {
+ "Last-Modified": _LM,
+ "Cache-Control": "public, max-age=0, must-revalidate",
+ },
+ "body": b"payload".hex(),
+ "expires": None,
+ }
+ if store_field:
+ entry["last_modified"] = _LM
+ return entry
+
+
+def _cache():
+ cache = AsyncContentCache.__new__(AsyncContentCache)
+ cache.auth = None
+ cache.default_base_key = "base"
+ cache.keys = ()
+ cache.default_expires_ttl = 60
+ cache.get_request_from_args = lambda args: args[0]
+ cache.make_key = lambda req: "key"
+ return cache
+
+
+async def _run_cached(cache, request, handler=None):
+ if handler is None:
+
+ async def handler(req):
+ raise AssertionError("handler must not run")
+
+ with override_settings(CACHE_ENABLED=True):
+ return await AsyncContentCache.__call__(cache, handler)(request)
+
+
+@pytest.mark.asyncio
+@pytest.mark.parametrize("store_field", [True, False], ids=["stored-field", "header-fallback"])
+async def test_cache_hit_304_does_not_rebuild_response(store_field):
+ """A matching If-Modified-Since 304s without reconstructing the cached response."""
+ cache = _cache()
+ cache.get_entry = AsyncMock(return_value=_entry(store_field=store_field))
+ cache.build_response = Mock(side_effect=AssertionError("must not reconstruct"))
+
+ with pytest.raises(HTTPNotModified) as exc:
+ await _run_cached(cache, Mock(headers={"If-Modified-Since": _LM}))
+
+ cache.build_response.assert_not_called()
+ assert exc.value.headers["Last-Modified"] == _LM
+ assert exc.value.headers["X-PULP-CACHE"] == "HIT"
+
+
+@pytest.mark.asyncio
+async def test_cache_hit_stale_if_modified_since_rebuilds_response():
+ """An older If-Modified-Since on a cache hit still reconstructs the full cached response."""
+ entry = _entry()
+ rebuilt = Mock(headers={"X-PULP-ARTIFACT-SIZE": None})
+ cache = _cache()
+ cache.get_entry = AsyncMock(return_value=entry)
+ cache.build_response = Mock(return_value=rebuilt)
+
+ response = await _run_cached(cache, Mock(headers={"If-Modified-Since": http_date(999_999_000)}))
+
+ cache.build_response.assert_called_once_with(entry)
+ assert response is rebuilt
+
+
+@pytest.mark.asyncio
+async def test_cache_miss_does_not_304_prepared_stream():
+ """A live stream that already started writing must not be converted into a 304."""
+ stream = Mock(headers={"Last-Modified": _LM}, prepared=True, status=200)
+ cache = _cache()
+ cache.get_entry = AsyncMock(return_value=None)
+ cache.make_entry = AsyncMock(return_value=stream)
+
+ async def handler(req):
+ raise AssertionError("handler is invoked via make_entry")
+
+ assert await _run_cached(cache, Mock(headers={"If-Modified-Since": _LM}), handler) is stream
+
+
+@pytest.mark.asyncio
+async def test_make_entry_does_not_cache_304():
+ """HTTPNotModified is HTTPSuccessful but must never be written to Redis."""
+ cache = _cache()
+ cache.set = AsyncMock()
+
+ async def handler():
+ raise HTTPNotModified(headers={"Last-Modified": _LM})
+
+ with pytest.raises(HTTPNotModified):
+ await cache.make_entry("k", "b", handler, (), {}, 60)
+
+ cache.set.assert_not_called()
+
+
+@pytest.mark.asyncio
+async def test_make_entry_stores_last_modified():
+ """A 200 with Last-Modified is stored so later cache hits can 304 without rebuilding."""
+ captured = {}
+ cache = _cache()
+
+ async def fake_set(key, value, expires=None, base_key=None):
+ captured["entry"] = json.loads(value)
+
+ cache.set = fake_set
+
+ async def handler():
+ return Response(body=b"hello", headers={"Last-Modified": _LM})
+
+ result = await cache.make_entry("k", "b", handler, (), {}, 60)
+
+ assert result.headers["Last-Modified"] == _LM
+ assert result.headers["X-PULP-CACHE"] == "MISS"
+ assert captured["entry"]["last_modified"] == _LM
+ assert captured["entry"]["headers"]["Last-Modified"] == _LM
+ assert captured["entry"]["type"] == "Response"
diff --git a/pulpcore/tests/unit/test_responses.py b/pulpcore/tests/unit/test_responses.py
new file mode 100644
index 00000000000..2c4d74ecf6e
--- /dev/null
+++ b/pulpcore/tests/unit/test_responses.py
@@ -0,0 +1,113 @@
+import os
+from datetime import datetime, timezone
+
+import pytest
+from aiohttp.test_utils import make_mocked_request
+from aiohttp.web import FileResponse
+from django.utils.http import http_date
+
+from pulpcore.responses import PulpFileResponse
+
+# _make_response / _FileResponseResult exist only on aiohttp 3.11+. Lowerbounds installs 3.10.
+_SKIP_MAKE_RESPONSE = pytest.mark.skipif(
+ not hasattr(FileResponse, "_make_response"),
+ reason="aiohttp FileResponse._make_response requires aiohttp 3.11+",
+)
+
+_PULP_LM = http_date(datetime(2024, 6, 1, tzinfo=timezone.utc).timestamp())
+# 2001-09-09; If-Modified-Since between this and _PULP_LM is the interesting case
+_FILE_MTIME = 1_000_000_000
+_IF_MODIFIED_SINCE_AFTER_MTIME = http_date(datetime(2022, 1, 1, tzinfo=timezone.utc).timestamp())
+
+
+def _artifact(tmp_path):
+ path = tmp_path / "artifact"
+ path.write_bytes(b"payload")
+ os.utime(path, (_FILE_MTIME, _FILE_MTIME))
+ return path
+
+
+@pytest.mark.parametrize("with_handler_lm", [True, False], ids=["handler-lm", "no-lm"])
+def test_pulp_file_response_ignores_file_mtime(tmp_path, with_handler_lm):
+ """aiohttp's file-mtime assignment must not advertise a filesystem Last-Modified."""
+ headers = {"Last-Modified": _PULP_LM} if with_handler_lm else None
+ response = PulpFileResponse(_artifact(tmp_path), headers=headers)
+ response.last_modified = 2_000_000_000
+ if with_handler_lm:
+ assert response.headers["Last-Modified"] == _PULP_LM
+ else:
+ assert "Last-Modified" not in response.headers
+
+
+@pytest.mark.parametrize("with_handler_lm", [True, False], ids=["handler-lm", "no-lm"])
+def test_pulp_file_response_never_emits_mtime_etag(tmp_path, with_handler_lm):
+ """mtime ETags are not advertised, with or without a Pulp Last-Modified."""
+ headers = {"Last-Modified": _PULP_LM} if with_handler_lm else None
+ response = PulpFileResponse(_artifact(tmp_path), headers=headers)
+ response.etag = "abc123"
+ assert "ETag" not in response.headers
+
+
+@_SKIP_MAKE_RESPONSE
+@pytest.mark.parametrize("with_handler_lm", [True, False], ids=["handler-lm", "no-lm"])
+def test_pulp_file_response_does_not_304_on_file_mtime(tmp_path, with_handler_lm):
+ """If-Modified-Since after file mtime must not 304; stock FileResponse would."""
+ from aiohttp.web_fileresponse import _FileResponseResult
+
+ path = _artifact(tmp_path)
+ headers = {"Last-Modified": _PULP_LM} if with_handler_lm else None
+ request = make_mocked_request(
+ "GET", "/", headers={"If-Modified-Since": _IF_MODIFIED_SINCE_AFTER_MTIME}
+ )
+
+ pulp = PulpFileResponse(str(path), headers=headers)
+ result, fobj, _st, _enc = pulp._make_response(request, "")
+ try:
+ assert result is _FileResponseResult.SEND_FILE
+ finally:
+ if fobj:
+ fobj.close()
+ if with_handler_lm:
+ assert pulp.headers["Last-Modified"] == _PULP_LM
+ else:
+ assert "Last-Modified" not in pulp.headers
+
+ stock = FileResponse(str(path))
+ result, fobj, _st, _enc = stock._make_response(
+ make_mocked_request(
+ "GET", "/", headers={"If-Modified-Since": _IF_MODIFIED_SINCE_AFTER_MTIME}
+ ),
+ "",
+ )
+ try:
+ assert result is _FileResponseResult.NOT_MODIFIED
+ finally:
+ if fobj:
+ fobj.close()
+
+
+@_SKIP_MAKE_RESPONSE
+def test_pulp_file_response_does_not_blank_if_range(tmp_path):
+ """If-Range stays available so aiohttp can refuse a stale Range instead of a corrupt 206."""
+ from aiohttp.web_fileresponse import _FileResponseResult
+
+ if_range = http_date(_FILE_MTIME)
+ request = make_mocked_request(
+ "GET",
+ "/",
+ headers={
+ "If-Range": if_range,
+ "Range": "bytes=0-1",
+ "If-Modified-Since": if_range,
+ },
+ )
+ response = PulpFileResponse(str(_artifact(tmp_path)), headers={"Last-Modified": _PULP_LM})
+ result, fobj, _st, _enc = response._make_response(request, "")
+ try:
+ assert result is _FileResponseResult.SEND_FILE
+ finally:
+ if fobj:
+ fobj.close()
+
+ assert request.if_range is not None
+ assert request.if_modified_since is None
From 0505dd8d125eaded0d6b6198ffa9713824c849ae Mon Sep 17 00:00:00 2001
From: Carlos Feria <2582866+carlosthe19916@users.noreply.github.com>
Date: Mon, 7 Sep 2026 15:44:51 +0200
Subject: [PATCH 2/2] fix: align code as per review
---
CHANGES/7929.feature | 1 -
pulpcore/app/models/publication.py | 22 +-
pulpcore/app/util.py | 17 +
pulpcore/cache/cache.py | 130 ++---
pulpcore/content/handler.py | 268 +++++-----
pulpcore/responses.py | 53 +-
.../test_content_if_modified_since.py | 457 ++++--------------
pulpcore/tests/unit/content/test_handler.py | 236 +--------
.../unit/models/test_publication_retention.py | 6 -
pulpcore/tests/unit/test_cache.py | 192 --------
pulpcore/tests/unit/test_responses.py | 113 -----
11 files changed, 287 insertions(+), 1208 deletions(-)
delete mode 100644 CHANGES/7929.feature
delete mode 100644 pulpcore/tests/unit/test_responses.py
diff --git a/CHANGES/7929.feature b/CHANGES/7929.feature
deleted file mode 100644
index b2098c9a935..00000000000
--- a/CHANGES/7929.feature
+++ /dev/null
@@ -1 +0,0 @@
-Added `Last-Modified` / `If-Modified-Since` (`304 Not Modified`) and `Cache-Control: public, max-age=0, must-revalidate` on content-app artifact responses (filesystem and `ArtifactResponse`; not object-storage 302s) so edge caches can revalidate after ContentGuard without re-fetching the body.
diff --git a/pulpcore/app/models/publication.py b/pulpcore/app/models/publication.py
index fe5474eddb6..734fbb4c11c 100644
--- a/pulpcore/app/models/publication.py
+++ b/pulpcore/app/models/publication.py
@@ -794,26 +794,14 @@ def get_fallback_ca(self, path):
"""
Return a ContentArtifact for path from the grace-period publication history, or None.
- See :meth:`get_fallback` for the publication that contained the unit.
- """
- ca, _publication = self.get_fallback(path)
- return ca
-
- def get_fallback(self, path):
- """
- Return ``(ContentArtifact, Publication)`` from grace-period history, or ``(None, None)``.
-
Iterates DistributedPublication records for this distribution from newest to oldest,
trying each publication until the path is found. Handles both pass-through and
non-pass-through (PublishedArtifact) publications.
- Returns ``(None, None)`` immediately when DISTRIBUTED_PUBLICATION_RETENTION_PERIOD is 0.
- The publication is the one that still contains the unit, which may be a superseded
- version — callers that need ``RepositoryContent.pulp_created`` must use that publication's
- repository version, not the distribution's current one.
+ Returns None immediately when DISTRIBUTED_PUBLICATION_RETENTION_PERIOD is 0.
"""
if not retain_distributed_pub_enabled():
- return None, None
+ return None
recent_dp = (
DistributedPublication.get_non_expired()
.filter(distribution=self)
@@ -829,7 +817,7 @@ def get_fallback(self, path):
.first()
)
if ca is not None:
- return ca, pub
+ return ca
else:
pa = (
pub.published_artifact.select_related(
@@ -840,8 +828,8 @@ def get_fallback(self, path):
.first()
)
if pa is not None:
- return pa.content_artifact, pub
- return None, None
+ return pa.content_artifact
+ return None
@hook(BEFORE_CREATE)
def _set_default_content_guard(self):
diff --git a/pulpcore/app/util.py b/pulpcore/app/util.py
index 76f2a5b47fb..a527938c463 100644
--- a/pulpcore/app/util.py
+++ b/pulpcore/app/util.py
@@ -17,6 +17,7 @@
from django.conf import settings
from django.db import connection
from django.db.models import Model, UUIDField
+from django.utils.http import parse_http_date
from rest_framework.reverse import reverse as drf_reverse
from rest_framework.serializers import ValidationError
@@ -693,6 +694,22 @@ def normalize_http_status(status):
return ""
+def check_request_was_modified(request, last_modified):
+ if not last_modified:
+ return True
+
+ if_modified_since = request.headers.get("If-Modified-Since")
+ if not if_modified_since:
+ return True
+
+ try:
+ last_modified_ts = parse_http_date(last_modified)
+ if_modified_ts = parse_http_date(if_modified_since)
+ return last_modified_ts > if_modified_ts
+ except (TypeError, ValueError):
+ return True
+
+
class HashingFileWriter(RawIOBase):
"""
A file-like object that handles writing data to disk with simultaneous
diff --git a/pulpcore/cache/cache.py b/pulpcore/cache/cache.py
index 49f42fdf20d..7e0344aa4ab 100644
--- a/pulpcore/cache/cache.py
+++ b/pulpcore/cache/cache.py
@@ -3,12 +3,11 @@
import time
from functools import wraps
-from aiohttp.web import FileResponse, HTTPSuccessful, Request, Response, StreamResponse
+from aiohttp.web import FileResponse, HTTPSuccessful, Request, Response
from aiohttp.web_exceptions import HTTPFound, HTTPNotModified
from django.conf import settings
from django.http import FileResponse as ApiFileResponse
from django.http import HttpResponse, HttpResponseRedirect
-from django.utils.http import parse_http_date_safe
from redis import ConnectionError
from redis.asyncio import ConnectionError as AConnectionError
from rest_framework.request import Request as ApiRequest
@@ -18,8 +17,9 @@
get_async_redis_connection,
get_redis_connection,
)
+from pulpcore.app.util import check_request_was_modified
from pulpcore.metrics import artifacts_size_counter
-from pulpcore.responses import ArtifactResponse, PulpFileResponse
+from pulpcore.responses import ArtifactResponse
DEFAULT_EXPIRES_TTL = settings.CACHE_SETTINGS["EXPIRES_TTL"]
@@ -307,7 +307,7 @@ class AsyncContentCache(AsyncCache):
"""Cache object meant to be used for the content app"""
RESPONSE_TYPES = {
- "FileResponse": PulpFileResponse,
+ "FileResponse": FileResponse,
"ArtifactResponse": ArtifactResponse,
"Response": Response,
"Redirect": HTTPFound,
@@ -350,93 +350,32 @@ async def cached_function(*args, **kwargs):
if self.auth:
await self.auth(request, self, bk)
key = self.make_key(request)
-
# Check cache
- entry = await self.get_entry(key, bk)
- if entry is not None:
- # Cache hit. Authorization has already run. If the client's If-Modified-Since
- # covers the stored last_modified, answer a bodyless 304 without reconstructing
- # the full response. Fall back to the header for entries cached before this field.
- last_modified = entry.get("last_modified") or entry.get("headers", {}).get(
- "Last-Modified"
+ response = await self.make_response(key, bk, request)
+ if response is None:
+ # Cache miss, create new entry
+ response = await self.make_entry(
+ key, bk, func, args, kwargs, self.default_expires_ttl
)
- if self._not_modified(request, last_modified):
- headers = dict(entry.get("headers") or {})
- headers["X-PULP-CACHE"] = "HIT"
- raise self._make_not_modified(headers, last_modified)
- response = self.build_response(entry)
- if size := response.headers.get("X-PULP-ARTIFACT-SIZE"):
- artifacts_size_counter.add(size)
- return response
-
- # Cache miss: build and cache the full response (a 304 is never stored). Still answer
- # a matching conditional request with a 304 from the fresh response's Last-Modified,
- # but never after a stream has already started writing.
- response = await self.make_entry(key, bk, func, args, kwargs, self.default_expires_ttl)
- if getattr(response, "prepared", False):
- return response
- last_modified = response.headers.get("Last-Modified")
- if self._not_modified(request, last_modified):
- raise self._make_not_modified(response.headers, last_modified)
+ elif size := response.headers.get("X-PULP-ARTIFACT-SIZE"):
+ artifacts_size_counter.add(size)
+
return response
return cached_function
- @staticmethod
- def _not_modified(request, last_modified):
- """True when the request's If-Modified-Since covers the given Last-Modified value.
-
- Ignore If-Modified-Since when If-None-Match is present, or when it is later than
- the server clock.
- """
- if not last_modified:
- return False
- if request.headers.get("If-None-Match"):
- return False
- if_modified_since = parse_http_date_safe(request.headers.get("If-Modified-Since", ""))
- if if_modified_since is None or if_modified_since > time.time():
- return False
- lm_epoch = parse_http_date_safe(last_modified)
- return lm_epoch is not None and lm_epoch <= if_modified_since
-
- @staticmethod
- def _make_not_modified(source_headers, last_modified):
- """Build a bodyless 304 echoing Last-Modified and any caching metadata already present."""
- headers = {"Last-Modified": last_modified}
- for name in ("Cache-Control", "X-PULP-CACHE"):
- if value := source_headers.get(name):
- headers[name] = value
- return HTTPNotModified(headers=headers)
-
def get_request_from_args(self, args):
"""Finds the request object from list of args"""
for arg in args:
if isinstance(arg, Request):
return arg
- async def get_entry(self, key, base_key):
- """Return the cached entry dict for ``key`` (deleting stale/invalid rows), or None."""
+ async def make_response(self, key, base_key, request=None):
+ """Tries to find the cached entry and turn it into a proper response"""
entry = await self.get(key, base_key)
if not entry:
return None
entry = json.loads(entry)
- response_type = entry.get("type")
- # None means "doesn't expire", unset/absent means "already expired".
- expires = entry.get("expires", -1)
- if (not response_type or response_type not in self.RESPONSE_TYPES) or (
- expires and expires < time.time()
- ):
- # Bad entry, delete from cache
- await self.delete(key, base_key)
- return None
- return entry
-
- def build_response(self, entry):
- """Turn a cached entry dict into a proper response object (marked as a cache HIT)."""
- entry = dict(entry) # do not mutate the caller's dict
- entry.pop("expires", None)
- entry.pop("last_modified", None)
- response_type = entry.pop("type")
if binary := entry.pop("body", None):
# raw binary data were translated to their hexadecimal representation and saved in
@@ -445,39 +384,38 @@ def build_response(self, entry):
# https://docs.aiohttp.org/en/stable/web_reference.html#response
entry["body"] = bytes.fromhex(binary)
- response = self.RESPONSE_TYPES[response_type](**entry)
+ response_type = entry.pop("type", None)
+ # None means "doesn't expire", unset means "already expired".
+ expires = entry.pop("expires", -1)
+ if (not response_type or response_type not in self.RESPONSE_TYPES) or (
+ expires and expires < time.time()
+ ):
+ # Bad entry, delete from cache
+ await self.delete(key, base_key)
+ return None
+
+ headers = entry.get("headers", {})
+ if request and not check_request_was_modified(
+ request, last_modified=headers.get("Last-Modified")
+ ):
+ response = HTTPNotModified(headers={"Cache-Control": headers.get("Cache-Control")})
+ else:
+ response = self.RESPONSE_TYPES[response_type](**entry)
response.headers.update({"X-PULP-CACHE": "HIT"})
return response
- async def make_response(self, key, base_key):
- """Tries to find the cached entry and turn it into a proper response"""
- entry = await self.get_entry(key, base_key)
- if entry is None:
- return None
- return self.build_response(entry)
-
async def make_entry(self, key, base_key, handler, args, kwargs, expires=DEFAULT_EXPIRES_TTL):
"""Gets the response for the request and try to turn it into a cacheable entry"""
try:
response = await handler(*args, **kwargs)
- except HTTPNotModified:
- # HTTPNotModified is HTTPSuccessful; do not swallow it into a cached entry.
- raise
- except (HTTPSuccessful, HTTPFound) as e:
+ except (HTTPSuccessful, HTTPFound, HTTPNotModified) as e:
response = e
original_response = response
- if isinstance(response, StreamResponse):
- if hasattr(response, "future_response"):
- response = response.future_response
-
- if getattr(response, "status", None) == 304:
- return original_response
+ if hasattr(response, "future_response"):
+ response = response.future_response
entry = {"headers": dict(response.headers), "status": response.status}
- if last_modified := response.headers.get("Last-Modified"):
- # Stored alongside headers so a cache hit can 304 without reconstructing the response.
- entry["last_modified"] = last_modified
if expires is not None:
# Redis TTL is not sufficient: https://github.com/pulp/pulpcore/issues/4845
entry["expires"] = expires + time.time()
diff --git a/pulpcore/content/handler.py b/pulpcore/content/handler.py
index 4b12af1ccc4..77bc04496b3 100644
--- a/pulpcore/content/handler.py
+++ b/pulpcore/content/handler.py
@@ -10,13 +10,14 @@
import django
from aiohttp.client_exceptions import ClientConnectionError, ClientResponseError
-from aiohttp.web import HTTPOk, StreamResponse
+from aiohttp.web import FileResponse, HTTPOk, StreamResponse
from aiohttp.web_exceptions import (
HTTPError,
HTTPForbidden,
HTTPFound,
HTTPMovedPermanently,
HTTPNotFound,
+ HTTPNotModified,
HTTPRequestRangeNotSatisfiable,
)
from asgiref.sync import sync_to_async
@@ -26,7 +27,7 @@
from yarl import URL
from pulpcore.constants import CHECKPOINT_TS_FORMAT, STORAGE_RESPONSE_MAP
-from pulpcore.responses import ArtifactResponse, PulpFileResponse
+from pulpcore.responses import ArtifactResponse
os.environ.setdefault("DJANGO_SETTINGS_MODULE", "pulpcore.app.settings")
django.setup()
@@ -57,6 +58,7 @@
)
from pulpcore.app.util import ( # noqa: E402
cache_key,
+ check_request_was_modified,
get_domain,
)
from pulpcore.cache import AsyncContentCache # noqa: E402
@@ -68,6 +70,8 @@
log = logging.getLogger(__name__)
+EDGE_CACHE_CONTROL = "public, max-age=0, must-revalidate"
+
class PathNotResolved(HTTPNotFound):
"""
@@ -525,9 +529,7 @@ def response_headers(path, distribution=None):
if content_type:
headers["Content-Type"] = content_type
- # Tell edge caches to revalidate on every use. Combined with Last-Modified below this
- # lets them confirm freshness with a lightweight If-Modified-Since instead of re-fetching.
- headers["Cache-Control"] = "public, max-age=0, must-revalidate"
+ headers["Cache-Control"] = EDGE_CACHE_CONTROL
# Let plugin-Distribution set headers for this path if it wants.
if distribution:
@@ -725,16 +727,17 @@ async def _match_and_stream(self, path, request):
content_handler_result = await sync_to_async(distro.content_handler)(original_rel_path)
if content_handler_result is not None:
if isinstance(content_handler_result, ContentArtifact):
- ch_repository, ch_repo_version, ch_publication = await sync_to_async(
- distro.get_repository_publication_and_version
- )()
- return await self._serve_ca(
- content_handler_result,
- headers,
- request,
- publication=ch_publication,
- repository_version=ch_repo_version,
+ await self._add_last_modified_header(
+ headers, content_artifact=content_handler_result, distribution=distro
)
+ if content_handler_result.artifact:
+ return await self._serve_content_artifact(
+ content_handler_result, headers, request
+ )
+ else:
+ return await self._stream_content_artifact(
+ request, StreamResponse(headers=headers), content_handler_result
+ )
else:
# the result is a response so just return it
return content_handler_result
@@ -762,6 +765,9 @@ async def _match_and_stream(self, path, request):
raise HTTPMovedPermanently(f"{request.path}/")
original_rel_path = index_path
headers = self.response_headers(original_rel_path, distro)
+ await self._add_last_modified_header(
+ headers, suggested_last_modified=publication.pulp_created
+ )
except ObjectDoesNotExist:
dir_list, dates, sizes = await self.list_directory(None, publication, rel_path)
dir_list.update(
@@ -791,7 +797,12 @@ async def _match_and_stream(self, path, request):
except ObjectDoesNotExist:
pass
else:
- return await self._serve_ca(ca, headers, request, publication=publication)
+ if ca.artifact:
+ return await self._serve_content_artifact(ca, headers, request)
+ else:
+ return await self._stream_content_artifact(
+ request, StreamResponse(headers=headers), ca
+ )
# pass-through
if publication.pass_through:
@@ -815,13 +826,31 @@ async def _match_and_stream(self, path, request):
except ObjectDoesNotExist:
pass
else:
- return await self._serve_ca(ca, headers, request, publication=publication)
+ await self._add_last_modified_header(
+ headers,
+ content_artifact=ca,
+ repository_version=publication.repository_version,
+ )
+ if ca.artifact:
+ return await self._serve_content_artifact(ca, headers, request)
+ else:
+ return await self._stream_content_artifact(
+ request, StreamResponse(headers=headers), ca
+ )
# Grace-period fallback: serve from a recently-superseded publication
if distro.SERVE_FROM_PUBLICATION:
- ca, fallback_publication = await sync_to_async(distro.get_fallback)(original_rel_path)
+ ca = await sync_to_async(distro.get_fallback_ca)(original_rel_path)
if ca is not None:
- return await self._serve_ca(ca, headers, request, publication=fallback_publication)
+ await self._add_last_modified_header(
+ headers, content_artifact=ca, repository_version=repo_version
+ )
+ if ca.artifact:
+ return await self._serve_content_artifact(ca, headers, request)
+ else:
+ return await self._stream_content_artifact(
+ request, StreamResponse(headers=headers), ca
+ )
if repo_version and not publication and not distro.SERVE_FROM_PUBLICATION:
# Look for index.html or list the directory
@@ -863,7 +892,15 @@ async def _match_and_stream(self, path, request):
except ObjectDoesNotExist:
pass
else:
- return await self._serve_ca(ca, headers, request, repository_version=repo_version)
+ await self._add_last_modified_header(
+ headers, content_artifact=ca, repository_version=repo_version
+ )
+ if ca.artifact:
+ return await self._serve_content_artifact(ca, headers, request)
+ else:
+ return await self._stream_content_artifact(
+ request, StreamResponse(headers=headers), ca
+ )
# If we haven't found a match yet, try to use pull-through caching with remote
if distro.remote:
@@ -880,10 +917,13 @@ async def _match_and_stream(self, path, request):
# Try to add content to repository if present & supported
if repository and repository.PULL_THROUGH_SUPPORTED:
await repository.async_pull_through_add_content(ca)
- # Serve the ContentArtifact if already created (streams if not yet saved)
- return await self._serve_ca(
- ca, headers, request, repository_version=repo_version
- )
+ # Try to stream the ContentArtifact if already created
+ if ca.artifact:
+ return await self._serve_content_artifact(ca, headers, request)
+ else:
+ return await self._stream_content_artifact(
+ request, StreamResponse(headers=headers), ca
+ )
else:
# Try to stream the RemoteArtifact and potentially save it as a new Content unit
save_artifact = (
@@ -919,6 +959,38 @@ class Error(HTTPError):
reason = None
raise PathNotResolved(path, reason=reason)
+ async def _add_last_modified_header(
+ self,
+ headers,
+ suggested_last_modified: datetime | None = None,
+ content_artifact=None,
+ repository_version=None,
+ distribution=None,
+ ):
+ """
+ Add the last-modified header to the response headers if not already present.
+ """
+
+ def _find_repo_add_time():
+ if not repository_version:
+ _, rv, _ = distribution.get_repository_publication_and_version()
+ else:
+ rv = repository_version
+ cpk = content_artifact.content_id
+
+ rc = rv._content_relationships().filter(content_id=cpk).first()
+ return rc.pulp_created if rc else rv.pulp_created
+
+ if "Last-Modified" not in headers:
+ last_modified = None
+ if suggested_last_modified is not None:
+ last_modified = suggested_last_modified
+ elif content_artifact and (repository_version or distribution):
+ last_modified = await sync_to_async(_find_repo_add_time)()
+
+ if last_modified is not None:
+ headers["Last-Modified"] = http_date(last_modified.timestamp())
+
async def _stream_content_artifact(self, request, response, content_artifact):
"""
Stream and optionally save a ContentArtifact by requesting it using the associated remote.
@@ -1074,81 +1146,6 @@ def _save_artifact(self, download_result, remote_artifact, request=None):
ret.update({ca.relative_path: ca for ca in cas})
return ret
- async def _content_last_modified(
- self, content_artifact, *, repository_version=None, publication=None
- ):
- """
- Return when the content unit was added to the repository being served, or None.
-
- Uses ``RepositoryContent.pulp_created`` (the time the unit joined the served repository
- version), which is the value the content app exposes as ``Last-Modified``. Returns None
- when no repository version is available or the unit has no membership row (e.g. publish-
- generated metadata), in which case no ``Last-Modified`` header is set.
- """
-
- def _get():
- repo_version = repository_version
- if repo_version is None and publication is not None:
- repo_version = publication.repository_version
- if repo_version is None:
- return None
- return (
- repo_version._content_relationships()
- .filter(content_id=content_artifact.content_id)
- .order_by("-pulp_created")
- .values_list("pulp_created", flat=True)
- .first()
- )
-
- return await sync_to_async(_get)()
-
- @staticmethod
- def _last_modified_http_date(last_modified):
- """Format a datetime as an HTTP ``Last-Modified`` value, or None."""
- if last_modified is None:
- return None
- return http_date(last_modified.timestamp())
-
- @staticmethod
- def _strip_cache_control(headers):
- """Drop Cache-Control so a response cannot be stored as a shared public copy."""
- headers.pop("Cache-Control", None)
- return headers
-
- @staticmethod
- def _maybe_not_modified(request, headers, last_modified_header, *, raise_304=True):
- """Return True when If-Modified-Since covers Last-Modified; optionally raise 304."""
- if not AsyncContentCache._not_modified(request, last_modified_header):
- return False
- if raise_304:
- raise AsyncContentCache._make_not_modified(headers, last_modified_header)
- return True
-
- async def _serve_ca(self, ca, headers, request, *, publication=None, repository_version=None):
- """Serve a ContentArtifact, attaching ``Last-Modified`` from pulp_created.
-
- Looks up when the unit joined the served repository version. Saved artifacts get that
- timestamp in ``_serve_content_artifact`` (after the redirect check, so object-storage
- 302s stay unmodified). On-demand units without a local artifact 304 before the remote
- fetch when If-Modified-Since covers that timestamp.
- """
- last_modified = await self._content_last_modified(
- ca, publication=publication, repository_version=repository_version
- )
- if ca.artifact:
- # Last-Modified is applied in `_serve_content_artifact` after the redirect check so
- # object-storage 302s do not advertise a Pulp validator they cannot honor.
- return await self._serve_content_artifact(
- ca, headers, request, last_modified=last_modified
- )
- last_modified_header = self._last_modified_http_date(last_modified)
- if last_modified_header:
- headers["Last-Modified"] = last_modified_header
- # 304 before opening the remote, including when the cache is on: streams are not
- # stored as cacheable file responses, and a started StreamResponse cannot become 304.
- self._maybe_not_modified(request, headers, last_modified_header)
- return await self._stream_content_artifact(request, StreamResponse(headers=headers), ca)
-
def _build_response_from_content_artifact(self, content_artifact, headers, request):
"""Helper method to build the correct response to serve a ContentArtifact."""
@@ -1177,33 +1174,27 @@ def _build_url(**kwargs):
storage = domain.get_storage()
headers["X-PULP-ARTIFACT-SIZE"] = str(artifact_file.size)
- def _object_storage_redirect(url):
- # Presigned Locations must not be stored by shared caches.
- return HTTPFound(url, headers=self._strip_cache_control(CIMultiDict(headers)))
-
if domain.storage_class == "pulpcore.app.models.storage.FileSystem":
path = storage.path(artifact_name)
if not os.path.exists(path):
raise Exception(_("Expected path '{}' is not found").format(path))
- return PulpFileResponse(path, headers=headers)
+ return FileResponse(path, headers=headers)
elif not domain.redirect_to_object_storage:
return ArtifactResponse(content_artifact.artifact, headers=headers)
elif domain.storage_class in (
"storages.backends.s3boto3.S3Boto3Storage",
"storages.backends.s3.S3Storage",
):
- return _object_storage_redirect(_build_url(http_method=request.method))
+ return HTTPFound(_build_url(http_method=request.method), headers=headers)
elif domain.storage_class in (
"storages.backends.azure_storage.AzureStorage",
"storages.backends.gcloud.GoogleCloudStorage",
):
- return _object_storage_redirect(_build_url())
+ return HTTPFound(_build_url(), headers=headers)
else:
raise NotImplementedError()
- async def _serve_content_artifact(
- self, content_artifact, headers, request, *, last_modified=None
- ):
+ async def _serve_content_artifact(self, content_artifact, headers, request):
"""
Handle response for a Content Artifact with the file present.
@@ -1215,8 +1206,6 @@ async def _serve_content_artifact(
respond with.
headers (dict): A dictionary of response headers.
request(aiohttp.web.Request) The request to prepare a response for.
- last_modified (datetime): When the content was added to the served repository, used
- for the ``Last-Modified`` header and ``If-Modified-Since`` handling. May be None.
Raises:
[aiohttp.web_exceptions.HTTPFound][]: When we need to redirect to the file
@@ -1228,44 +1217,33 @@ async def _serve_content_artifact(
"""
artifact_file = content_artifact.artifact.file
content_length = artifact_file.size
- last_modified_header = self._last_modified_http_date(last_modified)
- response = self._build_response_from_content_artifact(content_artifact, headers, request)
- if isinstance(response, HTTPFound):
- # Redirect (object-storage) responses are left without a Pulp validator. Presigned
- # Locations must not be stored by shared caches.
- self._strip_cache_control(response.headers)
- artifacts_size_counter.add(content_length)
- raise response
+ try:
+ range_start, range_stop = request.http_range.start, request.http_range.stop
+ if range_start or range_stop:
+ if range_stop and artifact_file.size and range_stop > artifact_file.size:
+ start = 0 if range_start is None else range_start
+ content_length = artifact_file.size - start
+ elif range_stop:
+ content_length = range_stop - range_start
+ except ValueError:
+ size = artifact_file.size or "*"
+ raise HTTPRequestRangeNotSatisfiable(headers={"Content-Range": f"bytes */{size}"})
- if last_modified_header is not None:
- response.headers["Last-Modified"] = last_modified_header
-
- # If-Modified-Since is checked as if Range were not present. A matching
- # If-Modified-Since must 304, not 416. When the cache is on, skip the handler 304 so
- # Redis can store a 200.
- would_304 = self._maybe_not_modified(
- request,
- response.headers,
- response.headers.get("Last-Modified"),
- raise_304=not settings.CACHE_ENABLED,
- )
+ response = self._build_response_from_content_artifact(content_artifact, headers, request)
- if not would_304:
- try:
- range_start, range_stop = request.http_range.start, request.http_range.stop
- if range_start or range_stop:
- if range_stop and artifact_file.size and range_stop > artifact_file.size:
- start = 0 if range_start is None else range_start
- content_length = artifact_file.size - start
- elif range_stop:
- content_length = range_stop - range_start
- except ValueError:
- size = artifact_file.size or "*"
- raise HTTPRequestRangeNotSatisfiable(headers={"Content-Range": f"bytes */{size}"})
+ if not check_request_was_modified(request, last_modified=headers.get("Last-Modified")):
+ nmod_response = HTTPNotModified(headers={"Cache-Control": EDGE_CACHE_CONTROL})
+ if settings.CACHE_ENABLED:
+ nmod_response.future_response = response
+ raise nmod_response
artifacts_size_counter.add(content_length)
- return response
+
+ if isinstance(response, HTTPFound):
+ raise response
+ else:
+ return response
async def _stream_remote_artifact(
self, request, response, remote_artifact, save_artifact, repository=None
@@ -1297,8 +1275,8 @@ async def _stream_remote_artifact(
)
)
- # If a Range cannot be satisfied, the response needs a Content-Range header with an
- # unsatisfied-range value.
+ # According to RFC7233 if a server cannot satisfy a Range request, the response needs to
+ # contain a Content-Range header with an unsatisfied-range value.
try:
range_start, range_stop = request.http_range.start, request.http_range.stop
size = remote_artifact.size
diff --git a/pulpcore/responses.py b/pulpcore/responses.py
index 25086f2e872..1b1fac62a0d 100644
--- a/pulpcore/responses.py
+++ b/pulpcore/responses.py
@@ -1,7 +1,7 @@
import asyncio
from aiohttp import hdrs
-from aiohttp.web import FileResponse, StreamResponse
+from aiohttp.web import StreamResponse
from aiohttp.web_exceptions import (
HTTPPartialContent,
HTTPRequestRangeNotSatisfiable,
@@ -9,57 +9,6 @@
from pulpcore.app.models import Artifact
-# aiohttp reads these as ``@reify`` properties off ``request._cache`` (private, present through
-# aiohttp 3.10–3.14). Seeding them to None suppresses only the headers that would 304 against file
-# mtime; If-Range / If-Match / If-Unmodified-Since stay intact so Range requests cannot return a
-# corrupt 206.
-_MTIME_304_REIFY_KEYS = ("if_modified_since", "if_none_match")
-
-
-def _suppress_mtime_conditionals(request):
- """Neutralize aiohttp's native mtime-based If-Modified-Since/ETag 304 handling for a request."""
- for key in _MTIME_304_REIFY_KEYS:
- request._cache[key] = None
-
-
-class PulpFileResponse(FileResponse):
- """A FileResponse that lets the content app own the ``Last-Modified`` validator.
-
- aiohttp's ``FileResponse`` overwrites ``Last-Modified`` with the file's mtime and runs its own
- ``If-Modified-Since``/``ETag`` handling against that mtime. The content app instead uses
- ``RepositoryContent.pulp_created`` (or omits the header) and answers conditional requests
- itself, so this class never advertises filesystem mtime as a validator.
- """
-
- async def prepare(self, request):
- # aiohttp < 3.11 runs the mtime-304 check inline in prepare(); 3.11+ moved it into
- # _make_response() (which prepare() calls). Seed here so it is neutralized on every
- # supported aiohttp version, not just those that expose _make_response().
- _suppress_mtime_conditionals(request)
- return await super().prepare(request)
-
- def _make_response(self, request, accept_encoding):
- _suppress_mtime_conditionals(request)
- return super()._make_response(request, accept_encoding)
-
- @property
- def last_modified(self):
- return FileResponse.last_modified.fget(self)
-
- @last_modified.setter
- def last_modified(self, value):
- # Never replace a handler Last-Modified, and never advertise the file mtime.
- return
-
- @property
- def etag(self):
- return FileResponse.etag.fget(self)
-
- @etag.setter
- def etag(self, value):
- # mtime-based ETags would disagree with RepositoryContent.pulp_created as Last-Modified.
- return
-
class ArtifactResponse(StreamResponse):
"""A response object can be used to send artifacts."""
diff --git a/pulpcore/tests/functional/api/using_plugin/test_content_if_modified_since.py b/pulpcore/tests/functional/api/using_plugin/test_content_if_modified_since.py
index 9046d41e4ed..f9b0adaf89d 100644
--- a/pulpcore/tests/functional/api/using_plugin/test_content_if_modified_since.py
+++ b/pulpcore/tests/functional/api/using_plugin/test_content_if_modified_since.py
@@ -1,7 +1,12 @@
-"""Tests for If-Modified-Since / 304 Not Modified on the content app."""
+"""Tests for If-Modified-Since / 304 Not Modified on the content app.
+When a client already has a copy of a file it can send an If-Modified-Since header
+asking the content app to reply "304 Not Modified" (an empty body) instead of
+re-sending the whole file. These tests cover that conversation.
+"""
+
+import time
from base64 import b64encode
-from time import sleep, time
from urllib.parse import urljoin
from uuid import uuid4
@@ -9,425 +14,173 @@
import requests
from django.utils.http import http_date, parse_http_date
-from pulpcore.client.pulp_file import FileRepositorySyncURL, PatchedfileFileDistribution
-from pulpcore.content.handler import Handler
-
-CACHE_CONTROL = Handler.response_headers("1.iso")["Cache-Control"]
-ONE_DAY_SECONDS = 86400
-
-
-def _get(url, headers=None):
- """GET the content-app response without following object-storage redirects."""
- return requests.get(url, headers=headers, allow_redirects=False)
+from pulpcore.client.pulp_file import (
+ FileRepositorySyncURL,
+ PatchedfileFileDistribution,
+)
+from pulpcore.content.handler import EDGE_CACHE_CONTROL
-def _assert_artifact_200(response):
+def assert_full_download(response):
+ """The server sent the whole file, plus the headers a client reuses to revalidate later."""
assert response.status_code == 200
assert response.content
- assert response.headers.get("Cache-Control") == CACHE_CONTROL
- assert response.headers.get("Last-Modified")
-
-
-def _get_and_assert_last_modified(url, headers=None):
- """GET the artifact, assert a full 200, and return (response, Last-Modified value)."""
- response = _get(url, headers=headers)
- _assert_artifact_200(response)
- return response, response.headers["Last-Modified"]
+ assert response.headers.get("Cache-Control") == EDGE_CACHE_CONTROL
+ # Last-Modified must be a real date in the past - never missing, epoch, or in the future.
+ last_modified = response.headers.get("Last-Modified")
+ assert last_modified
+ assert 0 < parse_http_date(last_modified) <= time.time()
-def _assert_304(response, last_modified):
+def assert_not_modified(response):
+ """The server skipped the download: a 304 with an empty body but the caching header intact."""
assert response.status_code == 304
assert response.content == b""
- assert response.headers.get("Last-Modified") == last_modified
- assert response.headers.get("Cache-Control") == CACHE_CONTROL
+ assert response.headers.get("Cache-Control") == EDGE_CACHE_CONTROL
-@pytest.fixture
-def redis_required(redis_status):
- """Skip when the content cache (Redis) is not reachable."""
- if not redis_status:
- pytest.skip("Could not connect to the Redis server")
+def assert_object_storage_redirect(response):
+ """On Azure/S3, pulpcore answers with a 302 to the object store, carrying the revalidation
+ headers - it does not stream the bytes itself."""
+ assert response.status_code == 302
+ assert response.headers.get("Location")
+ assert response.headers.get("Cache-Control") == EDGE_CACHE_CONTROL
+ # Last-Modified must be a real date in the past - never missing, epoch, or in the future.
+ last_modified = response.headers.get("Last-Modified")
+ assert last_modified
+ assert 0 < parse_http_date(last_modified) <= time.time()
@pytest.fixture
-def inline_storage(pulp_settings):
- """Skip when the instance redirects to object storage instead of serving bytes inline.
-
- The 304 path applies to filesystem/ArtifactResponse serving; object-storage 302s are not
- 304'd by design.
- """
+def assert_full_response(pulp_settings):
+ """Return the right assertion for a served (non-304) response on this backend: full bytes on
+ the filesystem, or a redirect to the object store on Azure/S3."""
backend = pulp_settings.STORAGES["default"]["BACKEND"]
redirects = (
backend != "pulpcore.app.models.storage.FileSystem"
and pulp_settings.REDIRECT_TO_OBJECT_STORAGE
)
- if redirects:
- pytest.skip("object-storage redirects are not 304'd by design")
+ return assert_object_storage_redirect if redirects else assert_full_download
@pytest.fixture
-def object_storage_redirects(pulp_settings):
- """Skip unless the instance redirects to object storage (302)."""
- backend = pulp_settings.STORAGES["default"]["BACKEND"]
- if (
- backend == "pulpcore.app.models.storage.FileSystem"
- or not pulp_settings.REDIRECT_TO_OBJECT_STORAGE
- ):
- pytest.skip("not using object-storage redirects")
-
-
-@pytest.fixture
-def published_file_distribution(
+def distribution(
file_repo_with_auto_publish,
file_remote_factory,
file_bindings,
file_distribution_factory,
- distribution_base_url,
monitor_task,
basic_manifest_path,
):
- """Immediate-sync a 3-file repo, distribute it, and return (repo, distro, base_url)."""
+ """Sync a small file repo, auto-publish it, and distribute it. Returns the distribution."""
remote = file_remote_factory(manifest_path=basic_manifest_path, policy="immediate")
body = FileRepositorySyncURL(remote=remote.pulp_href)
monitor_task(
file_bindings.RepositoriesFileApi.sync(file_repo_with_auto_publish.pulp_href, body).task
)
repo = file_bindings.RepositoriesFileApi.read(file_repo_with_auto_publish.pulp_href)
- distro = file_distribution_factory(repository=repo.pulp_href)
- return repo, distro, distribution_base_url(distro.base_url)
-
-
-@pytest.mark.parallel
-def test_artifact_get_sets_last_modified_and_cache_control(
- published_file_distribution, inline_storage, redis_status
-):
- """A plain 200 carries the Last-Modified validator and revalidate Cache-Control."""
- _repo, _distro, base_url = published_file_distribution
- url = urljoin(base_url, "1.iso")
-
- response = _get(url)
- _assert_artifact_200(response)
- last_modified = response.headers["Last-Modified"]
-
- if redis_status:
- assert response.headers.get("X-PULP-CACHE") == "MISS"
- cached = _get(url)
- _assert_artifact_200(cached)
- assert cached.headers.get("X-PULP-CACHE") == "HIT"
- assert cached.headers["Last-Modified"] == last_modified
-
-
-@pytest.mark.parallel
-def test_matching_if_modified_since_returns_304(
- published_file_distribution, inline_storage, redis_status
-):
- """An If-Modified-Since at or after the validator gets a bodyless 304."""
- _repo, _distro, base_url = published_file_distribution
- url = urljoin(base_url, "1.iso")
+ return file_distribution_factory(repository=repo.pulp_href)
- _first, last_modified = _get_and_assert_last_modified(url)
- matched = _get(url, headers={"If-Modified-Since": last_modified})
- _assert_304(matched, last_modified)
-
- sleep(2)
- later_response = _get(url, headers={"If-Modified-Since": http_date()})
- _assert_304(later_response, last_modified)
-
- if redis_status:
- assert matched.headers.get("X-PULP-CACHE") == "HIT"
- warm = _get(url)
- _assert_artifact_200(warm)
- assert warm.headers.get("X-PULP-CACHE") == "HIT"
- assert warm.content
+@pytest.fixture
+def distribution_url(distribution, distribution_base_url):
+ """The externally reachable base URL where `distribution` serves its files."""
+ return distribution_base_url(distribution.base_url)
@pytest.mark.parallel
-def test_stale_garbage_and_future_if_modified_since_return_200(
- published_file_distribution, inline_storage
-):
- """Older, unparseable, or future If-Modified-Since values fall back to a full 200."""
- _repo, _distro, base_url = published_file_distribution
- url = urljoin(base_url, "1.iso")
+def test_a_current_copy_is_not_redownloaded(distribution_url, assert_full_response):
+ """A client whose copy is already up to date gets a 304 instead of the file."""
+ url = urljoin(distribution_url, "1.iso")
- _first, last_modified = _get_and_assert_last_modified(url)
- last_modified_epoch = parse_http_date(last_modified)
+ # Download once and remember the date the server reported.
+ first = requests.get(url, allow_redirects=False)
+ assert_full_response(first)
+ served_date = first.headers["Last-Modified"]
- stale = http_date(last_modified_epoch - ONE_DAY_SECONDS)
- future = http_date(time() + ONE_DAY_SECONDS)
- for if_modified_since in (stale, "not a date", future):
- response = _get(url, headers={"If-Modified-Since": if_modified_since})
- _assert_artifact_200(response)
- assert response.headers["Last-Modified"] == last_modified
-
-
-@pytest.mark.parallel
-def test_last_modified_is_membership_not_version_time(
- file_bindings,
- file_repository_factory,
- file_content_unit_with_name_factory,
- file_distribution_factory,
- distribution_base_url,
- monitor_task,
- inline_storage,
-):
- """Last-Modified is the served unit's membership time, not the repo version's time."""
- repo = file_repository_factory(autopublish=True)
- content_a = file_content_unit_with_name_factory(f"{uuid4()}.iso")
- content_b = file_content_unit_with_name_factory(f"{uuid4()}.iso")
+ # Asking again with that exact date -> nothing changed -> 304.
+ reused = requests.get(url, headers={"If-Modified-Since": served_date}, allow_redirects=False)
+ assert_not_modified(reused)
- monitor_task(
- file_bindings.RepositoriesFileApi.modify(
- repo.pulp_href, {"add_content_units": [content_a.pulp_href]}
- ).task
- )
- repo = file_bindings.RepositoriesFileApi.read(repo.pulp_href)
- version_1 = file_bindings.RepositoriesFileVersionsApi.read(repo.latest_version_href)
-
- sleep(2)
-
- monitor_task(
- file_bindings.RepositoriesFileApi.modify(
- repo.pulp_href, {"add_content_units": [content_b.pulp_href]}
- ).task
- )
- repo = file_bindings.RepositoriesFileApi.read(repo.pulp_href)
- version_2 = file_bindings.RepositoriesFileVersionsApi.read(repo.latest_version_href)
-
- distro = file_distribution_factory(repository=repo.pulp_href)
- url = urljoin(distribution_base_url(distro.base_url), content_a.relative_path)
- response = _get(url)
- _assert_artifact_200(response)
-
- last_modified = parse_http_date(response.headers["Last-Modified"])
- v1_created = version_1.pulp_created.timestamp()
- v2_created = version_2.pulp_created.timestamp()
-
- # content_a joined the repo in version 1, so its validator predates version 2 entirely...
- assert last_modified < v2_created
- # ...and sits at version 1's creation (within HTTP-date's 1-second resolution), not later.
- assert last_modified <= v1_created + 1
+ # A client claiming an even newer copy than the server's is also told 304.
+ tomorrow = http_date(time.time() + 3600)
+ newer = requests.get(url, headers={"If-Modified-Since": tomorrow}, allow_redirects=False)
+ assert_not_modified(newer)
@pytest.mark.parallel
-def test_last_modified_is_membership_not_content_created(
- file_bindings,
- file_repository_factory,
- file_content_unit_with_name_factory,
- file_distribution_factory,
- distribution_base_url,
- monitor_task,
- inline_storage,
-):
- """One unit in two repos yields per-repo validators, not the unit's created time."""
- content = file_content_unit_with_name_factory(f"{uuid4()}.iso")
- repo_a = file_repository_factory(autopublish=True)
- monitor_task(
- file_bindings.RepositoriesFileApi.modify(
- repo_a.pulp_href, {"add_content_units": [content.pulp_href]}
- ).task
- )
- distro_a = file_distribution_factory(repository=repo_a.pulp_href)
- url_a = urljoin(distribution_base_url(distro_a.base_url), content.relative_path)
- response_a = _get(url_a)
- _assert_artifact_200(response_a)
-
- sleep(2)
+def test_a_stale_copy_gets_the_full_file(distribution_url, assert_full_response):
+ """A client whose copy predates the file downloads the whole thing again."""
+ url = urljoin(distribution_url, "1.iso")
- repo_b = file_repository_factory(autopublish=True)
- monitor_task(
- file_bindings.RepositoriesFileApi.modify(
- repo_b.pulp_href, {"add_content_units": [content.pulp_href]}
- ).task
- )
- distro_b = file_distribution_factory(repository=repo_b.pulp_href)
- url_b = urljoin(distribution_base_url(distro_b.base_url), content.relative_path)
- response_b = _get(url_b)
- _assert_artifact_200(response_b)
-
- last_modified_a = parse_http_date(response_a.headers["Last-Modified"])
- last_modified_b = parse_http_date(response_b.headers["Last-Modified"])
- content_created = content.pulp_created.timestamp()
-
- assert last_modified_b > last_modified_a
- assert last_modified_b > content_created
+ # "I last saw this at the dawn of time" -> the file is newer -> send it all.
+ long_ago = http_date(0)
+ response = requests.get(url, headers={"If-Modified-Since": long_ago}, allow_redirects=False)
+ assert_full_response(response)
@pytest.mark.parallel
-def test_content_guard_still_runs_before_304(
- published_file_distribution,
- inline_storage,
+def test_authorization_runs_before_revalidation(
+ distribution,
+ distribution_url,
+ assert_full_response,
pulpcore_bindings,
file_bindings,
gen_object_with_cleanup,
monitor_task,
):
- """Authorization runs on every request: a conditional GET is 403'd before any 304."""
- _repo, distro, base_url = published_file_distribution
- url = urljoin(base_url, "1.iso")
+ """A content guard is checked on every request - even one that would answer 304."""
+ url = urljoin(distribution_url, "1.iso")
+
+ # Protect the distribution: callers must send x-header: base64("123456").
guard = gen_object_with_cleanup(
pulpcore_bindings.ContentguardsHeaderApi,
- {
- "name": str(uuid4()),
- "header_name": "x-header",
- "header_value": "123456",
- },
+ {"name": str(uuid4()), "header_name": "x-header", "header_value": "123456"},
)
body = PatchedfileFileDistribution(content_guard=guard.pulp_href)
- monitor_task(file_bindings.DistributionsFileApi.partial_update(distro.pulp_href, body).task)
-
- auth_headers = {"x-header": b64encode(b"123456").decode("ascii")}
-
- denied = _get(url, headers={"If-Modified-Since": http_date()})
- assert denied.status_code == 403
-
- authorized = _get(url, headers=auth_headers)
- _assert_artifact_200(authorized)
- last_modified = authorized.headers["Last-Modified"]
-
- revalidated = _get(url, headers={**auth_headers, "If-Modified-Since": last_modified})
- _assert_304(revalidated, last_modified)
-
-
-@pytest.mark.parallel
-def test_published_metadata_has_no_last_modified(published_file_distribution, inline_storage):
- """Publish-generated metadata (PULP_MANIFEST) has no membership row, so no validator."""
- _repo, _distro, base_url = published_file_distribution
- url = urljoin(base_url, "PULP_MANIFEST")
-
- response = _get(url)
- assert response.status_code == 200
- assert response.content
- assert "Last-Modified" not in response.headers
-
- again = _get(url, headers={"If-Modified-Since": http_date()})
- assert again.status_code == 200
- assert again.content
- assert "Last-Modified" not in again.headers
-
-
-@pytest.mark.parallel
-def test_on_demand_304_does_not_fetch_remote(
- file_repo_with_auto_publish,
- generate_server_and_remote,
- file_bindings,
- file_distribution_factory,
- distribution_base_url,
- monitor_task,
- basic_manifest_path,
- inline_storage,
-):
- """An on-demand 304 is answered from the validator without touching the remote."""
- server, remote = generate_server_and_remote(
- manifest_path=basic_manifest_path, policy="on_demand"
- )
- body = FileRepositorySyncURL(remote=remote.pulp_href)
monitor_task(
- file_bindings.RepositoriesFileApi.sync(file_repo_with_auto_publish.pulp_href, body).task
+ file_bindings.DistributionsFileApi.partial_update(distribution.pulp_href, body).task
)
- repo = file_bindings.RepositoriesFileApi.read(file_repo_with_auto_publish.pulp_href)
- distro = file_distribution_factory(repository=repo.pulp_href)
- url = urljoin(distribution_base_url(distro.base_url), "1.iso")
-
- def iso_fetches():
- return [r for r in server.requests_record if "1.iso" in r.raw_path]
-
- assert iso_fetches() == []
-
- sleep(2)
- response = _get(url, headers={"If-Modified-Since": http_date()})
- assert response.status_code == 304
- assert response.content == b""
- assert response.headers.get("Last-Modified")
- assert response.headers.get("Cache-Control") == CACHE_CONTROL
- assert iso_fetches() == []
-
-
-@pytest.mark.parallel
-def test_matching_if_modified_since_beats_range(published_file_distribution, inline_storage):
- """A matching If-Modified-Since wins over a Range header: 304, not 206/416."""
- _repo, _distro, base_url = published_file_distribution
- url = urljoin(base_url, "1.iso")
- _first, last_modified = _get_and_assert_last_modified(url)
+ credentials = {"x-header": b64encode(b"123456").decode("ascii")}
+ tomorrow = http_date(time.time() + 3600)
- response = _get(url, headers={"If-Modified-Since": last_modified, "Range": "bytes=0-0"})
- _assert_304(response, last_modified)
-
-
-@pytest.mark.parallel
-def test_object_storage_redirect_is_not_304(published_file_distribution, object_storage_redirects):
- """Object-storage 302s carry no validator and never 304, even with If-Modified-Since."""
- _repo, _distro, base_url = published_file_distribution
- url = urljoin(base_url, "1.iso")
+ # No credentials -> rejected up front, before any 304 revalidation can happen.
+ denied = requests.get(url, headers={"If-Modified-Since": tomorrow}, allow_redirects=False)
+ assert denied.status_code == 403
- response = _get(url)
- assert response.status_code == 302
- assert "Last-Modified" not in response.headers
- assert "Cache-Control" not in response.headers
+ # With credentials the normal conversation works: full download, then 304.
+ authorized = requests.get(url, headers=credentials, allow_redirects=False)
+ assert_full_response(authorized)
- again = _get(url, headers={"If-Modified-Since": http_date()})
- assert again.status_code == 302
- assert "Last-Modified" not in again.headers
+ revalidated = requests.get(
+ url,
+ headers={**credentials, "If-Modified-Since": tomorrow},
+ allow_redirects=False,
+ )
+ assert_not_modified(revalidated)
@pytest.mark.parallel
-def test_cold_cache_miss_with_matching_ims_304s_but_caches_200(
- published_file_distribution, inline_storage, redis_required
+def test_cache_still_honors_conditional_requests(
+ distribution_url, assert_full_response, redis_status
):
- """A conditional first request 304s from the cache miss yet still stores a full 200.
-
- The handler defers its own 304 to Redis when the cache is on, so make_entry builds and caches
- the 200 while the miss path answers a bodyless 304. A plain follow-up must then be a full
- cache HIT, proving the 304 was never stored as an empty entry.
- """
- _repo, _distro, base_url = published_file_distribution
- url = urljoin(base_url, "1.iso")
-
- # First-ever request for this URL is conditional (cold cache).
- first = _get(url, headers={"If-Modified-Since": http_date()})
- assert first.status_code == 304
- assert first.content == b""
- last_modified = first.headers.get("Last-Modified")
- assert last_modified
- assert first.headers.get("Cache-Control") == CACHE_CONTROL
-
- # The miss stored a full 200, so a plain GET is a cache HIT with the same validator.
- warm = _get(url)
- _assert_artifact_200(warm)
- assert warm.headers.get("X-PULP-CACHE") == "HIT"
- assert warm.headers["Last-Modified"] == last_modified
-
-
-@pytest.mark.parallel
-def test_if_none_match_suppresses_304(published_file_distribution, inline_storage):
- """If-None-Match disables If-Modified-Since handling, so a match still returns a full 200."""
- _repo, _distro, base_url = published_file_distribution
- url = urljoin(base_url, "1.iso")
+ """A cached response revalidates to 304 but still serves the file to a client without a copy."""
+ if not redis_status:
+ pytest.skip("Could not connect to the Redis server")
- _first, last_modified = _get_and_assert_last_modified(url)
+ url = urljoin(distribution_url, "1.iso")
- response = _get(
- url, headers={"If-Modified-Since": last_modified, "If-None-Match": '"anything"'}
- )
- _assert_artifact_200(response)
- assert response.headers["Last-Modified"] == last_modified
+ # Warm the cache with a normal download.
+ assert_full_response(requests.get(url, allow_redirects=False))
+ # A cached response can still answer a conditional request with a 304.
+ tomorrow = http_date(time.time() + 3600)
+ revalidated = requests.get(url, headers={"If-Modified-Since": tomorrow}, allow_redirects=False)
+ assert_not_modified(revalidated)
+ assert revalidated.headers.get("X-PULP-CACHE") == "HIT"
-@pytest.mark.parallel
-def test_head_request_revalidates_with_304(published_file_distribution, inline_storage):
- """A conditional HEAD (as CDNs issue) sets the validator on 200 and 304s on revalidation."""
- _repo, _distro, base_url = published_file_distribution
- url = urljoin(base_url, "1.iso")
-
- initial = requests.head(url, allow_redirects=False)
- assert initial.status_code == 200
- last_modified = initial.headers["Last-Modified"]
- assert initial.headers.get("Cache-Control") == CACHE_CONTROL
-
- revalidated = requests.head(
- url, allow_redirects=False, headers={"If-Modified-Since": last_modified}
- )
- assert revalidated.status_code == 304
- assert revalidated.headers.get("Last-Modified") == last_modified
- assert revalidated.headers.get("Cache-Control") == CACHE_CONTROL
+ # A client without a copy still gets the full file from that same cache entry.
+ fresh = requests.get(url, allow_redirects=False)
+ assert_full_response(fresh)
+ assert fresh.headers.get("X-PULP-CACHE") == "HIT"
diff --git a/pulpcore/tests/unit/content/test_handler.py b/pulpcore/tests/unit/content/test_handler.py
index bf17b912743..045efdad6cd 100644
--- a/pulpcore/tests/unit/content/test_handler.py
+++ b/pulpcore/tests/unit/content/test_handler.py
@@ -1,19 +1,11 @@
import uuid
-from datetime import datetime, timedelta
-from datetime import timezone as dt_timezone
+from datetime import timedelta
from unittest.mock import AsyncMock, Mock
import pytest
import pytest_asyncio
-from aiohttp.web_exceptions import (
- HTTPFound,
- HTTPMovedPermanently,
- HTTPNotModified,
-)
-from asgiref.sync import sync_to_async
+from aiohttp.web_exceptions import HTTPMovedPermanently
from django.db import IntegrityError
-from django.test import override_settings
-from django.utils.http import http_date
from django_guid import clear_guid, set_guid
from pulpcore.app.models import AppStatus
@@ -206,18 +198,6 @@ async def create_distribution(remote, repository=None):
)
-async def _add_content_to_new_version(repo, content):
- """Add ``content`` to a new complete version of ``repo`` and return that version."""
- repo.CONTENT_TYPES = [Content]
-
- def _add():
- with repo.new_version() as version:
- version.add_content(Content.objects.filter(pk=content.pk))
- return repo.latest_version()
-
- return await sync_to_async(_add)()
-
-
@pytest.mark.asyncio
@pytest.mark.django_db
async def test_pull_through_remote_artifact_exists(request123, tmp_path):
@@ -606,218 +586,6 @@ def test_render_html_normal_name():
assert 'simple-dir/' in html
-_LAST_MODIFIED = datetime(2020, 1, 1, tzinfo=dt_timezone.utc)
-_LAST_MODIFIED_HTTP = http_date(_LAST_MODIFIED.timestamp())
-_IF_MODIFIED_SINCE_AFTER = http_date(datetime(2021, 1, 1, tzinfo=dt_timezone.utc).timestamp())
-_IF_MODIFIED_SINCE_BEFORE = http_date(datetime(2019, 1, 1, tzinfo=dt_timezone.utc).timestamp())
-_CACHE_CONTROL = "public, max-age=0, must-revalidate"
-
-
-class _UnsatisfiableRange:
- @property
- def start(self):
- raise ValueError()
-
- stop = None
-
-
-def _request(*, if_modified_since=None, http_range=None):
- return Mock(
- method="GET",
- http_range=http_range if http_range is not None else Mock(start=None, stop=None),
- headers={"If-Modified-Since": if_modified_since} if if_modified_since else {},
- )
-
-
-def _ca(*, artifact=True):
- ca = Mock()
- ca.relative_path = "file.iso"
- if artifact:
- ca.artifact.file.size = 7
- ca.artifact.file.name = "artifacts/obj"
- else:
- ca.artifact = None
- return ca
-
-
-def _handler_with_built_response(monkeypatch, built=None):
- handler = Handler()
- ca = _ca()
- if built is None:
- built = Mock(headers={"Cache-Control": _CACHE_CONTROL}, status=200)
- monkeypatch.setattr(handler, "_build_response_from_content_artifact", Mock(return_value=built))
- return handler, ca, built
-
-
-def _membership_pulp_created(version, content):
- return (
- version._content_relationships()
- .filter(content_id=content.pk)
- .values_list("pulp_created", flat=True)
- .get()
- )
-
-
-@pytest.mark.asyncio
-@pytest.mark.parametrize(
- "if_modified_since, last_modified, cache_enabled, expect_304",
- [
- (None, _LAST_MODIFIED, False, False),
- (_IF_MODIFIED_SINCE_AFTER, _LAST_MODIFIED, False, True),
- (_IF_MODIFIED_SINCE_BEFORE, _LAST_MODIFIED, False, False),
- (_IF_MODIFIED_SINCE_AFTER, None, False, False),
- (_IF_MODIFIED_SINCE_AFTER, _LAST_MODIFIED, True, False),
- ],
- ids=["no-if-modified-since", "fresh", "stale", "no-timestamp", "cache-on"],
-)
-async def test_serve_content_artifact_if_modified_since(
- monkeypatch, if_modified_since, last_modified, cache_enabled, expect_304
-):
- """Filesystem responses stamp Last-Modified.
-
- A matching If-Modified-Since is 304 unless the cache is on.
- """
- handler, ca, built = _handler_with_built_response(monkeypatch)
-
- with override_settings(CACHE_ENABLED=cache_enabled):
- if expect_304:
- with pytest.raises(HTTPNotModified) as exc:
- await handler._serve_content_artifact(
- ca,
- {},
- _request(if_modified_since=if_modified_since),
- last_modified=last_modified,
- )
- assert exc.value.headers["Last-Modified"] == _LAST_MODIFIED_HTTP
- assert exc.value.headers["Cache-Control"] == _CACHE_CONTROL
- else:
- response = await handler._serve_content_artifact(
- ca,
- {},
- _request(if_modified_since=if_modified_since),
- last_modified=last_modified,
- )
- assert response is built
- if last_modified is None:
- assert "Last-Modified" not in response.headers
- else:
- assert response.headers["Last-Modified"] == _LAST_MODIFIED_HTTP
-
-
-def test_response_headers_sets_cache_control():
- """All content responses instruct edge caches to revalidate on every use."""
- headers = Handler.response_headers("path/to/file.iso")
- assert headers["Cache-Control"] == _CACHE_CONTROL
-
-
-@pytest.mark.asyncio
-async def test_serve_content_artifact_redirect_is_not_304(monkeypatch):
- """Object-storage 302s never get a Pulp Last-Modified.
-
- A matching If-Modified-Since must not 304.
- """
- redirect = HTTPFound(
- "http://example.test/redirect",
- headers={"Cache-Control": _CACHE_CONTROL},
- )
- handler, ca, _built = _handler_with_built_response(monkeypatch, built=redirect)
-
- with override_settings(CACHE_ENABLED=False):
- with pytest.raises(HTTPFound) as exc:
- await handler._serve_content_artifact(
- ca,
- {},
- _request(if_modified_since=_IF_MODIFIED_SINCE_AFTER),
- last_modified=_LAST_MODIFIED,
- )
-
- assert "Last-Modified" not in exc.value.headers
- assert "Cache-Control" not in exc.value.headers
-
-
-@pytest.mark.asyncio
-async def test_serve_content_artifact_304_beats_unsatisfiable_range(monkeypatch):
- """A matching If-Modified-Since 304s even when Range would otherwise be 416."""
- handler, ca, _built = _handler_with_built_response(monkeypatch)
- request = _request(if_modified_since=_IF_MODIFIED_SINCE_AFTER, http_range=_UnsatisfiableRange())
-
- with override_settings(CACHE_ENABLED=False):
- with pytest.raises(HTTPNotModified) as exc:
- await handler._serve_content_artifact(ca, {}, request, last_modified=_LAST_MODIFIED)
-
- assert exc.value.status == 304
- assert exc.value.headers["Last-Modified"] == _LAST_MODIFIED_HTTP
-
-
-@pytest.mark.asyncio
-async def test_on_demand_conditional_before_stream(monkeypatch):
- """On-demand units 304 before the remote fetch; otherwise the stream carries Last-Modified."""
- handler = Handler()
- ca = _ca(artifact=False)
- monkeypatch.setattr(handler, "_content_last_modified", AsyncMock(return_value=_LAST_MODIFIED))
- handler._stream_content_artifact = AsyncMock(return_value="streamed")
-
- with pytest.raises(HTTPNotModified) as exc:
- await handler._serve_ca(
- ca,
- {"Cache-Control": _CACHE_CONTROL},
- Mock(headers={"If-Modified-Since": _LAST_MODIFIED_HTTP}),
- repository_version="rv",
- )
- handler._stream_content_artifact.assert_not_awaited()
- assert exc.value.headers["Last-Modified"] == _LAST_MODIFIED_HTTP
- assert exc.value.headers["Cache-Control"] == _CACHE_CONTROL
-
- result = await handler._serve_ca(ca, {}, Mock(headers={}), repository_version="rv")
- assert result == "streamed"
- _, stream_response, stream_ca = handler._stream_content_artifact.call_args.args
- assert stream_ca is ca
- assert stream_response.headers["Last-Modified"] == _LAST_MODIFIED_HTTP
-
-
-@pytest.mark.asyncio
-@pytest.mark.django_db
-async def test_content_last_modified_from_repository_membership():
- """Last-Modified is RepositoryContent.pulp_created for the served version, else omitted."""
- repo = await create_repository()
- content = await create_content()
- other = await create_content()
- publication = None
- try:
- ca = await create_content_artifact(content)
- handler = Handler()
- assert await handler._content_last_modified(ca) is None
-
- v1 = await _add_content_to_new_version(repo, content)
- expected = await sync_to_async(_membership_pulp_created)(v1, content)
- assert await handler._content_last_modified(ca, repository_version=v1) == expected
-
- publication = await sync_to_async(Publication.objects.create)(repository_version=v1)
- assert await handler._content_last_modified(ca, publication=publication) == expected
-
- def _add_other():
- with repo.new_version() as version:
- version.add_content(Content.objects.filter(pk=other.pk))
- return repo.latest_version()
-
- v2 = await sync_to_async(_add_other)()
- assert await handler._content_last_modified(ca, repository_version=v2) == expected
-
- def _remove():
- with repo.new_version() as version:
- version.remove_content(Content.objects.filter(pk=content.pk))
- return repo.latest_version()
-
- v3 = await sync_to_async(_remove)()
- assert await handler._content_last_modified(ca, repository_version=v3) is None
- finally:
- if publication is not None:
- await publication.adelete()
- await repo.adelete()
- await content.adelete()
- await other.adelete()
-
-
@pytest.mark.asyncio
@pytest.mark.django_db
async def test_async_pull_through_add(ca1, monkeypatch, app_status):
diff --git a/pulpcore/tests/unit/models/test_publication_retention.py b/pulpcore/tests/unit/models/test_publication_retention.py
index b5cc0404149..ce9b7470ea4 100644
--- a/pulpcore/tests/unit/models/test_publication_retention.py
+++ b/pulpcore/tests/unit/models/test_publication_retention.py
@@ -348,9 +348,6 @@ def test_returns_ca_when_content_in_publication(self, version_with_content, expe
pub_with_a = pub_factory(version_with_content, pass_through=True)
dist = dist_factory(pub=pub_with_a)
assert dist.get_fallback_ca(self.content_path) == expected_ca
- ca, publication = dist.get_fallback(self.content_path)
- assert ca == expected_ca
- assert publication.pk == pub_with_a.pk
def test_returns_none_when_content_not_in_publication(self, version_without_content):
"""Returns None when the served publication does not contain the content."""
@@ -382,9 +379,6 @@ def test_returns_ca_when_content_only_in_superseded_publication(
pub_without_a = pub_factory(version_without_content, pass_through=True)
update_dist(dist, pub=pub_without_a)
assert dist.get_fallback_ca(self.content_path) == expected_ca
- ca, publication = dist.get_fallback(self.content_path)
- assert ca == expected_ca
- assert publication.pk == pub_with_a.pk
def test_returns_none_when_repository_unset(self, version_with_content, expected_ca):
"""Returns None once the distribution's repository is cleared."""
diff --git a/pulpcore/tests/unit/test_cache.py b/pulpcore/tests/unit/test_cache.py
index 3cb01f9110f..6da69732e07 100644
--- a/pulpcore/tests/unit/test_cache.py
+++ b/pulpcore/tests/unit/test_cache.py
@@ -1,17 +1,9 @@
-import json
from time import sleep
-from time import time as now
-from unittest.mock import AsyncMock, Mock
import pytest
-from aiohttp.web import Response
-from aiohttp.web_exceptions import HTTPNotModified
-from django.test import override_settings
-from django.utils.http import http_date
import pulpcore.app.redis_connection
from pulpcore.cache import Cache
-from pulpcore.cache.cache import AsyncContentCache
@pytest.fixture
@@ -115,187 +107,3 @@ def test_clear(pulp_redisdb):
cache.redis.flushdb()
for key, _, base_key in tuples:
assert not cache.exists(key, base_key=base_key)
-
-
-def _request_with_if_modified_since(value):
- return Mock(headers={"If-Modified-Since": value} if value else {})
-
-
-_LM = http_date(1_000_000_000)
-
-
-def test_async_content_cache_not_modified():
- """If-Modified-Since is compared to Last-Modified at second resolution."""
- newer = http_date(1_000_000_060)
- older = http_date(999_999_940)
- future = http_date(now() + 86400)
- inm = Mock(headers={"If-Modified-Since": _LM, "If-None-Match": '"abc"'})
-
- assert AsyncContentCache._not_modified(_request_with_if_modified_since(_LM), _LM) is True
- assert AsyncContentCache._not_modified(_request_with_if_modified_since(newer), _LM) is True
- assert AsyncContentCache._not_modified(_request_with_if_modified_since(older), _LM) is False
- assert AsyncContentCache._not_modified(_request_with_if_modified_since(None), _LM) is False
- assert AsyncContentCache._not_modified(_request_with_if_modified_since(_LM), None) is False
- assert AsyncContentCache._not_modified(_request_with_if_modified_since("garbage"), _LM) is False
- assert AsyncContentCache._not_modified(inm, _LM) is False
- assert AsyncContentCache._not_modified(_request_with_if_modified_since(future), _LM) is False
-
-
-def test_async_content_cache_make_not_modified_echoes_metadata():
- """The 304 carries only validator/caching metadata already present on the source."""
- source = {
- "Cache-Control": "public, max-age=0, must-revalidate",
- "Content-Length": "1024",
- "X-PULP-CACHE": "HIT",
- }
-
- exc = AsyncContentCache._make_not_modified(source, _LM)
-
- assert isinstance(exc, HTTPNotModified)
- assert exc.headers["Last-Modified"] == _LM
- assert exc.headers["Cache-Control"] == "public, max-age=0, must-revalidate"
- assert exc.headers["X-PULP-CACHE"] == "HIT"
- assert "Content-Length" not in exc.headers
-
- bare = AsyncContentCache._make_not_modified({}, _LM)
- assert "X-PULP-CACHE" not in bare.headers
- assert "Cache-Control" not in bare.headers
-
-
-def test_async_content_cache_build_response_pops_last_modified():
- """build_response must not pass the stored last_modified field to the response constructor."""
- cache = AsyncContentCache.__new__(AsyncContentCache)
- entry = {
- "type": "Response",
- "status": 200,
- "headers": {"Last-Modified": _LM},
- "last_modified": _LM,
- "body": b"hello".hex(),
- }
-
- response = cache.build_response(entry)
-
- assert response.status == 200
- assert response.body == b"hello"
- assert response.headers["Last-Modified"] == _LM
- assert response.headers["X-PULP-CACHE"] == "HIT"
-
-
-def _entry(*, store_field=True):
- entry = {
- "type": "Response",
- "status": 200,
- "headers": {
- "Last-Modified": _LM,
- "Cache-Control": "public, max-age=0, must-revalidate",
- },
- "body": b"payload".hex(),
- "expires": None,
- }
- if store_field:
- entry["last_modified"] = _LM
- return entry
-
-
-def _cache():
- cache = AsyncContentCache.__new__(AsyncContentCache)
- cache.auth = None
- cache.default_base_key = "base"
- cache.keys = ()
- cache.default_expires_ttl = 60
- cache.get_request_from_args = lambda args: args[0]
- cache.make_key = lambda req: "key"
- return cache
-
-
-async def _run_cached(cache, request, handler=None):
- if handler is None:
-
- async def handler(req):
- raise AssertionError("handler must not run")
-
- with override_settings(CACHE_ENABLED=True):
- return await AsyncContentCache.__call__(cache, handler)(request)
-
-
-@pytest.mark.asyncio
-@pytest.mark.parametrize("store_field", [True, False], ids=["stored-field", "header-fallback"])
-async def test_cache_hit_304_does_not_rebuild_response(store_field):
- """A matching If-Modified-Since 304s without reconstructing the cached response."""
- cache = _cache()
- cache.get_entry = AsyncMock(return_value=_entry(store_field=store_field))
- cache.build_response = Mock(side_effect=AssertionError("must not reconstruct"))
-
- with pytest.raises(HTTPNotModified) as exc:
- await _run_cached(cache, Mock(headers={"If-Modified-Since": _LM}))
-
- cache.build_response.assert_not_called()
- assert exc.value.headers["Last-Modified"] == _LM
- assert exc.value.headers["X-PULP-CACHE"] == "HIT"
-
-
-@pytest.mark.asyncio
-async def test_cache_hit_stale_if_modified_since_rebuilds_response():
- """An older If-Modified-Since on a cache hit still reconstructs the full cached response."""
- entry = _entry()
- rebuilt = Mock(headers={"X-PULP-ARTIFACT-SIZE": None})
- cache = _cache()
- cache.get_entry = AsyncMock(return_value=entry)
- cache.build_response = Mock(return_value=rebuilt)
-
- response = await _run_cached(cache, Mock(headers={"If-Modified-Since": http_date(999_999_000)}))
-
- cache.build_response.assert_called_once_with(entry)
- assert response is rebuilt
-
-
-@pytest.mark.asyncio
-async def test_cache_miss_does_not_304_prepared_stream():
- """A live stream that already started writing must not be converted into a 304."""
- stream = Mock(headers={"Last-Modified": _LM}, prepared=True, status=200)
- cache = _cache()
- cache.get_entry = AsyncMock(return_value=None)
- cache.make_entry = AsyncMock(return_value=stream)
-
- async def handler(req):
- raise AssertionError("handler is invoked via make_entry")
-
- assert await _run_cached(cache, Mock(headers={"If-Modified-Since": _LM}), handler) is stream
-
-
-@pytest.mark.asyncio
-async def test_make_entry_does_not_cache_304():
- """HTTPNotModified is HTTPSuccessful but must never be written to Redis."""
- cache = _cache()
- cache.set = AsyncMock()
-
- async def handler():
- raise HTTPNotModified(headers={"Last-Modified": _LM})
-
- with pytest.raises(HTTPNotModified):
- await cache.make_entry("k", "b", handler, (), {}, 60)
-
- cache.set.assert_not_called()
-
-
-@pytest.mark.asyncio
-async def test_make_entry_stores_last_modified():
- """A 200 with Last-Modified is stored so later cache hits can 304 without rebuilding."""
- captured = {}
- cache = _cache()
-
- async def fake_set(key, value, expires=None, base_key=None):
- captured["entry"] = json.loads(value)
-
- cache.set = fake_set
-
- async def handler():
- return Response(body=b"hello", headers={"Last-Modified": _LM})
-
- result = await cache.make_entry("k", "b", handler, (), {}, 60)
-
- assert result.headers["Last-Modified"] == _LM
- assert result.headers["X-PULP-CACHE"] == "MISS"
- assert captured["entry"]["last_modified"] == _LM
- assert captured["entry"]["headers"]["Last-Modified"] == _LM
- assert captured["entry"]["type"] == "Response"
diff --git a/pulpcore/tests/unit/test_responses.py b/pulpcore/tests/unit/test_responses.py
deleted file mode 100644
index 2c4d74ecf6e..00000000000
--- a/pulpcore/tests/unit/test_responses.py
+++ /dev/null
@@ -1,113 +0,0 @@
-import os
-from datetime import datetime, timezone
-
-import pytest
-from aiohttp.test_utils import make_mocked_request
-from aiohttp.web import FileResponse
-from django.utils.http import http_date
-
-from pulpcore.responses import PulpFileResponse
-
-# _make_response / _FileResponseResult exist only on aiohttp 3.11+. Lowerbounds installs 3.10.
-_SKIP_MAKE_RESPONSE = pytest.mark.skipif(
- not hasattr(FileResponse, "_make_response"),
- reason="aiohttp FileResponse._make_response requires aiohttp 3.11+",
-)
-
-_PULP_LM = http_date(datetime(2024, 6, 1, tzinfo=timezone.utc).timestamp())
-# 2001-09-09; If-Modified-Since between this and _PULP_LM is the interesting case
-_FILE_MTIME = 1_000_000_000
-_IF_MODIFIED_SINCE_AFTER_MTIME = http_date(datetime(2022, 1, 1, tzinfo=timezone.utc).timestamp())
-
-
-def _artifact(tmp_path):
- path = tmp_path / "artifact"
- path.write_bytes(b"payload")
- os.utime(path, (_FILE_MTIME, _FILE_MTIME))
- return path
-
-
-@pytest.mark.parametrize("with_handler_lm", [True, False], ids=["handler-lm", "no-lm"])
-def test_pulp_file_response_ignores_file_mtime(tmp_path, with_handler_lm):
- """aiohttp's file-mtime assignment must not advertise a filesystem Last-Modified."""
- headers = {"Last-Modified": _PULP_LM} if with_handler_lm else None
- response = PulpFileResponse(_artifact(tmp_path), headers=headers)
- response.last_modified = 2_000_000_000
- if with_handler_lm:
- assert response.headers["Last-Modified"] == _PULP_LM
- else:
- assert "Last-Modified" not in response.headers
-
-
-@pytest.mark.parametrize("with_handler_lm", [True, False], ids=["handler-lm", "no-lm"])
-def test_pulp_file_response_never_emits_mtime_etag(tmp_path, with_handler_lm):
- """mtime ETags are not advertised, with or without a Pulp Last-Modified."""
- headers = {"Last-Modified": _PULP_LM} if with_handler_lm else None
- response = PulpFileResponse(_artifact(tmp_path), headers=headers)
- response.etag = "abc123"
- assert "ETag" not in response.headers
-
-
-@_SKIP_MAKE_RESPONSE
-@pytest.mark.parametrize("with_handler_lm", [True, False], ids=["handler-lm", "no-lm"])
-def test_pulp_file_response_does_not_304_on_file_mtime(tmp_path, with_handler_lm):
- """If-Modified-Since after file mtime must not 304; stock FileResponse would."""
- from aiohttp.web_fileresponse import _FileResponseResult
-
- path = _artifact(tmp_path)
- headers = {"Last-Modified": _PULP_LM} if with_handler_lm else None
- request = make_mocked_request(
- "GET", "/", headers={"If-Modified-Since": _IF_MODIFIED_SINCE_AFTER_MTIME}
- )
-
- pulp = PulpFileResponse(str(path), headers=headers)
- result, fobj, _st, _enc = pulp._make_response(request, "")
- try:
- assert result is _FileResponseResult.SEND_FILE
- finally:
- if fobj:
- fobj.close()
- if with_handler_lm:
- assert pulp.headers["Last-Modified"] == _PULP_LM
- else:
- assert "Last-Modified" not in pulp.headers
-
- stock = FileResponse(str(path))
- result, fobj, _st, _enc = stock._make_response(
- make_mocked_request(
- "GET", "/", headers={"If-Modified-Since": _IF_MODIFIED_SINCE_AFTER_MTIME}
- ),
- "",
- )
- try:
- assert result is _FileResponseResult.NOT_MODIFIED
- finally:
- if fobj:
- fobj.close()
-
-
-@_SKIP_MAKE_RESPONSE
-def test_pulp_file_response_does_not_blank_if_range(tmp_path):
- """If-Range stays available so aiohttp can refuse a stale Range instead of a corrupt 206."""
- from aiohttp.web_fileresponse import _FileResponseResult
-
- if_range = http_date(_FILE_MTIME)
- request = make_mocked_request(
- "GET",
- "/",
- headers={
- "If-Range": if_range,
- "Range": "bytes=0-1",
- "If-Modified-Since": if_range,
- },
- )
- response = PulpFileResponse(str(_artifact(tmp_path)), headers={"Last-Modified": _PULP_LM})
- result, fobj, _st, _enc = response._make_response(request, "")
- try:
- assert result is _FileResponseResult.SEND_FILE
- finally:
- if fobj:
- fobj.close()
-
- assert request.if_range is not None
- assert request.if_modified_since is None