Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KTOR-6001 Cache returns null when vary header has more fields in the cached response #3655

Merged
merged 2 commits into from
Jun 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions buildSrc/src/main/kotlin/test/server/tests/Cache.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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.*
Expand Down Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one more find function on line 23, should we also change it like here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the legacy storage, but I changed it as well. Thanks!

varyKeys.all { (key, value) -> it.varyKeys[key] == value }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we also check the equal size of maps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's ok if cached version has more vary properties, than the 304 response

}
}

override suspend fun findAll(url: Url): Set<CachedResponseData> = store[url] ?: emptySet()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down Expand Up @@ -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()))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,29 @@ class FileCacheTest : ClientLoader() {
}
}

@Test
fun testReuseCacheStorage() = clientTests(listOf("Js")) {
config {
install(HttpCache) {
publicStorage([email protected])
privateStorage([email protected])
}
}

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 {
Expand Down