From 559eb438b04f32142c39a4e0a80a41d356ecfda4 Mon Sep 17 00:00:00 2001 From: Mario Daniel Ruiz Saavedra Date: Mon, 18 Aug 2025 22:42:23 -0300 Subject: [PATCH 1/9] Add QUERY caching support --- .../commonJvmAndroid/kotlin/okhttp3/Cache.kt | 52 +++++++++++++++++-- .../okhttp3/internal/http/HttpMethod.kt | 1 + .../src/jvmTest/kotlin/okhttp3/CacheTest.kt | 48 +++++++++++++++++ 3 files changed, 97 insertions(+), 4 deletions(-) diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt index 6768e4bf9a4d..8a39e0d50372 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt @@ -26,6 +26,7 @@ import java.security.cert.CertificateFactory import java.util.TreeSet import okhttp3.HttpUrl.Companion.toHttpUrlOrNull import okhttp3.MediaType.Companion.toMediaTypeOrNull +import okhttp3.RequestBody.Companion.toRequestBody import okhttp3.internal.cache.CacheRequest import okhttp3.internal.cache.CacheStrategy import okhttp3.internal.cache.DiskLruCache @@ -191,7 +192,7 @@ class Cache internal constructor( get() = cache.isClosed() internal fun get(request: Request): Response? { - val key = key(request.url) + val key = key(request) val snapshot: DiskLruCache.Snapshot = try { cache[key] ?: return null @@ -242,7 +243,7 @@ class Cache internal constructor( val entry = Entry(response) var editor: DiskLruCache.Editor? = null try { - editor = cache.edit(key(response.request.url)) ?: return null + editor = cache.edit(key(response.request)) ?: return null entry.writeTo(editor) return RealCacheRequest(editor) } catch (_: IOException) { @@ -253,7 +254,7 @@ class Cache internal constructor( @Throws(IOException::class) internal fun remove(request: Request) { - cache.remove(key(request.url)) + cache.remove(key(request)) } internal fun update( @@ -464,6 +465,7 @@ class Cache internal constructor( private class Entry { private val url: HttpUrl + private val body : RequestBody? private val varyHeaders: Headers private val requestMethod: String private val protocol: Protocol @@ -543,6 +545,21 @@ class Cache internal constructor( } varyHeaders = varyHeadersBuilder.build() + val bodyLength = source.readDecimalLong() + body = when (bodyLength) { + -1L -> { + null + } + 0L -> { + RequestBody.EMPTY + } + else -> { + source.readByteArray(bodyLength).toRequestBody() + } + } + + source.readByte() // Read the trailing '\n' after the body. + val statusLine = StatusLine.parse(source.readUtf8LineStrict()) protocol = statusLine.protocol code = statusLine.code @@ -584,6 +601,7 @@ class Cache internal constructor( constructor(response: Response) { this.url = response.request.url + this.body = response.request.body this.varyHeaders = response.varyHeaders() this.requestMethod = response.request.method this.protocol = response.protocol @@ -609,6 +627,20 @@ class Cache internal constructor( .writeByte('\n'.code) } + if (requestMethod == "QUERY") { + // Write the body of a QUERY request. + if (body == null) { + sink.writeDecimalLong(-1L).writeByte('\n'.code) + } else { + + sink.writeDecimalLong(body.contentLength()) + body.writeTo(sink) + sink.writeByte('\n'.code) + } + } else { + // Write the body of a GET request. + sink.writeDecimalLong(0L).writeByte('\n'.code) + } sink.writeUtf8(StatusLine(protocol, code, message).toString()).writeByte('\n'.code) sink.writeDecimalLong((responseHeaders.size + 2).toLong()).writeByte('\n'.code) for (i in 0 until responseHeaders.size) { @@ -688,7 +720,7 @@ class Cache internal constructor( fun response(snapshot: DiskLruCache.Snapshot): Response { val contentType = responseHeaders["Content-Type"] val contentLength = responseHeaders["Content-Length"] - val cacheRequest = Request(url, varyHeaders, requestMethod) + val cacheRequest = Request(url, varyHeaders, requestMethod, body) return Response .Builder() .request(cacheRequest) @@ -752,6 +784,18 @@ class Cache internal constructor( .md5() .hex() + internal fun key(request: Request): String = + if ( + request.method == "QUERY" && + request.body != null && + !request.body.isOneShot() + ) { + key(request.url) + "_" + Buffer().also { request.body.writeTo(it) }.md5().hex() + } else { + // Reading this kind of body would consume it, so we can't cache it. + key(request.url) + } + @Throws(IOException::class) internal fun readInt(source: BufferedSource): Int { try { diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/HttpMethod.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/HttpMethod.kt index 0d3df7510a9a..4096ba376144 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/HttpMethod.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/HttpMethod.kt @@ -35,6 +35,7 @@ object HttpMethod { method == "PUT" || method == "PATCH" || method == "PROPPATCH" || + method == "QUERY" || // WebDAV method == "REPORT" ) diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt b/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt index 6a40e72db66c..3c7e57f63fbf 100644 --- a/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt +++ b/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt @@ -577,6 +577,54 @@ class CacheTest { assertThat(recordedRequest4.requestLine).isEqualTo("QUERY /bar HTTP/1.1") } + @Test + fun queryRequestsCacheTheBody() { + server.enqueue( + MockResponse + .Builder() + .addHeader("Cache-Control: max-age=60") + .body("ABC") + .build(), + ) + server.enqueue( + MockResponse + .Builder() + .addHeader("Cache-Control: max-age=60") + .body("DEF") + .build(), + ) + + val url = server.url("/same") + + // First QUERY request with body "foo" + val request1 = + Request + .Builder() + .url(url) + .query("foo".toRequestBody()) + .build() + val response1 = client.newCall(request1).execute() + assertThat(response1.body.string()).isEqualTo("ABC") + + // Second QUERY request with body "bar" + val request2 = + Request + .Builder() + .url(url) + .query("bar".toRequestBody()) + .build() + val response2 = client.newCall(request2).execute() + assertThat(response2.body.string()).isEqualTo("DEF") + + // Third QUERY request with body "foo" again, should be cached and return "ABC" + val response3 = client.newCall(request1).execute() + assertThat(response3.body.string()).isEqualTo("ABC") + + // Fourth QUERY request with body "bar" again, should be cached and return "DEF" + val response4 = client.newCall(request2).execute() + assertThat(response4.body.string()).isEqualTo("DEF") + } + @Test fun secureResponseCachingAndRedirects() { server.useHttps(handshakeCertificates.sslSocketFactory()) From df386b4f10ccc1bd6b9781af12a82aaf1eb2fa1d Mon Sep 17 00:00:00 2001 From: Mario Daniel Ruiz Saavedra Date: Tue, 19 Aug 2025 23:55:26 -0300 Subject: [PATCH 2/9] Address PR comments (cacheOverride, broken tests) --- .../commonJvmAndroid/kotlin/okhttp3/Cache.kt | 11 ++-- .../internal/cache/CacheInterceptor.kt | 24 ++++++-- .../src/jvmTest/kotlin/okhttp3/CacheTest.kt | 59 +++++++++++++++++++ 3 files changed, 83 insertions(+), 11 deletions(-) diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt index 8a39e0d50372..6824b561dd01 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt @@ -465,7 +465,7 @@ class Cache internal constructor( private class Entry { private val url: HttpUrl - private val body : RequestBody? + private val body: RequestBody? private val varyHeaders: Headers private val requestMethod: String private val protocol: Protocol @@ -546,7 +546,8 @@ class Cache internal constructor( varyHeaders = varyHeadersBuilder.build() val bodyLength = source.readDecimalLong() - body = when (bodyLength) { + body = + when (bodyLength) { -1L -> { null } @@ -556,7 +557,7 @@ class Cache internal constructor( else -> { source.readByteArray(bodyLength).toRequestBody() } - } + } source.readByte() // Read the trailing '\n' after the body. @@ -630,16 +631,16 @@ class Cache internal constructor( if (requestMethod == "QUERY") { // Write the body of a QUERY request. if (body == null) { + // Should not happen, but if it does, write a -1 to indicate no body. sink.writeDecimalLong(-1L).writeByte('\n'.code) } else { - sink.writeDecimalLong(body.contentLength()) body.writeTo(sink) sink.writeByte('\n'.code) } } else { // Write the body of a GET request. - sink.writeDecimalLong(0L).writeByte('\n'.code) + sink.writeDecimalLong(-1L).writeByte('\n'.code) } sink.writeUtf8(StatusLine(protocol, code, message).toString()).writeByte('\n'.code) sink.writeDecimalLong((responseHeaders.size + 2).toLong()).writeByte('\n'.code) diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/CacheInterceptor.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/CacheInterceptor.kt index 8e6581bdc264..c0a012033ddb 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/CacheInterceptor.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/CacheInterceptor.kt @@ -299,12 +299,24 @@ class CacheInterceptor( private fun Request.requestForCache(): Request { val cacheUrlOverride = cacheUrlOverride - return if (cacheUrlOverride != null && (method == "GET" || method == "POST")) { - newBuilder() - .get() - .url(cacheUrlOverride) - .cacheUrlOverride(null) - .build() + return if (cacheUrlOverride != null) { + when (method) { + "GET", "POST" -> { + newBuilder() + .get() + .url(cacheUrlOverride) + .cacheUrlOverride(null) + .build() + } + "QUERY" -> { + newBuilder() + .query(body!!) + .url(cacheUrlOverride) + .cacheUrlOverride(null) + .build() + } + else -> this + } } else { this } diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt b/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt index 3c7e57f63fbf..fa5e321a0c82 100644 --- a/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt +++ b/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt @@ -35,6 +35,7 @@ import java.util.Locale import java.util.TimeZone import java.util.concurrent.TimeUnit import java.util.concurrent.atomic.AtomicReference +import java.util.stream.Stream import javax.net.ssl.HostnameVerifier import kotlin.test.assertFailsWith import mockwebserver3.MockResponse @@ -45,6 +46,7 @@ import mockwebserver3.junit5.StartStop import okhttp3.Cache.Companion.key import okhttp3.Headers.Companion.headersOf import okhttp3.MediaType.Companion.toMediaType +import okhttp3.MediaType.Companion.toMediaTypeOrNull import okhttp3.RequestBody.Companion.toRequestBody import okhttp3.internal.addHeaderLenient import okhttp3.internal.cacheGet @@ -52,6 +54,7 @@ import okhttp3.internal.platform.Platform.Companion.get import okhttp3.java.net.cookiejar.JavaNetCookieJar import okhttp3.testing.PlatformRule import okio.Buffer +import okio.BufferedSink import okio.FileSystem import okio.ForwardingFileSystem import okio.GzipSink @@ -625,6 +628,52 @@ class CacheTest { assertThat(response4.body.string()).isEqualTo("DEF") } + @Test + fun oneshotBodyIsNotCachedForQueryRequest() { + server.enqueue( + MockResponse + .Builder() + .addHeader("Cache-Control: max-age=60") + .body("ABC") + .build(), + ) + + val url = server.url("/same") + + // QUERY request with body "foo" + val oneshotRequest = + object : RequestBody() { + val internalBody = Stream.of("foo") + + override fun isOneShot(): Boolean = true + + override fun contentType(): MediaType? = "application/text-plain".toMediaTypeOrNull() + + override fun writeTo(sink: BufferedSink) { + internalBody.forEach { item -> + sink.writeUtf8(item) + } + } + } + + val request1 = + Request + .Builder() + .url(url) + .query(oneshotRequest) + .build() + val response1 = client.newCall(request1).execute() + assertThat(response1.body.string()).isEqualTo("ABC") + + // QUERY request with body "foo" again, should not be cached + val response2 = client.newCall(request1).execute() + assertThat(response2.body.string()).isEqualTo("ABC") + + // Check that the cache did not store the response + assertThat(cache.requestCount()).isEqualTo(2) + assertThat(cache.hitCount()).isEqualTo(0) + } + @Test fun secureResponseCachingAndRedirects() { server.useHttps(handshakeCertificates.sslSocketFactory()) @@ -1181,6 +1230,16 @@ class CacheTest { testRequestMethod("GET", true) } + @Test + fun requestMethodQueryIsCached() { + testRequestMethod("QUERY", true) + } + + @Test + fun requestMethodQueryIsCachedWithOverride() { + testRequestMethod("QUERY", true, withOverride = true) + } + @Test fun requestMethodHeadIsNotCached() { // We could support this but choose not to for implementation simplicity From ee25b26c60f280b091fca0eb73fc08cc57bc14ed Mon Sep 17 00:00:00 2001 From: Mario Daniel Ruiz Saavedra Date: Wed, 20 Aug 2025 19:27:07 -0300 Subject: [PATCH 3/9] Move cache oneshot body checks to the get method --- okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt index 6824b561dd01..edfdc1a4d4d3 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt @@ -192,6 +192,11 @@ class Cache internal constructor( get() = cache.isClosed() internal fun get(request: Request): Response? { + if (request.body != null && request.body.isOneShot() && request.method == "QUERY") { + // Don't cache one-shot QUERY requests, since we need to cache by using the body, + // and we can't consume the body twice + return null + } val key = key(request) val snapshot: DiskLruCache.Snapshot = try { @@ -788,12 +793,10 @@ class Cache internal constructor( internal fun key(request: Request): String = if ( request.method == "QUERY" && - request.body != null && - !request.body.isOneShot() + request.body != null ) { key(request.url) + "_" + Buffer().also { request.body.writeTo(it) }.md5().hex() } else { - // Reading this kind of body would consume it, so we can't cache it. key(request.url) } From 84080d03b858c7730e4b06317846e585a67213d8 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Sat, 30 Aug 2025 11:53:07 +0100 Subject: [PATCH 4/9] Avoid changing cache format --- .../commonJvmAndroid/kotlin/okhttp3/Cache.kt | 88 ++++++-------- .../kotlin/okhttp3/Request.kt | 3 + .../okhttp3/internal/UnreadableRequestBody.kt | 50 ++++++++ .../internal/cache/CacheInterceptor.kt | 24 +--- .../okhttp3/internal/http/HttpMethod.kt | 4 + .../src/jvmTest/kotlin/okhttp3/CacheTest.kt | 115 ++++++++---------- 6 files changed, 155 insertions(+), 129 deletions(-) create mode 100644 okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/UnreadableRequestBody.kt diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt index edfdc1a4d4d3..4455390af841 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt @@ -26,7 +26,7 @@ import java.security.cert.CertificateFactory import java.util.TreeSet import okhttp3.HttpUrl.Companion.toHttpUrlOrNull import okhttp3.MediaType.Companion.toMediaTypeOrNull -import okhttp3.RequestBody.Companion.toRequestBody +import okhttp3.internal.UnreadableRequestBody import okhttp3.internal.cache.CacheRequest import okhttp3.internal.cache.CacheStrategy import okhttp3.internal.cache.DiskLruCache @@ -192,7 +192,7 @@ class Cache internal constructor( get() = cache.isClosed() internal fun get(request: Request): Response? { - if (request.body != null && request.body.isOneShot() && request.method == "QUERY") { + if (request.body != null && request.body.isOneShot()) { // Don't cache one-shot QUERY requests, since we need to cache by using the body, // and we can't consume the body twice return null @@ -234,13 +234,19 @@ class Cache internal constructor( return null } - if (requestMethod != "GET" && requestMethod != "QUERY") { + if (!HttpMethod.isCacheable(requestMethod)) { // Don't cache non-GET and non-QUERY responses. We're technically allowed to cache HEAD // requests and some POST requests, but the complexity of doing so is high and the benefit // is low. return null } + if (response.request.body != null && response.request.body.isOneShot()) { + // Don't cache one-shot QUERY requests, since we need to cache by using the body, + // and we can't consume the body twice + return null + } + if (response.hasVaryAll()) { return null } @@ -470,7 +476,6 @@ class Cache internal constructor( private class Entry { private val url: HttpUrl - private val body: RequestBody? private val varyHeaders: Headers private val requestMethod: String private val protocol: Protocol @@ -550,22 +555,6 @@ class Cache internal constructor( } varyHeaders = varyHeadersBuilder.build() - val bodyLength = source.readDecimalLong() - body = - when (bodyLength) { - -1L -> { - null - } - 0L -> { - RequestBody.EMPTY - } - else -> { - source.readByteArray(bodyLength).toRequestBody() - } - } - - source.readByte() // Read the trailing '\n' after the body. - val statusLine = StatusLine.parse(source.readUtf8LineStrict()) protocol = statusLine.protocol code = statusLine.code @@ -607,7 +596,6 @@ class Cache internal constructor( constructor(response: Response) { this.url = response.request.url - this.body = response.request.body this.varyHeaders = response.varyHeaders() this.requestMethod = response.request.method this.protocol = response.protocol @@ -633,20 +621,6 @@ class Cache internal constructor( .writeByte('\n'.code) } - if (requestMethod == "QUERY") { - // Write the body of a QUERY request. - if (body == null) { - // Should not happen, but if it does, write a -1 to indicate no body. - sink.writeDecimalLong(-1L).writeByte('\n'.code) - } else { - sink.writeDecimalLong(body.contentLength()) - body.writeTo(sink) - sink.writeByte('\n'.code) - } - } else { - // Write the body of a GET request. - sink.writeDecimalLong(-1L).writeByte('\n'.code) - } sink.writeUtf8(StatusLine(protocol, code, message).toString()).writeByte('\n'.code) sink.writeDecimalLong((responseHeaders.size + 2).toLong()).writeByte('\n'.code) for (i in 0 until responseHeaders.size) { @@ -726,7 +700,12 @@ class Cache internal constructor( fun response(snapshot: DiskLruCache.Snapshot): Response { val contentType = responseHeaders["Content-Type"] val contentLength = responseHeaders["Content-Length"] - val cacheRequest = Request(url, varyHeaders, requestMethod, body) + val cacheRequest = Request( + url = url, + headers = varyHeaders, + method = requestMethod, + body = if (HttpMethod.requiresRequestBody(requestMethod)) UnreadableRequestBody() else null + ) return Response .Builder() .request(cacheRequest) @@ -782,24 +761,37 @@ class Cache internal constructor( private const val ENTRY_BODY = 1 private const val ENTRY_COUNT = 2 - @JvmStatic - fun key(url: HttpUrl): String = - url - .toString() - .encodeUtf8() - .md5() - .hex() + internal fun key(request: Request): String { + return key2(request).also { + println("request key $it") + } + } - internal fun key(request: Request): String = + /** Returns the cache key for a request */ + internal fun key2(request: Request): String { if ( - request.method == "QUERY" && + // Request such as PUT, DELETE that invalidates a GET + !HttpMethod.invalidatesCache(request.method) && + + // QUERY request that considers the request body in the cache key + HttpMethod.isCacheable(request.method) && + HttpMethod.permitsRequestBody(request.method) && request.body != null ) { - key(request.url) + "_" + Buffer().also { request.body.writeTo(it) }.md5().hex() - } else { - key(request.url) + return Buffer().apply { + writeUtf8(request.method) + writeUtf8(request.url.toString()) + request.body.writeTo(this) + }.md5().hex() } + return request.url + .toString() + .encodeUtf8() + .md5() + .hex() + } + @Throws(IOException::class) internal fun readInt(source: BufferedSource): Int { try { diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Request.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Request.kt index 071490becf87..b37b212a948c 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Request.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Request.kt @@ -317,6 +317,9 @@ class Request internal constructor( open fun patch(body: RequestBody): Builder = method("PATCH", body) + /** + * A QUERY request with a body. If `body.isOneShot()` is true, then caching will be disabled. + */ open fun query(body: RequestBody): Builder = method("QUERY", body) open fun method( diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/UnreadableRequestBody.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/UnreadableRequestBody.kt new file mode 100644 index 000000000000..8af7a45fd00f --- /dev/null +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/UnreadableRequestBody.kt @@ -0,0 +1,50 @@ +/* + * Copyright (C) 2022 Square, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package okhttp3.internal + +import okhttp3.RequestBody +import okio.Buffer +import okio.BufferedSink +import okio.Source +import okio.Timeout + +internal class UnreadableRequestBody() : RequestBody(), + Source { + override fun contentType() = failUnsupported() + + override fun contentLength() = failUnsupported() + + override fun writeTo(sink: BufferedSink) = failUnsupported() + + override fun read( + sink: Buffer, + byteCount: Long, + ): Long = failUnsupported() + + private fun failUnsupported(): Nothing { + throw IllegalStateException( + """ + |Unreadable RequestBody! These Request objects have bodies that are stripped: + | * Response.cacheResponse.request + """.trimMargin(), + ) + } + + override fun timeout() = Timeout.NONE + + override fun close() { + } +} diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/CacheInterceptor.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/CacheInterceptor.kt index c0a012033ddb..a5e8a1b5939a 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/CacheInterceptor.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/CacheInterceptor.kt @@ -299,24 +299,12 @@ class CacheInterceptor( private fun Request.requestForCache(): Request { val cacheUrlOverride = cacheUrlOverride - return if (cacheUrlOverride != null) { - when (method) { - "GET", "POST" -> { - newBuilder() - .get() - .url(cacheUrlOverride) - .cacheUrlOverride(null) - .build() - } - "QUERY" -> { - newBuilder() - .query(body!!) - .url(cacheUrlOverride) - .cacheUrlOverride(null) - .build() - } - else -> this - } + return if (cacheUrlOverride != null && (method == "GET" || method == "POST" || method == "QUERY")) { + newBuilder() + .get() + .url(cacheUrlOverride) + .cacheUrlOverride(null) + .build() } else { this } diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/HttpMethod.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/HttpMethod.kt index 4096ba376144..08e592b5a3f1 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/HttpMethod.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/HttpMethod.kt @@ -46,4 +46,8 @@ object HttpMethod { fun redirectsWithBody(method: String): Boolean = method == "PROPFIND" fun redirectsToGet(method: String): Boolean = method != "PROPFIND" + + fun isCacheable(requestMethod: String): Boolean { + return requestMethod == "GET" || requestMethod == "QUERY" + } } diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt b/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt index fa5e321a0c82..6ea4399b2ced 100644 --- a/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt +++ b/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt @@ -55,6 +55,7 @@ import okhttp3.java.net.cookiejar.JavaNetCookieJar import okhttp3.testing.PlatformRule import okio.Buffer import okio.BufferedSink +import okio.ByteString.Companion.encodeUtf8 import okio.FileSystem import okio.ForwardingFileSystem import okio.GzipSink @@ -514,13 +515,6 @@ class CacheTest { .body("ABC") .build(), ) - server.enqueue( - MockResponse - .Builder() - .code(HttpURLConnection.HTTP_MOVED_PERM) - .addHeader("Location: /foo") - .build(), - ) // QUERY responses server.enqueue( MockResponse @@ -529,55 +523,28 @@ class CacheTest { .body("DEF") .build(), ) - server.enqueue( - MockResponse - .Builder() - .code(HttpURLConnection.HTTP_MOVED_PERM) - .addHeader("Location: /baz") - .build(), - ) - val request1 = + val requestGet1 = Request .Builder() .url(server.url("/foo")) .get() .build() - val response1 = client.newCall(request1).execute() + val response1 = client.newCall(requestGet1).execute() assertThat(response1.body.string()).isEqualTo("ABC") val recordedRequest1 = server.takeRequest() assertThat(recordedRequest1.requestLine).isEqualTo("GET /foo HTTP/1.1") - val request2 = - Request - .Builder() - .url(server.url("/bar")) - .get() - .build() - val response2 = client.newCall(request2).execute() - assertThat(response2.body.string()).isEqualTo("ABC") - val recordedRequest2 = server.takeRequest() - assertThat(recordedRequest2.requestLine).isEqualTo("GET /bar HTTP/1.1") - val request3 = + val requestQuery1 = Request .Builder() - .url(server.url("/baz")) - .query(RequestBody.EMPTY) - .build() - val response3 = client.newCall(request3).execute() - assertThat(response3.body.string()).isEqualTo("DEF") - val recordedRequest3 = server.takeRequest() - assertThat(recordedRequest3.requestLine).isEqualTo("QUERY /baz HTTP/1.1") - val request4 = - Request - .Builder() - .url(server.url("/bar")) + .url(server.url("/foo")) .query(RequestBody.EMPTY) .build() - val response4 = client.newCall(request4).execute() - assertThat(response4.body.string()).isEqualTo("DEF") - val recordedRequest4 = server.takeRequest() - assertThat(recordedRequest4.requestLine).isEqualTo("QUERY /bar HTTP/1.1") + val response2 = client.newCall(requestQuery1).execute() + assertThat(response2.body.string()).isEqualTo("DEF") + val recordedRequest2 = server.takeRequest() + assertThat(recordedRequest2.requestLine).isEqualTo("QUERY /foo HTTP/1.1") } @Test @@ -634,46 +601,60 @@ class CacheTest { MockResponse .Builder() .addHeader("Cache-Control: max-age=60") - .body("ABC") + .body("ABC1") + .build(), + ) + server.enqueue( + MockResponse + .Builder() + .addHeader("Cache-Control: max-age=60") + .body("ABC2") .build(), ) val url = server.url("/same") // QUERY request with body "foo" - val oneshotRequest = - object : RequestBody() { - val internalBody = Stream.of("foo") - - override fun isOneShot(): Boolean = true - - override fun contentType(): MediaType? = "application/text-plain".toMediaTypeOrNull() - - override fun writeTo(sink: BufferedSink) { - internalBody.forEach { item -> - sink.writeUtf8(item) - } - } - } + val body = "foo" val request1 = Request .Builder() .url(url) - .query(oneshotRequest) + .query(body.toOneShotRequestBody()) .build() val response1 = client.newCall(request1).execute() - assertThat(response1.body.string()).isEqualTo("ABC") + assertThat(response1.body.string()).isEqualTo("ABC1") // QUERY request with body "foo" again, should not be cached - val response2 = client.newCall(request1).execute() - assertThat(response2.body.string()).isEqualTo("ABC") + val request2 = + Request + .Builder() + .url(url) + .query(body.toOneShotRequestBody()) + .build() + val response2 = client.newCall(request2).execute() + assertThat(response2.body.string()).isEqualTo("ABC2") // Check that the cache did not store the response assertThat(cache.requestCount()).isEqualTo(2) assertThat(cache.hitCount()).isEqualTo(0) } + private fun String.toOneShotRequestBody(): RequestBody = object : RequestBody() { + val internalBody = Stream.of(this) + + override fun isOneShot(): Boolean = true + + override fun contentType(): MediaType? = "application/text-plain".toMediaTypeOrNull() + + override fun writeTo(sink: BufferedSink) { + internalBody.forEach { item -> + sink.writeUtf8(this@toOneShotRequestBody) + } + } + } + @Test fun secureResponseCachingAndRedirects() { server.useHttps(handshakeCertificates.sslSocketFactory()) @@ -1312,7 +1293,7 @@ class CacheTest { val response1 = client.newCall(request).execute() response1.body.close() assertThat(response1.header("X-Response-ID")).isEqualTo("1") - val response2 = get(url) + val response2 = client.newCall(request).execute() response2.body.close() if (expectCached) { assertThat(response2.header("X-Response-ID")).isEqualTo("1") @@ -1322,7 +1303,7 @@ class CacheTest { } private fun requestBodyOrNull(requestMethod: String): RequestBody? = - if (requestMethod == "POST" || requestMethod == "PUT") "foo".toRequestBody("text/plain".toMediaType()) else null + if (requestMethod == "POST" || requestMethod == "PUT" || requestMethod == "QUERY") "foo".toRequestBody("text/plain".toMediaType()) else null @Test fun postInvalidatesCache() { @@ -3135,6 +3116,14 @@ class CacheTest { assertThat(response.body.string()).isEqualTo("body") } + fun key(url: HttpUrl): String = + url + .toString() + .encodeUtf8() + .md5() + .hex() + .also { println("url key $it") } + /** * Old implementations of OkHttp's response cache wrote header fields like ":status: 200 OK". This * broke our cached response parser because it split on the first colon. This regression test From f6139031b10c5eb1d37041dcb96a10b61f78941a Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Sat, 30 Aug 2025 11:59:59 +0100 Subject: [PATCH 5/9] Cleanup --- okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt | 8 +------- .../kotlin/okhttp3/internal/UnreadableRequestBody.kt | 2 +- .../kotlin/okhttp3/internal/cache/CacheInterceptor.kt | 2 +- 3 files changed, 3 insertions(+), 9 deletions(-) diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt index 4455390af841..8bf55602ca5e 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt @@ -761,14 +761,8 @@ class Cache internal constructor( private const val ENTRY_BODY = 1 private const val ENTRY_COUNT = 2 - internal fun key(request: Request): String { - return key2(request).also { - println("request key $it") - } - } - /** Returns the cache key for a request */ - internal fun key2(request: Request): String { + internal fun key(request: Request): String { if ( // Request such as PUT, DELETE that invalidates a GET !HttpMethod.invalidatesCache(request.method) && diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/UnreadableRequestBody.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/UnreadableRequestBody.kt index 8af7a45fd00f..e6169c355c1e 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/UnreadableRequestBody.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/UnreadableRequestBody.kt @@ -1,5 +1,5 @@ /* - * Copyright (C) 2022 Square, Inc. + * Copyright (C) 2025 Block, Inc. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/CacheInterceptor.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/CacheInterceptor.kt index a5e8a1b5939a..953e0b09ba1d 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/CacheInterceptor.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/CacheInterceptor.kt @@ -299,7 +299,7 @@ class CacheInterceptor( private fun Request.requestForCache(): Request { val cacheUrlOverride = cacheUrlOverride - return if (cacheUrlOverride != null && (method == "GET" || method == "POST" || method == "QUERY")) { + return if (cacheUrlOverride != null) { newBuilder() .get() .url(cacheUrlOverride) From 50a1d19fa7658c6b0133e1e0d5c1f3b19a414fb8 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Sat, 30 Aug 2025 12:07:47 +0100 Subject: [PATCH 6/9] Stream request body in hash --- .../commonJvmAndroid/kotlin/okhttp3/Cache.kt | 27 +++++++++++-------- .../okhttp3/internal/UnreadableRequestBody.kt | 10 +++---- .../okhttp3/internal/http/HttpMethod.kt | 4 +-- .../src/jvmTest/kotlin/okhttp3/CacheTest.kt | 26 +++++++++++------- 4 files changed, 39 insertions(+), 28 deletions(-) diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt index 8bf55602ca5e..cbdb073e71cf 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt @@ -46,10 +46,12 @@ import okio.ByteString.Companion.toByteString import okio.FileSystem import okio.ForwardingSink import okio.ForwardingSource +import okio.HashingSink import okio.Path import okio.Path.Companion.toOkioPath import okio.Sink import okio.Source +import okio.blackholeSink import okio.buffer /** @@ -700,12 +702,13 @@ class Cache internal constructor( fun response(snapshot: DiskLruCache.Snapshot): Response { val contentType = responseHeaders["Content-Type"] val contentLength = responseHeaders["Content-Length"] - val cacheRequest = Request( - url = url, - headers = varyHeaders, - method = requestMethod, - body = if (HttpMethod.requiresRequestBody(requestMethod)) UnreadableRequestBody() else null - ) + val cacheRequest = + Request( + url = url, + headers = varyHeaders, + method = requestMethod, + body = if (HttpMethod.requiresRequestBody(requestMethod)) UnreadableRequestBody() else null, + ) return Response .Builder() .request(cacheRequest) @@ -772,11 +775,13 @@ class Cache internal constructor( HttpMethod.permitsRequestBody(request.method) && request.body != null ) { - return Buffer().apply { - writeUtf8(request.method) - writeUtf8(request.url.toString()) - request.body.writeTo(this) - }.md5().hex() + val hashingSink = HashingSink.md5(blackholeSink()) + hashingSink.buffer().use { + it.writeUtf8(request.method) + it.writeUtf8(request.url.toString()) + request.body.writeTo(it) + } + return hashingSink.hash.hex() } return request.url diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/UnreadableRequestBody.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/UnreadableRequestBody.kt index e6169c355c1e..c0531b9a1b6b 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/UnreadableRequestBody.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/UnreadableRequestBody.kt @@ -21,7 +21,8 @@ import okio.BufferedSink import okio.Source import okio.Timeout -internal class UnreadableRequestBody() : RequestBody(), +internal class UnreadableRequestBody : + RequestBody(), Source { override fun contentType() = failUnsupported() @@ -34,14 +35,13 @@ internal class UnreadableRequestBody() : RequestBody(), byteCount: Long, ): Long = failUnsupported() - private fun failUnsupported(): Nothing { + private fun failUnsupported(): Nothing = throw IllegalStateException( """ |Unreadable RequestBody! These Request objects have bodies that are stripped: | * Response.cacheResponse.request - """.trimMargin(), - ) - } + """.trimMargin(), + ) override fun timeout() = Timeout.NONE diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/HttpMethod.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/HttpMethod.kt index 08e592b5a3f1..77d2d9da5a73 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/HttpMethod.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/http/HttpMethod.kt @@ -47,7 +47,5 @@ object HttpMethod { fun redirectsToGet(method: String): Boolean = method != "PROPFIND" - fun isCacheable(requestMethod: String): Boolean { - return requestMethod == "GET" || requestMethod == "QUERY" - } + fun isCacheable(requestMethod: String): Boolean = requestMethod == "GET" || requestMethod == "QUERY" } diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt b/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt index 6ea4399b2ced..57a576a4d5a6 100644 --- a/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt +++ b/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt @@ -641,19 +641,20 @@ class CacheTest { assertThat(cache.hitCount()).isEqualTo(0) } - private fun String.toOneShotRequestBody(): RequestBody = object : RequestBody() { - val internalBody = Stream.of(this) + private fun String.toOneShotRequestBody(): RequestBody = + object : RequestBody() { + val internalBody = Stream.of(this) - override fun isOneShot(): Boolean = true + override fun isOneShot(): Boolean = true - override fun contentType(): MediaType? = "application/text-plain".toMediaTypeOrNull() + override fun contentType(): MediaType? = "application/text-plain".toMediaTypeOrNull() - override fun writeTo(sink: BufferedSink) { - internalBody.forEach { item -> - sink.writeUtf8(this@toOneShotRequestBody) + override fun writeTo(sink: BufferedSink) { + internalBody.forEach { item -> + sink.writeUtf8(this@toOneShotRequestBody) + } } } - } @Test fun secureResponseCachingAndRedirects() { @@ -1303,7 +1304,14 @@ class CacheTest { } private fun requestBodyOrNull(requestMethod: String): RequestBody? = - if (requestMethod == "POST" || requestMethod == "PUT" || requestMethod == "QUERY") "foo".toRequestBody("text/plain".toMediaType()) else null + if (requestMethod == "POST" || + requestMethod == "PUT" || + requestMethod == "QUERY" + ) { + "foo".toRequestBody("text/plain".toMediaType()) + } else { + null + } @Test fun postInvalidatesCache() { From 52ba409da31cb0b21d96aa05b8f0b2829aba93b5 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Sat, 30 Aug 2025 12:09:20 +0100 Subject: [PATCH 7/9] delimit with \0 --- okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt index cbdb073e71cf..143fc4164044 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt @@ -778,7 +778,9 @@ class Cache internal constructor( val hashingSink = HashingSink.md5(blackholeSink()) hashingSink.buffer().use { it.writeUtf8(request.method) + it.writeByte(0) it.writeUtf8(request.url.toString()) + it.writeByte(0) request.body.writeTo(it) } return hashingSink.hash.hex() From b657407d744ed817328d14abd1658c2d37f6d52d Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Sat, 30 Aug 2025 12:21:37 +0100 Subject: [PATCH 8/9] Make sure request method cache test, also checks against GET --- .../kotlin/okhttp3/internal/cache/CacheInterceptor.kt | 3 ++- okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt | 11 +++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/CacheInterceptor.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/CacheInterceptor.kt index 953e0b09ba1d..1a66c2d4cf20 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/CacheInterceptor.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/cache/CacheInterceptor.kt @@ -299,7 +299,8 @@ class CacheInterceptor( private fun Request.requestForCache(): Request { val cacheUrlOverride = cacheUrlOverride - return if (cacheUrlOverride != null) { + // Allow POST caching only when there is a cacheUrlOverride + return if (cacheUrlOverride != null && (HttpMethod.isCacheable(method) || method == "POST")) { newBuilder() .get() .url(cacheUrlOverride) diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt b/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt index 57a576a4d5a6..08d178d8f3c2 100644 --- a/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt +++ b/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt @@ -1301,6 +1301,17 @@ class CacheTest { } else { assertThat(response2.header("X-Response-ID")).isEqualTo("2") } + if (!expectCached) { + server.enqueue( + MockResponse + .Builder() + .addHeader("X-Response-ID: 3") + .build(), + ) + val response3 = get(url) + response3.body.close() + assertThat(response3.header("X-Response-ID")).isEqualTo("3") + } } private fun requestBodyOrNull(requestMethod: String): RequestBody? = From 8a14f7ff8212e2b1b414c0382bb8f2fbbb6555b0 Mon Sep 17 00:00:00 2001 From: Yuri Schimke Date: Sat, 30 Aug 2025 12:23:33 +0100 Subject: [PATCH 9/9] avoid API change --- okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt | 8 ++++++++ okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt | 9 --------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt index 143fc4164044..34b00bd722b1 100644 --- a/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt +++ b/okhttp/src/commonJvmAndroid/kotlin/okhttp3/Cache.kt @@ -764,6 +764,14 @@ class Cache internal constructor( private const val ENTRY_BODY = 1 private const val ENTRY_COUNT = 2 + @JvmStatic + fun key(url: HttpUrl): String = + url + .toString() + .encodeUtf8() + .md5() + .hex() + /** Returns the cache key for a request */ internal fun key(request: Request): String { if ( diff --git a/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt b/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt index 08d178d8f3c2..d9b084391b95 100644 --- a/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt +++ b/okhttp/src/jvmTest/kotlin/okhttp3/CacheTest.kt @@ -55,7 +55,6 @@ import okhttp3.java.net.cookiejar.JavaNetCookieJar import okhttp3.testing.PlatformRule import okio.Buffer import okio.BufferedSink -import okio.ByteString.Companion.encodeUtf8 import okio.FileSystem import okio.ForwardingFileSystem import okio.GzipSink @@ -3135,14 +3134,6 @@ class CacheTest { assertThat(response.body.string()).isEqualTo("body") } - fun key(url: HttpUrl): String = - url - .toString() - .encodeUtf8() - .md5() - .hex() - .also { println("url key $it") } - /** * Old implementations of OkHttp's response cache wrote header fields like ":status: 200 OK". This * broke our cached response parser because it split on the first colon. This regression test