Non throwing Retrofit Either adapter

[lukaszkalnik]

Currently implemented Retrofit adapters (ArrowEitherCallAdapter and ArrowResponseECallAdapter) only wrap HTTP errors (i.e. valid server responses with codes 4xx and 5xx) in Left.
They still throw exceptions in the following cases:

1. Loss of connection to network/server
2. Timeout
3. Malformed JSON
4. Response body is null (which is a valid use case when you get the response 204 No Content)

Especially for mobile apps this is problematic, as losing a connection or experiencing a timeout is quite a normal case for them. The application has to deal with it gracefully.

Therefore I decided to implement a Retrofit adapter which converts all possible exceptions to domain errors (i.e. wrapping them with Left).

This adapter requires the developer to define his Retrofit API passing the provided CallError type as the Left type argument for returned Either:

import arrow.core.Either
import arrow.retrofit.adapter.either.networkhandling.CallError
import retrofit2.http.GET

data class User(val name: String)

interface MyService {

  @GET("/user/me")
  suspend fun user(): Either<CallError, User>
}

The adapter then automatically wraps ocurring exceptions in appropriate CallError subclasses.

This adapter also allows for null response body in case the defined Retrofit API expects a Unit return type. This is useful e.g. when expecting a 204 No Content response:

  // This allows a null response body
  @POST("/")
  suspend fun postSomething(@Body something: String): Either<CallError, Unit>

Note that both ArrowEitherCallAdapter and ArrowResponseECallAdapter always throw when response body is null, effectively not allowing processing of responses like 204 No Content, which might be worth fixing. This will be fixed by #2625

[hoc081098]

Very nice improvement

[nomisRev]

@lukaszkalnik is this ready for review? I saw you were still making some improvements yesterday.
If yes, I'll make some time to thoroughly review this great contribution so we can merge it 🙌

[LordRaydenMK]

Looks good to me, left a few questions/suggestions

[nomisRev]

@arrow-kt/maintainers should add @lukaszkalnik and @LordRaydenMK to the maintainers team?

This PR looks good to me, and ready to be merged. Thanks for the review @LordRaydenMK & @hoc081098! 🙏

[andre-krueger]

What about having a route that can return a 500 with no body, but for other failure status codes the body is of a custom type, that you want to match in the error handling?
When using CallError, that information is lost and you would have to deserialize the body from the HttpError class.

The only other solution I can think of is to use an OkHttp Interceptor, that throws a custom exception when the response code is 500 and then handle that with Either.catch in the api methods. That way one can keep the custom error type.

Any other solution?

This doesn't seem to check if the errorBody actually contains any data (only that errorBody is not null), which is not the case when there is no body. With that check in place and declaring the Left type (the custom error type) of the api method as optional, would that be a solution?

[lukaszkalnik]

What about having a route that can return a 500 with no body, but for other failure status codes the body is of a custom type, that you want to match in the error handling?
When using CallError, that information is lost and you would have to deserialize the body from the HttpError class.

It's not lost, error code and body are still in the HttpError class, only you have to parse them manually (as you mention yourself).

The only other solution I can think of is to use an OkHttp Interceptor, that throws a custom exception when the response code is 500 and then handle that with Either.catch in the api methods. That way one can keep the custom error type.

The whole point of this adapter is to avoid having to catch exceptions in your API calls.

Any other solution?

You never have guarantees about what really comes inside the error body. As Jake Wharton writes here, errors may come from proxies, wifi portals, applications, you may suddenly have a human-readable HTML page inside the error response, and now you're dealing with two errors.

I'm not sure if making this adapter more complex by allowing custom response body type is the way to go. I would rather have a choice - for general usage just take this adapter. When you need project-specific response body parsing then I would suggest creating your own adapter which supports that.

The main premise of this adapter is to avoid throwing exceptions in case of network errors and to handle them as domain errors.

This doesn't seem to check if the errorBody actually contains any data (only that errorBody is not null), which is not the case when there is no body. With that check in place and declaring the Left type (the custom error type) of the api method as optional, would that be a solution?

This is not my code. I'm not sure what you mean here.

[lukaszkalnik]

@andre-krueger Ok, I think I understood what you mean.
You want to keep the error type free to define by the user of this adapter, so that in case of a strictly defined HTTP error body JSON (for 4xx/5xx responses) it can be automatically parsed.

This unfortunately won't work for the following reasons:

1. The error type has to be opinionated if this adapter should convert network exceptions (like connection lost, timeout, malformed JSON etc.) to domain errors. This means that the adapter can only return an error as an instance of the sealed class hierarchy which contains the IOError and UnexpectedCallError types (which wrap the network exceptions). If we let the user define the error type, then we have no way to enforce having these types in the hierarchy which we could then use to wrap the network errors.
2. Currently we have 3 adapters in this module, but we only have one factory for them. Which adapter is instantiated is dynamically checked based on the declared wrapper type (Either or ResponseE) and error type. This adapter is automatically created by the factory when the user declares CallError as error type. I admit that this can be trivially solved with just creating a custom factory for this adapter, which always returns exactly this adapter.

[lukaszkalnik]

@nomisRev will there be some further review on this or could this be merged?

[andre-krueger]

@lukasz-kalnik-gcx
Ok.
When playing around with the EitherCallAdapterFactory code, it wasn't possible to return an empty ResponseBody here anyway (common for 500 status codes, which normally don't have any response body).
When that PR is merged, I think it's the best solution to use the CallError class and just do the deserialization for routes that both can return an empty error body and a custom error body.

[lukasz-kalnik-gcx]

@lukasz-kalnik-gcx Ok. When playing around with the EitherCallAdapterFactory code, it wasn't possible to return an empty ResponseBody here anyway (common for 500 status codes, which normally don't have any response body).

By "empty" response body do you mean empty or null? Retrofit makes it impossible to have a null errorBody. All methods which construct errorBody requireNonNull it. Look in the Retrofit source code

So this will never happen.

When that PR is merged, I think it's the best solution to use the CallError class and just do the deserialization for routes that both can return an empty error body and a custom error body.

Yes, that's how you can go about it. Or you can create a custom CallAdapter based on this one which includes your custom error body type.

[andre-krueger]

I was using an express server to test the 500 response and was sending this for the route:
res.status(500).send()

That's what curl replied with, so it seems like an empty body.

< HTTP/1.1 500 Internal Server Error
< X-Powered-By: Express
< Date: Fri, 11 Feb 2022 12:49:37 GMT
< Connection: keep-alive
< Keep-Alive: timeout=5
< Content-Length: 0

So it failed here with an exception (it was EOFException, when I remember correctly).

[lukasz-kalnik-gcx]

I was using an express server to test the 500 response and was sending this for the route: res.status(500).send()

That's what curl replied with, so it seems like an empty body.

< HTTP/1.1 500 Internal Server Error
< X-Powered-By: Express
< Date: Fri, 11 Feb 2022 12:49:37 GMT
< Connection: keep-alive
< Keep-Alive: timeout=5
< Content-Length: 0

So it failed here with an exception (it was EOFException, when I remember correctly).

Yes, so the body was indeed empty and whatever JSON converter you have registered failed parsing it and threw the EOFException.
You can mitigate it with a custom converter like described in this Stackoverflow question.

Also you can take a look at square/retrofit#1554

[andre-krueger]

Unfortunately, no luck with that.
Would have been good if it was as easy as to make the error type nullable in the Retrofit api definitions, in order to allow an empty error body.

I will wait until these changes are merged - until then I just return the custom error in an interceptor in case of the 500 status response.

[raulraja]

great addition, thanks @lukaszkalnik !

[i-walker]

Thanks @lukaszkalnik 👍🏾

[nomisRev]

Thank you so much @lukasz-kalnik-gcx for you patience, and the great addition 🙏 Finally merged!