-
Notifications
You must be signed in to change notification settings - Fork 9.2k
Add QUERY caching support #9027
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
base: master
Are you sure you want to change the base?
Conversation
|
Failing on existing tests
|
@martinbonnin Do your worst! |
if (requestMethod == "QUERY") { | ||
// Write the body of a QUERY request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to store the request body in cache?
Looking at the code, it looks like storing a request in the cache is already destructive as only the vary
headers of the request are stored (not all of them).
I think I wouldn't mind too much if Cache.Entry.body
was missing and saving some bytes there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Storing a hash instead would seem a lot less disruptive to the format. Perhaps it could be stored as a synthetic header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we even need to store anything? What's the use case for reading that request body from the cache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, it just needs to be part of the key. I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can avoid changing the format, that would be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted
okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/CacheInterceptor.kt
Outdated
Show resolved
Hide resolved
@swankjesse can you take a look. We should land this before a new release. |
Continuing the work from #8550, this PR adds logic for cache support on QUERY requests. I am a bit unsure on visibility for the new key method vs the old one, so I've left it unsolved. Also, on whether the cost of hashing the body would make it worth it (I guess some sort of warning should be added)