From 331eabd19d1e3aae35d5eda15b03503943354a27 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Sat, 3 Feb 2024 11:18:25 +0000 Subject: [PATCH 01/12] Add a cacheUrlOverride --- .../okhttp3/dnsoverhttps/DnsOverHttps.kt | 26 +++++++--- .../okhttp3/dnsoverhttps/DnsOverHttpsTest.kt | 51 +++++++++++++++++++ okhttp/src/main/kotlin/okhttp3/Request.kt | 15 ++++++ .../internal/cache/CacheInterceptor.kt | 19 ++++++- .../okhttp3/internal/cache/CacheStrategy.kt | 5 +- okhttp/src/test/java/okhttp3/CacheTest.kt | 9 ++-- 6 files changed, 110 insertions(+), 15 deletions(-) diff --git a/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt b/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt index 2944ddebb947..ea817e4a1fed 100644 --- a/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt +++ b/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 { diff --git a/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt b/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt index 6cdac3bc1fe4..867510009a87 100644 --- a/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt +++ b/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() } diff --git a/okhttp/src/main/kotlin/okhttp3/Request.kt b/okhttp/src/main/kotlin/okhttp3/Request.kt index 4e399789c562..39a81996893f 100644 --- a/okhttp/src/main/kotlin/okhttp3/Request.kt +++ b/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, 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, 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) } } diff --git a/okhttp/src/main/kotlin/okhttp3/internal/cache/CacheInterceptor.kt b/okhttp/src/main/kotlin/okhttp3/internal/cache/CacheInterceptor.kt index e86c2470ba0c..839d8f85158f 100644 --- a/okhttp/src/main/kotlin/okhttp3/internal/cache/CacheInterceptor.kt +++ b/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 + } +} diff --git a/okhttp/src/main/kotlin/okhttp3/internal/cache/CacheStrategy.kt b/okhttp/src/main/kotlin/okhttp3/internal/cache/CacheStrategy.kt index 7e447cc57dfb..a5bc1c9d26b8 100644 --- a/okhttp/src/main/kotlin/okhttp3/internal/cache/CacheStrategy.kt +++ b/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 } } } diff --git a/okhttp/src/test/java/okhttp3/CacheTest.kt b/okhttp/src/test/java/okhttp3/CacheTest.kt index e6921ddf495e..3f51b6a820ff 100644 --- a/okhttp/src/test/java/okhttp3/CacheTest.kt +++ b/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 = """ From 73a5befad460f8c1b47a9ecf34a403731775b087 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Sat, 3 Feb 2024 11:39:28 +0000 Subject: [PATCH 02/12] Add a cacheUrlOverride --- .../okhttp3/dnsoverhttps/DnsOverHttpsTest.kt | 55 ++++++++++++++++++- .../internal/cache/CacheInterceptor.kt | 16 +++--- .../okhttp3/internal/cache/CacheStrategy.kt | 5 +- okhttp/src/test/java/okhttp3/CacheTest.kt | 9 ++- 4 files changed, 66 insertions(+), 19 deletions(-) diff --git a/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt b/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt index 867510009a87..5b5d5acb0cd9 100644 --- a/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt +++ b/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 diff --git a/okhttp/src/main/kotlin/okhttp3/internal/cache/CacheInterceptor.kt b/okhttp/src/main/kotlin/okhttp3/internal/cache/CacheInterceptor.kt index 839d8f85158f..c4470421af07 100644 --- a/okhttp/src/main/kotlin/okhttp3/internal/cache/CacheInterceptor.kt +++ b/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 } diff --git a/okhttp/src/main/kotlin/okhttp3/internal/cache/CacheStrategy.kt b/okhttp/src/main/kotlin/okhttp3/internal/cache/CacheStrategy.kt index a5bc1c9d26b8..7e447cc57dfb 100644 --- a/okhttp/src/main/kotlin/okhttp3/internal/cache/CacheStrategy.kt +++ b/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 } } } diff --git a/okhttp/src/test/java/okhttp3/CacheTest.kt b/okhttp/src/test/java/okhttp3/CacheTest.kt index 3f51b6a820ff..e6921ddf495e 100644 --- a/okhttp/src/test/java/okhttp3/CacheTest.kt +++ b/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 = """ From 2587c3606c8a5fbeaaee7e6b05b39a42182cfb07 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Sat, 3 Feb 2024 11:48:01 +0000 Subject: [PATCH 03/12] Add a cacheUrlOverride --- .../okhttp3/dnsoverhttps/DnsOverHttps.kt | 18 ++++++------- .../okhttp3/dnsoverhttps/DnsOverHttpsTest.kt | 26 +------------------ okhttp/src/main/kotlin/okhttp3/Request.kt | 3 ++- 3 files changed, 11 insertions(+), 36 deletions(-) diff --git a/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt b/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt index ea817e4a1fed..cc3872e5e194 100644 --- a/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt +++ b/okhttp-dnsoverhttps/src/main/kotlin/okhttp3/dnsoverhttps/DnsOverHttps.kt @@ -186,9 +186,7 @@ class DnsOverHttps internal constructor( throw unknownHostException } - private fun getCacheOnlyResponse( - request: Request, - ): Response? { + private fun getCacheOnlyResponse(request: Request): Response? { if (client.cache != null) { try { // Use the cache without hitting the network first @@ -200,10 +198,11 @@ class DnsOverHttps internal constructor( var cacheUrl = request.url - val cacheRequest = request.newBuilder() - .cacheControl(onlyIfCached) - .cacheUrlOverride(cacheUrl) - .build() + val cacheRequest = + request.newBuilder() + .cacheControl(onlyIfCached) + .cacheUrlOverride(cacheUrl) + .build() val cacheResponse = client.newCall(cacheRequest).execute() @@ -258,7 +257,7 @@ class DnsOverHttps internal constructor( url(url) .cacheUrlOverride( url.newBuilder() - .addQueryParameter("hostname", hostname).build() + .addQueryParameter("hostname", hostname).build(), ) .post(query.toRequestBody(DNS_MESSAGE)) } else { @@ -326,8 +325,7 @@ 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 { diff --git a/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt b/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt index 5b5d5acb0cd9..00d07822f42f 100644 --- a/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt +++ b/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt @@ -32,12 +32,9 @@ 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 @@ -59,27 +56,6 @@ 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 @@ -343,7 +319,7 @@ class DnsOverHttpsTest { private fun buildLocalhost( bootstrapClient: OkHttpClient, includeIPv6: Boolean, - post: Boolean = false + post: Boolean = false, ): DnsOverHttps { val url = server.url("/lookup?ct") return DnsOverHttps.Builder().client(bootstrapClient) diff --git a/okhttp/src/main/kotlin/okhttp3/Request.kt b/okhttp/src/main/kotlin/okhttp3/Request.kt index 39a81996893f..8c990565d8cf 100644 --- a/okhttp/src/main/kotlin/okhttp3/Request.kt +++ b/okhttp/src/main/kotlin/okhttp3/Request.kt @@ -327,7 +327,8 @@ class Request internal constructor(builder: Builder) { * * If set, this will also allow caching for POST requests. */ - fun cacheUrlOverride(cacheUrlOverride: HttpUrl?) = apply { + fun cacheUrlOverride(cacheUrlOverride: HttpUrl?) = + apply { this.cacheUrlOverride = cacheUrlOverride } From 4e436b7027a4ba001cb641e7fa1f23b492d152b7 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Sat, 3 Feb 2024 11:49:48 +0000 Subject: [PATCH 04/12] Add a cacheUrlOverride --- .../okhttp3/dnsoverhttps/DnsOverHttpsTest.kt | 72 +++++++------------ 1 file changed, 24 insertions(+), 48 deletions(-) diff --git a/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt b/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt index 00d07822f42f..78bbaaac78fc 100644 --- a/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt +++ b/okhttp-dnsoverhttps/src/test/java/okhttp3/dnsoverhttps/DnsOverHttpsTest.kt @@ -173,26 +173,20 @@ class DnsOverHttpsTest { val cache = Cache("cache".toPath(), (100 * 1024).toLong(), cacheFs) val cachedClient = bootstrapClient.newBuilder().cache(cache).build() val cachedDns = buildLocalhost(cachedClient, false) - server.enqueue( - dnsResponse( - "0000818000010003000000000567726170680866616365626f6f6b03636f6d0000010001c00c000500010" + - "0000a6d000603617069c012c0300005000100000cde000c04737461720463313072c012c04200010001000" + - "0003b00049df00112", - ) - .newBuilder() - .setHeader("cache-control", "private, max-age=298") - .build(), - ) - server.enqueue( - dnsResponse( - "0000818000010003000000000567726170680866616365626f6f6b03636f6d0000010001c00c000500010" + - "0000a6d000603617069c012c0300005000100000cde000c04737461720463313072c012c04200010001000" + - "0003b00049df00112", + + repeat(2) { + server.enqueue( + dnsResponse( + "0000818000010003000000000567726170680866616365626f6f6b03636f6d0000010001c00c000500010" + + "0000a6d000603617069c012c0300005000100000cde000c04737461720463313072c012c04200010001000" + + "0003b00049df00112", + ) + .newBuilder() + .setHeader("cache-control", "private, max-age=298") + .build(), ) - .newBuilder() - .setHeader("cache-control", "private, max-age=298") - .build(), - ) + } + var result = cachedDns.lookup("google.com") assertThat(result).containsExactly(address("157.240.1.18")) var recordedRequest = server.takeRequest() @@ -217,36 +211,18 @@ class DnsOverHttpsTest { 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(), - ) - server.enqueue( - dnsResponse( - "0000818000010003000000000567726170680866616365626f6f6b03636f6d0000010001c00c000500010" + - "0000a6d000603617069c012c0300005000100000cde000c04737461720463313072c012c04200010001000" + - "0003b00049df00112", + repeat(2) { + server.enqueue( + dnsResponse( + "0000818000010003000000000567726170680866616365626f6f6b03636f6d0000010001c00c000500010" + + "0000a6d000603617069c012c0300005000100000cde000c04737461720463313072c012c04200010001000" + + "0003b00049df00112", + ) + .newBuilder() + .setHeader("cache-control", "private, max-age=298") + .build(), ) - .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")) From 93faf850ce0131b0a3fed6d1ed1dc4e5205a0552 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Sat, 3 Feb 2024 11:57:30 +0000 Subject: [PATCH 05/12] Add a cacheUrlOverride --- okhttp/api/okhttp.api | 2 ++ 1 file changed, 2 insertions(+) diff --git a/okhttp/api/okhttp.api b/okhttp/api/okhttp.api index 8b7d85d030e6..f784a8736968 100644 --- a/okhttp/api/okhttp.api +++ b/okhttp/api/okhttp.api @@ -1030,6 +1030,7 @@ public final class okhttp3/Request { public synthetic fun (Lokhttp3/HttpUrl;Lokhttp3/Headers;Ljava/lang/String;Lokhttp3/RequestBody;ILkotlin/jvm/internal/DefaultConstructorMarker;)V public final fun body ()Lokhttp3/RequestBody; public final fun cacheControl ()Lokhttp3/CacheControl; + public final fun cacheUrlOverride ()Lokhttp3/HttpUrl; public final fun header (Ljava/lang/String;)Ljava/lang/String; public final fun headers ()Lokhttp3/Headers; public final fun headers (Ljava/lang/String;)Ljava/util/List; @@ -1048,6 +1049,7 @@ public class okhttp3/Request$Builder { public fun addHeader (Ljava/lang/String;Ljava/lang/String;)Lokhttp3/Request$Builder; public fun build ()Lokhttp3/Request; public fun cacheControl (Lokhttp3/CacheControl;)Lokhttp3/Request$Builder; + public final fun cacheUrlOverride (Lokhttp3/HttpUrl;)Lokhttp3/Request$Builder; public final fun delete ()Lokhttp3/Request$Builder; public fun delete (Lokhttp3/RequestBody;)Lokhttp3/Request$Builder; public static synthetic fun delete$default (Lokhttp3/Request$Builder;Lokhttp3/RequestBody;ILjava/lang/Object;)Lokhttp3/Request$Builder; From f14ab40a7f4b252c86c4d390a967250d2aae6cee Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Sat, 3 Feb 2024 12:19:00 +0000 Subject: [PATCH 06/12] Add Cache Test --- okhttp/src/test/java/okhttp3/CacheTest.kt | 68 ++++++++++++++++++++--- 1 file changed, 61 insertions(+), 7 deletions(-) diff --git a/okhttp/src/test/java/okhttp3/CacheTest.kt b/okhttp/src/test/java/okhttp3/CacheTest.kt index e6921ddf495e..18812fb1b5dd 100644 --- a/okhttp/src/test/java/okhttp3/CacheTest.kt +++ b/okhttp/src/test/java/okhttp3/CacheTest.kt @@ -982,44 +982,51 @@ class CacheTest { @Test fun requestMethodOptionsIsNotCached() { - testRequestMethod("OPTIONS", false) + testRequestMethod("OPTIONS", false, true) } @Test fun requestMethodGetIsCached() { - testRequestMethod("GET", true) + testRequestMethod("GET", true, true) } @Test fun requestMethodHeadIsNotCached() { // We could support this but choose not to for implementation simplicity - testRequestMethod("HEAD", false) + testRequestMethod("HEAD", false, true) } @Test fun requestMethodPostIsNotCached() { // We could support this but choose not to for implementation simplicity - testRequestMethod("POST", false) + testRequestMethod("POST", false, true) + } + + @Test + fun requestMethodPostIsNotCachedUnlessOverride() { + // We could support this but choose not to for implementation simplicity + testRequestMethod("POST", true, withOverride = true) } @Test fun requestMethodPutIsNotCached() { - testRequestMethod("PUT", false) + testRequestMethod("PUT", false, true) } @Test fun requestMethodDeleteIsNotCached() { - testRequestMethod("DELETE", false) + testRequestMethod("DELETE", false, true) } @Test fun requestMethodTraceIsNotCached() { - testRequestMethod("TRACE", false) + testRequestMethod("TRACE", false, true) } private fun testRequestMethod( requestMethod: String, expectCached: Boolean, + withOverride: Boolean = false, ) { // 1. Seed the cache (potentially). // 2. Expect a cache hit or miss. @@ -1038,6 +1045,11 @@ class CacheTest { val request = Request.Builder() .url(url) + .apply { + if (withOverride) { + cacheUrlOverride(url) + } + } .method(requestMethod, requestBodyOrNull(requestMethod)) .build() val response1 = client.newCall(request).execute() @@ -3250,6 +3262,48 @@ CLEAN $urlKey ${entryMetadata.length} ${entryBody.length} ) } + @Test + fun getHasCorrectResponseIsCorrect() { + val request = Request(server.url("/abc")) + + val response = testBasicCachingRules(request) + + assertThat(response.request.url).isEqualTo(request.url) + assertThat(response.cacheResponse!!.request.url).isEqualTo(request.url) + } + + @Test + fun postWithOverrideResponseIsCorrect() { + val url = server.url("/abc?token=123") + val cacheUrlOverride = url.newBuilder().removeAllQueryParameters("token").build() + + val request = + Request.Builder() + .url(url) + .method("POST", "XYZ".toRequestBody()) + .cacheUrlOverride(cacheUrlOverride) + .build() + + val response = testBasicCachingRules(request) + + assertThat(response.request.url).isEqualTo(request.url) + assertThat(response.cacheResponse!!.request.url).isEqualTo(cacheUrlOverride) + } + + private fun testBasicCachingRules(request: Request): Response { + val mockResponse = + MockResponse.Builder() + .addHeader("Last-Modified: " + formatDate(-1, TimeUnit.HOURS)) + .addHeader("Expires: " + formatDate(1, TimeUnit.HOURS)) + .status("HTTP/1.1 200 Fantastic") + server.enqueue(mockResponse.build()) + + client.newCall(request).execute().use { + it.body.bytes() + } + return client.newCall(request).execute() + } + private operator fun get(url: HttpUrl): Response { val request = Request.Builder() From 69cd233cec06334838ffc964e827d9ca48036837 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Sat, 3 Feb 2024 12:25:01 +0000 Subject: [PATCH 07/12] Add Cache Test --- okhttp/src/test/java/okhttp3/CacheTest.kt | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/okhttp/src/test/java/okhttp3/CacheTest.kt b/okhttp/src/test/java/okhttp3/CacheTest.kt index 18812fb1b5dd..d53f65c81aae 100644 --- a/okhttp/src/test/java/okhttp3/CacheTest.kt +++ b/okhttp/src/test/java/okhttp3/CacheTest.kt @@ -982,30 +982,24 @@ class CacheTest { @Test fun requestMethodOptionsIsNotCached() { - testRequestMethod("OPTIONS", false, true) + testRequestMethod("OPTIONS", false) } @Test fun requestMethodGetIsCached() { - testRequestMethod("GET", true, true) + testRequestMethod("GET", true) } @Test fun requestMethodHeadIsNotCached() { // We could support this but choose not to for implementation simplicity - testRequestMethod("HEAD", false, true) + testRequestMethod("HEAD", false) } @Test fun requestMethodPostIsNotCached() { // We could support this but choose not to for implementation simplicity - testRequestMethod("POST", false, true) - } - - @Test - fun requestMethodPostIsNotCachedUnlessOverride() { - // We could support this but choose not to for implementation simplicity - testRequestMethod("POST", true, withOverride = true) + testRequestMethod("POST", false) } @Test @@ -1015,12 +1009,12 @@ class CacheTest { @Test fun requestMethodDeleteIsNotCached() { - testRequestMethod("DELETE", false, true) + testRequestMethod("DELETE", false) } @Test fun requestMethodTraceIsNotCached() { - testRequestMethod("TRACE", false, true) + testRequestMethod("TRACE", false) } private fun testRequestMethod( From b88c544ffa184ca6e451121a641835cbf0f9dd76 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Sat, 3 Feb 2024 16:07:24 +0000 Subject: [PATCH 08/12] Fix test --- .../okhttp3/internal/cache/CacheInterceptor.kt | 10 +++++++--- okhttp/src/test/java/okhttp3/CacheTest.kt | 13 ++++++++++++- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/okhttp/src/main/kotlin/okhttp3/internal/cache/CacheInterceptor.kt b/okhttp/src/main/kotlin/okhttp3/internal/cache/CacheInterceptor.kt index c4470421af07..47c4bf013123 100644 --- a/okhttp/src/main/kotlin/okhttp3/internal/cache/CacheInterceptor.kt +++ b/okhttp/src/main/kotlin/okhttp3/internal/cache/CacheInterceptor.kt @@ -290,11 +290,15 @@ class CacheInterceptor(internal val cache: Cache?) : Interceptor { } private fun Request.requestForCache(): Request { - return cacheUrlOverride?.let { + val cacheUrlOverride = cacheUrlOverride + + return if (cacheUrlOverride != null && (method == "GET" || method == "POST")) { newBuilder() .get() - .url(it) + .url(cacheUrlOverride) .cacheUrlOverride(null) .build() - } ?: this + } else { + this + } } diff --git a/okhttp/src/test/java/okhttp3/CacheTest.kt b/okhttp/src/test/java/okhttp3/CacheTest.kt index d53f65c81aae..5a6dc7219ad4 100644 --- a/okhttp/src/test/java/okhttp3/CacheTest.kt +++ b/okhttp/src/test/java/okhttp3/CacheTest.kt @@ -1002,9 +1002,20 @@ class CacheTest { testRequestMethod("POST", false) } + @Test + fun requestMethodPostIsNotCachedUnlessOverriden() { + // Supported via cacheUrlOverride + testRequestMethod("POST", true, withOverride = true) + } + @Test fun requestMethodPutIsNotCached() { - testRequestMethod("PUT", false, true) + testRequestMethod("PUT", false) + } + + @Test + fun requestMethodPutIsNotCachedEvenWithOverride() { + testRequestMethod("PUT", false, withOverride = true) } @Test From b4538fbeb953a99ef095df55b1da0b9b3481e2bd Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Thu, 21 Mar 2024 21:50:39 +0000 Subject: [PATCH 09/12] Update okhttp/src/main/kotlin/okhttp3/Request.kt Co-authored-by: Jesse Wilson --- okhttp/src/main/kotlin/okhttp3/Request.kt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/okhttp/src/main/kotlin/okhttp3/Request.kt b/okhttp/src/main/kotlin/okhttp3/Request.kt index 8c990565d8cf..57cdc93f331e 100644 --- a/okhttp/src/main/kotlin/okhttp3/Request.kt +++ b/okhttp/src/main/kotlin/okhttp3/Request.kt @@ -325,7 +325,8 @@ class Request internal constructor(builder: Builder) { * 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. + * Note that POST requests will not be sent to the server if this URL is set + * and matches a cached response. */ fun cacheUrlOverride(cacheUrlOverride: HttpUrl?) = apply { From cd425282a4e8fa8deb57d63c5875ce912d84cdd6 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Thu, 21 Mar 2024 21:53:00 +0000 Subject: [PATCH 10/12] Update okhttp/src/test/java/okhttp3/CacheTest.kt Co-authored-by: Jesse Wilson --- okhttp/src/test/java/okhttp3/CacheTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/okhttp/src/test/java/okhttp3/CacheTest.kt b/okhttp/src/test/java/okhttp3/CacheTest.kt index 5a6dc7219ad4..abad342bf30c 100644 --- a/okhttp/src/test/java/okhttp3/CacheTest.kt +++ b/okhttp/src/test/java/okhttp3/CacheTest.kt @@ -1003,7 +1003,7 @@ class CacheTest { } @Test - fun requestMethodPostIsNotCachedUnlessOverriden() { + fun requestMethodPostIsNotCachedUnlessOverridden() { // Supported via cacheUrlOverride testRequestMethod("POST", true, withOverride = true) } From 286fd7079c02dccaffc21763f86324f74765fa26 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Thu, 21 Mar 2024 21:53:07 +0000 Subject: [PATCH 11/12] Update okhttp/src/test/java/okhttp3/CacheTest.kt Co-authored-by: Jesse Wilson --- okhttp/src/test/java/okhttp3/CacheTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/okhttp/src/test/java/okhttp3/CacheTest.kt b/okhttp/src/test/java/okhttp3/CacheTest.kt index abad342bf30c..b61740a6964b 100644 --- a/okhttp/src/test/java/okhttp3/CacheTest.kt +++ b/okhttp/src/test/java/okhttp3/CacheTest.kt @@ -3268,7 +3268,7 @@ CLEAN $urlKey ${entryMetadata.length} ${entryBody.length} } @Test - fun getHasCorrectResponseIsCorrect() { + fun getHasCorrectResponse() { val request = Request(server.url("/abc")) val response = testBasicCachingRules(request) From ba9b0fa3afc87849d937538304f8cf4152fa5d7a Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Thu, 21 Mar 2024 21:53:23 +0000 Subject: [PATCH 12/12] Update okhttp/src/test/java/okhttp3/CacheTest.kt Co-authored-by: Jesse Wilson --- okhttp/src/test/java/okhttp3/CacheTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/okhttp/src/test/java/okhttp3/CacheTest.kt b/okhttp/src/test/java/okhttp3/CacheTest.kt index b61740a6964b..707f0fcebd95 100644 --- a/okhttp/src/test/java/okhttp3/CacheTest.kt +++ b/okhttp/src/test/java/okhttp3/CacheTest.kt @@ -3278,7 +3278,7 @@ CLEAN $urlKey ${entryMetadata.length} ${entryBody.length} } @Test - fun postWithOverrideResponseIsCorrect() { + fun postWithOverrideResponse() { val url = server.url("/abc?token=123") val cacheUrlOverride = url.newBuilder().removeAllQueryParameters("token").build()