Add no-store, no-cache in Cache-Control headers by Bharat23 · Pull Request #373 · long2ice/fastapi-cache

Bharat23 · 2024-01-10T04:01:05Z

Changes

Removed the logic to never use cache if either of no-cache/no-store was present
Instead they work as follows
no-cache: doesn't use cache even if the value is present but stores the response in the cache
no-store: can use cache if present but will not add/update to cache
If both are passed, it will neither store nor use the cache. Will remove the max-age and ETag as well.
Test cases

Associated Issue: #144

zozoh94 · 2024-07-04T15:46:36Z

Can I do something to help merging the PR ? I need it in one of my project.

Bharat23 · 2024-07-06T20:56:19Z

Can I do something to help merging the PR ? I need it in one of my project.

@zozoh94 the owner @long2ice seems to be inactive. Probably adding more 👍 (+1) to the PR by community will help.

sultaniman · 2024-09-02T09:46:00Z

Hey @long2ice do you have time to review and push thru this fix please?

long2ice · 2024-09-02T09:49:57Z

Thanks! Please resolve conflicts.

sultaniman · 2024-09-02T11:11:22Z

@Bharat23 @zozoh94 can anyone rebase and resolve conflicts?

Bharat23 · 2024-09-07T22:18:56Z

Thanks! Please resolve conflicts.

@long2ice resolved the conflict.

sultaniman · 2024-09-08T10:37:02Z

fastapi_cache/decorator.py

+        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(",")])


This should be set comprehension imo

{cache_control_val.strip().lower() for cache_control_val in cache_control_header.split(",")}

@sultaniman thanks for pointing it out. Pushed the suggested change.

sultaniman · 2024-09-09T09:12:39Z

fastapi_cache/decorator.py

+    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()}


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

Yes, it looks redundant if Fastapi takes care of it. I pushed the fix.

sultaniman · 2024-09-09T09:15:14Z

fastapi_cache/decorator.py

+        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(",")}


And here as well.

this is case conversion of the header value which Fastapi doesn't convert based on some of my testing

Yeah, sorry for confusion, I mean header keys are case insensitive.

sultaniman

lgtm

sultaniman · 2024-09-11T13:14:43Z

@long2ice can you please take a look again?

long2ice · 2024-09-11T13:24:44Z

Thanks!

sultaniman · 2024-09-11T15:22:00Z

@Bharat23 @zozoh94 I think we need to make tests pass, thanks to @long2ice for enabling auto merge.

Bharat23 · 2024-09-11T19:13:01Z

@Bharat23 @zozoh94 I think we need to make tests pass, thanks to @long2ice for enabling auto merge.

I'll take care of them.

sultaniman · 2024-09-11T19:58:23Z

Thanks @Bharat23

Bharat23 · 2024-09-11T20:58:59Z

@Bharat23 @zozoh94 I think we need to make tests pass, thanks to @long2ice for enabling auto merge.

@sultaniman @long2ice
Had to alter and remove some test cases as they were breaking due to the change introduced in this PR

sultaniman · 2024-09-16T10:13:24Z

tests/test_decorator.py

        now = response.json().get("now")
        now_ = pendulum.now()
-        assert pendulum.parse(now) == now_
+        assert pendulum.parse(now).to_atom_string() == now_.to_atom_string()


Why do we need to convert to_atom_string since we are comparing datetime values?

sultaniman · 2024-09-18T11:19:00Z

@long2ice can you please check again once you have time?

sultaniman · 2024-09-19T11:04:11Z

@Bharat23 can you please ignore mypy warnings for line 26 and 30 in tests/test_decorator.py, I can't push changes into your repo, otherwise I would do it.

Bharat23 · 2024-09-20T03:39:54Z

@Bharat23 can you please ignore mypy warnings for line 26 and 30 in tests/test_decorator.py, I can't push changes into your repo, otherwise I would do it.

@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?

sultaniman · 2024-09-20T08:12:16Z

@Bharat23 I meant the linter failed in test/test_decorator.py I suggested to use per line ignore instead of a global ignore.

Bharat23 · 2024-09-21T21:22:12Z

@Bharat23 I meant the linter failed in test/test_decorator.py I suggested to use per line ignore instead of a global ignore.

@sultaniman done

Bharat23 · 2024-10-01T15:57:04Z

@sultaniman @long2ice, can we get this merged please? Thanks

sultaniman · 2024-10-02T07:26:10Z

I would also be happy to get this PR done, only @long2ice can merge it.

sultaniman · 2024-10-02T13:06:20Z

@Bharat23 looks like the datetime dependency has minor upgrade and it no longer has to_atom_string method

/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).

Bharat23 · 2024-10-13T00:36:14Z

@Bharat23 looks like the datetime dependency has minor upgrade and it no longer has to_atom_string method

/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).

@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.

vicchi · 2024-11-11T09:11:59Z

@Bharat23 @sultaniman Just looking at this PR now ... as it's a change to existing behaviour I'm minded to roadmap this for 1.0.0 which I'll be working on as soon as 0.3.0 is out the door. But happy to be persueded otherwise

ermiyaeskandary · 2024-12-28T21:36:47Z

+1; this brings the package in line with the RFC.

Add no-store, no-cache in Cache-Control headers

85443b4

Merge branch 'main' into main

83ff773

merge: upstream repo

d52bb8a

update README with Notes on cache control section

7986360

sultaniman reviewed Sep 8, 2024

View reviewed changes

convert list to set comprehension

4b37ca7

Bharat23 requested a review from sultaniman September 9, 2024 04:45

sultaniman reviewed Sep 9, 2024

View reviewed changes

remove header key case conversion

6e180e7

sultaniman approved these changes Sep 10, 2024

View reviewed changes

long2ice enabled auto-merge September 11, 2024 13:24

github-actions bot added the auto-merge Auto-merge enabled label Sep 11, 2024

Add changelog

fe5a9d7

auto-merge was automatically disabled September 11, 2024 20:53
Head branch was pushed to by a user without write access

github-actions bot removed the auto-merge Auto-merge enabled label Sep 11, 2024

sultaniman reviewed Sep 16, 2024

View reviewed changes

long2ice added 2 commits September 19, 2024 15:32

chore: update deps

bbd3a2d

Merge branch 'main' into main

6767c01

long2ice approved these changes Sep 19, 2024

View reviewed changes

long2ice force-pushed the main branch from bbd3a2d to 157a913 Compare September 19, 2024 07:38

Merge branch 'main' into main

39ff947

fix lint ignore comment

c2020b4

replace global module ignore to inline

7cccf80

long2ice enabled auto-merge October 2, 2024 07:39

github-actions bot added the auto-merge Auto-merge enabled label Oct 2, 2024

fix mypy issue

7f13673

auto-merge was automatically disabled October 13, 2024 00:32
Head branch was pushed to by a user without write access

github-actions bot removed the auto-merge Auto-merge enabled label Oct 13, 2024

vicchi added the enhancement New feature or request label Nov 11, 2024

vicchi added this to the Version 1.0.0a1 milestone Nov 11, 2024

Uh oh!

Conversation

Bharat23 commented Jan 10, 2024

Changes

Associated Issue: #144

Uh oh!

zozoh94 commented Jul 4, 2024

Uh oh!

Bharat23 commented Jul 6, 2024

Uh oh!

sultaniman commented Sep 2, 2024

Uh oh!

long2ice commented Sep 2, 2024

Uh oh!

sultaniman commented Sep 2, 2024

Uh oh!

Bharat23 commented Sep 7, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sultaniman left a comment

Choose a reason for hiding this comment

Uh oh!

sultaniman commented Sep 11, 2024

Uh oh!

long2ice commented Sep 11, 2024

Uh oh!

sultaniman commented Sep 11, 2024

Uh oh!

Bharat23 commented Sep 11, 2024

Uh oh!

sultaniman commented Sep 11, 2024

Uh oh!

Bharat23 commented Sep 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sultaniman commented Sep 18, 2024

Uh oh!

sultaniman commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bharat23 commented Sep 20, 2024

Uh oh!

sultaniman commented Sep 20, 2024

Uh oh!

Bharat23 commented Sep 21, 2024

Uh oh!

Bharat23 commented Oct 1, 2024

Uh oh!

sultaniman commented Oct 2, 2024

Uh oh!

sultaniman commented Oct 2, 2024

Uh oh!

Bharat23 commented Oct 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vicchi commented Nov 11, 2024

Uh oh!

ermiyaeskandary commented Dec 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

sultaniman commented Sep 19, 2024 •

edited

Loading

Bharat23 commented Oct 13, 2024 •

edited

Loading