-
-
Notifications
You must be signed in to change notification settings - Fork 190
Add no-store, no-cache in Cache-Control headers #373
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 5 commits
85443b4
83ff773
d52bb8a
7986360
4b37ca7
6e180e7
fe5a9d7
bbd3a2d
6767c01
39ff947
c2020b4
7cccf80
7f13673
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
| TypeVar, | ||
| Union, | ||
| cast, | ||
| Set, | ||
| ) | ||
|
|
||
| if sys.version_info >= (3, 10): | ||
|
|
@@ -81,7 +82,23 @@ def _uncacheable(request: Optional[Request]) -> bool: | |
| return False | ||
| if request.method != "GET": | ||
| return True | ||
| return request.headers.get("Cache-Control") == "no-store" | ||
| return False | ||
|
|
||
| def _extract_cache_control_headers(request: Optional[Request]) -> Set[str]: | ||
| """Extracts Cache-Control header | ||
| 1. Convert all header to lowercase to make it case insensitive | ||
| 2. Split on comma (,) | ||
| 3. Strip whitespaces | ||
| 4. convert to all lower case | ||
|
|
||
| returns an empty set if header not set | ||
| """ | ||
| if request is not None: | ||
| headers = {header_key.lower(): header_val for header_key, header_val in request.headers.items()} | ||
| cache_control_header = headers.get("cache-control", None) | ||
| if cache_control_header: | ||
| return {cache_control_val.strip().lower() for cache_control_val in cache_control_header.split(",")} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And here as well.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is case conversion of the header value which Fastapi doesn't convert based on some of my testing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, sorry for confusion, I mean header keys are case insensitive. |
||
| return set() | ||
|
|
||
|
|
||
| def cache( | ||
|
|
@@ -161,6 +178,8 @@ async def ensure_async_func(*args: P.args, **kwargs: P.kwargs) -> R: | |
| key_builder = key_builder or FastAPICache.get_key_builder() | ||
| backend = FastAPICache.get_backend() | ||
| cache_status_header = FastAPICache.get_cache_status_header() | ||
| cache_control_headers = _extract_cache_control_headers(request=request) | ||
| response_headers = {"Cache-Control": cache_control_headers.copy()} | ||
|
|
||
| cache_key = key_builder( | ||
| func, | ||
|
|
@@ -174,21 +193,30 @@ async def ensure_async_func(*args: P.args, **kwargs: P.kwargs) -> R: | |
| cache_key = await cache_key | ||
| assert isinstance(cache_key, str) # noqa: S101 # assertion is a type guard | ||
|
|
||
| ttl, cached = 0, None | ||
| try: | ||
| ttl, cached = await backend.get_with_ttl(cache_key) | ||
| # no-cache: Assume cache is not present. i.e. treat it as a miss | ||
| if "no-cache" not in cache_control_headers: | ||
| ttl, cached = await backend.get_with_ttl(cache_key) | ||
| etag = f"W/{hash(cached)}" | ||
| response_headers["Cache-Control"].add(f"max-age={ttl}") | ||
| response_headers["Etag"] = {f"ETag={etag}"} | ||
| except Exception: | ||
| logger.warning( | ||
| f"Error retrieving cache key '{cache_key}' from backend:", | ||
| exc_info=True, | ||
| ) | ||
| ttl, cached = 0, None | ||
|
|
||
| if cached is None or (request is not None and request.headers.get("Cache-Control") == "no-cache") : # cache miss | ||
| result = await ensure_async_func(*args, **kwargs) | ||
| to_cache = coder.encode(result) | ||
|
|
||
| try: | ||
| await backend.set(cache_key, to_cache, expire) | ||
| # no-store: do not store the value in cache | ||
| if "no-store" not in cache_control_headers: | ||
| await backend.set(cache_key, to_cache, expire) | ||
| response_headers["Cache-Control"].add(f"max-age={expire}") | ||
| response_headers["Etag"] = {f"W/{hash(to_cache)}"} | ||
| except Exception: | ||
| logger.warning( | ||
| f"Error setting cache key '{cache_key}' in backend:", | ||
|
|
@@ -198,25 +226,22 @@ async def ensure_async_func(*args: P.args, **kwargs: P.kwargs) -> R: | |
| if response: | ||
| response.headers.update( | ||
| { | ||
| "Cache-Control": f"max-age={expire}", | ||
| "ETag": f"W/{hash(to_cache)}", | ||
| **{header_key: ",".join(sorted(header_val)) for header_key, header_val in response_headers.items()}, | ||
| cache_status_header: "MISS", | ||
| } | ||
| ) | ||
|
|
||
| else: # cache hit | ||
| if response: | ||
| etag = f"W/{hash(cached)}" | ||
| response.headers.update( | ||
| { | ||
| "Cache-Control": f"max-age={ttl}", | ||
| "ETag": etag, | ||
| **{header_key: ",".join(sorted(header_val)) for header_key, header_val in response_headers.items()}, | ||
| cache_status_header: "HIT", | ||
| } | ||
| ) | ||
|
|
||
| if_none_match = request and request.headers.get("if-none-match") | ||
| if if_none_match == etag: | ||
| if "Etag" in response_headers and if_none_match == response_headers["Etag"]: | ||
| response.status_code = HTTP_304_NOT_MODIFIED | ||
| return response | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can omit lowecasing since FastAPI uses starlette where headers are case-insensitive https://fastapi.tiangolo.com/tutorial/header-params/?h=insensitive#automatic-conversion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it looks redundant if Fastapi takes care of it. I pushed the fix.