Consider updating the docs for isLast on ApolloResponse if they are not up to date

[StylianosGakis]

Description

I am reading the docs over here

apollo-kotlin/libraries/apollo-api/src/commonMain/kotlin/com/apollographql/apollo/api/ApolloResponse.kt

Lines 94 to 96 in 19fc22b

	* There can be false negatives where [isLast] is false if the producer does not know in advance if
	* other items are emitted. For an example, the CacheAndNetwork fetch policy doesn't emit the network
	* item if it fails.

And I am trying to understand if the text For an example, the CacheAndNetwork fetch policy doesn't emit the network item if it fails. remains to be true in version 4.x too.

Looking inside the interceptor itself

apollo-kotlin/libraries/apollo-normalized-cache/src/commonMain/kotlin/com/apollographql/apollo/cache/normalized/FetchPolicyInterceptors.kt

Lines 111 to 130 in 19fc22b

	/**
	* An interceptor that emits the response from the cache first, and then emits the response(s) from the network.
	*/
	val CacheAndNetworkInterceptor = object : ApolloInterceptor {
	override fun <D : Operation.Data> intercept(request: ApolloRequest<D>, chain: ApolloInterceptorChain): Flow<ApolloResponse<D>> {
	return flow {
	val cacheResponse = chain.proceed(
	request = request
	.newBuilder()
	.fetchFromCache(true)
	.build()
	).single()
	emit(cacheResponse.newBuilder().isLast(false).build())
	val networkResponses = chain.proceed(request)
	emitAll(networkResponses)
	}
	}
	}

I see that the chain simply proceeds as-is after we try to hit the cache first. How is the network request ignored in this case?
There was a brief discussion about this here https://slack-chats.kotlinlang.org/t/22727664/with-4-x-error-handling-i-wanted-to-see-if-it-s-possible-to-#ff570c24-3ca2-487e-92c1-56fe05dfc1e6 too, and I hope this helps.

[BoD]

Thanks! I've removed the example about CacheAndNetwork here.

I've left the caveat about false negatives for now - I'm a bit reluctant to remove it since it still makes sense that some producers may not have a way to know when an item is the last one, even if I can't think of specific examples of that.

[StylianosGakis]

That's great, thanks for addressing this!

When we mention that some producers may not know when the item is the last one, is that for other interceptors that users of this library may be creating themselves, or something that even if you can't think of a real example, may still potentiall happen using any of the CacheFirst, CacheOnly, NetworkOnly, NetworkFirst and CacheAndNetwork fetch policies?

[BoD]

User defined interceptors is certainly a case. This should never happen due to the regular fetch policies.

I was also thinking of something like @defer where the backend emits several items - in that instance we actually do know when an item is the last, but I'm imagining maybe future GraphQL features won't be able to guarantee it.

[StylianosGakis]

Sounds very good, thanks for the clarification. And I agree that the docs can keep this open-ended for the reasons who describe.

[martinbonnin]

FWIW, I'm thinking we should probably deprecate isLast altogether.

AFAIK, the only usage is for watchers to subscribe to the store before emiting the last item of the initial query. So that callers get a guarantee that whatever change they do after they have received it will be seen by the watcher. It's still a bit kludgy and I'm hoping we can find a way to restructure this without requiring an isLast property.

@StylianosGakis did you have a specific need for isLast ?

[StylianosGakis]

Hehe I do in fact have one, let me try and explain.

It stems from a discussion we've had in kotlinlang slack before. And I discussed this with my colleague to see what would make the most sense for how we typically use this API.

So our use case was to be able to use CacheAndNetwork, without having to on the call site know that you are using CacheAndNetwork. And by that I mean that with this policy, in a flow you'd get back things in this order:

1. No response yet
2. A cache fail response quickly
3. After a small delay, the real network response.

This on the UI would be reflected like this:

1. Loading screen
2. Error screen (basically flickering, since network comes soon after
3. Success screen

What we want to experience instead is:

1. Loading screen
2. Success screen

By skipping the intermediate error emission.

This was materialized in this code here:
https://github.com/HedvigInsurance/android/blob/6d90b9592a52c0b4bccfa66fb509f50df6ff52af/app/apollo/apollo-core/src/main/kotlin/com/hedvig/android/apollo/ApolloCallExt.kt#L55-L65

Now how is this related to CacheAndNetwork?
It is the one policy where we get a cache miss exception, but where we know that we are going to get a new response soon anyway, so we can safely just ignore that one, keep the screens on their loading state, and just show whatever the network request would show instead.

With CacheOnly for example, where isLast is true, we know we are not getting a new emission anyway, so in that case we do in fact want to show the error screen, since there's nothing "saving us" from there, it is the final thing they will see on that screen.

I hope this makes sense, and if you got ideas on how to achieve this without isLast then I am all ears 😊

[StylianosGakis]

Initially I thought it would be fine to emit both things, and on the call site where one does know which policy they set, they could be careful to ignore such cache misses manually, but after a discussion with my colleague we decided against it since it felt like more room for human error than we would like to have for a thing which we do so often in the entire codebase.

[martinbonnin]

Would something like this work?

fun <D : Operation.Data> ApolloCall<D>.safeFlow(): Flow<Either<ApolloOperationError, D>> {
  return flow {
    var hasEmitted = false
    var errorResponse: ApolloResponse<D>? = null
    toFlow().collect {
      if (it.exception != null) {
        // Some errors may be followed by valid responses.
        // In that case, wait for the next response to come instead.
        errorResponse = it
      } else {
        hasEmitted = true
        emit(it)
      }
    }
    if (!hasEmitted && errorResponse != null){
      // Flow has terminated without a valid response, emit the error one if it exists.
      emit(errorResponse)
    }
  }.map { either { parseResponse(it) } }
}

It's slightly different in meaning but I think it achieves the same thing in practice. I'd argue it's also maybe more correct, i.e. not only about CacheMiss only anymore: it's a Flow that may contain errors that we want to hide. Unless there are only errors, in which case we want the last one to surface?

[StylianosGakis]

In the CacheAndNetwork scenario, won't this do exactly this?

1. Collect first cache miss response
2. it.exception is not null, so we assign it to errorResponse
3. hasEmitted is false, since we have emitted nothing so far. errorResponse is not null since we just set it
4. We emit the cache miss

Which is what I wanted to avoid here?

Did you by any chance mean that the

if (!hasEmitted && errorResponse != null) {
  // Flow has terminated without a valid response, emit the error one if it exists.
  emit(errorResponse)
}

Should be after the closing bracket of the inner collect? And we should only arrive there if the collection has finished anyway, and we are for sure not expecting any new emissions?

[StylianosGakis]

Ah yeah that must be what you mean. What can I say, this looks much much much better than what I did. It also guards against scenarios where isLast may be implemented wrongly, due to perhaps some interceptor that we add in the future somehow.

I think you just won yourself a isLast deprecation 😂 I will not protest at all if you do it :D

[martinbonnin]

Should be after the closing bracket of the inner collect? And we should only arrive there if the collection has finished anyway, and we are for sure not expecting any new emissions?

Yup, that was 100% the intent, sorry! I just edited the codeblock

[martinbonnin]

scenarios where isLast may be implemented wrongly

This is exactly what's making me itchy about isLast. It's very tedious to track and maintain everywhere.

[StylianosGakis]

Yup, I can totally understand, as I was trying to make use of it, it was tricky both to see how it's used inside the library itself, how it's set or not set, and then this doc threw me off a bit too. Overall not relying on it makes me much more confident indeed!