Add a cacheUrlOverride

Author: yschimke

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

@@ -22,6 +22,7 @@ import assertk.assertions.containsExactlyInAnyOrder
 import assertk.assertions.hasMessage
 import assertk.assertions.isEqualTo
 import assertk.assertions.isInstanceOf
+import assertk.assertions.isNull
 import java.io.EOFException
 import java.io.File
 import java.io.IOException
@@ -191,7 +192,6 @@ class DnsOverHttpsTest {
   // 3. successful network response
   // 4. successful stale cached GET response
   // 5. unsuccessful response
-  // TODO how closely to follow POST rules on caching?
   @Test
   fun usesCache() {
     val cache = Cache("cache".toPath(), (100 * 1024).toLong(), cacheFs)
@@ -207,14 +207,33 @@ class DnsOverHttpsTest {
         .setHeader("cache-control", "private, max-age=298")
         .build(),
     )
+    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()
+    var recordedRequest = server.takeRequest()
     assertThat(recordedRequest.method).isEqualTo("GET")
     assertThat(recordedRequest.path)
       .isEqualTo("/lookup?ct&dns=AAABAAABAAAAAAAABmdvb2dsZQNjb20AAAEAAQ")
+
     result = cachedDns.lookup("google.com")
+    assertThat(server.takeRequest(1, TimeUnit.MILLISECONDS)).isNull()
     assertThat(result).isEqualTo(listOf(address("157.240.1.18")))
+
+    result = cachedDns.lookup("www.google.com")
+    assertThat(result).containsExactly(address("157.240.1.18"))
+    recordedRequest = server.takeRequest()
+    assertThat(recordedRequest.method).isEqualTo("GET")
+    assertThat(recordedRequest.path)
+      .isEqualTo("/lookup?ct&dns=AAABAAABAAAAAAAAA3d3dwZnb29nbGUDY29tAAABAAE")
   }
 
   @Test
@@ -232,14 +251,44 @@ class DnsOverHttpsTest {
         .setHeader("cache-control", "private, max-age=298")
         .build(),
     )
+    server.enqueue(
+      dnsResponse(
+        "0000818000010003000000000567726170680866616365626f6f6b03636f6d0000010001c00c000500010" +
+          "0000a6d000603617069c012c0300005000100000cde000c04737461720463313072c012c04200010001000" +
+          "0003b00049df00112",
+      )
+        .newBuilder()
+        .setHeader("cache-control", "private, max-age=298")
+        .build(),
+    )
+    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()
+    var recordedRequest = server.takeRequest()
     assertThat(recordedRequest.method).isEqualTo("POST")
     assertThat(recordedRequest.path)
       .isEqualTo("/lookup?ct")
+
     result = cachedDns.lookup("google.com")
+    assertThat(server.takeRequest(0, TimeUnit.MILLISECONDS)).isNull()
     assertThat(result).isEqualTo(listOf(address("157.240.1.18")))
+
+    result = cachedDns.lookup("www.google.com")
+    assertThat(result).containsExactly(address("157.240.1.18"))
+    recordedRequest = server.takeRequest(0, TimeUnit.MILLISECONDS)!!
+    assertThat(recordedRequest.method).isEqualTo("POST")
+    assertThat(recordedRequest.path)
+      .isEqualTo("/lookup?ct")
   }
 
   @Test

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

@@ -126,19 +126,18 @@ 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)) {
+      val cacheNetworkRequest = networkRequest.requestForCache()
+
+      if (response.promisesBody() && CacheStrategy.isCacheable(response, cacheNetworkRequest)) {
         // Offer this request to the cache.
-        val cacheRequest = cache.put(response)
+        val cacheRequest = cache.put(response.newBuilder().request(cacheNetworkRequest).build())
         return cacheWritingResponse(cacheRequest, response).also {
           if (cacheResponse != null) {
             // This will log a conditional cache miss only.
@@ -291,12 +290,11 @@ class CacheInterceptor(internal val cache: Cache?) : Interceptor {
 }
 
 private fun Request.requestForCache(): Request {
-  return if (cacheUrlOverride != null) {
+  return cacheUrlOverride?.let {
     newBuilder()
       .get()
+      .url(it)
       .cacheUrlOverride(null)
       .build()
-  } else {
-    this
-  }
+  } ?: 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)) {
+      if (!isCacheable(cacheResponse, request)) {
         return CacheStrategy(request, null)
       }
 
@@ -292,6 +292,7 @@ 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.
@@ -333,7 +334,7 @@ class CacheStrategy internal constructor(
       }
 
       // A 'no-store' directive on request or response prevents the response from being cached.
-      return !response.cacheControl.noStore && !response.request.cacheControl.noStore
+      return !response.cacheControl.noStore && !request.cacheControl.noStore
     }
   }
 }

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

@@ -43,7 +43,6 @@ 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
@@ -2708,7 +2707,7 @@ class CacheTest {
         .build(),
     )
     val url = server.url("/")
-    val urlKey = keyUrl(url)
+    val urlKey = key(url)
     val entryMetadata =
       """
       $url
@@ -2757,7 +2756,7 @@ CLEAN $urlKey ${entryMetadata.length} ${entryBody.length}
   @Test
   fun testGoldenCacheHttpsResponseOkHttp27() {
     val url = server.url("/")
-    val urlKey = keyUrl(url)
+    val urlKey = key(url)
     val prefix = get().getPrefix()
     val entryMetadata =
       """
@@ -2805,7 +2804,7 @@ CLEAN $urlKey ${entryMetadata.length} ${entryBody.length}
   @Test
   fun testGoldenCacheHttpsResponseOkHttp30() {
     val url = server.url("/")
-    val urlKey = keyUrl(url)
+    val urlKey = key(url)
     val prefix = get().getPrefix()
     val entryMetadata =
       """
@@ -2857,7 +2856,7 @@ CLEAN $urlKey ${entryMetadata.length} ${entryBody.length}
   @Test
   fun testGoldenCacheHttpResponseOkHttp30() {
     val url = server.url("/")
-    val urlKey = keyUrl(url)
+    val urlKey = key(url)
     val prefix = get().getPrefix()
     val entryMetadata =
       """