diff --git a/changelog.d/323.fixed.md b/changelog.d/323.fixed.md new file mode 100644 index 0000000..7d958b1 --- /dev/null +++ b/changelog.d/323.fixed.md @@ -0,0 +1,8 @@ +**An `async` key builder is rejected when `@cache` is applied instead of failing every request with a 500.** +`@cache(key_builder=...)` accepted an `async def` builder (or an object with an +`async def __call__`, or a `functools.partial` of either) and then raised +`TypeError` on every request, plus a "coroutine was never awaited" warning. It +now raises `CacheXError` at decoration time, and `invalidate(request, +key_builder=...)` raises it before touching the backend. A builder that returns +anything but a `str` raises `CacheXError` on the request, whatever `fail_open` +says. Key builders must be sync functions returning `str`. diff --git a/docs/HTTP_CACHING.md b/docs/HTTP_CACHING.md index 1c4e340..8a50d1c 100644 --- a/docs/HTTP_CACHING.md +++ b/docs/HTTP_CACHING.md @@ -448,6 +448,16 @@ called for `no_store=True`, `private=True`, routes without a `ttl`, or requests with `Authorization` or a session on a route without `public=True` or `cache_authorized=True`. Before 0.3.8 it was, only to feed a debug log. Keep it free of side effects. +The key builder must be a sync function that returns a `str`; it is called +without being awaited. An `async def` function, an object with an +`async def __call__`, or a `functools.partial` of either is rejected with +`CacheXError` when `@cache` is applied, and by `invalidate()` before it touches +the backend. A builder that still returns something other than a `str` (for +example a sync wrapper that returns a coroutine) raises `CacheXError` on the +request. `fail_open` does not cover this: it is a mistake in the route, not a +backend failure. Read anything async (a user from the database, say) in a +dependency or middleware and put it on `request.state` for the builder. + ## Clearing the cache ### By path or pattern diff --git a/fastapi_cachex/cache.py b/fastapi_cachex/cache.py index 6c35006..f628117 100644 --- a/fastapi_cachex/cache.py +++ b/fastapi_cachex/cache.py @@ -8,6 +8,7 @@ from collections.abc import Callable from collections.abc import Mapping from collections.abc import Sequence +from functools import partial from functools import update_wrapper from functools import wraps from inspect import Parameter @@ -270,6 +271,49 @@ def default_key_builder(request: Request) -> str: return build_cache_key(request) +_ASYNC_KEY_BUILDER_MSG = ( + "key_builder must be a sync function returning str; async key builders " + "are not supported (the key is built without awaiting)" +) + + +def _validate_key_builder(builder: Callable[..., object]) -> None: + """Reject a key builder whose call returns a coroutine. + + ``functools.partial`` layers are unwrapped first, so a partial of an async + callable object is caught as well. A builder this cannot see through + (say a sync wrapper returning a coroutine) is caught by ``_build_key``. + + Raises: + CacheXError: If calling ``builder`` would return a coroutine. + """ + target = builder + while isinstance(target, partial): + target = target.func + if _is_coroutine_callable(target): + raise CacheXError(_ASYNC_KEY_BUILDER_MSG) + + +def _build_key(builder: CacheKeyBuilder, request: Request) -> str: + """Call ``builder`` and check that it returned a ``str``. + + A returned coroutine is closed, so it does not also trigger a "never + awaited" ``RuntimeWarning``. ``fail_open`` does not cover this: it is a + programming error in the route, not a backend failure. + + Raises: + CacheXError: If ``builder`` returns anything but a ``str``. + """ + key: object = builder(request) + if isinstance(key, str): + return key + if inspect.iscoroutine(key): + key.close() + raise CacheXError(_ASYNC_KEY_BUILDER_MSG) + msg = f"key_builder must return a str, got {type(key).__name__}" + raise CacheXError(msg) + + async def invalidate( request: Request, key_builder: CacheKeyBuilder | None = None, @@ -287,7 +331,8 @@ async def invalidate( a request to the same route/method as the cached one (e.g. build it via ``request.app.url_path_for(...)`` for a GET route). key_builder: Custom key builder used by the target route's ``@cache`` - decorator, if any. If None, uses ``default_key_builder``. + decorator, if any. If None, uses ``default_key_builder``. Must be + a sync callable returning a ``str``, as for ``@cache``. vary: The target route's ``vary`` names, if any. Only the variant selected by ``request``'s own values for those headers is deleted; ``clear_path()`` clears every variant of a path. @@ -299,12 +344,16 @@ async def invalidate( True if a cache entry existed and was deleted, False otherwise. Raises: - CacheXError: If ``vary`` is not a list of header names. + CacheXError: If ``vary`` is not a list of header names, if + ``key_builder`` is an ``async`` callable, or if it returns + something other than a ``str``. Raised before the backend is + touched. """ builder = key_builder or default_key_builder + _validate_key_builder(builder) vary_names = _validate_vary(vary) cache_key = _append_key_components( - builder(request), _vary_components(request, vary_names) + _build_key(builder, request), _vary_components(request, vary_names) ) try: @@ -649,7 +698,7 @@ def _serialize_result(route: "APIRoute", result: object) -> object: ) -def _is_coroutine_callable(func: HandlerCallable) -> bool: +def _is_coroutine_callable(func: Callable[..., object]) -> bool: """Report whether calling `func` returns a coroutine. `inspect.iscoroutinefunction` already sees through `functools.partial`; @@ -791,7 +840,13 @@ def cache( key_builder: Custom function to build cache keys. If None, uses ``default_key_builder``. To add a component (user ID, tenant, locale) to the default key, return - ``build_cache_key(request, component)``. + ``build_cache_key(request, component)``. Must be a sync callable + returning a ``str``: an ``async def`` function, an object with an + ``async def __call__`` or a ``functools.partial`` of either is + rejected when the decorator is applied, and a builder that + returns anything but a ``str`` fails the request with + ``CacheXError`` (``fail_open`` does not apply; it covers backend + errors only). fail_open: When the backend raises, log a warning and answer without the cache: a failed read counts as a miss and a failed write leaves the response unstored. ``False`` lets the error propagate, @@ -837,7 +892,9 @@ def cache( ``stale_ttl`` are not given together, if ``public`` and ``private`` are both set, if ``ttl`` is not an ``int``, is negative or is larger than ``MAX_TTL``, or if ``vary`` is not a - list of header field names (a single string is rejected). + list of header field names (a single string is rejected), or if + ``key_builder`` is an ``async`` callable. At request time, if + ``key_builder`` returns anything but a ``str``. """ def decorator(func: HandlerCallable) -> AsyncResponseCallable: @@ -863,6 +920,8 @@ def decorator(func: HandlerCallable) -> AsyncResponseCallable: msg = f"ttl must be at most {MAX_TTL} seconds" raise CacheXError(msg) vary_names = _validate_vary(vary) + if key_builder is not None: + _validate_key_builder(key_builder) if any(name.lower() == "cookie" for name in vary_names): # stacklevel=2: the caller applying the decorator, i.e. the line # of the user's @cache(...). @@ -1039,7 +1098,7 @@ async def serve(*args: Any, **kwargs: Any) -> Response: # Built only here: the branches above never touch the backend, so a # custom key builder would run for nothing. cache_key = _append_key_components( - builder(req), _vary_components(req, vary_names) + _build_key(builder, req), _vary_components(req, vary_names) ) try: diff --git a/i18n/zh-TW/docs/HTTP_CACHING.md b/i18n/zh-TW/docs/HTTP_CACHING.md index 53ebbe3..d2d4db8 100644 --- a/i18n/zh-TW/docs/HTTP_CACHING.md +++ b/i18n/zh-TW/docs/HTTP_CACHING.md @@ -281,6 +281,8 @@ async def my_dashboard(user: CurrentUser, response: Response): key builder 只在 `@cache` 讀取或寫入後端時執行，因此 `no_store=True`、`private=True`、沒有 `ttl` 的路由，以及路由未設定 `public=True` 或 `cache_authorized=True` 時帶有 `Authorization` 或 Session 的請求，都不會呼叫它。0.3.8 以前它仍會被呼叫，但只用於除錯日誌。請讓它不帶副作用。 +key builder 必須是回傳 `str` 的同步函式，呼叫時不會被 await。`async def` 函式、具有 `async def __call__` 的物件，或包裝上述兩者的 `functools.partial`，都會在套用 `@cache` 時以 `CacheXError` 拒絕；`invalidate()` 也會在存取後端之前拒絕它們。仍然回傳非 `str` 的 builder（例如回傳協程的同步包裝函式）會在請求時拋出 `CacheXError`。`fail_open` 不涵蓋這種情況：這是路由的錯誤，不是後端故障。需要非同步讀取的資料（例如從資料庫取得使用者），請在依賴項或中介軟體中讀取，放到 `request.state` 供 builder 使用。 + ## 清除快取 {#clearing-the-cache} ### 依路徑或模式 {#by-path-or-pattern} diff --git a/tests/test_cache.py b/tests/test_cache.py index 4350c69..7e8fdfe 100644 --- a/tests/test_cache.py +++ b/tests/test_cache.py @@ -14,6 +14,7 @@ from fastapi_cachex.backends.memory import MemoryBackend from fastapi_cachex.cache import cache +from fastapi_cachex.cache import invalidate from fastapi_cachex.exceptions import CacheXError from fastapi_cachex.proxy import BackendProxy from fastapi_cachex.types import CacheEntry @@ -781,3 +782,110 @@ def test_public_and_private_raises(): @cache(public=True, private=True) async def bad_endpoint(): pass + + +async def _async_key_builder(request: Request) -> str: + return request.url.path + + +async def _async_key_builder_with_prefix(prefix: str, request: Request) -> str: + return prefix + request.url.path + + +class _AsyncCallableKeyBuilder: + async def __call__(self, request: Request) -> str: + return request.url.path + + +@pytest.mark.parametrize( + "builder", + [ + _async_key_builder, + _AsyncCallableKeyBuilder(), + partial(_async_key_builder_with_prefix, "p:"), + partial(partial(_AsyncCallableKeyBuilder())), + ], + ids=["async-def", "async-call-object", "partial", "partial-of-async-object"], +) +def test_async_key_builder_raises_at_decoration(builder): + """An async key builder is rejected when @cache is applied (#323).""" + with pytest.raises(CacheXError, match="key_builder must be a sync function"): + + @cache(ttl=60, key_builder=builder) + async def bad_endpoint(): + pass + + +def _get_request(path: str) -> Request: + return Request( + { + "type": "http", + "method": "GET", + "path": path, + "query_string": b"", + "headers": [(b"host", b"testserver")], + } + ) + + +class _RecordingBackend(MemoryBackend): + def __init__(self) -> None: + super().__init__() + self.touched = False + + async def get_and_delete(self, key: str) -> CacheEntry | None: + self.touched = True + return await super().get_and_delete(key) + + +async def test_invalidate_rejects_async_key_builder_before_the_backend(): + """invalidate() raises for an async builder without touching the backend.""" + backend = _RecordingBackend() + BackendProxy.set(backend) + with pytest.raises(CacheXError, match="key_builder must be a sync function"): + await invalidate(_get_request("/x"), key_builder=_async_key_builder) # type: ignore[arg-type] + assert backend.touched is False + + +def _returns_coroutine(request: Request) -> str: + """A sync wrapper around an async builder: undetectable when applied.""" + return _async_key_builder(request) # type: ignore[return-value] + + +def _returns_int(request: Request) -> str: + return 42 # type: ignore[return-value] + + +_NON_STR_BUILDERS = pytest.mark.parametrize( + ("builder", "match"), + [ + (_returns_coroutine, "key_builder must be a sync function"), + (_returns_int, "key_builder must return a str, got int"), + ], + ids=["coroutine", "int"], +) + + +@_NON_STR_BUILDERS +def test_key_builder_returning_non_str_raises_at_request_time(builder, match): + """A non-str key fails the request with CacheXError, not a TypeError. + + fail_open does not hide it, and a returned coroutine is closed, so no + "never awaited" RuntimeWarning is emitted (filterwarnings=error). + """ + local_app = FastAPI() + + @local_app.get("/non-str-key") + @cache(ttl=60, key_builder=builder, fail_open=True) + async def endpoint(): + return {"ok": True} + + with pytest.raises(CacheXError, match=match): + TestClient(local_app).get("/non-str-key") + + +@_NON_STR_BUILDERS +async def test_invalidate_key_builder_returning_non_str_raises(builder, match): + """invalidate() raises the same CacheXError for a non-str key.""" + with pytest.raises(CacheXError, match=match): + await invalidate(_get_request("/x"), key_builder=builder)