diff --git a/changelog.d/328.changed.md b/changelog.d/328.changed.md new file mode 100644 index 0000000..cc6efdb --- /dev/null +++ b/changelog.d/328.changed.md @@ -0,0 +1,4 @@ +**`@cache(no_store=True)` warns about the arguments it overrides.** A +`UserWarning` naming them is emitted when the decorator is applied with +`no_store=True` and `ttl`, `stale`, `no_cache`, `public`, `private`, +`immutable` or `must_revalidate`. The response is unchanged: `no-store` only. diff --git a/changelog.d/363.fixed.md b/changelog.d/363.fixed.md new file mode 100644 index 0000000..4483e86 --- /dev/null +++ b/changelog.d/363.fixed.md @@ -0,0 +1,5 @@ +**A bare `@cache()` no longer sends an empty `Cache-Control` header.** With +neither `ttl` nor a directive, the decorator has no directive of its own: it +keeps the handler's `Cache-Control`, or sends none, and adds only the ETag. A +response that sets a cookie or answers a request with credentials is still +sent with `private`. diff --git a/docs/CACHE_FLOW.md b/docs/CACHE_FLOW.md index b43e0dd..148380d 100644 --- a/docs/CACHE_FLOW.md +++ b/docs/CACHE_FLOW.md @@ -163,6 +163,7 @@ The header value is built once per decorated route: |-----------|----------------------| | `no_store=True` | `no-store` (overrides everything else) | | `no_cache=True` | `no-cache`, plus `must-revalidate` if requested; `public`/`private`/`max-age`/`stale-*`/`immutable` are omitted | +| none (a bare `@cache()`) | none of its own; the handler's header, if any, is kept (`private` for a cookie or credentials, as on every route) | | anything else | in order: `public` or `private`, `max-age=`, `must-revalidate`, `stale-while-revalidate=` or `stale-if-error=`, `immutable` | > [!NOTE] diff --git a/docs/HTTP_CACHING.md b/docs/HTTP_CACHING.md index d808bf7..8b14868 100644 --- a/docs/HTTP_CACHING.md +++ b/docs/HTTP_CACHING.md @@ -78,6 +78,17 @@ header, and the server-side cache behaves the same with or without them. `no_store` only `no-store`. How the other arguments combine is described in [Cache flow](CACHE_FLOW.md#2-cache-control-directives). +`no_store=True` together with any other caching argument (`ttl`, `stale`, +`no_cache`, `public`, `private`, `immutable`, `must_revalidate`) emits a +`UserWarning` pointing at your `@cache(...)` line when the decorator is +applied: `no_store` overrides them, and the warning names them. + +A bare `@cache()`, with neither `ttl` nor a directive, stores nothing and has no +`Cache-Control` of its own. It adds an ETag and answers a matching +`If-None-Match` with `304`, and keeps the handler's own `Cache-Control` (or +sends none). As on every route, a response that sets a cookie or answers a +request with `Authorization` or a session is still sent with `private`. + ### The request's `Cache-Control` is ignored A client's own `Cache-Control` request header (`no-cache`, `max-age=0`, as sent diff --git a/fastapi_cachex/cache.py b/fastapi_cachex/cache.py index 36356c3..21699bb 100644 --- a/fastapi_cachex/cache.py +++ b/fastapi_cachex/cache.py @@ -259,6 +259,14 @@ def _request_credential(request: Request) -> str | None: ) +def _no_store_ignored_warning(ignored: list[str]) -> str: + return ( + f"cache no_store ignores {', '.join(ignored)}: @cache(no_store=True) " + "stores nothing and sends only Cache-Control: no-store, so the other " + "arguments have no effect. Remove them, or drop no_store." + ) + + # How the one-time bypass warning names each `_request_credential` result. _CREDENTIAL_DESCRIPTIONS = { "Authorization header": "an Authorization header", @@ -739,7 +747,8 @@ def _not_modified( response = Response(status_code=HTTP_304_NOT_MODIFIED) _append_headers(response, _revalidation_headers(headers)) response.headers["ETag"] = etag - response.headers["Cache-Control"] = cache_control + if cache_control: + response.headers["Cache-Control"] = cache_control response.headers.update(age or {}) return response @@ -781,12 +790,16 @@ def _cache_control_for( A handler that marked its response ``private`` or ``no-store`` keeps its own header; the decorator's would widen what the handler allowed. A response that sets a cookie gets ``private_cache_control``, so a shared cache in - front of the app does not store it either. + front of the app does not store it either. When the decorator has no + directive to send (a bare ``@cache()``), the handler's own header is kept, + or none is sent (#363); a cookie still gets ``private_cache_control``. """ if _marked_unshareable(response): return ", ".join(response.headers.getlist("cache-control")) if "set-cookie" in response.headers: return private_cache_control + if not cache_control: + return ", ".join(response.headers.getlist("cache-control")) return cache_control @@ -794,9 +807,9 @@ def _with_cache_control( response: Response, cache_control: str, private_cache_control: str ) -> Response: if not _marked_unshareable(response): - response.headers["Cache-Control"] = _cache_control_for( - response, cache_control, private_cache_control - ) + value = _cache_control_for(response, cache_control, private_cache_control) + if value: + response.headers["Cache-Control"] = value return response @@ -1062,6 +1075,17 @@ def cache( ``key_builder``, or if ``key_builder`` is an ``async`` callable. At request time, if ``key_builder`` returns anything but a ``str``. + + Without ``ttl`` or any directive (a bare ``@cache()``), nothing is stored + and the decorator has no ``Cache-Control`` of its own: it adds an ETag, + answers a matching ``If-None-Match`` with 304 and keeps the handler's own + ``Cache-Control``, except that a response setting a cookie or answering a + request with credentials is still sent with ``private``. + + Warns: + UserWarning: When the decorator is applied, if ``vary`` lists + ``Cookie``, or if ``no_store`` is combined with another caching + argument it overrides. """ def decorator(func: HandlerCallable) -> AsyncResponseCallable: @@ -1092,6 +1116,27 @@ def decorator(func: HandlerCallable) -> AsyncResponseCallable: # stacklevel=2: the caller applying the decorator, i.e. the line # of the user's @cache(...). warnings.warn(_COOKIE_VARY_WARNING, UserWarning, stacklevel=2) + # Arguments no_store overrides are accepted but warned about (#328); + # rejecting them would break routes that work today. + if no_store: + ignored = [ + name + for name, value in ( + ("ttl", ttl), + ("stale", stale), + ("stale_ttl", stale_ttl), + ("no_cache", no_cache), + ("public", public), + ("private", private), + ("immutable", immutable), + ("must_revalidate", must_revalidate), + ) + if value is not None and value is not False + ] + if ignored: + warnings.warn( + _no_store_ignored_warning(ignored), UserWarning, stacklevel=2 + ) # Analyze the original function's signature sig: Signature = inspect.signature(func) diff --git a/i18n/zh-TW/docs/CACHE_FLOW.md b/i18n/zh-TW/docs/CACHE_FLOW.md index efc0a1a..0b97fbc 100644 --- a/i18n/zh-TW/docs/CACHE_FLOW.md +++ b/i18n/zh-TW/docs/CACHE_FLOW.md @@ -126,6 +126,7 @@ cache_key = "|".join( |------|------------------------| | `no_store=True` | `no-store`(覆蓋其他所有設定) | | `no_cache=True` | `no-cache`,若有要求則加上 `must-revalidate`;省略 `public`/`private`/`max-age`/`stale-*`/`immutable` | +| 無(不帶參數的 `@cache()`) | 不送出自己的標頭;保留 handler 的標頭(若有)(遇到 Cookie 或憑證時送出 `private`,與其他路由相同) | | 其他情況 | 依序為:`public` 或 `private`、`max-age=`、`must-revalidate`、`stale-while-revalidate=` 或 `stale-if-error=`、`immutable` | > [!NOTE] diff --git a/i18n/zh-TW/docs/HTTP_CACHING.md b/i18n/zh-TW/docs/HTTP_CACHING.md index 0c12382..6aa2217 100644 --- a/i18n/zh-TW/docs/HTTP_CACHING.md +++ b/i18n/zh-TW/docs/HTTP_CACHING.md @@ -57,6 +57,10 @@ async def items(): ... `no_cache=True` 與 `no_store=True` 會取代標頭的其餘內容:使用 `no_cache` 時只送出 `no-cache`(若有設定,再加上 `must-revalidate`),使用 `no_store` 時只送出 `no-store`。其他參數如何組合,請見[快取流程](CACHE_FLOW.md#2-cache-control-directives)。 +`no_store=True` 搭配其他任何快取參數(`ttl`、`stale`、`no_cache`、`public`、`private`、`immutable`、`must_revalidate`)時,套用裝飾器時 `@cache` 會發出指向你 `@cache(...)` 那一行的 `UserWarning`:`no_store` 會覆蓋這些參數,警告會列出它們。 + +不帶參數的 `@cache()`,既沒有 `ttl` 也沒有任何指令,不會儲存任何內容,也沒有自己的 `Cache-Control`。它會加上 ETag,以 `304` 回應相符的 `If-None-Match`,並保留 handler 自己的 `Cache-Control`(沒有就不送)。與其他路由一樣,設定 Cookie 的回應,或回應帶有 `Authorization` 或 Session 的請求時,仍會送出 `private`。 + ### 請求的 `Cache-Control` 會被忽略 {#the-requests-cache-control-is-ignored} 用戶端自己送出的 `Cache-Control` 請求標頭(例如瀏覽器強制重新整理時送出的 `no-cache`、`max-age=0` 等)不會改變 `@cache` 的行為。這是刻意的設計:若請求標頭能繞過快取,任何用戶端都能讓每個請求直接打到你的 handler。條件式請求仍會處理:`If-None-Match` 相符時回 304。 diff --git a/tests/test_cache.py b/tests/test_cache.py index dbbf77b..26d4028 100644 --- a/tests/test_cache.py +++ b/tests/test_cache.py @@ -34,7 +34,7 @@ async def default_cache_endpoint(): response = client.get("/default") assert response.status_code == 200 - assert response.headers["Cache-Control"] == "" + assert "Cache-Control" not in response.headers assert "ETag" in response.headers @@ -627,10 +627,12 @@ def test_no_store_and_no_cache_combined(): """no_store=True takes priority; only 'no-store' appears in Cache-Control.""" combo_app = FastAPI() - @combo_app.get("/no-store-no-cache") - @cache(no_store=True, no_cache=True) - async def no_store_no_cache_endpoint(): - return Response(content=b"data", media_type="text/plain") + with pytest.warns(UserWarning, match="no_store ignores no_cache"): + + @combo_app.get("/no-store-no-cache") + @cache(no_store=True, no_cache=True) + async def no_store_no_cache_endpoint(): + return Response(content=b"data", media_type="text/plain") combo_client = TestClient(combo_app) r = combo_client.get("/no-store-no-cache") @@ -640,7 +642,7 @@ async def no_store_no_cache_endpoint(): assert "no-cache" not in cc -@pytest.mark.parametrize(("ttl", "cache_control"), [(None, ""), (0, "max-age=0")]) +@pytest.mark.parametrize(("ttl", "cache_control"), [(None, None), (0, "max-age=0")]) def test_without_a_positive_ttl_nothing_is_stored_or_served(ttl, cache_control): """No positive ttl means no server-side cache, and no 304 from a stale ETag (#110). @@ -664,14 +666,14 @@ async def no_ttl_endpoint(): r1 = no_ttl_client.get("/no-ttl") assert r1.status_code == 200 - assert r1.headers["Cache-Control"] == cache_control + assert r1.headers.get("Cache-Control") == cache_control old_etag = r1.headers["ETag"] assert backend.cache == {} # An unchanged response still revalidates, against the fresh render. r2 = no_ttl_client.get("/no-ttl", headers={"If-None-Match": old_etag}) assert r2.status_code == 304 - assert r2.headers["Cache-Control"] == cache_control + assert r2.headers.get("Cache-Control") == cache_control assert len(calls) == 2 # The data changes: the old validator must not get a 304 any more. diff --git a/tests/test_cache_age.py b/tests/test_cache_age.py index d89d371..feb49e6 100644 --- a/tests/test_cache_age.py +++ b/tests/test_cache_age.py @@ -175,6 +175,7 @@ async def test_no_cache_revalidation_sends_no_age(clock, client_for): assert "age" not in full.headers +@pytest.mark.filterwarnings("ignore:cache no_store ignores:UserWarning") @pytest.mark.parametrize( ("cache_kwargs", "method", "headers"), [ diff --git a/tests/test_cache_arguments.py b/tests/test_cache_arguments.py new file mode 100644 index 0000000..7f1b819 --- /dev/null +++ b/tests/test_cache_arguments.py @@ -0,0 +1,157 @@ +"""`@cache` arguments that send no directive or contradict each other (#328, #363). + +A bare `@cache()` has no directive to send: it used to set an empty +`Cache-Control` header, replacing the handler's own one. It now keeps the +handler's header and only adds the ETag, which makes it an ETag-only mode, so +it does not warn. `no_store` overrides every other caching argument, so passing +one with it warns. +""" + +from collections.abc import AsyncIterator + +import pytest +from fastapi import FastAPI +from fastapi import Response +from fastapi.responses import StreamingResponse +from fastapi.testclient import TestClient + +from fastapi_cachex.cache import cache + + +def _bare_app(handler_cache_control: str | None = None, **kwargs: object) -> TestClient: + app = FastAPI() + + @app.get("/bare") + @cache(**kwargs) # type: ignore[arg-type] + async def bare(response: Response): + if handler_cache_control is not None: + response.headers["Cache-Control"] = handler_cache_control + return {"ok": True} + + return TestClient(app) + + +def test_bare_cache_sends_no_cache_control() -> None: + client = _bare_app() + + first = client.get("/bare") + assert "Cache-Control" not in first.headers + assert "ETag" in first.headers + + revalidated = client.get("/bare", headers={"If-None-Match": first.headers["ETag"]}) + assert revalidated.status_code == 304 + assert "Cache-Control" not in revalidated.headers + + +@pytest.mark.parametrize("handler_header", ["max-age=30", "private, max-age=30"]) +def test_bare_cache_keeps_the_handlers_cache_control(handler_header: str) -> None: + client = _bare_app(handler_header) + + first = client.get("/bare") + assert first.headers["Cache-Control"] == handler_header + + revalidated = client.get("/bare", headers={"If-None-Match": first.headers["ETag"]}) + assert revalidated.status_code == 304 + assert revalidated.headers["Cache-Control"] == handler_header + + +def test_bare_cache_with_vary_sends_vary_and_no_cache_control() -> None: + response = _bare_app(vary=["Accept-Language"]).get("/bare") + + assert "Cache-Control" not in response.headers + assert response.headers["Vary"] == "Accept-Language" + + +def test_bare_cache_streaming_response_gets_no_cache_control() -> None: + app = FastAPI() + + async def chunks() -> AsyncIterator[bytes]: + yield b"x" + + @app.get("/stream") + @cache() + async def stream(): + return StreamingResponse(chunks(), media_type="text/plain") + + assert "Cache-Control" not in TestClient(app).get("/stream").headers + + +def test_bare_cache_still_sends_private_for_a_cookie() -> None: + app = FastAPI() + + @app.get("/cookie") + @cache() + async def cookie(response: Response): + response.set_cookie("c", "1") + response.headers["Cache-Control"] = "max-age=30" + return {} + + # As on every route: a response that sets a cookie is sent with the + # decorator's private header, in place of the handler's own one. + assert TestClient(app).get("/cookie").headers["Cache-Control"] == "private" + + +def test_bare_cache_still_sends_private_for_a_credential_request() -> None: + response = _bare_app("max-age=30").get( + "/bare", headers={"Authorization": "Bearer t"} + ) + + assert response.headers["Cache-Control"] == "private" + + +@pytest.mark.parametrize( + ("kwargs", "ignored"), + [ + ({"ttl": 60}, "ttl"), + ({"ttl": 0}, "ttl"), + ({"stale": "revalidate", "stale_ttl": 30}, "stale, stale_ttl"), + ({"no_cache": True}, "no_cache"), + ({"public": True}, "public"), + ({"private": True}, "private"), + ({"immutable": True}, "immutable"), + ({"must_revalidate": True}, "must_revalidate"), + ({"ttl": 60, "public": True}, "ttl, public"), + ], +) +def test_no_store_warns_about_the_arguments_it_overrides( + kwargs: dict[str, object], ignored: str +) -> None: + app = FastAPI() + + with pytest.warns( + UserWarning, match=f"cache no_store ignores {ignored}:" + ) as record: + + @app.get("/none") + @cache(no_store=True, **kwargs) # type: ignore[arg-type] + async def none(): + return {} + + assert len(record) == 1 + assert record[0].filename == __file__ + # The warning changes nothing at request time. + assert TestClient(app).get("/none").headers["Cache-Control"] == "no-store" + + +@pytest.mark.parametrize( + "kwargs", + [ + {"no_store": True}, + {"ttl": 60}, + {"ttl": 0}, + {"public": True}, + {"no_cache": True}, + {"must_revalidate": True}, + {"private": True}, + {"immutable": True}, + {"stale": "revalidate", "stale_ttl": 30}, + {}, + {"vary": ["Accept-Language"]}, + {"ttl": 60, "public": False, "no_cache": False}, + ], +) +def test_arguments_that_do_something_do_not_warn(kwargs: dict[str, object]) -> None: + # filterwarnings = error turns any warning into a failure here. + @cache(**kwargs) # type: ignore[arg-type] + async def handler(): + return {} diff --git a/tests/test_cache_internals.py b/tests/test_cache_internals.py index 8d2ba2f..83b3def 100644 --- a/tests/test_cache_internals.py +++ b/tests/test_cache_internals.py @@ -27,6 +27,7 @@ async def item() -> dict[str, str]: return TestClient(app), calls +@pytest.mark.filterwarnings("ignore:cache no_store ignores:UserWarning") @pytest.mark.parametrize( "cache_kwargs", [ diff --git a/tests/test_cache_private.py b/tests/test_cache_private.py index 85ca05e..7aa6174 100644 --- a/tests/test_cache_private.py +++ b/tests/test_cache_private.py @@ -8,6 +8,7 @@ from collections.abc import AsyncIterator +import pytest from fastapi import FastAPI from fastapi import Request from fastapi import Response @@ -134,10 +135,12 @@ def test_no_store_still_wins_over_private(): app = FastAPI() client = TestClient(app) - @app.get("/private-no-store") - @cache(private=True, no_store=True) - async def private_no_store(): - return Response(content="x", media_type="text/plain") + with pytest.warns(UserWarning, match="no_store ignores private"): + + @app.get("/private-no-store") + @cache(private=True, no_store=True) + async def private_no_store(): + return Response(content="x", media_type="text/plain") assert client.get("/private-no-store").headers["Cache-Control"] == "no-store" diff --git a/tests/test_cache_unshareable.py b/tests/test_cache_unshareable.py index fa5d0f6..df5aa8a 100644 --- a/tests/test_cache_unshareable.py +++ b/tests/test_cache_unshareable.py @@ -482,10 +482,12 @@ async def opted_in(): def test_no_store_decorator_wins_over_set_cookie(): app = FastAPI() - @app.get("/nothing") - @cache(no_store=True, public=True) - async def nothing(response: Response): - response.set_cookie("c", "1") - return {} + with pytest.warns(UserWarning, match="no_store ignores public"): + + @app.get("/nothing") + @cache(no_store=True, public=True) + async def nothing(response: Response): + response.set_cookie("c", "1") + return {} assert TestClient(app).get("/nothing").headers["Cache-Control"] == "no-store" diff --git a/tests/test_cache_vary.py b/tests/test_cache_vary.py index e9abe7d..11e96e2 100644 --- a/tests/test_cache_vary.py +++ b/tests/test_cache_vary.py @@ -154,6 +154,7 @@ def test_vary_header_on_miss_hit_and_304() -> None: assert response.headers["vary"] == "Accept-Language" +@pytest.mark.filterwarnings("ignore:cache no_store ignores:UserWarning") @pytest.mark.parametrize( ("cache_kwargs", "request_headers"), [