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

Proper Cache-Control: private support #141

Open
kyrias opened this issue Feb 22, 2017 · 7 comments · May be fixed by #167
Open

Proper Cache-Control: private support #141

kyrias opened this issue Feb 22, 2017 · 7 comments · May be fixed by #167

Comments

@kyrias
Copy link

kyrias commented Feb 22, 2017

Responses where Cache-Control is set to private may be cached but must not be cached in a shared cache. I have a use of cachecontrol for an API client I'm writing, but it has support for multiple users, so I need to implement user-specific caching support soon.

My idea for how to implement it is to have a parameter you pass to CacheControl with a header name. Then when a response has Cache-Control: private set it would try to fetch a header with that key from the request/response and then then prepend a hash of that to the cache key.

I currently have it implemented locally for the redis backend, though it's currently very implementation specific, only checking for Authorization, and only for the redis backend.

Any thoughts/opinion on this idea?

@ionrock
Copy link
Contributor

ionrock commented Feb 22, 2017

That is a tough one! I'm curious about what qualifies as "shared". For example, if I were using the file cache, would using a different directory be enough to be considered private? I suppose the same goes for the redis cache where private and public items might be cached.

As for an implementation, I think what I would do is define something like a namespace function that is passed to the CacheControl wrapper. If defined, the function would return a prefix key that would be prepended when storing it in the cache.

That sounds pretty similar to what your doing with the exception being that a function is passed in that allows accessing anything in the request to be used when constructing the private namespace.

I hope that helps a bit! It sounds like an interesting problem and I'm interested to see how you solve it!

@kyrias
Copy link
Author

kyrias commented Feb 23, 2017

Essentially it shouldn't be shared as in it shouldn't be possible for the result to be returned for other users.

I do like the idea of a namespace function though, hm. I'll try it out, thanks!

@jowrjowr
Copy link

jowrjowr commented Aug 4, 2017

This issue just dinged me pretty hard.

Looking at the standard for this:

https://tools.ietf.org/html/rfc7234#page-21

   is intended for a single user and MUST NOT be stored by a shared
   cache.  A private cache MAY store the response and reuse it for later
   requests, even if the response would normally be non-cacheable.

This is something cachecontrol should not be doing at all, unless specifically enabled in a heuristic which you already have sufficient support for.

I was going mental trying to figure out how something with Cache-Control: private was getting cached and then I ripped apart the cachecontrol code proving it.

Pretty please don't break standards like that :(

@ionrock
Copy link
Contributor

ionrock commented Aug 4, 2017

@jowrjowr My understanding, which I'll admit might be wrong, would be a caching proxy where multiple users are sharing static resources in the same cache. In a client, it gets murky. For example, if I have an API client and I want a shared directory, the use the content might be exactly the same, in which case I'd argue that the cache is not "shared" from the perspective that, even though different processes are using the cache, there is still a single "user" utilizing the cached values.

A good example is the use case that drove me to write CacheControl in the first place. I had a REST based gettext replacement and I wanted to have a shared cache between processes on the same machine. Each program that instantiated the gettext functionality would get a client configured with CacheControl using a local bsddb database. This allowed a client in one process to cache the value for another process which might need a cached value. From this perspective, I'm considering the cache "private" as it only pertains to a single set of users that require the same use case.

Maybe this is the wrong way to think about it! Please let me know your use case and hopefully we can find a better solution.

@jowrjowr
Copy link

jowrjowr commented Aug 4, 2017 via email

@ionrock
Copy link
Contributor

ionrock commented Aug 7, 2017

@jowrjowr Ah OK, that does make sense why that would be difficult. I'm reluctant to simply stop caching when private is declared, but I do think it would make sense to define a cache as "shared" or not. If the cache is declared as shared, then the private directive will be honored as the spec prescribes.

That would require some code changes:

cache = RedisCache(redis_url, shared=True)
sess = CacheControl(requests.Session(), cache=cache)

That seems pretty reasonable to me and reflects more closely your point about the spec.

@jowrjowr
Copy link

jowrjowr commented Aug 7, 2017 via email

ionrock pushed a commit that referenced this issue Aug 7, 2017
In the spec (https://tools.ietf.org/html/rfc7234#section-5.2.2.6) it talks
about the `private` directive and that a shared cache MUST NOT store
the response.

This starts by defining the cache as being "shared" and testing for
the "shared" attribute on the cache.

Fixes: #141
@ionrock ionrock linked a pull request Aug 7, 2017 that will close this issue
ionrock pushed a commit that referenced this issue Aug 7, 2017
In the spec (https://tools.ietf.org/html/rfc7234#section-5.2.2.6) it talks
about the `private` directive and that a shared cache MUST NOT store
the response.

This starts by defining the cache as being "shared" and testing for
the "shared" attribute on the cache.

Fixes: #141
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants