Improve 204 (No content) handling and polymorphism

[sky-franciscoaleixo]

Description

There has been a PR (#63) that introduces the capability of returning ApiResult<Unit, ...> when a 204 occurs. This is generally fine if we expect that that is the only successful behavior.

But what if an endpoint can return a 200 with a specific body response, but can also return a 204 with no body? I'd like to open discussion and maybe ask for some guidance regarding this. I've also seen the README which kind of talks about 200 polymorphism but it again seems to only relate with errors. There's a few options to resolve this:

• Change ApiResult<out T : Any, out E : Any>, so that at least T can be nullable. This would allow nullable responses, and fix this specific issue, but there may be reasons as to why you might not want to do this.
• Allow some way to pass HTTP status code through Retrofit annotations in success responses. This way we could implement custom behavior in custom Converter.Factorys. I tried this, and this is already possible in case of an error, but not in the case of success AFAIK.
• I could try catch NPE, but that's not really a good solution
• I could use Response<MyDto?> and try to convert manually to ApiResult, but then I'd have to reimplement the whole CallAdapter logic.

Is there any other option? Thank you.

Requirements (place an x in each of the [ ])

• I've read and understood the Contributing guidelines and have done my best effort to follow them.
• I've read and agree to the Code of Conduct.
• I've searched for any related issues and avoided creating a duplicate issue.

[ZacSweers]

Retrofit handles this by just coercing to Unit, no?

[sky-franciscoaleixo]

Well I haven't used RxJava in a long while, but interestingly you had a PR for their CallAdapter square/retrofit#3003 related with that 😆

Regardless, their stance seems to be this square/retrofit#2242 (comment)

Side note, it seems like Retrofit2 also foces null body when it receives a 204/205.

[ZacSweers]

Heh, good find. I suspect the Optional approach there is your best bet

[sky-franciscoaleixo]

Thanks for the response! I guess my last question on this is, is there any technical reason we can't have nullable type on ApiResult? Is it just a philosophical stance, or is there any other reason?

[ZacSweers]

It's not possible in the type system to only allow this for 204 responses. If we allowed it here we'd allow it everywhere, and the whole point of this library is to require stronger types. Not only this, but there's no way at the Java level to inspect the type for nullability without metadata parsing since it's a kotlin construct.

[sky-franciscoaleixo]

Late answer from me, but that's fair, thanks!

[sky-franciscoaleixo]

I'm going to close this discussion and suggest if anyone has this same problem to follow the Optional approach suggested above 👍

[sky-franciscoaleixo]

I'm reopening this as using Optional<> won't work. This is because as mentioned above Retrofit forces a null response, but this also means that it won't pass through any Converter at all, so using an OptionalConverter won't ever work.

The only options I see are either intercepting these 204/205 requests and forcing some other response, or hammering some logic on ApiResultCallAdapter to support Optional there which doesn't make any sense. If I end up finding an acceptable workaround I'll post here.

[ZacSweers]

Is this really an EitherNet issue at this point? Seems more a limitation of Retrofit

[sky-franciscoaleixo]

I'd say at this point it is mostly a limitation of Retrofit, I just left this as other people could find themselves in the same situation. I'll leave the solution that works ok below with an expected return of ApiResult<Optional<T>, Throwable> and a specific annotation @OptionalNoContent, although I don't really like it.

internal class NoContentInterceptor : Interceptor {
    private val noContentHttpStatusCodes = arrayOf(204, 205)

    override fun intercept(chain: Interceptor.Chain): Response {
        val request = chain.request()
        val response = chain.proceed(request)
        
        if (request.getAnnotation<OptionalNoContent>() == null ||
            !response.isSuccessful ||
            !noContentHttpStatusCodes.contains(response.code)
        ) {
            return response
        }

        return response
            .newBuilder()
            .code(200)
            .body("".toResponseBody("text/plain".toMediaType()))
            .build()
    }
}

and

internal object NoContentConverterFactory : Converter.Factory() {
    override fun responseBodyConverter(
        type: Type,
        annotations: Array<out Annotation>,
        retrofit: Retrofit,
    ): Converter<ResponseBody, *>? {
        if (getRawType(type) != Optional::class.java || annotations.find { annotation -> annotation is OptionalNoContent } == null) {
            return null
        } else {
            val innerType = getParameterUpperBound(0, type as ParameterizedType)
            val delegate = retrofit.responseBodyConverter<Any?>(innerType, annotations)
            return NoContentConverter(delegate)
        }
    }

    private class NoContentConverter<T : Any>(
        private val delegate: Converter<ResponseBody, T>? = null,
    ) : Converter<ResponseBody, Optional<T>> {

        override fun convert(value: ResponseBody): Optional<T> {
            if (value.contentLength() <= 0L) {
                return Optional.empty()
            }
            return Optional.ofNullable(delegate?.convert(value))
        }
    }