fix(web): never 304 a rewritten page — pages drop conditional validators, assets keep them

2026-08-31 01:35:28 -04:00
parent 9518d9d5d1
commit c564e317ed
6 changed files with 742 additions and 81 deletions
@@ -0,0 +1,200 @@
+"""Integration: the phase-54 revalidation contract against the REAL app.
+
+Phase 33's two cache-busting layers (``asset_version()`` +
+``CachingMiddleware``) rewrite the known HTML pages to carry
+``?v=<token>`` asset references — but the ``etag`` / ``last-modified``
+validators Starlette publishes for a page describe the STATIC FILE, not
+the rewritten body this process built from its own token. A conditional
+GET that matched those validators used to 304 out of the rewrite: the
+browser kept the HTML it already had, whose ``?v=`` pinned the PREVIOUS
+commit's CSS/JS — cached ``immutable`` for a year. This suite pins the
+fix end-to-end (real ``app.main:app``, real ``StaticFiles`` mount on the
+real ``frontend/`` tree, real ``asset_version()`` token — no mocks):
+
+* every known page (``HTML_PAGES``) AND the dynamic ``/shared/<token>``
+  page 200s on a conditional GET, always with the current
+  ``?v=<token>`` body and no validators;
+* ``/assets/*`` is untouched: ``immutable`` for a year, validators
+  intact, a conditional GET on the versioned URL still 304s (that 304
+  is safe — the URL itself carries the version);
+* ``/api/*`` stays byte-identical: no ``cache-control`` injected, no
+  validators, conditional headers pass through (the SSE chat stream's
+  pass-through is pinned by ``test_chat_api.py`` — the regression run
+  below).
+
+Requires:  podman compose up -d db
+"""
+from __future__ import annotations
+
+import os
+import re
+from collections.abc import Iterator
+from pathlib import Path
+
+import httpx
+import pytest
+from fastapi.testclient import TestClient
+from sqlalchemy import text
+from sqlalchemy.orm import Session
+
+from app.core.caching import (
+    ASSET_CACHE_CONTROL,
+    HTML_CACHE_CONTROL,
+    HTML_PAGES,
+    asset_version,
+)
+
+REPO = Path(__file__).resolve().parents[2]
+FRONTEND = REPO / "frontend"
+
+
+def _token_ref_re(token: str) -> re.Pattern[str]:
+    """A local ``assets/…`` href/src reference that already carries
+    ``?v=<token>`` (the middleware's rewrite output)."""
+    return re.compile(r'(?:src|href)="(?:/)?assets/[^"?#]*\?v=' + re.escape(token) + r'"')
+
+
+def file_validators(page_file: Path) -> tuple[str, str]:
+    """The etag / last-modified Starlette would stamp on the underlying
+    static file — exactly what a browser revalidates against.
+
+    ``stat_result`` is passed up front: starlette 1.x's ``FileResponse``
+    defers the stat to ``__call__``, so without it the headers carry no
+    validators yet (same pattern as the unit suite's ``_file_validators``).
+    """
+    from starlette.responses import FileResponse
+
+    headers = FileResponse(page_file, stat_result=os.stat(page_file)).headers
+    return headers["etag"], headers["last-modified"]
+
+
+def _page_file(path: str) -> Path:
+    """The static file backing a page path (``/`` → ``index.html``)."""
+    name = path.lstrip("/") or "index.html"
+    file = FRONTEND / name
+    assert file.is_file(), f"missing page file for {path}: {file}"
+    return file
+
+
+def _assert_page_contract(response: httpx.Response, token: str) -> None:
+    """The phase-54 page contract on any known page: a full 200 with the
+    CURRENT process token on the asset refs, ``Cache-Control: no-cache``,
+    and NO validators (a page must never be revalidated against a
+    validator this process published)."""
+    assert response.status_code == 200, (
+        f"a page path must never 304 (got {response.status_code})"
+    )
+    assert response.headers["cache-control"] == HTML_CACHE_CONTROL
+    assert "etag" not in response.headers
+    assert "last-modified" not in response.headers
+    assert f"?v={token}" in response.text
+    assert _token_ref_re(token).search(response.text), (
+        "no local assets/ ref carries ?v=<current token>"
+    )
+
+
+@pytest.fixture(autouse=True)
+def clean_chats(db: Session) -> Iterator[None]:
+    """``saved_chats`` is global state — the share test writes one row
+    (house pattern from ``test_chats_api.py``)."""
+    db.execute(text("TRUNCATE saved_chats"))
+    db.commit()
+    yield
+    db.execute(text("TRUNCATE saved_chats"))
+    db.commit()
+
+
+def test_every_known_page_200s_on_conditional_get(client: TestClient) -> None:
+    """THE phase-54 regression, real app: for EVERY known page, a plain
+    GET sets the contract, then a conditional GET carrying the static
+    FILE's etag (what a browser captured pre-fix) must still 200 with
+    the SAME rewritten body — pre-fix the loop failed on the very first
+    304, pinning the browser on the previous commit's immutable assets."""
+    token = asset_version()
+    for path in HTML_PAGES:
+        plain = client.get(path)
+        _assert_page_contract(plain, token)
+
+        etag, _ = file_validators(_page_file(path))
+        conditional = client.get(path, headers={"if-none-match": etag})
+        _assert_page_contract(conditional, token)
+        assert conditional.content == plain.content, (
+            f"{path}: the conditional 200 must serve the same rewritten body"
+        )
+
+
+def test_dynamic_shared_page_200s_on_conditional_get(admin_client: TestClient) -> None:
+    """The dynamic /shared/<token> page (phase 51) gets the same contract
+    via the real save+share flow: a conditional GET carrying the
+    ``shared.html`` file's etag must still 200 with the rewritten body —
+    the route's ``FileResponse`` honours conditional headers, so without
+    the inbound strip this page 304'd too."""
+    token = asset_version()
+    created = admin_client.post(
+        "/api/chats",
+        json={
+            "messages": [
+                {"who": "user", "text": "How did I install gitlab?"},
+                {"who": "brain", "text": "You've got this!"},
+            ],
+            "share": True,
+        },
+    )
+    assert created.status_code == 201
+    share_url = created.json()["share_url"]
+
+    plain = admin_client.get(share_url)
+    _assert_page_contract(plain, token)
+
+    etag, _ = file_validators(FRONTEND / "shared.html")
+    conditional = admin_client.get(share_url, headers={"if-none-match": etag})
+    _assert_page_contract(conditional, token)
+    assert conditional.content == plain.content
+
+
+def test_assets_keep_immutable_validators_and_304(client: TestClient) -> None:
+    """The inbound strip must NOT have widened to /assets/*: the versioned
+    asset URL keeps its validators and still 304s on a conditional GET —
+    that 304 is safe because the URL itself carries ?v=<token>."""
+    token = asset_version()
+    url = f"/assets/styles.css?v={token}"
+
+    plain = client.get(url)
+    assert plain.status_code == 200
+    assert plain.headers["cache-control"] == ASSET_CACHE_CONTROL
+    assert "etag" in plain.headers
+    assert "last-modified" in plain.headers
+
+    conditional = client.get(url, headers={"if-none-match": plain.headers["etag"]})
+    assert conditional.status_code == 304  # versioned-URL 304s stay safe
+    assert conditional.content == b""
+    assert conditional.headers["cache-control"] == ASSET_CACHE_CONTROL
+
+
+def test_api_paths_get_no_cache_headers_and_untouched_stream(client: TestClient) -> None:
+    """/api/* stays byte-identical: no cache-control injected, no
+    validators published, and conditional headers pass through to the
+    route untouched (the SSE chat stream's pass-through is pinned by
+    ``tests/integration/test_chat_api.py`` — the regression run below)."""
+    plain = client.get("/api/health")
+    assert plain.status_code == 200
+    assert "cache-control" not in plain.headers
+    assert "etag" not in plain.headers
+    assert "last-modified" not in plain.headers
+
+    conditional = client.get("/api/health", headers={"if-none-match": "x"})
+    assert conditional.status_code == 200  # pass-through — the strip is page-scoped
+    assert conditional.json() == plain.json()
+    assert "cache-control" not in conditional.headers
+
+
+def test_page_token_matches_process_token(client: TestClient) -> None:
+    """The token embedded in the served page equals ``asset_version()`` —
+    the per-process ``functools.cache`` contract: one page load can never
+    mix two versions (phase 54, assumption 5)."""
+    token = asset_version()
+    response = client.get("/")
+    assert response.status_code == 200
+    match = re.search(r'styles\.css\?v=([^"]+)"', response.text)
+    assert match is not None, "styles.css ref not found in the served page"
+    assert match.group(1) == token