Is HTTP Cache supposed to evict cached queries when the same query is exectued again and results in network failure?

[Emplexx]

Question

Consider this chain of events

• A query is already cached in the HTTP Cache
• It is successfully read from it with a cache first policy
• It is executed again with a network only fetch policy, and fails because there is no internet
• Then executed again with a cache first policy

I would expect it to still be in the cache, but when this scenario happens in my case, the query is no longer there and I am getting a cache miss exception.

After investigating, I found that HttpCacheApolloInterceptor removes queries from cache when the response was unsuccessful (has GQL errors or exceptions). However, looking at the code for CachingHttpInterceptor, the network fetch networkMightThrow function calls chain.proceed(request) first thing, which would throw a network exception when there is no internet, so the rest of networkMightThrow, the part that puts the response in the cache, won't get called (from what I understand).

Is removing the cached query response in cases like these intentional?

[martinbonnin]

This is probably not intentional indeed. I'll remove that check. Thanks for bringing this to our attention 🙏

[Emplexx]

If I can make a suggestion, perhaps it would be more desirable to instead do if (!response.hasErrors() && response.exception == null) cache.write(...) in the HttpCacheApolloInterceptor, and remove writing to cache inside of networkMightThrow from the CachingHttpInterceptor? This way even in cases different from mine, the successful query in the cache would be preserved, because only successful responses are written. (I realise this goes a bit against the current naming of the interceptor classes though)

[martinbonnin]

The issue is that by the time we come in onEach {} (and we know if there are GraphQL errors), the HTTP request has been parsed to a GraphQL request already.

If we wanted to postpone writing to the cache to that moment, we'd have to keep both requests in memory, which doesn't feel great.

I can definitely understand your point though. It's weird to have data being removed because now they have GraphQL errors. All in all, I'd say, using response.errors to decide whether to store the queries or not was a mistake and that it should use HTTP semantics instead (i.e. codes != 200 shouldn't be cached).

Not sure how many implementations in the wild are relying on that behaviour so I'm not thrilled at the idea of changing it but maybe we could add a flag to switch to that behaviour.

[martinbonnin]

maybe we could add a flag to switch to that behaviour.

If we decide to go that way, I'd like to document a use case. Can you share how your backend is working?

• What is the status code if you have GraphQL errors?
• Do you actually want to cache the GraphQL errors?
• All in all, would a flag like doNotCacheNon2xx would work for you?

[Emplexx]

I'm using an API and as far as I can tell they do not use GraphQL errors, and I do not handle them in my application (i.e. it's either success or failure in the form of ApolloException). Given that networkMightThrow only caches when there was no ApolloNetworkException and response.statusCode in 200..299 && !doNotStore, for me just removing that || response.exception != null from HttpCacheApolloInterceptor already works!

As for what I'm using this for, I have just one screen which fetches a query, and I'm trying to somewhat emulate the CacheThenNetwork policy from the normalized cache. It should always show the cached query, if any, and then if it's outdated (I'm using the X-APOLLO-SERVED-DATE header to check this) fetch it again from network. While I was testing it I noticed that it only works once, i.e. before I try to make a network request while offline (this effectively means that if a user is offline and opens this screen - and either the data is outdated or they try to force refresh it - both will try to fetch query with NetworkOnly policy - they will only be able to see the cached data once)

[martinbonnin]

Gotcha, thanks for the context 🙏

For now, I'd suggest we do not add a doNotCacheNon2xx until we have more feedback. If a needs arise, we should definitely investigate this though. Longuer term, I'm hoping to have more robusts such as the QUERY HTTP method.

[martinbonnin]

I'm closing this as the initial use case should now be handled by #6314. More fine grained error handling will probably require taking a deeper look at the upcoming GraphQL over HTTP spec.

If the need for more fine grained error handling arises, please open a separate issue.

[github-actions]

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Kotlin usage and allow us to serve you better.