KTOR-4894 Fixed HttpCache client plugin ignoring Request Cache-Contro…

…l directives (#3167)

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()

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
         }
 

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

ktor-client/ktor-client-core/common/test/ShouldValidateTest.kt

@@ -0,0 +1,73 @@
+/*
+ * 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)
+    }
+}

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 {