Skip to content

Simplify generic types; handle no content response #56

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Simplify generic types; handle no content response #56

wants to merge 1 commit into from

Conversation

argenkiwi
Copy link

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

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

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

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

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

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

argenkiwi commented Feb 8, 2022 via email

@haroldadmin
Copy link
Owner

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

argenkiwi commented Feb 10, 2022 via email

@fergusonm
Copy link

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

argenkiwi commented Feb 10, 2022 via email

@haroldadmin
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@argenkiwi @fergusonm @haroldadmin