apollographql / apollo-kotlin

:rocket:  A strongly-typed, caching GraphQL client for the JVM, Android, and Kotlin multiplatform.
https://www.apollographql.com/docs/kotlin
MIT License
3.72k stars 655 forks source link

KN: Wrapping NSError from NSURLSession in Exceptions #3307

Closed ProVir closed 2 years ago

ProVir commented 3 years ago

Hi!

Now all the errors from NSURLSession received as an Apollo NetworkException, inside which an IOException is nested without the original NSError.

Because of this, I can't get the original NSError in iOS and work it out correctly, because I don't know its domain and code.

Please add the ability to get the original NSError object in all your exceptions, where they are.

ApolloNetworkException(
    message = "Failed to execute GraphQL http network request",
    cause = IOException(error.localizedDescription)
)
martinbonnin commented 3 years ago

Thanks for sending this. The hard part is that we want the API to be MPP as much as possible. Ideally we want an API that allows the code in commonMain (where NSError is not accessible) to be able to handle all kinds of errors. Can you share what your error handling logic is like and how you're doing it on the JVM/Android?

ProVir commented 3 years ago

Common logic for all platforms is good. But it is precisely for iOS that there is not enough opportunity to get the original NSError error, and this check will already be only from the iOS part.

Currently, only message is reported in IOException - you can get it from any exception anyway.

Example 1:

class AppleIOException(val error: NSError) : IOException(error.localizedDescription)

AppleIOException can be received and so only when running on ios - therefore, other platforms do not need to know about it, and for output in the common part of the message, it remains so.

Example 2 (we use it in our project):

commonMain:

data class AppleError(
    val domain: String,
    val code: Long,
    val message: String,
    val rawError: Any)

class ApolloNetworkException(val error: AppleError?, message: String? = null, cause: Throwable? = null) : 
    ApolloException(message = message, cause = cause)

appleMain:

val AppleError.error: NSError
    get() = rawError as NSError

fun NSError.toAppleError() = AppleError(
    domain = domain ?: "",
    code = code,
    message = localizedDescription,
    rawError = this
)

In this example, we can use a wrapper on NSError in the common code, but it can only be created in the ios part.

martinbonnin commented 3 years ago

We could expose rawError: Any? that can be cast to NSError on iOS or Exception on Android but I'd like to do that as a last resort as there could be some value in extracting more info from NSError and expose that as common code.

The current rationale is that ApolloNetworkException is a generic error that is handled with a generic message like "Please retry later" on the UI side.

If more fine grained UI logic is needed like for an example:

If the above fine-grained logic is needed for iOS, there's no reason it shouldn't be available on Android too. Do you have a list of Apple domain/codes you need to read?

ProVir commented 3 years ago

It is important for us to know not only that this is an ApolloNetworkException, but also the original exception that we take from cause.

In Android and iOS, we use this knowledge to correctly display the message, depending on whether it is a connection problem or not. We also need the original error from the network client for logging in order to more accurately determine the original problem.

For example, in Android, we determine the type of error this way, getting it from ApolloNetworkException.cause (jvmMain):

actual fun Throwable.isJvmConnectionError(): Boolean = when (this) {
    is SocketTimeoutException,
    is ConnectException,
    is SocketException,
    is UnknownHostException,
    is MalformedURLException,
    is NoRouteToHostException,
    is SSLHandshakeException -> true
    else -> cause?.isJvmConnectionError() ?: false
}

For iOS, we can also determine the type of error if we know the original one (appleMain):

fun NSError.isConnectionError(): Boolean {
    if (domain != NSURLErrorDomain)
        return false

    return listOf(
        NSURLErrorNetworkConnectionLost,
        NSURLErrorNotConnectedToInternet
    ).contains(code)
}

Also, each platform has logging, in the case of android, the original exception is used, and in the case of ios, the original error with all the contents, including userInfo.

It is no longer relevant to the question, but here is the current implementation of our final exception as example, which provides all the necessary information through an abstract class (commonMain):

class NetworkException(cause: ApolloNetworkException): CommonException(null, cause), AppleErrorProvider {
    override val appleError = cause.findAppleError()

    override val errorName: String = appleError?.domain ?: cause.defaultErrorName()

    override val errorMessage = appleError?.message
        ?: cause.cause?.findFirstMessage() ?: cause.message ?: ""

    override val errorType: ErrorType = appleError?.getCommonErrorType()
        ?: if (cause.isJvmConnectionError())
            ErrorType.Connection
        else
            ErrorType.Server

    override val errorCode: Long = appleError?.code ?: errorType.defaultCode()
}

internal expect fun ApolloNetworkException.findAppleError(): AppleError?

Using AppleErrorPovider, we determine on the iOS (swift) side that there is an NSError for this exception and get it from AppleError.

martinbonnin commented 3 years ago

Many thanks for all the details. This is helpful. I can see how the errors do not always map 1:1 and having the original error allows more detailed diagnostics 👍 . I'll open a PR in a bit.

ProVir commented 3 years ago

Thank You! 👍

martinbonnin commented 3 years ago

PR there: https://github.com/apollographql/apollo-android/pull/3315

Let me know what you think.

ProVir commented 3 years ago

I looked at it - a good solution.

Perhaps it would be good to add to appleMain extension:

fun ApolloNetworkException.error: NSError? = platformCause as? NSError

In Android now available ApolloNetworkException.cause: Throwable? - according to the same logic.

martinbonnin commented 3 years ago

Good idea. I added ApolloNetworkException.nsError.

I don't think Android needs anything else since cause is already accessible?

ProVir commented 3 years ago

Yes, the standard exception feature - cause is already available for android, because unlike ios, kotlin is based on jvm approaches. Therefore, error problems usually occur only in iOS due to a different approach - NSError is not the heir of Throwable.

ProVir commented 2 years ago

Tell me, when will the next alpha version be?

martinbonnin commented 2 years ago

Hopefully soon-ish :). Ideally there'll be some Java codegen for the next alpha but we can always do intermediate alphas if this ends up taking too long.

Also, you can always use the SNAPSHOT. There's a 3.0.0-alpha04-SNAPSHOT with the fix: https://oss.sonatype.org/content/repositories/snapshots/com/apollographql/apollo3/apollo-api/3.0.0-alpha04-SNAPSHOT/