airbnb / mavericks

Mavericks: Android on Autopilot
https://airbnb.io/mavericks/
Apache License 2.0
5.84k stars 498 forks source link

Combining Mavericks with "custom" network response and error handling #710

Open langsmith opened 9 months ago

langsmith commented 9 months ago

@gpeal @elihart @rossbacher, I made what I thought was a pretty specific and thorough thread at https://github.com/airbnb/mavericks/issues/692 but never heard back, which is totally fine. I get that life happens and open-source work is a beast of its own. Really hoping for a response on this ticket's topics 👇🏽 though

tldr; Is there a way to combine the latest version of Mavericks with "custom" network response and error handling? A way to combine Mavericks with Retrofit API Response/callback/adapters?


This whole ticket is born out two high-level end goals, which are to:


Here are examples of the JSON body that the backend sends the app when there's a error:

{"context": {"reason": "Incorrect username or password."}, "error_code": "incorrect_username_password_combination", "message": "incorrect username password combination"}

or

{"context": {"argument_name": "organization_qid", "reason": "required key not provided"}, "error_code": "bad_argument_value", "message": "bad argument value: organization_qid"}

I've already got the JSON object modeled as POJO 👇🏽

Screenshot 2024-02-09 at 11 54 29 AM Screenshot 2024-02-09 at 11 54 34 AM

Ideally, the Mavericks Async Fail would end up with the message in it.

When looking at Mavericks' data class Fail, it seems that users aren't given much flexibility.

303105367-286613c2-dcd2-4d4f-ac81-15cf6b4cbb82

Ideally, I wouldn't have to create my own custom sealed class setup with Uninitialized, Error, Success, Loading, and so on. I like Mavericks' Async class, especially because it has Loading built in and I've got various progress indicator UI tied to Loading.


App context

Single activity, multiple fragment Android app. Using Kotlin, Retrofit, Coroutines, Moshi, View binding, Navigation Component, and Hilt. Nothing crazy.

App architecture: Fragment <-> ViewModel <-> Repository <–> RetrofitService. Nothing crazy.

Mavericks and Retrofit Gradle versions

val mavericksVersion = "3.0.9"
implementation ("com.airbnb.android:mavericks:$mavericksVersion")
implementation ("com.airbnb.android:mavericks-navigation:$mavericksVersion")
val retrofitVersion = "2.9.0"
implementation ("com.squareup.retrofit2:retrofit:$retrofitVersion")
implementation ("com.jakewharton.retrofit:retrofit2-kotlin-coroutines-adapter:0.9.2")
implementation ("com.jakewharton.retrofit:retrofit2-kotlinx-serialization-converter:0.8.0")
implementation("com.squareup.okhttp3:logging-interceptor:4.12.0")

Log in as an example scenario of what I'm talking about:

LoginState has val loginToken: Async<LoginToken> = Uninitialized and 👇🏽 in the ViewModel

init {
    onAsync(LoginState::loginToken, onSuccess = { token ->
        ...
    }, onFail = {
        Timber.d("LoginState loginToken = $it, cause = ${it.cause}, message = ${it.message}")
    })
}

ViewModel's log in method 👇🏽

internal fun performLogin() = withState { state ->
    suspend {
        loginRepository.getLoginTokenFromBackend(
            state.usernameText,
            state.passwordText,
            encodeUri = { stringToEncode: String? -> Uri.encode(stringToEncode) }
        )
    }.execute { copy(loginToken = it) }
}

Repository method 👇🏽

suspend fun getLoginTokenFromBackend(
        username: String,
        password: String,
        encodeUri: ((String) -> String)? = null
    ): LoginToken {
        ...encoding code here...
        return retrofitService.getLoginToken(
            authorizationHeader = authHeader, 
            username = "op=$encodedUsername"
        )
    }

Retrofit service endpoint is

 @GET(...)
    suspend fun getLoginToken(
        ...
    ): LoginToken

Right now, this setup works correctly and shows a circular progress indicator when loginToken is Loading, thanks to the State's derived property val showProgressInButton = loginToken is Loading and the Fragment's invalidate()

override fun invalidate() = withState(viewModel) { state ->
    binding.progressInLoginButton.isVisible = state.showProgressInButton
}

After submitting an incorrect password, the backend sends a 401 error with a JSON

{"context": {"reason": "Incorrect username or password."}, "error_code": "incorrect_username_password_combination", "message": "incorrect username password combination"}
Screenshot 2024-02-07 at 4 56 34 PM

The Timber line above prints LoginState loginToken = retrofit2.HttpException: HTTP 401 , cause = null, message = HTTP 401

cause and message are null. Is there a recommended way to (simply) go from receiving the JSON body to the Mavericks Fail having some sort of content from the JSON body? A hacky way? Casting somehow?

Or is Mavericks intentionally designed to just throw a pretty generic Fail with the error code and nothing else? Does the backend error need to be structured differently for Mavericks' code to correctly interpret it and provide more info in the Fail?

Because I don't see a way to get access to the JSON info, I'm looking into the often suggested route of creating and using a custom sealed class with networking states. From what it looks like, it's essentially my own version of Mavericks' Async.kt class 👇🏽


Custom Result.kt situation

I'm sure there are issues with it, but I've got 👇🏽 for now.

open class Result<out T> {
    data class Success<T>(val data: T) : Result<T>()
    object EmptySuccess : Result<Nothing>()
    data class Error(val exception: Exception) : Result<Nothing>()
    data class ApiError(val apiError: MerchantException) : Result<Nothing>()
    data class Unauthorized(val unauthorized: MerchantException) : Result<Nothing>()
    object Loading : Result<Nothing>()
    object Uninitialized : Result<Nothing>()
}

So, with the Result.kt class above, I can do val loginToken: Result<LoginToken?> = Result.Uninitialized in the LoginState.

Doing Result<LoginToken> instead of Async<LoginToken> now means:

If I MUST have a custom Result.kt class to create custom errors, any thoughts on a way to somehow combine it with Mavericks? I have👇🏽 working well

fun <T> Result<T>.mappedResponse(): Async<T?>? = when (val result = this) {
        is Result.Success<*> -> Success(result.resultData)
        is Result.Error -> Fail(result.exception)
        is Result.ApiError -> Fail(result.apiError)
        is Result.Unauthorized -> Fail(result.unauthorized)
        is Result.Loading -> Loading()
        else -> Uninitialized()
    }

and

internal fun performLogin() = withState { state ->
    setState { copy(loginToken = Loading()) }
    suspend {
        loginRepository.getLoginTokenFromBackend(
            state.usernameText,
            state.passwordText,
            encodeUri = { stringToEncode: String? -> Uri.encode(stringToEncode) }
        )
    }.execute { 
      it.invoke()?.mappedResponse()?.let {
        copy(loginToken = it)
      } ?: copy()
    }
}

Capturing exceptions

I've created a handleResponse() method, which takes in a Retrofit2 Response object and checks the response object. I'm hoping to use it for all networking response object values that are wrapped with Result in any of my ViewModels (e.g. val loginToken: Result<LoginToken?> = Result.Uninitialized)

Do notice that it returns some type of Result from the custom Result.kt class I made and described above.

fun <T> handleResponse(response: Response<T>): Result<T> {
    try {
        val contentType = response.raw().body?.contentType()
        val responseIsFromS3Server = "AmazonS3" == response.headers()["Server"]
        if (response.isSuccessful) {
            val code = response.code()
            val responseBody = response.body()
                ?: return Result.ApiError(
                    MerchantException(
                        supportCode = SupportCode.TRANSPORT_EXCEPTION,
                        message = "response contained no body"
                    )
                )
            if (code == 500) {
                return Result.ApiError(MerchantException(SupportCode.HUB_EXCEPTION))
            } else if (code == 502) {
                return Result.ApiError(MerchantException(SupportCode.BAD_GATEWAY_EXCEPTION))
            } else if (code == 511) {
                return Result.ApiError(MerchantException(SupportCode.CAPTIVE_PORTAL_DETECTED))
            } else if (code in 200..204 && (jsonMediaType == contentType || responseIsFromS3Server)) {
                return Result.Success(responseBody)
            } else if (code in 400..499 && responseIsFromS3Server) {
                return Result.ApiError(MerchantException(cause = translateS3Error(response.raw().body)))
            } else if (code in 200..204) {
                val contentName = contentType?.toString() ?: "null"
                val message = "Unexpected content type: $contentName"
                return Result.ApiError(
                    MerchantException(
                        SupportCode.CONTENT_TYPE_EXCEPTION,
                        message
                    )
                )
            } else {
                return Result.ApiError(
                    MerchantException(
                        supportCode = SupportCode.COMMUNICATION_EXCEPTION,
                        message = "Unexpected status code: $code"
                    )
                )
            }
        } else {
            if ("application/vnd.error+json".toMediaTypeOrNull() == contentType) {
                val metadataErrorResponse = convertErrorBody(moshi, response.errorBody())
                return metadataErrorResponse?.let {
                    Result.Error(
                        SwallowedException.fromMetadata(
                            context = context,
                            supportMetadata = it,
                            moshi = moshi
                        )
                    )
                } ?: Result.Error(Exception("Something went wrong"))
            } else {
                return Result.ApiError(
                    MerchantException(
                        message = "Non-successful response without detailed error body",
//                            supportCode = SupportCode.UNKNOWN_ERROR
                    )
                )
            }
        }
    } catch (e: Exception) {
        return when (e) {
            is FileNotFoundException -> Result.ApiError(
                SwallowedException(
                    supportCode = SupportCode.PHOTO_FILE_NOT_FOUND,
                    cause = e,
                )
            )
            else -> Result.ApiError(
                MerchantException(
                    supportCode = SupportCode.TRANSPORT_EXCEPTION,
                    cause = e
                )
            )
        }
    }
}

val metadataErrorResponse = convertErrorBody(moshi, response.errorBody()) above is what parses the error's JSON body.

This handleResponse() method allows me to do 👇🏽 in the repository

suspend fun getLoginTokenFromBackend(
        username: String,
        password: String,
        encodeUri: ((String) -> String)? = null
    ): Result<LoginToken?> {
        ...encoding code here...
        return handleResponse(
            retrofitService.getLoginToken(
                authorizationHeader = authHeader, username = "op=$encodedUsername"
            )
        )
    }

Retrofit endpoint is now wrapped with Response

): Response<LoginToken>

Running setState { copy(loginToken = Loading()) } at the beginning of the ViewModel method works and correctly leads to showing the circular progress indicator in the fragment. Ideally, I wouldn't have to (remember to) add this setState { copy(loginToken = Loading()) } code before all calls to any repository in all of my ViewModels.

I tried my hand at a custom implemention of .execute with the .customExecute() method below, but it didn't work.

suspend fun <T> Flow<Result<T>>.customExecute(stateReducer: S.(Result<T>) -> S) {
    setState { stateReducer(Result.Loading) }
    safeCollect { setState { stateReducer(it) } }
}

👇🏽 didn't work 😕 with the method above

viewModelScope.launch(IO) {
    flowOf(
        loginRepository.getLoginTokenFromBackend(
            state.usernameText,
            state.passwordText,
            encodeUri = { stringToEncode: String? -> Uri.encode(stringToEncode) }
        )
    ).customExecute {
        it.mappedResponse()?.let { response ->
            copy(loginToken = response)
        } ?: copy()
    }
}

Any ideas on what a correct custom .execute might look like so that Loading is always first set? Is the .customExecute() method above, close to being correct?

Some other way to handle the response instead of 👇🏽?

it.mappedResponse()?.let { response ->
    copy(loginToken = response)
} ?: copy()

Regarding error handling, I've seen:

They're all in my ticket's territory but not really answering what I'm wondering. So yea, @gpeal @elihart @rossbacher, I'd really appreciate any and all thoughts on how to approach this. It seems like a fairly standard use case for Mavericks-loving users. I hope this comment, and hopefully discussion, becomes a resource for anyone else trying to figure out similar things with Mavericks.

elihart commented 8 months ago

Hey there. Regarding https://github.com/airbnb/mavericks/issues/692, we don't use Hilt or the navigation component at Airbnb, so I don't really have an opinion or expertise to share. Maybe other people in the community have feedback for you on it though.

Or is Mavericks intentionally designed to just throw a pretty generic Fail with the error code and nothing else? Does the backend error need to be structured differently for Mavericks' code to correctly interpret it and provide more info in the Fail?

Check out the source code for the execute function

try {
                val result = invoke()
                setState { reducer(Success(result)) }
            } catch (e: CancellationException) {
                @Suppress("RethrowCaughtException")
                throw e
            } catch (@Suppress("TooGenericExceptionCaught") e: Throwable) {
                setState { reducer(Fail(e, value = retainValue?.get(this)?.invoke())) }
            }

Mavericks just catches any exception thrown by your suspend function and saves it in the Fail class. So if you want specific data out of the exception, you need to throw an exception containing that failure data. Mavericks has nothing to do with parsing your json or interpreting your exception.

If you do have a custom exception class with your data, you would have to cast it from the Fail instance, which isn't ideal for type safety, but also not terrible if you limit where you need to do so. I suppose the Fail class could have a generic type for the exception, but it's a bit late to introduce that at this point. For us, we have standardized that the errorMessage of the Throwable from network requests is the user facing message to show, so we can easily extract that to present in the UI. For example, it sounds like you need to change your networking layer to throw an exception with your reason field as the error message.