isFromCache is potentially confusing

[rohandhruva]

Version

3.x-4.0.0-beta.5

Summary

ApolloResponse has an extension function, isFromCache. When using FetchPolicy.CacheFirst, if there's a cache miss, an ApolloResponse is emitted where data == null and it.exception is a CacheMissException. For this response, I would expect that isFromCache returns false. This is because it relies on isCacheHit being true, which is not the case for this response. Link to code

I think it'd be ideal to make it return true, but I'm not sure if we can change this API now. Maybe we can introduce a new API, isResponseFromCache that does not rely on cacheHit being true, perhaps it returns isCacheHit || it.exception is CacheMissException?

Steps to reproduce the behavior

No response

Logs

(Your logs here)

[BoD]

Thanks for reporting! That's a good point. I think in v3 it was potentially less confusing since if you had a cache miss it usually meant you didn't have a response, but an exception (but not always thanks to emitCacheMisses(true)).

In v4 of course now you'll have a response, and the current implementation of isFromCache ought to be named e.g. isDataFromCache (or just isCacheHit maybe).

It looks like what you really want is cacheInfo != null. We could add a isResponseFromCache that does that.

At least we need to add a KDoc on isFromCache to clarify it's only about the data! And maybe deprecate it in favor of one named more explicitly?

[BoD]

After talking a bit with @martinbonnin we think keeping isFromCache and changing its behavior to be about the response rather than the data only could be all right actually (provided that we clarify this in the Kdoc). What do you think @rohandhruva ?

[rohandhruva]

After talking a bit with @martinbonnin we think keeping isFromCache and changing its behavior to be about the response rather than the data only could be all right actually (provided that we clarify this in the Kdoc).

I think that's definitely the preferable approach here! By default if you're migrating from v3 to v4 with the v3 exception handling enabled, my guess is that you won't be affected by it at all (unless you also have emitCacheMisses set to true). So this is the perfect opportunity to fix isFromCache and document it.

It looks like what you really want is cacheInfo != null.

I will need to double check, but I seem to remember that while response.cacheInfo is nullable, it is never actually null, even for responses that come from the network. Perhaps something like isFromCache == cacheInfo?.cacheMissException != null?

[BoD]

Perhaps something like isFromCache == cacheInfo?.cacheMissException != null

Good one! I opened #5805 with that.

[martinbonnin]

@rohandhruva do you mind ellaborating a bit more on what do you use isFromCache for? If there's no data, you should end up in the exception handling branch where you would typically check the type of the exception. Do you need isFromCache there as well?

if (response.data != null) {
  // Handle (potentially partial) data
} else {
  // Do you use `isFromCache` here?
}

[rohandhruva]

Sure -- the main use case here is logging. We use .toFlow() and then our custom .trackedFlow() extension method that logs various metrics associated with this request.

One of the things we track is the time it took to read data from the cache. This is logged even if it's a cache miss, because that information helps us determine how long the cache lookup took.

More generally, though, since isFromCache is an extension function on ApolloResponse, I think it makes sense to return true even if there's no data. Technically, the cache miss response was also from cache, so isFromCache == false seems incorrect in that case. If we end up introducing a new ApolloResponse<*>.Data.isFromCache, I think it makes sense to return true like it does now.

[martinbonnin]

main use case here is logging [... we] track is the time it took to read data from the cache

Makes sense. Are you using CacheInfo for this? CacheInfo was made specifically for those use cases and gives a lot more information than just isFromCache.

More generally, though, since isFromCache is an extension function on ApolloResponse, I think it makes sense to return true even if there's no data

I'm trying to account for a future where ApolloResponse is split into interface ApolloResult, ApolloResponse: ApolloResult and ApolloError: ApolloResult. Is isFromCache on ApolloResponse, ApolloError, both?

Also, the cache can be seen as a GraphQL execution service in itself and we could have partially cached data as well at some point.

All in all, I'm really not against changing the meaning of isFromCache. I don't think the current behaviour makes more sense than the proposed one but I'm also trying to understand how we want that API to be used.

[rohandhruva]

Are you using CacheInfo for this? CacheInfo was made specifically for those use cases and gives a lot more information than just isFromCache.

Not yet, but I can definitely look into it.

However, I've also noticed that CacheInfo is never null, even for requests that come back from the network, so it's not a good proxy for isResponseFromCache: Boolean.

I'm trying to account for a future where ApolloResponse is split into interface ApolloResult, ApolloResponse: ApolloResult and ApolloError: ApolloResult. Is isFromCache on ApolloResponse, ApolloError, both?

In that future, if we are trying to match the current APIs, I would imagine we'll have something like ApolloResult.isFromCache to maintain the symmetry. Assuming ApolloResult is a sealed interface, it can have an val isFromCache that is overridden in ApolloResult and ApolloError.

I know it's generalizing based on the current view of the APIs, but in general, I think we need isFromCache from any type that the client emits: which is ApolloResponse for now and presumably ApolloResult in the future.

Also, the cache can be seen as a GraphQL execution service in itself and we could have partially cached data as well at some point.

I don't think it necessarily conflicts with .isFromCache either -- maybe at that point we'll need an additional ApolloResponse.Data.isPartial: Boolean?

[martinbonnin]

Re: cache timing information, if we could find a proper API to do so, I think it'd be a big win. I know @AdamMTGreenberg was also looking into it at some point (see #3221). Maybe CacheInfo is missing some stuff and i'd very much like to address this if it's the case (See also mandatory read: https://publicobject.com/2022/05/01/eventlisteners-are-good/)

Re: ApolloResult, the day we start modeling the cache as a "regular" GraphQL execution service, cache misses won't be modeled as an ApolloError. ApolloError mostly means there's been a network error. If the cache lives in the same process, it's a possibility that reading from the cache will never trigger an error (except OOM maybe? even there...). In that case, I'd rather not have ApolloError.isFromCache always set to false.

I know it's very prospective stuff and I don't want to bikeshed this too much so if #5805 makes things clearer in the current situation, I'm all in for merging it 👍

[martinbonnin]

PS: I'll probably die on the no isPartial boolean hill 😄 . My current mantra is:

• by default everything is partial and any nullable field can be an error
• unless you opt-in @catch(to: THROW) and then nothing is partial

[rohandhruva]

I agree with your general approach to isPartial being "technically not correct" unless we opt in to @catch(to: THROW)... but how does it apply to usage of @defer / @stream? They have an isLast (iirc), which kind of seems like isPartial if you squint hard enough 😁

That said, if we do eventually support partial responses from the cache at some point in the future, I think there's value in indicating this to the UI somehow. Maybe in that case isFromCache is enough and you deal with null values as you normally would if they were null in the network response (or show a loading indicator based on isFromCache == true or isLast == false).

[martinbonnin]

isLast is actually one of the reasons why I'd like to define the API a bit better there. isLast is an outlier at the moment...

isFromCache is enough and you deal with null values as you normally would if they were null in the network response

That would be the idea

show a loading indicator based on isFromCache == true or isLast == false

Is the intent to skip the loading indicator if a cache response is present to avoid a blinking effect?
The indicator could be displayed when the cache response is received. That's actually one of the nice thing of having the cache miss as regular emissions. You can expect that first emission always.

flow.collect {response ->
   if (response.data == null) {
     // we got a cache miss, display a progress bar while we go to the network
     showProgressBar = true
  }
}

[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.