Simplify generic types; handle no content response

[argenkiwi]

Hi @haroldadmin. I hope you have enjoyed the holidays. I am submitting this pull request just so you can see a slightly different approach I am using regarding the handling of 204 (no content) responses. I also simplified the generic typing of the classes to make them a little less verbose and improve polymorphism.

Feel free to reject the pull request once you've looked into it. It is only meant for sharing.

[fergusonm]

Another solution to this problem is to side step the entire issue altogether. Personally, I feel there's no need to expose the entire 204, empty body problem to the caller. (Or even the response code for that matter.) That's a network level implementation details in my opinion. The caller simply cares about success or failure.

Given that this library relies on retrofit and okhttp I simply convert all 204s and it's lesser used cousin the 205 into 200s with an empty body using an OkHttp interceptor. Then from the caller's point of view it's just a Success with a Unit body.

object EmptyBodyInterceptor : Interceptor {

    override fun intercept(chain: Interceptor.Chain): Response {
        val response = chain.proceed(chain.request())

        // Bail on non-204s and 205s
        if (!response.isSuccessful || response.code.let { it != 204 && it != 205 }) {
            return response
        }

        // if for some reason there's a body in a 204 then just convert it to a 200
        if ((response.body?.contentLength() ?: -1) >= 0) {
            return response.newBuilder().code(200).build()
        }

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

[argenkiwi]

How would converting the body to Unit work when you expect a different return type for an endpoint? Would that not cause issues with the parsing of the response?

In any case, my intent here was to preserve the interface for 'Success' so there is no need to care about the implementations for it. Attempting to get the body for a 'NoContent' response will cause an exception which, in the rare cases where it occurs, you have the option to handle this.

You could argue it's the same approach this library takes regarding errors: if you don't need to know what specific error you received, you only need to deal with the 'Error' interface.

[fergusonm]

How would converting the body to Unit work when you expect a different return type for an endpoint? Would that not cause issues with the parsing of the response?

I mean if your Retrofit interface has a return type of a endpoint that returns a 204 to be anything other than Unit, regardless of whether or not you use a NetworkResponse wrapper, it's going to fail. So it's no different than how any of it is handled today, with any Retrofit type of implementation.

In your implementation, attempting to get the body of a NoContent does indeed cause an exception. I think that's bad. It's imposing a new requirement that forces the caller to be aware that the web service call is a 204 returning endpoint and to avoid getting the body.

The only difference with my approach is the caller doesn't need to know about a NoContent success case. It's just a Success with an Unit body. The caller doesn't care that it's a 204 or a 200 or any other 200 series response for that matter. The caller can naively get the body and can get back a Unit, which isn't terribly useful but doesn't cause an exception and doesn't require guards.

You could argue it's the same approach this library takes regarding errors: if you don't need to know what specific error you received, you only need to deal with the 'Error' interface.

It's not though because the caller now has to be careful about not getting the body if a NoContent is returned. It now has to check if the Success is a NoContent and then to guard against getting the body.

Why bother exposing those types of situations to the caller at all?

[argenkiwi]

I am not sure we understand the problem the same way. There will be situations in which and endpoint returns a specific type but can also return a no-content response. Treating the 204 as Unit does solve that scenario. The only alternatives I can think of are:

• Making the body field nullable for all success responses, which adds a ridiculous amount of overhead for the majority of cases in which you don't care about 204s.
• Pretending the issue doesn't exist and continue to handle it as an exception and work around it when handling the response in the client (basically what we have in V4).

[fergusonm]

Ah yes, I didn't realize that was the situation being described here. In that case where the web service contract is "flexible" then yeah my interceptor won't work but neither with any type of Retrofit implementation other than manual processing of the Response object. Anything that declares a firm contract for the web service response is going to break if 204s are sometimes returned but not always.

There's a third choice, which is to remove the body property from the Success sealed interface and make it solely a property of OK. The caller still needs to sort through the success types to determine if there's a body to look at but it's less error prone than emitting an exception. That's a pretty gnarly breaking change though.

[argenkiwi]

I like the 3rd alternative you propose as it would simplify the implementation. I also agree it would be a significant breaking change.

Maybe the body property could just have a custom getter that returns the body from the response. It could be marked as deprecated and a couple of alternatives could be suggested, like a 'bodyOrThrow' and a 'bodyOrNull' properties/functions.

Get Outlook for Androidhttps://aka.ms/AAb9ysg

---

From: Michael Ferguson @.> Sent: Wednesday, February 9, 2022 9:07:56 AM To: haroldadmin/NetworkResponseAdapter @.> Cc: Leandro @.>; Author @.> Subject: Re: [haroldadmin/NetworkResponseAdapter] Simplify generic types; handle no content response (PR #56)

Ah yes, I didn't realize that was the situation being described here. In that case where the web service contract is "flexible" then yeah my interceptor won't work but neither with any type of Retrofit implementation other than manual processing of the Response object. Anything that declares a firm contract for the web service response is going to break if 204s are sometimes returned but not always.

There's a third choice, which is to remove the body property from the Success sealed interface and make it solely a property of OK. The caller still needs to sort through the success types to determine if there's a body to look at but it's less error prone than emitting an exception. That's a pretty gnarly breaking change though.

— Reply to this email directly, view it on GitHubhttps://github.com/haroldadmin/NetworkResponseAdapter/pull/56#issuecomment-1033014727, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABVXPIFH2PIBJXVPCET6TJ3U2FZZZANCNFSM5NZDIZKQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you authored the thread.Message ID: @.***>

[haroldadmin]

Thanks for the suggestions, I really appreciate the input on the design of the NetworkResponse interface.

I like the idea of making Success a sealed interface to model contentful and empty responses separately. I am not a fan of requiring users to change how they use the library. Do you have ideas on how we can retain the existing semantics of the NetworkResponse.Success class?

Sticking to semver is important, so if this does turn out to be a breaking change then we should change to v6.

[argenkiwi]

The aim of the change was to keep the interface for Success the same so no alterations to existing when statements would be required.

NoContent exposes a body property of type Nothing, which means accessing it, directly or indirectly through the Success interface, will throw an exception. But you would only need to worry about it in the, in my experience, rare cases in which an endpoint could return a 204.

Get Outlook for Androidhttps://aka.ms/AAb9ysg

---

From: Kshitij Chauhan @.> Sent: Wednesday, February 9, 2022 6:25:54 PM To: haroldadmin/NetworkResponseAdapter @.> Cc: Leandro @.>; Author @.> Subject: Re: [haroldadmin/NetworkResponseAdapter] Simplify generic types; handle no content response (PR #56)

Thanks for the suggestions, I really appreciate the input on the design of the NetworkResponse interface.

I like the idea of making Success a sealed interface to model contentful and empty responses separately. I am not a fan of requiring users to change how they use the library. Do you have ideas on how we can retain the existing semantics of the NetworkResponse.Success class?

Sticking to semver is important, so if this does turn out to be a breaking change then we should change to v6.

— Reply to this email directly, view it on GitHubhttps://github.com/haroldadmin/NetworkResponseAdapter/pull/56#issuecomment-1033365344, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABVXPIGC6HWYHMGVNWQQ4ELU2H3GFANCNFSM5NZDIZKQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you authored the thread.Message ID: @.***>

[fergusonm]

Alterations to existing statements are required though. I'd argue they are mandatory.

No matter what route you go with this is a breaking change. Any user of the library had a contract where they could safely access a Success.body property without concern of exceptions being thrown. With this change this is no longer true.

With a previous version the caller would have had:

            when (response) {
                is NetworkResponse.Error -> {
                    // Do something with the error
                }
                // Maybe the specific error classes if the caller cared about it            
                is NetworkResponse.Success -> {
                    val something = response.body // this may now throw an exception where before it did not!
                }
            }

If the caller doesn't change their code, they don't see a no-content response as Nothing, it's still whatever data type it was before, likely Unit.

By leaving the body property as part of the Success sealed interface you now introduce a potential issue with the upgrade path of the library. Old code that references Success.body may now, unexpectedly, throw an exception. (Regardless of the issues around a 204. I'm just talking pure API interfaces here.) I disagree with that. This forces the user to go through their entire code base and make sure to never reference Success.body without a try/catch and to convert to using OK or NoContent.

If that's a requirement it should be explicit in the API.

In my opinion throwing an exception is contrary to much of the intent of this library. The whole point of the call adapter is to convert what would have normally been Retrofit exceptions into concrete objects that is much more safely handled by the caller.

[argenkiwi]

Michael Ferguson Friday, 11 February, 2:46 am If the caller doesn't change their code, they don't see a no-content response as Nothing, it's still whatever data type it was before, likely Unit. I don't think this assumption is correct. If a resource is optional from a REST API perspective, an endpoint can return a typed response (not Unit) or a no-content response. Both scenarios are considered a success from the REST API perspective. Casting a response to Unit when a different type is expected introduced issues on its own.

In any case, properly fixing the issue will require a breaking change, I agree. The current implementation assumes a successful response is not nullable/optional which is incorrect from the HTTP protocol perspective.

As I mentioned before, ignoring the issue is also an option. Users will have to handle 204s as server errors, which you could argue would prevent unexpected crashes at the least, although the outcome might not be what was expected by the developer.

Finally, I just wanted to clarify again that the PR was just to share a transitional approach. For a concrete project I am working on, I've had to replace this library with my own implementation because V4 was not ready for exhaustive when statements (which will be required soon) and the current V5 will not really solve the 204 issue I was facing while still requiring a lot of rework on the current implementation. I have learnt a lot from this project and I thank you for your work.

I think maybe this project should just aim to help with small apps with relatively simple APIs and requirements. If you add too much complexity, it will stop being helpful for that kind of use. For larger projects the effort of implementing an project-specific abstraction is relatively negligible.

Get Outlook for Androidhttps://aka.ms/AAb9ysg

---

From: Michael Ferguson @.> Sent: Friday, February 11, 2022 2:46:14 AM To: haroldadmin/NetworkResponseAdapter @.> Cc: Leandro @.>; Author @.> Subject: Re: [haroldadmin/NetworkResponseAdapter] Simplify generic types; handle no content response (PR #56)

Alterations to existing statements are required though. I'd argue they are mandatory.

No matter what route you go with this is a breaking change. Any user of the library had a contract where they could safely access a Success.body property without concern of exceptions being thrown. With this change this is no longer true.

With a previous version the caller would have had:

        when (response) {
            is NetworkResponse.Error -> {
                // Do something with the error
            }
            // Maybe the specific error classes if the caller cared about it
            is NetworkResponse.Success -> {
                val something = response.body // this may now throw an exception where before it did not!
            }
        }

If the caller doesn't change their code, they don't see a no-content response as Nothing, it's still whatever data type it was before, likely Unit.

By leaving the body property as part of the Success sealed interface you now introduce a potential issue with the upgrade path of the library. Old code that references Success.body may now, unexpectedly, throw an exception. (Regardless of the issues around a 204. I'm just talking pure API interfaces here.) I disagree with that. This forces the user to go through their entire code base and make sure to never reference Success.body without a try/catch and to convert to using OK or NoContent.

If that's a requirement it should be explicit in the API.

In my opinion throwing an exception is contrary to much of the intent of this library. The whole point of the call adapter is to convert what would have normally been Retrofit exceptions into concrete objects that is much more safely handled by the caller.

— Reply to this email directly, view it on GitHubhttps://github.com/haroldadmin/NetworkResponseAdapter/pull/56#issuecomment-1034940167, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABVXPIDR4DIED7BQH3UBQZTU2O6SNANCNFSM5NZDIZKQ. Triage notifications on the go with GitHub Mobile for iOShttps://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Androidhttps://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub. You are receiving this because you authored the thread.Message ID: @.***>

[haroldadmin]

Excellent points in the discussion so far.

Dividing Success into Ok and NoContent forces us to either move the body property to Ok, or throw an exception on the body property on NoContent. Both of these changes break the contract of the existing API.

I agree that Ok and NoContent better model the problem domain. Even if it's okay to break backward compat, this change introduces certain usability issues:

1. Consumers would need to be aware of handling Ok/NoContent types for ALL responses even if its guaranteed that their API always responds in the same manner.
2. NoContent is just one type of special response. What if an API responds with a different success data models for the same endpoint based on some conditions? Would we to introduce a Success<S1, S2..., Sn> interface to model 'n' alternative response types?

Given these issues, I am not sure if this change provides a net benefit over the current Unit-based special case for no content responses.

I'm of the opinion that polymorphic and multi-morphic response bodies should be dealt with using data deserialization libraries. If your API responds with different types of data based on certain conditions, then it's responsibility to make your success body type can model those different data responses. For example, take a look at polymorphic deserialization in kotlinx.serialization and Moshi.

This library only helps you distinguish between successful and failed network calls. It does not concern itself with modelling API responses correctly.

---

The current implementation assumes a successful response is not nullable/optional which is incorrect from the HTTP protocol perspective.

The v5 version of the library does not make this assumption. S and E generic parameters of NetworkResponse do not have an upper bound of Any, so they can very well be assigned to nullable types.