Deprecate ApolloCompositeException with Error level and use suppressed exceptions instead

[WildOrangutan]

Use case
I'm finding ApolloCompositeException annoying to handle. It contains multiple exceptions and you kind of have to pick one exception, and based on it show error message to user. Along side that, you have to potentially handle and report all other errors (e.g. Firebase).

For example:

try {
    graphqlService.mutate(Whatever())
} catch(e: ApolloCompositeException) {
    when (val causes = e.suppressedExceptions) {
        causes.any { it is ApolloNetworkException } -> TODO("handle network")
        causes.any { it is HttpCacheMissException } -> TODO("handle cache")
        else -> TODO("handle other")
    }
}

Describe the solution you'd like
I may be missing some context, why it was designed in such a manner. But ideally, I would prefer smth like:

try {
    graphqlService.mutate(Whatever())
} catch(e: Exception) {
    when (e) {
        is ApolloNetworkException -> TODO("handle network")
        is HttpCacheMissException -> TODO("handle cache")
        else -> TODO("handle other")
    }
}

[martinbonnin]

Hi ! 👋100% agree the exception handling could use some more ergonomic APIs. Not 100% about this specific case though. Mutations typically do not hit the cache so I don't think you should get an ApolloCompositeException ? Can you share the stacktrace when/if that happens?

[WildOrangutan]

Ah, I see. I just wrote some random example, by using my imagination. So it probably only happens for querying then 🙂

[martinbonnin]

I see 👍. For queries, the problem happens if you have both a network and a cache error. The caller might want to have access to the cache miss error fir an example. If you're not using the cache, you can simply ignore ApolloCompositeException

[WildOrangutan]

I intend to use caching, so this exception is still relevant for me.

Is composite exception there only because of cache-miss? What if you exposed some sort of callback for cache-miss instead? Like cache-miss "interceptor", or smth along those lines?
I think if that would avoid composite exception, it would make exception handling less mind boggling. You will know better if that makes sense.

[martinbonnin]

Is composite exception there only because of cache-miss?

Yes, mostly

I think if that would avoid composite exception, it would make exception handling less mind boggling

Let's say you're doing a CacheFirst query that fails because both cache and network fail. What exception would you expect there?

[WildOrangutan]

In case of "cache miss callback", I'd say network exception.

But hm, in case of NetworkFirst this might get weird. ApolloCompositeException makes more sense when you take this into account.

Would it be possible to rather limit ApolloCompositeException a bit? By limit I mean set more specific type, that ApolloCompositeException can contain. Would make a lot of difference I think. For example changing type of exceptions: val suppressedExceptions: ApolloException.

When I first try to handle this exception, I had no clue what I can get in suppressedExceptions and it's still hard to predict what to expect. I had no idea, where to find "network exception". Someone could put another ApolloCompositeException into it, which was also something I was wondering if it's possible. At first I even thought, that I will directly get smth like UnknownHostException.

Hope you can see where I'm coming at :)

[martinbonnin]

I 100% see the pain point there. Something we can do for sure is update the documentation (see #4065)

changing type of exceptions: val suppressedExceptions: ApolloException.

We can't do that because suppressedExceptions is built-in in kotlin.Throwable. But we could add something like:

// A helper function that returns the [suppressedExceptions] as a list of [ApolloException]
val suppressedApolloExceptions: List<ApolloException>.

Would that help? Also ping @BoD @akshay253101 since we had a related discussion in #3957, do you have any opinion on this?

[BoD]

In my opinion, yes, a suppressedApolloExceptions would be a nice to have, its presence kind of implicitly explains that the underlying exception can only be ApolloException (in fact we should probably also make this explicit in the doc).

Just for the sake of the argument (as we can't really change this now) I think the ideal would have been:

• if there's only one exception, throw that
• if there are 2, throw the 2nd one, but the 1st one is accessible through the standard suppressedExceptions (and clearly document that fact)

[WildOrangutan]

From my pov, this would definitely better message what you can expect to handle. It would even better if we could somehow guarantee, that suppressedApolloExceptions won't contain ApolloCompositeException again :). Maybe also at least document that.

[akshay253101]

ApolloCompositeException takes first and second exception as Throwable and added to suppressedException, So in constructor we can restrict it to ApolloException

[martinbonnin]

if there's only one exception, throw that

This we do already, right?

if there are 2, throw the 2nd one, but the 1st one is accessible through the standard suppressedExceptions (and clearly document that fact)

I like this !

as we can't really change this now

Maybe we can? It's a behaviour change but also kind of an edge case so it should be transparent for the vast majority of users ? And it's not API/ABI breaking so there's no risk of a runtime exception from a transitive usage or such things.

For @akshay253101 use case (getting the network error from a Http NetworkFirst query), it means changing:

if (exception is ApolloCompositeException) {
   // get the network error
   exception.suppressedExceptions[0] as? ApolloException
}

to

if (exception.suppressedExceptions.isNotEmpty()) {
     // get the network error
     exception.suppressedExceptions[0] as? ApolloException
}

[BoD]

This we do already, right?

✅

Changing the type of exception thrown would require clients who handle it to update the catch accordingly so I guess that's why I assumed we "can't" really do it 😅.

But it's true at least it shouldn't make the code crash - and of course we would clearly warn of the behavior change in the changelog. Might be a bit annoying in the short run, on the other hand this would allow deprecating ApolloCompositeException which seems to be confusing so that'd be for the best longer term.

[WildOrangutan]

if there are 2, throw the 2nd one, but the 1st one is accessible through the standard suppressedExceptions (and clearly document that fact)

Hm, I also like the idea. I'm worried how annoying would it be to go "down the rabbit hole" to find exception, that you're looking for. Maybe some sort of helper method would make it bearable.

I guess it's not that bad, since it's only 2 atm. But it's again hard to predict how to handle it.

[martinbonnin]

Changing the type of exception thrown would require clients who handle it to update the catch accordingly so I guess that's why I assumed we "can't" really do it 😅.

Maybe we deprecate ApolloCompositeException with Error level? This way the symbol is still there (no ABI breaking change) but it forces all callers to update explicitely? My favorite timing to do this would be at the same time as reworking the way we "throw" in general (see #4003). We could make a "error handling" release that simplifies the error handling a bunch

[BoD]

Maybe we deprecate ApolloCompositeException with Error level?

Totally 👍

We could make a "error handling" release that simplifies the error handling a bunch

Agreed as well

[martinbonnin]

Fixed with #4718

I wend with throwing the first exception and adding all later ones as suppressed, similar to what would happen with try-with-resources