apollographql / apollo-kotlin

:rocket:  A strongly-typed, caching GraphQL client for the JVM, Android, and Kotlin multiplatform.
https://www.apollographql.com/docs/kotlin
MIT License
3.76k stars 653 forks source link

Http cache getting evicted when response has an error #837

Closed anup1245 closed 6 years ago

anup1245 commented 6 years ago

When the response has an error, the http cache still tries to cache the data and the old cached data is lost.

The error may be present in a field, so the response code is still 200. But the cache fails when it tries to update the old data with new data(containing error).

Can the check

  public boolean isSuccessful() {
    return code >= 200 && code < 300;
  }

be updated to handle error objects in response also, even if response code is 200?

Thanks!

anup1245 commented 6 years ago

in the logs i have seen that when there is an error in response, Cache MISS for request: %s, with cache key: %s is seen.

anup1245 commented 6 years ago

@sav007 In this test

@Test public void cacheSeveralResponses() throws Exception {
    enqueueResponse("/HttpCacheTestAllPlanets.json");
    Rx2Apollo.from(apolloClient
        .query(new AllPlanetsQuery()))
        .test()
        .awaitDone(TIME_OUT_SECONDS, TimeUnit.SECONDS)
        .assertValue(new Predicate<Response<AllPlanetsQuery.Data>>() {
          @Override public boolean test(Response<AllPlanetsQuery.Data> response) throws Exception {
            return !response.hasErrors();
          }
        });
    checkCachedResponse("/HttpCacheTestAllPlanets.json");

    enqueueResponse("/HttpCacheTestDroidDetails.json");
    Rx2Apollo.from(apolloClient
        .query(new DroidDetailsQuery()))
        .test()
        .awaitDone(TIME_OUT_SECONDS, TimeUnit.SECONDS)
        .assertValue(new Predicate<Response<DroidDetailsQuery.Data>>() {
          @Override public boolean test(Response<DroidDetailsQuery.Data> response) throws Exception {
            return !response.hasErrors();
          }
        });
    checkCachedResponse("/HttpCacheTestDroidDetails.json");

    enqueueResponse("/HttpCacheTestAllFilms.json");
    Rx2Apollo.from(apolloClient
        .query(new AllFilmsQuery()))
        .test()
        .awaitDone(TIME_OUT_SECONDS, TimeUnit.SECONDS)
        .assertValue(new Predicate<Response<AllFilmsQuery.Data>>() {
          @Override public boolean test(Response<AllFilmsQuery.Data> response) throws Exception {
            return !response.hasErrors();
          }
        });
    checkCachedResponse("/HttpCacheTestAllFilms.json");
  }

Adding at the end

  "errors": [
    {
      "message": "Exception"
    }
  ]

to HttpCacheTestAllPlanets.json the test fails

anup1245 commented 6 years ago

@sav007 It is seen that in ApolloParseInterceptor.java, in the method parse

try {
        OperationResponseParser parser = new OperationResponseParser(operation, responseFieldMapper, scalarTypeAdapters,
            normalizer);
        Response parsedResponse = parser.parse(httpResponse.body().source())
            .toBuilder()
            .fromCache(httpResponse.cacheResponse() != null)
            .build();
        if (parsedResponse.hasErrors() && httpCache != null) {
          httpCache.removeQuietly(cacheKey);
        }
        return new InterceptorResponse(httpResponse, parsedResponse, normalizer.records());
      } catch (Exception rethrown) {
        logger.e(rethrown, "Failed to parse network response for operation: %s", operation);
        closeQuietly(httpResponse);
        if (httpCache != null) {
          httpCache.removeQuietly(cacheKey);
        }
        throw new ApolloParseException("Failed to parse http response", rethrown);
      }

This code

  if (parsedResponse.hasErrors() && httpCache != null) {
          httpCache.removeQuietly(cacheKey);
        }

removes cache data if there is error

sav007 commented 6 years ago

@anup1245 unfortunately the issue is that writing to the http cache happens while we reading and parsing http response (and this response with HTTP OK status code, otherwise we just abort parsing and ). If we remove this code then we will cache error response from the server for this query.

Why server returns error for your case?

anup1245 commented 6 years ago

@sav007 If there is an error, can we not cache data and return old data if present ?

Its a timeout exception, trying to fix that.

sav007 commented 6 years ago

Unfortunately by the time we do check: if (parsedResponse.hasErrors() && httpCache != null) we have already cached data . Http cache is agnostic to the errors inside the response, it checks only that response was HTTP OK 200 status code.

anup1245 commented 6 years ago

@sav007 so is there a way around this ? Or the response needs to be fixed is the only solution? Any thoughts on how to overcome this ?

sav007 commented 6 years ago

You have 2 options for now:

  1. response needs to be fixed
  2. you can try normalized cache
sav007 commented 6 years ago

Closing due to inactivity