Proxito: handle http to https redirects for all requests #10199
Conversation
|
Note for myself: looks like I'm missing adding tests for this new behavior |
| CACHE_TAG_HEADER = "Cache-Tag" | ||
| CDN_CACHE_CONTROL_HEADER = "CDN-Cache-Control" |
There was a problem hiding this comment.
We are using a constants.py on each Django application. Probably makes sense to keep that same pattern here as well?
| :param response: The response to add cache tags to. | ||
| :param cache_tags: A list of cache tags to add to the response. | ||
| """ | ||
| cache_tags = cache_tags.copy() |
There was a problem hiding this comment.
Do we copy them for any particular reason?
There was a problem hiding this comment.
We are extending this list below, we don't want to change the original list.
| :param force: If ``True``, the header will be set to public even if it | ||
| was already set to private. |
There was a problem hiding this comment.
I think it's not clear what force means in this context. Probably it's worth to rename this attribute to something like public= or private= depending what's the best default.
There was a problem hiding this comment.
For me force is more clear than public or private, since we are forcing the request to be cached or not depending on the method being called.
There was a problem hiding this comment.
My point is that sometimes force=True means to force to be public and some others force=True means to be private. See https://github.com/readthedocs/readthedocs.org/pull/10199/files/c5a54a93eeb027a4187edae5c5c9610d7fbbe155#diff-47aa393923b9e640f34198f21bb776acd52797a770630d8c5a5bfd63346931c1R61
Something more explicit here would be better in my opinion. If you like the force word, probably force="public" and force="private" is a better middle ground and still pretty clear.
There was a problem hiding this comment.
But they are used in different functions, the name of the function already specifies if the request will be forced to be public or private (cache_response, private_response), that's why it's just a boolean, we aren't using one function to cache the response or make it private, we are using two different functions.
There was a problem hiding this comment.
Yeah, I saw that. IMO, it's still hard to realize what I am forcing to when passing the boolean, that's why I raised this comment here. I got confused while doing the review.
|
There is only one unresolved discussion #10199 (comment), but just a name, so moving on. |
In order to be able to cache the redirects,
I had to refactor some of our code
(and kind of a little in the direction of #10124).
Closes #10144