HTTP cache: do not remove cached entries on transport errors (#6314)

* HTTP cache: do not remove cached entries on transport errors

See #6313

* only filter out ApolloNetworkErrors

Author: martinbonnin

libraries/apollo-http-cache/src/main/kotlin/com/apollographql/apollo/cache/http/internal/HttpCacheApolloInterceptor.kt

@@ -9,6 +9,7 @@ import com.apollographql.apollo.api.Subscription
 import com.apollographql.apollo.cache.http.CachingHttpInterceptor
 import com.apollographql.apollo.cache.http.HttpFetchPolicy
 import com.apollographql.apollo.cache.http.HttpFetchPolicyContext
+import com.apollographql.apollo.exception.ApolloNetworkException
 import com.apollographql.apollo.interceptor.ApolloInterceptor
 import com.apollographql.apollo.interceptor.ApolloInterceptorChain
 import kotlinx.coroutines.flow.Flow
@@ -48,9 +49,17 @@ internal class HttpCacheApolloInterceptor(
         .run {
           if (request.operation is Query<*>) {
             onEach { response ->
-              // Revert caching of responses with errors
               val cacheKey = synchronized(apolloRequestToCacheKey) { apolloRequestToCacheKey[request.requestUuid.toString()] }
-              if (response.hasErrors() || response.exception != null) {
+              /**
+               * We revert caching if:
+               * - there are GraphQL errors
+               * - or if data was received (not an ApolloNetworkException) but could not be parsed to a valid GraphQL response
+               *
+               * This is fragile as it might remove useful data.
+               * A better version of this should probably delegate that decision to the server using HTTP codes.
+               * See https://github.com/apollographql/apollo-kotlin/issues/6313#issuecomment-2531538619
+               */
+              if (response.hasErrors() || response.exception != null && response.exception !is ApolloNetworkException) {
                 try {
                   cacheKey?.let { cachingHttpInterceptor.cache.remove(it) }
                 } catch (_: IOException) {

tests/http-cache/src/test/kotlin/FaultyEngineTest.kt

@@ -0,0 +1,86 @@
+import com.apollographql.apollo.ApolloClient
+import com.apollographql.apollo.api.http.HttpRequest
+import com.apollographql.apollo.api.http.HttpResponse
+import com.apollographql.apollo.cache.http.httpCache
+import com.apollographql.apollo.exception.ApolloNetworkException
+import com.apollographql.apollo.network.http.HttpEngine
+import com.apollographql.apollo.network.http.HttpNetworkTransport
+import httpcache.GetRandomQuery
+import kotlinx.coroutines.runBlocking
+import okio.Buffer
+import okio.IOException
+import okio.Source
+import okio.Timeout
+import okio.buffer
+import java.io.File
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertIs
+
+class FaultySource: Source {
+
+  private val data = Buffer().writeUtf8("{ \"data\":")
+
+  override fun close() {
+
+  }
+
+  override fun read(sink: Buffer, byteCount: Long): Long {
+    val remaining = data.size.coerceAtMost(byteCount)
+    if (remaining == 0L) {
+      throw IOException("failed to read")
+    }
+    data.read(sink, remaining)
+
+    return remaining
+  }
+
+  override fun timeout(): Timeout {
+    return Timeout.NONE
+  }
+}
+
+class FaultyEngine: HttpEngine {
+  val requests = mutableListOf<HttpRequest>()
+
+  override suspend fun execute(request: HttpRequest): HttpResponse {
+    requests.add(request)
+
+    return HttpResponse.Builder(200)
+        .body(FaultySource().buffer())
+        .addHeader("content-length", "30")
+        .build()
+  }
+}
+
+class FaultyEngineTest {
+
+  @Test
+  fun httpErrorsAreNotCached() = runBlocking {
+    val engine = FaultyEngine()
+
+    val dir = File("build/httpCache")
+    dir.deleteRecursively()
+
+    val apolloClient = ApolloClient.Builder().apply {
+      httpCache(dir, Long.MAX_VALUE)
+      networkTransport(
+          HttpNetworkTransport.Builder()
+              .serverUrl("unused")
+              .interceptors(httpInterceptors)
+              .httpEngine(engine)
+              .build()
+      )
+      httpInterceptors(emptyList())
+    }.build()
+
+    apolloClient.query(GetRandomQuery()).execute().apply {
+      assertIs<ApolloNetworkException>(exception)
+    }
+    apolloClient.query(GetRandomQuery()).execute().apply {
+      assertIs<ApolloNetworkException>(exception)
+    }
+
+    assertEquals(2, engine.requests.size)
+  }
+}

tests/http-cache/src/test/kotlin/HttpCacheTest.kt

@@ -11,6 +11,7 @@ import com.apollographql.apollo.cache.http.httpFetchPolicy
 import com.apollographql.apollo.cache.http.isFromHttpCache
 import com.apollographql.apollo.exception.ApolloNetworkException
 import com.apollographql.apollo.exception.HttpCacheMissException
+import com.apollographql.apollo.exception.JsonEncodingException
 import com.apollographql.mockserver.MockServer
 import com.apollographql.mockserver.awaitRequest
 import com.apollographql.mockserver.enqueueError
@@ -21,6 +22,7 @@ import httpcache.GetRandom2Query
 import httpcache.GetRandomQuery
 import httpcache.RandomSubscription
 import httpcache.SetRandomMutation
+import junit.framework.TestCase.assertFalse
 import kotlinx.coroutines.delay
 import kotlinx.coroutines.runBlocking
 import okhttp3.Interceptor
@@ -31,6 +33,7 @@ import kotlin.test.Test
 import kotlin.test.assertEquals
 import kotlin.test.assertIs
 import kotlin.test.assertNotNull
+import kotlin.test.assertTrue
 
 class HttpCacheTest {
   lateinit var mockServer: MockServer
@@ -215,18 +218,71 @@ class HttpCacheTest {
     }
   }
 
+  /**
+   * Whether an incomplete Json is an IO error is still an open question:
+   * - [ResponseParser] considers yes (and throws an ApolloNetworkException)
+   * - [ProxySource] considers no (and doesn't abort)
+   *
+   * This isn't great and will need to be revisited if that ever becomes a bigger problem
+   */
   @Test
-  fun incompleteJsonIsNotCached() = runTest(before = { before() }, after = { tearDown() }) {
+  fun incompleteJsonTriggersNetworkException() = runTest(before = { before() }, after = { tearDown() }) {
     mockServer.enqueueString("""{"data":""")
+    apolloClient.query(GetRandomQuery()).execute().apply {
+      assertIs<ApolloNetworkException>(exception)
+    }
+
+    apolloClient.query(GetRandomQuery()).httpFetchPolicy(HttpFetchPolicy.CacheOnly).execute().apply {
+      /**
+       * Because there's a disagreement between ProxySource and HttpCacheApolloInterceptor, the value is stored in the
+       * HTTP cache and is replayed here
+       */
+      assertIs<ApolloNetworkException>(exception)
+    }
+  }
+
+  @Test
+  fun malformedJsonIsNotCached() = runTest(before = { before() }, after = { tearDown() }) {
+    mockServer.enqueueString("""{"data":}""")
     apolloClient.query(GetRandomQuery()).execute().exception.apply {
-      assertIs<ApolloNetworkException>(this)
+      assertIs<JsonEncodingException>(this)
     }
     // Should not have been cached
-    assertIs<HttpCacheMissException>(
-        apolloClient.query(GetRandomQuery()).httpFetchPolicy(HttpFetchPolicy.CacheOnly).execute().exception
-    )
+    apolloClient.query(GetRandomQuery()).httpFetchPolicy(HttpFetchPolicy.CacheOnly).execute().apply {
+      assertIs<HttpCacheMissException>(exception)
+    }
+  }
+
+  @Test
+  fun ioErrorDoesNotRemoveData() = runTest(before = { before() }, after = { tearDown() }) {
+    mockServer.enqueueString("""
+        {
+          "data": {
+            "random": "42"
+          }
+        }
+      """.trimIndent())
+
+    // Warm the cache
+    apolloClient.query(GetRandomQuery()).execute().apply {
+      assertEquals(42, data?.random)
+      assertFalse(isFromHttpCache)
+    }
+
+    // Go offline
+    mockServer.close()
+    apolloClient.query(GetRandomQuery()).httpFetchPolicy(HttpFetchPolicy.NetworkOnly).execute().apply {
+      assertIs<ApolloNetworkException>(exception)
+    }
+
+    // The data is still there
+    apolloClient.query(GetRandomQuery()).execute().apply {
+      assertEquals(42, data?.random)
+      assertTrue(isFromHttpCache)
+    }
   }
 
+
   @Test
   fun responseWithGraphQLErrorIsNotCached() = runTest(before = { before() }, after = { tearDown() }) {
     mockServer.enqueueString("""