Add no-store, no-cache in Cache-Control headers #373
Conversation
|
Can I do something to help merging the PR ? I need it in one of my project. |
|
Hey @long2ice do you have time to review and push thru this fix please? |
|
Thanks! Please resolve conflicts. |
@long2ice resolved the conflict. |
fastapi_cache/decorator.py
Outdated
| 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 set([cache_control_val.strip().lower() for cache_control_val in cache_control_header.split(",")]) |
There was a problem hiding this comment.
This should be set comprehension imo
{cache_control_val.strip().lower() for cache_control_val in cache_control_header.split(",")}There was a problem hiding this comment.
@sultaniman thanks for pointing it out. Pushed the suggested change.
fastapi_cache/decorator.py
Outdated
| 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()} |
There was a problem hiding this comment.
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.
Yes, it looks redundant if Fastapi takes care of it. I pushed the fix.
| 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.
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.
Yeah, sorry for confusion, I mean header keys are case insensitive.
|
@long2ice can you please take a look again? |
|
Thanks! |
|
Thanks @Bharat23 |
Head branch was pushed to by a user without write access
tests/test_decorator.py
Outdated
| @@ -23,18 +24,18 @@ def test_datetime() -> None: | |||
| assert response.headers.get("X-FastAPI-Cache") == "MISS" | |||
| now = response.json().get("now") | |||
| now_ = pendulum.now() | |||
| assert pendulum.parse(now) == now_ | |||
| assert pendulum.parse(now).to_atom_string() == now_.to_atom_string() | |||
There was a problem hiding this comment.
Why do we need to convert to_atom_string since we are comparing datetime values?
|
@long2ice can you please check again once you have time? |
|
@Bharat23 can you please ignore mypy warnings for line 26 and 30 in |
@sultaniman I didn't understand. I am not seeing any mypy issue on 26 and 30. Can you point me to how/where I can see that and what specific error should I ignore? |
|
@Bharat23 I meant the linter failed in |
@sultaniman done |
|
@sultaniman @long2ice, can we get this merged please? Thanks |
|
I would also be happy to get this PR done, only @long2ice can merge it. |
|
@Bharat23 looks like the datetime dependency has minor upgrade and it no longer has /home/runner/work/fastapi-cache/fastapi-cache/tests/test_decorator.py
/home/runner/work/fastapi-cache/fastapi-cache/tests/test_decorator.py:37:16 - error: Type of "to_atom_string" is partially unknown
Type of "to_atom_string" is "Unknown | (() -> str)" (reportUnknownMemberType)
/home/runner/work/fastapi-cache/fastapi-cache/tests/test_decorator.py:37:20 - error: Cannot access attribute "to_atom_string" for class "Date"
Attribute "to_atom_string" is unknown (reportAttributeAccessIssue)
/home/runner/work/fastapi-cache/fastapi-cache/tests/test_decorator.py:37:20 - error: Cannot access attribute "to_atom_string" for class "Time"
Attribute "to_atom_string" is unknown (reportAttributeAccessIssue)
/home/runner/work/fastapi-cache/fastapi-cache/tests/test_decorator.py:37:20 - error: Cannot access attribute "to_atom_string" for class "Duration"
Attribute "to_atom_string" is unknown (reportAttributeAccessIssue)
4 errors, 0 warnings, 0 informations
WARNING: there is a new pyright version available (v1.1.381 -> v1.1.383). |
Head branch was pushed to by a user without write access
@sultaniman the method exists, this is a mypy issue. There is a mismatch between how the local vs github workflow env is configured because they contradict each other. Ultimately I had to add 2 ignores to satisfy both. Hopefully this is the last commit before this can be merged. |
|
@Bharat23 @sultaniman Just looking at this PR now ... as it's a change to existing behaviour I'm minded to roadmap this for |
|
+1; this brings the package in line with the RFC. |
Changes
no-cache/no-storewas presentno-cache: doesn't use cache even if the value is present but stores the response in the cacheno-store: can use cache if present but will not add/update to cacheAssociated Issue: #144