Allow OkHttp to cache unsuccessful responses (#549)

coil-base/src/main/java/coil/fetch/HttpFetcher.kt

@@ -69,6 +69,7 @@ internal abstract class HttpFetcher<T : Any>(private val callFactory: Call.Facto
 
         val response = callFactory.newCall(request.build()).await()
         if (!response.isSuccessful) {
+            response.body()?.close()
             throw HttpException(response)
         }
         val body = checkNotNull(response.body()) { "Null response body!" }

coil-base/src/test/java/coil/fetch/HttpFetcherTest.kt

@@ -5,6 +5,7 @@ import android.webkit.MimeTypeMap
 import androidx.core.net.toUri
 import androidx.test.core.app.ApplicationProvider
 import coil.bitmap.BitmapPool
+import coil.network.HttpException
 import coil.size.PixelSize
 import coil.util.createMockWebServer
 import coil.util.createOptions
@@ -14,6 +15,7 @@ import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.runBlocking
 import kotlinx.coroutines.test.TestCoroutineDispatcher
 import kotlinx.coroutines.test.resetMain
+import okhttp3.Cache
 import okhttp3.Call
 import okhttp3.HttpUrl
 import okhttp3.MediaType
@@ -26,7 +28,9 @@ import org.junit.Test
 import org.junit.runner.RunWith
 import org.robolectric.RobolectricTestRunner
 import org.robolectric.Shadows
+import java.io.File
 import kotlin.test.assertEquals
+import kotlin.test.assertFailsWith
 import kotlin.test.assertFalse
 import kotlin.test.assertNull
 import kotlin.test.assertTrue
@@ -35,6 +39,7 @@ import kotlin.test.assertTrue
 @OptIn(ExperimentalCoroutinesApi::class)
 class HttpFetcherTest {
 
+    private lateinit var cache: Cache
     private lateinit var context: Context
     private lateinit var mainDispatcher: TestCoroutineDispatcher
     private lateinit var server: MockWebServer
@@ -46,7 +51,8 @@ class HttpFetcherTest {
         context = ApplicationProvider.getApplicationContext()
         mainDispatcher = createTestMainDispatcher()
         server = createMockWebServer(context, "normal.jpg")
-        callFactory = OkHttpClient()
+        cache = Cache(File("build/cache"), 1024).apply { evictAll() }
+        callFactory = OkHttpClient().newBuilder().cache(cache).build()
         pool = BitmapPool(0)
     }
 
@@ -113,4 +119,17 @@ class HttpFetcherTest {
         val body5 = ResponseBody.create(null, byteArrayOf())
         assertNull(fetcher.getMimeType(url5, body5))
     }
+
+    @Test
+    fun `not found response is cached`() {
+        val uri = createMockWebServer(context).url("/notfound.jpg").toString().toUri()
+        val fetcher = HttpUriFetcher(callFactory)
+
+        assertFailsWith<HttpException> {
+            runBlocking {
+                fetcher.fetch(pool, uri, PixelSize(100, 100), createOptions(context))
+            }
+        }
+        assertEquals(uri.toString(), cache.urls().next())
+    }
 }

coil-test/src/main/java/coil/util/SharedTestFunctions.kt

@@ -30,10 +30,18 @@ import kotlin.coroutines.CoroutineContext
 
 fun createMockWebServer(context: Context, vararg images: String): MockWebServer {
     return MockWebServer().apply {
-        images.forEach { image ->
-            val buffer = Buffer()
-            context.assets.open(image).source().buffer().readAll(buffer)
-            enqueue(MockResponse().setBody(buffer))
+        if (images.isNotEmpty()) {
+            images.forEach { image ->
+                val buffer = Buffer()
+                context.assets.open(image).source().buffer().readAll(buffer)
+                enqueue(MockResponse().setBody(buffer))
+            }
+        } else {
+            enqueue(
+                MockResponse()
+                    .setResponseCode(404)
+                    .addHeader("Cache-Control", "public,max-age=60")
+            )
         }
         start()
     }