KTOR-4894 Fixed HttpCache client plugin ignoring Request Cache-Control directives (#3167)

a77241ad · Rustam · GitHub · 82eaa1e8 · a77241ad · a77241ad
--- a/buildSrc/src/main/kotlin/test/server/tests/Cache.kt
+++ b/buildSrc/src/main/kotlin/test/server/tests/Cache.kt
@@ -45,7 +45,7 @@ internal fun Application.cacheTestServer() {
            }

            /**
-             * Return same etag for first 2 responses.
+             * Return same etag for the first 2 responses.
             */
            get("/etag") {
                val maxAge = call.request.queryParameters["max-age"]?.toIntOrNull()

--- a/ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.kt
+++ b/ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.kt
@@ -91,7 +91,7 @@ public class HttpCache private constructor(
                    return@intercept
                }
                val cachedCall = cache.produceResponse().call
-                val validateStatus = cache.shouldValidate(context)
+                val validateStatus = shouldValidate(cache.expires, cache.response.headers, context.headers)

                if (validateStatus == ValidateStatus.ShouldNotValidate) {
                    proceedWithCache(scope, cachedCall)
@@ -185,10 +185,11 @@ public class HttpCache private constructor(
    private suspend fun cacheResponse(response: HttpResponse): HttpResponse {
        val request = response.call.request
        val responseCacheControl: List<HeaderValue> = response.cacheControl()
+        val requestCacheControl: List<HeaderValue> = request.cacheControl()

        val storage = if (CacheControl.PRIVATE in responseCacheControl) privateStorage else publicStorage

-        if (CacheControl.NO_STORE in responseCacheControl) {
+        if (CacheControl.NO_STORE in responseCacheControl || CacheControl.NO_STORE in requestCacheControl) {
            return response
        }


--- a/ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCacheEntry.kt
+++ b/ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCacheEntry.kt
@@ -5,12 +5,12 @@
 package io.ktor.client.plugins.cache

 import io.ktor.client.call.*
-import io.ktor.client.request.*
 import io.ktor.client.statement.*
 import io.ktor.http.*
 import io.ktor.util.*
 import io.ktor.util.date.*
 import io.ktor.utils.io.core.*
+import kotlin.collections.*

 @OptIn(InternalAPI::class)
 internal suspend fun HttpCacheEntry(response: HttpResponse): HttpCacheEntry {
@@ -90,15 +90,27 @@ internal fun HttpResponse.cacheExpires(fallback: () -> GMTDate = { GMTDate() }):
    } ?: fallback()
 }

-internal fun HttpCacheEntry.shouldValidate(request: HttpRequestBuilder): ValidateStatus {
-    val cacheControl = parseHeaderValue(responseHeaders[HttpHeaders.CacheControl])
-    val validMillis = expires.timestamp - getTimeMillis()
+internal fun shouldValidate(
+    cacheExpires: GMTDate,
+    responseHeaders: Headers,
+    requestHeaders: HeadersBuilder
+): ValidateStatus {
+    val responseCacheControl = parseHeaderValue(responseHeaders[HttpHeaders.CacheControl])
+    val requestCacheControl = parseHeaderValue(requestHeaders[HttpHeaders.CacheControl])

-    if (CacheControl.NO_CACHE in cacheControl) return ValidateStatus.ShouldValidate
+    if (CacheControl.NO_CACHE in requestCacheControl) return ValidateStatus.ShouldValidate
+
+    val requestMaxAge = requestCacheControl.firstOrNull { it.value.startsWith("max-age=") }
+        ?.value?.split("=")
+        ?.get(1)?.let { it.toIntOrNull() ?: 0 }
+    if (requestMaxAge == 0) return ValidateStatus.ShouldValidate
+
+    val validMillis = cacheExpires.timestamp - getTimeMillis()
+
+    if (CacheControl.NO_CACHE in responseCacheControl) return ValidateStatus.ShouldValidate
    if (validMillis > 0) return ValidateStatus.ShouldNotValidate
-    if (CacheControl.MUST_REVALIDATE in cacheControl) return ValidateStatus.ShouldValidate
+    if (CacheControl.MUST_REVALIDATE in responseCacheControl) return ValidateStatus.ShouldValidate

-    val requestCacheControl = parseHeaderValue(request.headers[HttpHeaders.CacheControl])
    val maxStale = requestCacheControl.firstOrNull { it.value.startsWith("max-stale=") }
        ?.value?.substring("max-stale=".length)
        ?.toIntOrNull() ?: 0

--- a/ktor-client/ktor-client-core/common/test/ShouldValidateTest.kt
+++ b/ktor-client/ktor-client-core/common/test/ShouldValidateTest.kt
+/*
+ * Copyright 2014-2022 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
+ */
+
+package io.ktor.client.plugins.cache.tests
+
+import io.ktor.client.plugins.cache.*
+import io.ktor.http.*
+import io.ktor.util.date.*
+import kotlin.test.*
+
+class ShouldValidateTest {
+
+    @Test
+    fun testNoCacheInRequestReturnsShouldValidate() {
+        val result = shouldValidate(
+            GMTDate(),
+            Headers.Empty,
+            HeadersBuilder().apply { append(HttpHeaders.CacheControl, "no-cache") }
+        )
+        assertEquals(ValidateStatus.ShouldValidate, result)
+    }
+
+    @Test
+    fun testNoCacheInResponseReturnsShouldValidate() {
+        val result = shouldValidate(
+            GMTDate(),
+            headersOf(HttpHeaders.CacheControl, "no-cache"),
+            HeadersBuilder()
+        )
+        assertEquals(ValidateStatus.ShouldValidate, result)
+    }
+
+    @Test
+    fun testMaxAge0InRequestReturnsShouldValidate() {
+        val result = shouldValidate(
+            GMTDate(),
+            headersOf(HttpHeaders.CacheControl, "max-age=0"),
+            HeadersBuilder()
+        )
+        assertEquals(ValidateStatus.ShouldValidate, result)
+    }
+
+    @Test
+    fun testExpiresInPastAndMustRevalidateReturnsShouldValidate() {
+        val result = shouldValidate(
+            GMTDate(getTimeMillis() - 1),
+            headersOf(HttpHeaders.CacheControl, "must-revalidate"),
+            HeadersBuilder()
+        )
+        assertEquals(ValidateStatus.ShouldValidate, result)
+    }
+
+    @Test
+    fun testExpiresInPastAndMaxStaleInFutureReturnsShouldWarn() {
+        val result = shouldValidate(
+            GMTDate(getTimeMillis() - 1),
+            Headers.Empty,
+            HeadersBuilder().apply { append(HttpHeaders.CacheControl, "max-stale=10") }
+        )
+        assertEquals(ValidateStatus.ShouldWarn, result)
+    }
+
+    @Test
+    fun testExpiresInPastReturnsShouldValidate() {
+        val result = shouldValidate(
+            GMTDate(getTimeMillis() - 1),
+            Headers.Empty,
+            HeadersBuilder()
+        )
+        assertEquals(ValidateStatus.ShouldValidate, result)
+    }
+}
--- 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
@@ -384,6 +384,87 @@ class CacheTest : ClientLoader() {
        }
    }

+    @Test
+    fun testNoStoreRequest() = clientTests(listOf("Js")) {
+        config {
+            install(HttpCache) {
+                storage = this
+            }
+        }
+
+        test { client ->
+            val url = Url("$TEST_SERVER/cache/etag")
+
+            val first = client.get(url) {
+                header(HttpHeaders.CacheControl, "no-store")
+            }.body<String>()
+            assertEquals(0, storage!!.publicStorage.findByUrl(url).size)
+
+            val second = client.get(url).body<String>()
+            assertEquals(1, storage!!.publicStorage.findByUrl(url).size)
+
+            assertNotEquals(first, second)
+        }
+    }
+
+    @Test
+    fun testNoCacheRequest() = clientTests(listOf("Js")) {
+        config {
+            install(HttpCache) {
+                storage = this
+            }
+        }
+
+        test { client ->
+            var requestsCount = 0
+            client.sendPipeline.intercept(HttpSendPipeline.Engine) {
+                requestsCount++
+            }
+
+            val url = Url("$TEST_SERVER/cache/etag?max-age=30")
+
+            val first = client.get(url).body<String>()
+            assertEquals(1, storage!!.publicStorage.findByUrl(url).size)
+
+            val second = client.get(url) {
+                header(HttpHeaders.CacheControl, "no-cache")
+            }.body<String>()
+            assertEquals(1, storage!!.publicStorage.findByUrl(url).size)
+
+            assertEquals(2, requestsCount)
+            assertEquals(first, second)
+        }
+    }
+
+    @Test
+    fun testRequestWithMaxAge0() = clientTests(listOf("Js")) {
+        config {
+            install(HttpCache) {
+                storage = this
+            }
+        }
+
+        test { client ->
+            var requestsCount = 0
+            client.sendPipeline.intercept(HttpSendPipeline.Engine) {
+                requestsCount++
+            }
+
+            val url = Url("$TEST_SERVER/cache/etag?max-age=30")
+
+            val first = client.get(url).body<String>()
+            assertEquals(1, storage!!.publicStorage.findByUrl(url).size)
+
+            val second = client.get(url) {
+                header(HttpHeaders.CacheControl, "max-age=0")
+            }.body<String>()
+            assertEquals(1, storage!!.publicStorage.findByUrl(url).size)
+
+            assertEquals(2, requestsCount)
+            assertEquals(first, second)
+        }
+    }
+
    @Test
    fun testExpires() = clientTests {
        config {
@@ -496,7 +577,6 @@ class CacheTest : ClientLoader() {
        }
    }

-    @OptIn(InternalAPI::class)
    @Test
    fun testWithLogging() = clientTests {
        config {