Skip to content
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

CACHE_ENABLED should not affect CACHE_CONTROL_S_MAXAGE #16772

Open
viters opened this issue Dec 9, 2022 · 6 comments
Open

CACHE_ENABLED should not affect CACHE_CONTROL_S_MAXAGE #16772

viters opened this issue Dec 9, 2022 · 6 comments

Comments

@viters
Copy link
Contributor

viters commented Dec 9, 2022

Describe the Bug

If I define CACHE_CONTROL_S_MAXAGE > 0 and set CACHE_ENABLED = false, Cache-Control header is set to no-cache. In my opinion I should be able to set s-maxage without enabling Directus internal cache system.

The workaround now is to create a hook that would modify Cache-Control before response is sent.

Expected behavior

CACHE_CONTROL_S_MAXAGE = 600 and CACHE_ENABLED = false should result in response with header Cache-Control: (public/private same as CACHE_ENABLED=true), s-maxage=600.

Errors Shown

No response

What version of Directus are you using?

9.19.1

What version of Node.js are you using?

N/A

What database are you using?

N/A

What browser are you using?

N/A

How are you deploying Directus?

N/A

@rijkvanzanten
Copy link
Member

Linear: ENG-111

@rijkvanzanten
Copy link
Member

Shouldn't CACHE_ENABLED=false result in a no-store cache-control header? Therefore ending with a no-store, s-maxage=600?

@viters
Copy link
Contributor Author

viters commented Jan 11, 2023

@rijkvanzanten
no-store means that any intermediate cache is prevented from storing the response, thus s-maxage will not be taken into account. no-store is more aggressive than no-cache, from MDN:

Note that no-cache does not mean "don't cache". no-cache allows caches to store a response but requires them to revalidate it before reuse. If the sense of "don't cache" that you want is actually "don't store", then no-store is the directive to use.

I still think that CACHE_ENABLED=false should just not affect Cache-Control header. Also, maybe it would be useful to allow overwriting Cache-Control entirely with different ENV for advanced usages.

@rijkvanzanten
Copy link
Member

I still think that CACHE_ENABLED=false should just not affect Cache-Control header

Ohh that's a very interesting take! So you're suggesting one should be able to control the cache-control header completely separate from directus' own caching 🤔 I guess that makes sense!

@viters
Copy link
Contributor Author

viters commented Jan 12, 2023

Ohh that's a very interesting take! So you're suggesting one should be able to control the cache-control header completely separate from directus' own caching 🤔 I guess that makes sense!

Yeah, kinda. In other words, defaulting to maxage=CACHE_TTL when CACHE_ENABLED=true and public/private based on Authorization, but with more control if it is needed (like with CACHE_CONTROL_S_MAXAGE).

@rijkvanzanten
Copy link
Member

I like the idea of effectively decoupling the header from the main cache itself, as there are use cases where you want the header to do something differently from how the cache operates (like with s-max-age). At the same time are some parts that should remain automatic (like public/private). I think we can add a couple additional CACHE_CONTROL_X environment variables to control the individual parts, where we support an auto flag for keeping the current behavior 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 📋 Backlog
Development

No branches or pull requests

3 participants