Proxito: simplify caching logic #10067
Conversation
stsewd
force-pushed
the
simplify-caching-logic
branch
from
3f7024c to
9300676
Compare
Caching is enabled for all projects now. We always check for the privacy level to explicitly cache or not a response, if the response isn't explicitly cached or not, we use the default, this default depends if we are on .org or .com (we cache everything on .org, and on .com we cache responses attached to public versions). Closes #10031
stsewd
force-pushed
the
simplify-caching-logic
branch
from
9300676 to
119500c
Compare
|
@stsewd Love these PR comments, and seems like we're doing a good job of also adding code comments. They're useful on deleted code generally, and I really appreciate the context you share, and it gets lost if it doesn't live with the code! This PR is great, so 💯 |
There was a problem hiding this comment.
Excited for this! I had a couple larger ideas for additional features in my comments, but I don't want to block this PR on additional ideas. I'm fine getting this cleaned up and merged, with additional features done in future PR's. I'm really liking this small approach to PR's you're doing, so don't let me feature creep it :)
readthedocs/core/mixins.py
Outdated
|
|
||
| Views that can be cached should always return the same response for all | ||
| users (anonymous and authenticated users), like when the version attached | ||
| to the request is public. | ||
|
|
||
| To cache a view you can either set the `cache_request` attribute to `True`, | ||
| To explicitly cache a view you can either set the `cache_response` attribute to `True`/`False`, | ||
| or override the `can_be_cached` method. |
There was a problem hiding this comment.
Which takes precedence? We should likely document the order we check things, so it's clear how the logic is interpreted.
There was a problem hiding this comment.
Why do we have two different ways of doing the same? Can we have just one? 😄
| HTTP_HOST="project.dev.readthedocs.io", | ||
| ) | ||
| self.assertEqual(resp.status_code, 200) | ||
| extension = "zip" if type_ == MEDIA_TYPE_HTMLZIP else type_ |
There was a problem hiding this comment.
This seems like a code smell... Probably should have named htmlzip as zip... 🙃
readthedocs/core/mixins.py
Outdated
|
|
||
| Views that can be cached should always return the same response for all | ||
| users (anonymous and authenticated users), like when the version attached | ||
| to the request is public. | ||
|
|
||
| To cache a view you can either set the `cache_request` attribute to `True`, | ||
| To explicitly cache a view you can either set the `cache_response` attribute to `True`/`False`, | ||
| or override the `can_be_cached` method. |
There was a problem hiding this comment.
Why do we have two different ways of doing the same? Can we have just one? 😄
| # Always cache this view, since it's the same for all users. | ||
| cache_response = True |
There was a problem hiding this comment.
It was a little confusing to me that we are sometimes using this variable as a class-variable and some other times we are declaring/changing inside a method, making it an instance-variable.
I think this is fine, but we could expand the original comment in the CDN mixin explaining this in a better way.
stsewd
force-pushed
the
simplify-caching-logic
branch
from
caa1245 to
00a84b1
Compare
| To explicitly cache a view you can either set the `cache_response` | ||
| attribute to `True`/`False`, or override the `can_be_cached` method | ||
| (which defaults to return the `cache_response` attribute). | ||
| If set to `None` (default), the cache header won't be set, so the default |
Adding! |
Instead of relying on our Cloudflare's rules, we are defining explicitly what not to cache in our code.
I like that we are explicitly asking to cache or not things in our code, but I don't like that we depend on the
ALLOW_PRIVATE_REPOSsetting, but that's better to cache all types of responses on .org.After deploying this, we can remove several of our CF rules.
Closes #10031