Skip to content

Conversation

desiderantes
Copy link
Contributor

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)

@yschimke
Copy link
Collaborator

./gradlew :spotlessApply to cleanup formatting issues

@yschimke
Copy link
Collaborator

Failing on existing tests

DnsOverHttpsTest > usesCacheOnlyIfFresh() FAILED
    java.net.UnknownHostException: google.com
        at okhttp3.dnsoverhttps.DnsOverHttps.throwBestFailure(DnsOverHttps.kt:163)
        at okhttp3.dnsoverhttps.DnsOverHttps.lookupHttps(DnsOverHttps.kt:87)
        at okhttp3.dnsoverhttps.DnsOverHttps.lookup(DnsOverHttps.kt:68)
        at okhttp3.dnsoverhttps.DnsOverHttpsTest.usesCacheOnlyIfFresh(DnsOverHttpsTest.kt:297)

        Caused by:
        java.io.IOException: canceled due to java.lang.IllegalArgumentException: method GET must not have a request body.
            at okhttp3.internal.connection.RealCall$AsyncCall.run(RealCall.kt:542)
            at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
            at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
            at java.base/java.lang.Thread.run(Thread.java:1583)

@yschimke
Copy link
Collaborator

@martinbonnin Do your worst!

Comment on lines 636 to 637
if (requestMethod == "QUERY") {
// Write the body of a QUERY request.

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.

Copy link
Collaborator

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?

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?

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted

@yschimke yschimke requested a review from swankjesse August 30, 2025 11:24
@yschimke
Copy link
Collaborator

yschimke commented Aug 30, 2025

@swankjesse can you take a look. We should land this before a new release.

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 this pull request may close these issues.

3 participants