Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KN: Wrapping NSError from NSURLSession in Exceptions #3307

Closed
ProVir opened this issue Aug 17, 2021 · 12 comments
Closed

KN: Wrapping NSError from NSURLSession in Exceptions #3307

ProVir opened this issue Aug 17, 2021 · 12 comments

Comments

@ProVir
Copy link
Contributor

ProVir commented Aug 17, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

ProVir commented Aug 23, 2021

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
Copy link
Contributor

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:

  • DNS error
  • SSL handshake failure
  • Could not connect to host
  • etc...

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
Copy link
Contributor Author

ProVir commented Aug 24, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

ProVir commented Aug 24, 2021

Thank You! 👍

@martinbonnin
Copy link
Contributor

PR there: #3315

Let me know what you think.

@ProVir
Copy link
Contributor Author

ProVir commented Aug 24, 2021

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
Copy link
Contributor

Good idea. I added ApolloNetworkException.nsError.

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

@ProVir
Copy link
Contributor Author

ProVir commented Aug 24, 2021

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
Copy link
Contributor Author

ProVir commented Aug 25, 2021

Tell me, when will the next alpha version be?

@martinbonnin
Copy link
Contributor

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/

@ProVir ProVir closed this as completed Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants