RFC: v4 error handling

[BoD]

Error handling is an important aspect of a client library and as we’re looking at 4.0, we think it can be improved and are seeking feedback.

Exceptions vs GraphQL errors

2 kinds of errors need to be handled:

• Non-GraphQL errors: typically network issues and cache misses
• GraphQL errors: an operation has correctly been executed and a result has been received, and it contains a non empty errors field

Current situation

Currently, non-GraphQL errors are surfaced by throwing exceptions, whereas GraphQL errors are returned in ApolloResponse.errors. (See v3 doc error section).

Some drawbacks:

• These 2 different ways make it difficult to handle errors at the same area of the code
• Exceptions must be caught or they will crash your application
• Exceptions are terminal in Flows:
  • surfacing first a cache miss and then a network result (e.g. when using CacheFirst strategy) is not straightforward

Proposed change

Similarly to ApolloResponse.errors, a new field ApolloResponse.exception: ApolloException is added, and the response is emitted (.toFlow()) or returned (.execute()), instead of throwing.

execute():

Before	try { val response = client.query(MyQuery()).execute() if (response.data != null) { // Handle (potentially partial) data } else { // Handle GraphQL errors } } catch (e: ApolloException) { // Handle network error }
After	val response = client.query(MyQuery()).execute() if (response.data != null) { // Handle (potentially partial) data } else { // Something wrong happened if (response.exception != null) { // Handle non-GraphQL errors } else { // Handle GraphQL errors } }

.toFlow():

Before	client.subscription(MySubscription()).toFlow() .catch { e -> // Handle network error } .collect { response -> if (response.data != null) { // Handle (potentially partial) data } else { // Handle GraphQL errors } }
After	client.subscription(MySubscription()).toFlow().collect { response -> if (response.data != null) { // Handle (potentially partial) data } else { // Something wrong happened if (response.exception != null) { // Handle non-GraphQL errors } else { // Handle GraphQL errors } } }

This addresses the drawbacks above, and arguably is a more consistent API - at the cost of a breaking change. To ease the transition, methods that throw like in 3.x will be added. Projects will be able to use them temporarily to keep the old behavior and migrate to the new one progressively.

Partial data and nullability

In the area of error handling, pain points related to handling GraphQL errors and nullability have also been identified:

• Some projects don't need to handle partial data, or only at a few specific places
• Many projects have a lot of nullable fields in their schemas that in reality can rarely be null (only in error cases)

To improve usability around this, v4 will introduce:

• Error aware parsing: GraphQL errors are handled at parse time and data is guaranteed null when there are errors / non-null otherwise
• @catch and FieldResult to handle partial data for specific fields
• @semanticNonNull to make fields generated as non null

This is introduced in #5405, and documentation is in #5407.

Feedback

Please comment on this ticket with any feedback you may have!

[kevincianfarini]

I personally feel that this is a burden that Apollo should handle and shouldn't be pushed onto library consumers. There's two cases you mention above -- CacheMissException and the various types of IO exceptions. I haven't experienced any pain with the latter (it's working as expected), but I have experienced pain with CacheMissException. Namely when debugging our ApolloCache it tends to be a red herring.

I would propose that Apollo handle these internally as values. CacheMissException is the clearly odd one out here -- I personally don't think it's exceptional for a cache to be lacking a value. I feel it should be modeled like so:

internal sealed interface CacheResult<T> {
  object None : CacheResult<Nothing>
  class Some(val data: T) : CacheResult<T>
}

Then when dealing with cache misses, exposing the flow is simply a case of calling filterIsInstance and mapping. Likewise, execute can apply the proper filtering for the cache, but I'm not sure there's much of a use case there. I have never personally used execute with a cache and I don't even know if it's possible.

I don't currently know if it's possible for a cache value to be nullable or not, but that might simplify this even further. No need to model is as a sealed hierarchy if so.

---

Exceptions must be caught or they will crash your application

For network exceptions this is what I expect. Hoisting this functionality into a ApolloResponse.exception value seems really unintuitive . It's extremely easy to forget that manual check and then you'd just end up in another exceptional case elsewhere when you try to erroneously read the associated data.

If handling network exceptions is painful from an Apollo library maintenance standpoint I'd expect Apollo to model them as a value internally and rethrow them at places where a library consumer interacts with them.

---

I don't personally use any fetch policies aside from NetworkOnly and CacheOnly so I'm sure that alleviates some of this pain. I imagine it's pretty tedious to handle exceptions bubbling up in a flow that's emitting from both the cache and the network in some fashion.

[kevincianfarini]

For what its worth, I generally try and model exceptions for uses which are exceptional. I don't try to model exceptions when a user or consumer of a library has done something incorrectly.

However, the following point made resonates with me quite a bit.

These 2 different ways make it difficult to handle errors at the same area of the code

We experienced this issue and actually leaned the opposite way. We created our own execute extension which throws an exception specific to our codebase.

sealed class CustomException(
    message: String? = null,
    cause: ApolloException? = null,
) : RuntimeException(message, cause)

class CustomNetworkException(
    message: String? = null,
    cause: ApolloException? = null,
) : CustomException(message, cause)

class SemanticException(
    val semanticError: SemanticError,
    message: String,
) : CustomException(message, null)

suspend fun <D: Operation.Data> ApolloCall<D>.executeCustomException(): D {
   try {
       val response = execute()
       if (response.hasErrors()) {
           throw SemanticException(response.getSemanticError())
       } else {
           return response.dataAssertNoErrors
       }

   } catch (e: ApolloException) {
       throw when (e) {
           is ApolloNetworkException,
           is ApolloHttpException,
           is ApolloWebSocketClosedException -> CustomNetworkException(cause = e)
           else -> e
       }
   }
}

private fun Response<*>.getSemanticError(): SemanticError { /* omitted */ }

I like this because it's pragmatic and allows us to have one paradigm for handling errors. I dislike this because it's not "pure" from an exceptions standpoint. At use site it looks something like this:

suspend fun doAThing() = try {
  client.query(/*omitted*/).executeWithCustomException()
} catch (e: SemanticException) {
  // do something with the semantic error. 
} catch (e: CustomNetworkException) {
  // do something 
}

[martinbonnin]

Then when dealing with cache misses, exposing the flow is simply a case of calling filterIsInstance and mapping

Can you elaborate a bit more? Currently everything is an ApolloResponse<D>. How would you introduce CacheResult in the mix? response.cacheResult?? Something else?

It's extremely easy to forget that manual check and then you'd just end up in another exceptional case elsewhere when you try to erroneously read the associated data

The idea is that you need to check for data != null:

val data = someResponse.data
if (data != null) {
  // woohoo
  println(data.foo.bar)
} else {
  println("woops...)
}

At least you're forced to check nullability (or force unwrap but then there's not much we can do) while exception catching get forgotten a lot. The Google Play SDK Index retrieves the crashes with apollo in the stack trace and this is #1 by very very far.

I imagine it's pretty tedious to handle exceptions bubbling up in a flow that's emitting from both the cache and the network in some fashion.

Things get really hairy if someone does this:

apolloClient.query(query).fetchPolicy(CacheFirst).refetchPolicy(NetworkOnly).watch().collect {
  // stuff
}

That's a use case if you need to refresh your data when something changes in your cache.
What should we do when a network error happens when refetching from the network? Throwing terminates the flow which is often not what you want. By emitting errors as regular responses, the users gets to control the behaviour there.

[sonatard]

How can I handle exception when using toSate() in v3 ?

[BoD]

Hi @sonatard this ticket is about error handling in v4. I've just opened #5019 about toState() in v3.

[LiamDeGrey]

I like how errors are being handled by V4 so far (4.0.0-beta.2), however, I'm interested to know why ApolloGraphQLException.errors has been deprecated in 4.0.0-beta.3?
We have a strong use-case for surfacing all errors available via the ApolloGraphQLException rather than directly from ApolloResponse.

[martinbonnin]

@LiamDeGrey In beta.3, ApolloGraphQLException is always thrown with a single GraphQL error, the first one that is encountered during parsing (if you opt-in error aware parsing, else they are just ignored). In all cases, using ApolloGraphQLException.errors will always return a list of size 1.

We changed our mind from always setting an exception on GraphQL errors because that breaks the partial data use cases and this has bitten a few early 4.0 adopters (and forced us to add very weird code too).

Can you share more about your use case? There might be ways to address it differently.