diff --git a/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/cache/ETagRevalidation.kt b/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/cache/ETagRevalidation.kt index 286eb1c98..a189d5d4d 100644 --- a/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/cache/ETagRevalidation.kt +++ b/apps/flipcash/core/src/main/kotlin/com/flipcash/app/core/cache/ETagRevalidation.kt @@ -36,16 +36,17 @@ fun ImageRequest.Builder.etagRevalidation() = apply { } /** - * A [CacheStrategy] that performs conditional revalidation using `ETag` / `Last-Modified` - * headers stored in Coil's disk-cache metadata. + * A [CacheStrategy] that respects `Cache-Control: max-age` and performs conditional + * revalidation using `ETag` / `Last-Modified` headers stored in Coil's disk-cache metadata. * * **Read behaviour:** * - If [ETagRevalidationKey] is **not** set on the request, the cached response is returned * immediately (same as [CacheStrategy.DEFAULT]). - * - If the key **is** set, cached `etag` and `last-modified` headers are copied into - * `If-None-Match` / `If-Modified-Since` on the outgoing [NetworkRequest], forcing a - * network round-trip. The server can then respond with `304` to avoid re-downloading the - * image body. + * - If the key **is** set but the cached entry is still fresh (within `max-age`), the cached + * response is returned without a network trip. + * - If the entry is stale, cached `etag` and `last-modified` headers are copied into + * `If-None-Match` / `If-Modified-Since` on the outgoing [NetworkRequest]. The server can + * then respond with `304` to avoid re-downloading the image body. * * **Write behaviour:** delegates entirely to [CacheStrategy.DEFAULT], which merges headers on * `304` responses and writes new bodies on `2xx` responses. @@ -62,6 +63,10 @@ class ETagCacheStrategy : CacheStrategy { return CacheStrategy.ReadResult(cacheResponse) } + if (!isStale(cacheResponse)) { + return CacheStrategy.ReadResult(cacheResponse) + } + val cachedHeaders = cacheResponse.headers val etag = cachedHeaders["etag"] val lastModified = cachedHeaders["last-modified"] @@ -91,6 +96,27 @@ class ETagCacheStrategy : CacheStrategy { ): CacheStrategy.WriteResult { return CacheStrategy.DEFAULT.write(cacheResponse, networkRequest, networkResponse, options) } + + companion object { + private val MAX_AGE_REGEX = Regex("""max-age\s*=\s*(\d+)""") + + /** + * Returns `true` if the cached response has exceeded its `max-age`, or if + * `responseMillis` is missing (pre-existing cache entries). + */ + internal fun isStale(cacheResponse: NetworkResponse): Boolean { + val responseMillis = cacheResponse.responseMillis + if (responseMillis <= 0L) return true + + val cacheControl = cacheResponse.headers["cache-control"] ?: return true + val maxAgeSeconds = MAX_AGE_REGEX.find(cacheControl) + ?.groupValues?.get(1)?.toLongOrNull() + ?: return true + + val ageMillis = System.currentTimeMillis() - responseMillis + return ageMillis > maxAgeSeconds * 1000 + } + } } /** diff --git a/apps/flipcash/core/src/test/kotlin/com/flipcash/app/core/cache/ETagCacheStrategyTest.kt b/apps/flipcash/core/src/test/kotlin/com/flipcash/app/core/cache/ETagCacheStrategyTest.kt index 098b48140..e3fcfb565 100644 --- a/apps/flipcash/core/src/test/kotlin/com/flipcash/app/core/cache/ETagCacheStrategyTest.kt +++ b/apps/flipcash/core/src/test/kotlin/com/flipcash/app/core/cache/ETagCacheStrategyTest.kt @@ -13,8 +13,10 @@ import org.robolectric.RuntimeEnvironment import org.robolectric.annotation.Config import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFalse import kotlin.test.assertNotNull import kotlin.test.assertNull +import kotlin.test.assertTrue @OptIn(ExperimentalCoilApi::class) @RunWith(RobolectricTestRunner::class) @@ -30,52 +32,121 @@ class ETagCacheStrategyTest { return Options(context = RuntimeEnvironment.getApplication(), extras = extras) } - private val baseCacheResponse = NetworkResponse( - code = 200, - headers = NetworkHeaders.EMPTY, - ) + private fun freshCacheResponse( + maxAgeSeconds: Long = 3600, + etag: String? = null, + lastModified: String? = null, + ): NetworkResponse { + val headers = NetworkHeaders.Builder() + .set("cache-control", "public, max-age=$maxAgeSeconds") + if (etag != null) headers.set("etag", etag) + if (lastModified != null) headers.set("last-modified", lastModified) + return NetworkResponse( + code = 200, + responseMillis = System.currentTimeMillis() - 60_000, // 1 minute ago + headers = headers.build(), + ) + } + + private fun staleCacheResponse( + maxAgeSeconds: Long = 3600, + etag: String? = null, + lastModified: String? = null, + ): NetworkResponse { + val headers = NetworkHeaders.Builder() + .set("cache-control", "public, max-age=$maxAgeSeconds") + if (etag != null) headers.set("etag", etag) + if (lastModified != null) headers.set("last-modified", lastModified) + return NetworkResponse( + code = 200, + responseMillis = System.currentTimeMillis() - (maxAgeSeconds * 1000 + 60_000), + headers = headers.build(), + ) + } private val baseNetworkRequest = NetworkRequest(url = "https://example.com/icon.png") + // region key disabled + @Test fun `without key returns cache response`() = runTest { - val result = strategy.read(baseCacheResponse, baseNetworkRequest, options(false)) + val result = strategy.read(staleCacheResponse(), baseNetworkRequest, options(false)) assertNotNull(result.response) assertNull(result.request) } + // endregion + + // region freshness + @Test - fun `with key and cached etag adds If-None-Match`() = runTest { - val cached = baseCacheResponse.copy( + fun `with key and fresh cache returns cache response`() = runTest { + val cached = freshCacheResponse(etag = "\"abc123\"") + val result = strategy.read(cached, baseNetworkRequest, options(true)) + assertNotNull(result.response) + assertNull(result.request) + } + + @Test + fun `with key and stale cache revalidates`() = runTest { + val cached = staleCacheResponse(etag = "\"abc123\"") + val result = strategy.read(cached, baseNetworkRequest, options(true)) + assertNotNull(result.request) + assertNull(result.response) + } + + @Test + fun `with key and no cache-control header treats as stale`() = runTest { + val cached = NetworkResponse( + code = 200, + responseMillis = System.currentTimeMillis(), headers = NetworkHeaders.Builder() .set("etag", "\"abc123\"") - .build() + .build(), ) val result = strategy.read(cached, baseNetworkRequest, options(true)) assertNotNull(result.request) - assertNull(result.response) - assertEquals("\"abc123\"", result.request!!.headers["if-none-match"]) } @Test - fun `with key and cached last-modified adds If-Modified-Since`() = runTest { - val cached = baseCacheResponse.copy( + fun `with key and zero responseMillis treats as stale`() = runTest { + val cached = NetworkResponse( + code = 200, + responseMillis = 0L, headers = NetworkHeaders.Builder() - .set("last-modified", "Wed, 21 Oct 2015 07:28:00 GMT") - .build() + .set("cache-control", "public, max-age=3600") + .set("etag", "\"abc123\"") + .build(), ) val result = strategy.read(cached, baseNetworkRequest, options(true)) assertNotNull(result.request) + } + + // endregion + + // region conditional headers + + @Test + fun `stale cache with etag adds If-None-Match`() = runTest { + val cached = staleCacheResponse(etag = "\"abc123\"") + val result = strategy.read(cached, baseNetworkRequest, options(true)) + assertNotNull(result.request) + assertEquals("\"abc123\"", result.request!!.headers["if-none-match"]) + } + + @Test + fun `stale cache with last-modified adds If-Modified-Since`() = runTest { + val cached = staleCacheResponse(lastModified = "Wed, 21 Oct 2015 07:28:00 GMT") + val result = strategy.read(cached, baseNetworkRequest, options(true)) + assertNotNull(result.request) assertEquals("Wed, 21 Oct 2015 07:28:00 GMT", result.request!!.headers["if-modified-since"]) } @Test - fun `with key and both headers adds both conditional headers`() = runTest { - val cached = baseCacheResponse.copy( - headers = NetworkHeaders.Builder() - .set("etag", "\"abc123\"") - .set("last-modified", "Wed, 21 Oct 2015 07:28:00 GMT") - .build() + fun `stale cache with both headers adds both conditional headers`() = runTest { + val cached = staleCacheResponse( + etag = "\"abc123\"", + lastModified = "Wed, 21 Oct 2015 07:28:00 GMT", ) val result = strategy.read(cached, baseNetworkRequest, options(true)) assertNotNull(result.request) @@ -84,11 +155,51 @@ class ETagCacheStrategyTest { } @Test - fun `with key and no cached headers sends plain request`() = runTest { - val result = strategy.read(baseCacheResponse, baseNetworkRequest, options(true)) + fun `stale cache with no etag or last-modified sends plain request`() = runTest { + val cached = staleCacheResponse() + val result = strategy.read(cached, baseNetworkRequest, options(true)) assertNotNull(result.request) - assertNull(result.response) assertNull(result.request!!.headers["if-none-match"]) assertNull(result.request!!.headers["if-modified-since"]) } + + // endregion + + // region isStale + + @Test + fun `isStale returns false when within max-age`() { + val response = freshCacheResponse(maxAgeSeconds = 3600) + assertFalse(ETagCacheStrategy.isStale(response)) + } + + @Test + fun `isStale returns true when past max-age`() { + val response = staleCacheResponse(maxAgeSeconds = 3600) + assertTrue(ETagCacheStrategy.isStale(response)) + } + + @Test + fun `isStale returns true when no cache-control`() { + val response = NetworkResponse( + code = 200, + responseMillis = System.currentTimeMillis(), + headers = NetworkHeaders.EMPTY, + ) + assertTrue(ETagCacheStrategy.isStale(response)) + } + + @Test + fun `isStale returns true when responseMillis is zero`() { + val response = NetworkResponse( + code = 200, + responseMillis = 0L, + headers = NetworkHeaders.Builder() + .set("cache-control", "public, max-age=3600") + .build(), + ) + assertTrue(ETagCacheStrategy.isStale(response)) + } + + // endregion }