Add a cacheUrlOverride

Author: yschimke

okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt

@@ -186,16 +186,24 @@ class DnsOverHttps internal constructor(
     throw unknownHostException
   }
 
-  private fun getCacheOnlyResponse(request: Request): Response? {
-    if (!post && client.cache != null) {
+  private fun getCacheOnlyResponse(
+    request: Request,
+  ): Response? {
+    if (client.cache != null) {
       try {
         // Use the cache without hitting the network first
         // 504 code indicates that the Cache is stale
-        val preferCache =
+        val onlyIfCached =
           CacheControl.Builder()
             .onlyIfCached()
             .build()
-        val cacheRequest = request.newBuilder().cacheControl(preferCache).build()
+
+        var cacheUrl = request.url
+
+        val cacheRequest = request.newBuilder()
+          .cacheControl(onlyIfCached)
+          .cacheUrlOverride(cacheUrl)
+          .build()
 
         val cacheResponse = client.newCall(cacheRequest).execute()
 
@@ -247,7 +255,12 @@ class DnsOverHttps internal constructor(
       val query = DnsRecordCodec.encodeQuery(hostname, type)
 
       if (post) {
-        url(url).post(query.toRequestBody(DNS_MESSAGE))
+        url(url)
+          .cacheUrlOverride(
+            url.newBuilder()
+              .addQueryParameter("hostname", hostname).build()
+          )
+          .post(query.toRequestBody(DNS_MESSAGE))
       } else {
         val encoded = query.base64Url().replace("=", "")
         val requestUrl = url.newBuilder().addQueryParameter("dns", encoded).build()
@@ -313,7 +326,8 @@ class DnsOverHttps internal constructor(
         this.bootstrapDnsHosts = bootstrapDnsHosts
       }
 
-    fun bootstrapDnsHosts(vararg bootstrapDnsHosts: InetAddress): Builder = bootstrapDnsHosts(bootstrapDnsHosts.toList())
+    fun bootstrapDnsHosts(vararg bootstrapDnsHosts: InetAddress): Builder =
+      bootstrapDnsHosts(bootstrapDnsHosts.toList())
 
     fun systemDns(systemDns: Dns) =
       apply {

okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt

@@ -31,9 +31,12 @@ import java.util.concurrent.TimeUnit
 import mockwebserver3.MockResponse
 import mockwebserver3.MockWebServer
 import okhttp3.Cache
+import okhttp3.Call
 import okhttp3.Dns
+import okhttp3.EventListener
 import okhttp3.OkHttpClient
 import okhttp3.Protocol
+import okhttp3.Response
 import okhttp3.testing.PlatformRule
 import okio.Buffer
 import okio.ByteString.Companion.decodeHex
@@ -55,6 +58,27 @@ class DnsOverHttpsTest {
   private val bootstrapClient =
     OkHttpClient.Builder()
       .protocols(listOf(Protocol.HTTP_2, Protocol.HTTP_1_1))
+      .eventListener(object : EventListener() {
+        override fun callStart(call: Call) {
+          println("callStart " + call.request().url + " " + call.request().cacheUrlOverride)
+        }
+
+        override fun satisfactionFailure(call: Call, response: Response) {
+          println("satisfactionFailure " + call.request().url)
+        }
+
+        override fun cacheHit(call: Call, response: Response) {
+          println("cacheHit " + call.request().url)
+        }
+
+        override fun cacheMiss(call: Call) {
+          println("cacheMiss " + call.request().url)
+        }
+
+        override fun cacheConditionalHit(call: Call, cachedResponse: Response) {
+          println("cacheConditionalHit " + call.request().url)
+        }
+      })
       .build()
 
   @BeforeEach
@@ -193,6 +217,31 @@ class DnsOverHttpsTest {
     assertThat(result).isEqualTo(listOf(address("157.240.1.18")))
   }
 
+  @Test
+  fun usesCacheEvenForPost() {
+    val cache = Cache("cache".toPath(), (100 * 1024).toLong(), cacheFs)
+    val cachedClient = bootstrapClient.newBuilder().cache(cache).build()
+    val cachedDns = buildLocalhost(cachedClient, false, post = true)
+    server.enqueue(
+      dnsResponse(
+        "0000818000010003000000000567726170680866616365626f6f6b03636f6d0000010001c00c000500010" +
+          "0000a6d000603617069c012c0300005000100000cde000c04737461720463313072c012c04200010001000" +
+          "0003b00049df00112",
+      )
+        .newBuilder()
+        .setHeader("cache-control", "private, max-age=298")
+        .build(),
+    )
+    var result = cachedDns.lookup("google.com")
+    assertThat(result).containsExactly(address("157.240.1.18"))
+    val recordedRequest = server.takeRequest()
+    assertThat(recordedRequest.method).isEqualTo("POST")
+    assertThat(recordedRequest.path)
+      .isEqualTo("/lookup?ct")
+    result = cachedDns.lookup("google.com")
+    assertThat(result).isEqualTo(listOf(address("157.240.1.18")))
+  }
+
   @Test
   fun usesCacheOnlyIfFresh() {
     val cache = Cache(File("./target/DnsOverHttpsTest.cache"), 100 * 1024L)
@@ -245,12 +294,14 @@ class DnsOverHttpsTest {
   private fun buildLocalhost(
     bootstrapClient: OkHttpClient,
     includeIPv6: Boolean,
+    post: Boolean = false
   ): DnsOverHttps {
     val url = server.url("/lookup?ct")
     return DnsOverHttps.Builder().client(bootstrapClient)
       .includeIPv6(includeIPv6)
       .resolvePrivateAddresses(true)
       .url(url)
+      .post(post)
       .build()
   }
 

okhttp/src/main/kotlin/okhttp3/Request.kt

@@ -54,6 +54,9 @@ class Request internal constructor(builder: Builder) {
   @get:JvmName("body")
   val body: RequestBody? = builder.body
 
+  @get:JvmName("cacheUrlOverride")
+  val cacheUrlOverride: HttpUrl? = builder.cacheUrlOverride
+
   internal val tags: Map<KClass<*>, Any> = builder.tags.toMap()
 
   internal var lazyCacheControl: CacheControl? = null
@@ -183,6 +186,7 @@ class Request internal constructor(builder: Builder) {
     internal var method: String
     internal var headers: Headers.Builder
     internal var body: RequestBody? = null
+    internal var cacheUrlOverride: HttpUrl? = null
 
     /** A mutable map of tags, or an immutable empty map if we don't have any. */
     internal var tags = mapOf<KClass<*>, Any>()
@@ -202,6 +206,7 @@ class Request internal constructor(builder: Builder) {
           else -> request.tags.toMutableMap()
         }
       this.headers = request.headers.newBuilder()
+      this.cacheUrlOverride = request.cacheUrlOverride
     }
 
     open fun url(url: HttpUrl): Builder =
@@ -316,6 +321,16 @@ class Request internal constructor(builder: Builder) {
       tag: T?,
     ) = commonTag(type.kotlin, tag)
 
+    /**
+     * Override the [Request.url] for caching, if it is either polluted with
+     * transient query params, or has a canonical URL possibly for a CDN.
+     *
+     * If set, this will also allow caching for POST requests.
+     */
+    fun cacheUrlOverride(cacheUrlOverride: HttpUrl?) = apply {
+        this.cacheUrlOverride = cacheUrlOverride
+      }
+
     open fun build(): Request = Request(this)
   }
 }

okhttp/src/main/kotlin/okhttp3/internal/cache/CacheInterceptor.kt

@@ -25,6 +25,7 @@ import okhttp3.EventListener
 import okhttp3.Headers
 import okhttp3.Interceptor
 import okhttp3.Protocol
+import okhttp3.Request
 import okhttp3.Response
 import okhttp3.internal.closeQuietly
 import okhttp3.internal.connection.RealCall
@@ -44,7 +45,7 @@ class CacheInterceptor(internal val cache: Cache?) : Interceptor {
   @Throws(IOException::class)
   override fun intercept(chain: Interceptor.Chain): Response {
     val call = chain.call()
-    val cacheCandidate = cache?.get(chain.request())
+    val cacheCandidate = cache?.get(chain.request().requestForCache())
 
     val now = System.currentTimeMillis()
 
@@ -125,14 +126,17 @@ class CacheInterceptor(internal val cache: Cache?) : Interceptor {
       }
     }
 
+    val cacheNetworkRequest = networkRequest.requestForCache()
+
     val response =
       networkResponse!!.newBuilder()
         .cacheResponse(cacheResponse?.stripBody())
         .networkResponse(networkResponse.stripBody())
+        .request(cacheNetworkRequest)
         .build()
 
     if (cache != null) {
-      if (response.promisesBody() && CacheStrategy.isCacheable(response, networkRequest)) {
+      if (response.promisesBody() && CacheStrategy.isCacheable(response)) {
         // Offer this request to the cache.
         val cacheRequest = cache.put(response)
         return cacheWritingResponse(cacheRequest, response).also {
@@ -285,3 +289,14 @@ class CacheInterceptor(internal val cache: Cache?) : Interceptor {
     }
   }
 }
+
+private fun Request.requestForCache(): Request {
+  return if (cacheUrlOverride != null) {
+    newBuilder()
+      .get()
+      .cacheUrlOverride(null)
+      .build()
+  } else {
+    this
+  }
+}

okhttp/src/main/kotlin/okhttp3/internal/cache/CacheStrategy.kt

@@ -152,7 +152,7 @@ class CacheStrategy internal constructor(
       // If this response shouldn't have been stored, it should never be used as a response source.
       // This check should be redundant as long as the persistence store is well-behaved and the
       // rules are constant.
-      if (!isCacheable(cacheResponse, request)) {
+      if (!isCacheable(cacheResponse)) {
         return CacheStrategy(request, null)
       }
 
@@ -292,7 +292,6 @@ class CacheStrategy internal constructor(
     /** Returns true if [response] can be stored to later serve another request. */
     fun isCacheable(
       response: Response,
-      request: Request,
     ): Boolean {
       // Always go to network for uncacheable response codes (RFC 7231 section 6.1), This
       // implementation doesn't support caching partial content.
@@ -334,7 +333,7 @@ class CacheStrategy internal constructor(
       }
 
       // A 'no-store' directive on request or response prevents the response from being cached.
-      return !response.cacheControl.noStore && !request.cacheControl.noStore
+      return !response.cacheControl.noStore && !response.request.cacheControl.noStore
     }
   }
 }

okhttp/src/test/java/okhttp3/CacheTest.kt

@@ -43,6 +43,7 @@ import mockwebserver3.RecordedRequest
 import mockwebserver3.SocketPolicy.DisconnectAtEnd
 import mockwebserver3.junit5.internal.MockWebServerInstance
 import okhttp3.Cache.Companion.key
+import okhttp3.Cache.Companion.keyUrl
 import okhttp3.Headers.Companion.headersOf
 import okhttp3.MediaType.Companion.toMediaType
 import okhttp3.RequestBody.Companion.toRequestBody
@@ -2707,7 +2708,7 @@ class CacheTest {
         .build(),
     )
     val url = server.url("/")
-    val urlKey = key(url)
+    val urlKey = keyUrl(url)
     val entryMetadata =
       """
       $url
@@ -2756,7 +2757,7 @@ CLEAN $urlKey ${entryMetadata.length} ${entryBody.length}
   @Test
   fun testGoldenCacheHttpsResponseOkHttp27() {
     val url = server.url("/")
-    val urlKey = key(url)
+    val urlKey = keyUrl(url)
     val prefix = get().getPrefix()
     val entryMetadata =
       """
@@ -2804,7 +2805,7 @@ CLEAN $urlKey ${entryMetadata.length} ${entryBody.length}
   @Test
   fun testGoldenCacheHttpsResponseOkHttp30() {
     val url = server.url("/")
-    val urlKey = key(url)
+    val urlKey = keyUrl(url)
     val prefix = get().getPrefix()
     val entryMetadata =
       """
@@ -2856,7 +2857,7 @@ CLEAN $urlKey ${entryMetadata.length} ${entryBody.length}
   @Test
   fun testGoldenCacheHttpResponseOkHttp30() {
     val url = server.url("/")
-    val urlKey = key(url)
+    val urlKey = keyUrl(url)
     val prefix = get().getPrefix()
     val entryMetadata =
       """