KTOR-6001 Cache returns null when vary header has more fields in the …

…cached response (#3655)
ktorio · Jun 26, 2023 · cbf747e · cbf747e
1 parent cf1937b
commit cbf747e
Show file tree

Hide file tree

Showing 7 changed files with 79 additions and 5 deletions.
diff --git a/buildSrc/src/main/kotlin/test/server/tests/Cache.kt b/buildSrc/src/main/kotlin/test/server/tests/Cache.kt
@@ -9,6 +9,7 @@ import io.ktor.http.content.*
 import io.ktor.server.application.*
 import io.ktor.server.plugins.cachingheaders.*
 import io.ktor.server.plugins.conditionalheaders.*
+import io.ktor.server.request.*
 import io.ktor.server.response.*
 import io.ktor.server.routing.*
 import io.ktor.util.date.*
@@ -55,6 +56,19 @@ internal fun Application.cacheTestServer() {
                 call.respondText(current.toString())
             }
 
+            get("/etag-304") {
+                if (call.request.header("If-None-Match") == "My-ETAG") {
+                    call.response.header("Etag", "My-ETAG")
+                    call.response.header("Vary", "Origin")
+                    call.respond(HttpStatusCode.NotModified)
+                    return@get
+                }
+
+                call.response.header("Etag", "My-ETAG")
+                call.response.header("Vary", "Origin, Accept-Encoding")
+                call.respondText(contentType = ContentType.Application.Json) { "{}" }
+            }
+
             get("/last-modified") {
                 val current = counter.incrementAndGet()
                 val response = TextContent("$current", ContentType.Text.Plain)

diff --git a/ktor-client/ktor-client-apache/jvm/src/io/ktor/client/engine/apache/ApacheRequestProducer.kt b/ktor-client/ktor-client-apache/jvm/src/io/ktor/client/engine/apache/ApacheRequestProducer.kt
@@ -21,6 +21,7 @@ import org.apache.http.client.methods.*
 import org.apache.http.client.utils.*
 import org.apache.http.entity.*
 import org.apache.http.nio.*
+import org.apache.http.nio.ContentEncoder
 import org.apache.http.nio.protocol.*
 import org.apache.http.protocol.*
 import java.nio.*

diff --git a/...ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/UnlimitedCacheStorage.kt b/...ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/UnlimitedCacheStorage.kt
@@ -22,7 +22,9 @@ internal class UnlimitedCacheStorage : HttpCacheStorage() {
 
     override fun find(url: Url, varyKeys: Map<String, String>): HttpCacheEntry? {
         val data = store.computeIfAbsent(url) { ConcurrentSet() }
-        return data.find { it.varyKeys == varyKeys }
+        return data.find {
+            varyKeys.all { (key, value) -> it.varyKeys[key] == value }
+        }
     }
 
     override fun findByUrl(url: Url): Set<HttpCacheEntry> = store[url] ?: emptySet()
@@ -42,7 +44,9 @@ internal class UnlimitedStorage : CacheStorage {
 
     override suspend fun find(url: Url, varyKeys: Map<String, String>): CachedResponseData? {
         val data = store.computeIfAbsent(url) { ConcurrentSet() }
-        return data.find { it.varyKeys == varyKeys }
+        return data.find {
+            varyKeys.all { (key, value) -> it.varyKeys[key] == value }
+        }
     }
 
     override suspend fun findAll(url: Url): Set<CachedResponseData> = store[url] ?: emptySet()

diff --git a/...-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt b/...-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt
@@ -43,7 +43,9 @@ internal class CachingCacheStorage(
             store[url] = delegate.findAll(url)
         }
         val data = store.getValue(url)
-        return data.find { it.varyKeys == varyKeys }
+        return data.find {
+            varyKeys.all { (key, value) -> it.varyKeys[key] == value }
+        }
     }
 
     override suspend fun findAll(url: Url): Set<CachedResponseData> {
@@ -76,7 +78,10 @@ private class FileCacheStorage(
     }
 
     override suspend fun find(url: Url, varyKeys: Map<String, String>): CachedResponseData? {
-        return readCache(key(url)).find { it.varyKeys == varyKeys }
+        val data = readCache(key(url))
+        return data.find {
+            varyKeys.all { (key, value) -> it.varyKeys[key] == value }
+        }
     }
 
     private fun key(url: Url) = hex(MessageDigest.getInstance("MD5").digest(url.toString().encodeToByteArray()))

diff --git a/ktor-client/ktor-client-core/jvm/test/CachingCacheStorageTest.kt b/ktor-client/ktor-client-core/jvm/test/CachingCacheStorageTest.kt
@@ -98,7 +98,9 @@ private class InMemoryCacheStorage : CacheStorage {
     override suspend fun find(url: Url, varyKeys: Map<String, String>): CachedResponseData? {
         findCalledCount++
         val cache = store.computeIfAbsent(url) { mutableSetOf() }
-        return cache.find { it.varyKeys == varyKeys }
+        return cache.find {
+            varyKeys.all { (key, value) -> it.varyKeys[key] == value }
+        }
     }
 
     override suspend fun findAll(url: Url): Set<CachedResponseData> {

diff --git a/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/plugins/CacheTest.kt b/ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/plugins/CacheTest.kt
@@ -95,6 +95,31 @@ class CacheTest : ClientLoader() {
         }
     }
 
+    @Test
+    fun testReuseCacheStorage() = clientTests(listOf("Js")) {
+        val publicStorage = CacheStorage.Unlimited()
+        val privateStorage = CacheStorage.Unlimited()
+        config {
+            install(HttpCache) {
+                publicStorage(publicStorage)
+                privateStorage(privateStorage)
+            }
+        }
+
+        test { client ->
+            val client1 = client.config { }
+            val client2 = client.config { }
+            val url = Url("$TEST_SERVER/cache/etag-304")
+
+            val first = client1.get(url)
+            val second = client2.get(url)
+
+            assertEquals(HttpStatusCode.OK, first.status)
+            assertEquals(HttpStatusCode.OK, second.status)
+            assertEquals(first.body<String>(), second.body<String>())
+        }
+    }
+
     @Test
     fun testLastModified() = clientTests(listOf("Js")) {
         val publicStorage = CacheStorage.Unlimited()

diff --git a/ktor-client/ktor-client-tests/jvm/test/io/ktor/client/tests/FileCacheTest.kt b/ktor-client/ktor-client-tests/jvm/test/io/ktor/client/tests/FileCacheTest.kt
@@ -75,6 +75,29 @@ class FileCacheTest : ClientLoader() {
         }
     }
 
+    @Test
+    fun testReuseCacheStorage() = clientTests(listOf("Js")) {
+        config {
+            install(HttpCache) {
+                publicStorage(this@FileCacheTest.publicStorage)
+                privateStorage(this@FileCacheTest.privateStorage)
+            }
+        }
+
+        test { client ->
+            val client1 = client.config { }
+            val client2 = client.config { }
+            val url = Url("$TEST_SERVER/cache/etag-304")
+
+            val first = client1.get(url)
+            val second = client2.get(url)
+
+            assertEquals(HttpStatusCode.OK, first.status)
+            assertEquals(HttpStatusCode.OK, second.status)
+            assertEquals(first.body<String>(), second.body<String>())
+        }
+    }
+
     @Test
     fun testLongPath() = clientTests {
         config {