search

JakeWharton / retrofit2-kotlin-coroutines-adapter

A Retrofit 2 adapter for Kotlin coroutine's Deferred type.
Apache License 2.0
1.97k stars 129 forks source link

Read nullability information of response body type. #5

Open JakeWharton opened 6 years ago

JakeWharton commented 6 years ago

Without depending on a ridiculously massive library...

luckyhandler commented 6 years ago

Will this handle kotlin.KotlinNullPointerException in CoroutineCallAdapterFactory.kt:102 in case of a non-existent body?

AndrewWestberg commented 6 years ago

We're seeing this issue when sending HTTP DELETE request that is returning a 204 with no response body.

davyskiba commented 5 years ago

@AndrewWestberg @itsmortoncornelius do you have any workaround for this?

luckyhandler commented 5 years ago

@davyskiba I don't use this library in this case but return a call which I by myself wrap in a GlobalScope.async.

AndrewWestberg commented 5 years ago

@davyskiba We do the same. Wrap Call with GlobalScope.async. It's pretty ugly and I'd rather be able to use this Call Adapter for all calls instead.

davyskiba commented 5 years ago

@itsmortoncornelius @AndrewWestberg thanks, will try this approach

cbruegg commented 5 years ago

Even using Kotlin reflection, the current design of Retrofit wouldn't allow this, or so I believe. CallAdapter.Factory.get only receives the returnType: Type, annotations and the Retrofit instance. None of these provide access to the Java method that was invoked. Similarly, the adapt method in CallAdapter only receives a Call<T> that contains no reference to the invoked method.

One way to work around this is an additional annotation denoting that the body may be null. PR #31 implements this.

stravag commented 5 years ago

I created a pull request that should fix this issue: https://github.com/JakeWharton/retrofit2-kotlin-coroutines-adapter/pull/36

JakeWharton commented 5 years ago

Your PR ignores any nullability declaration and allows the propagation of null into a non-null type.

LouisCAD commented 5 years ago

@JakeWharton Is this really a problem in practice? I don't care having a NPE in library code (that I don't really control) when it would happen anyway in my code.

In fact, I think it's better to have the library not check for nulls there. If you want a nullable type or use Void for existing API definition from Kotlin code with coroutines, you're out of luck. On the other hand, having an NPE in your code because something went wrong, instead of inside library code, is not a big deal IMO.

LouisCAD commented 5 years ago

Of course, #36 doesn't really fixes this issue since it does not read nullability, but I think it's a pragmatic solution.

JakeWharton commented 5 years ago

I don't

LouisCAD commented 5 years ago

I don't want to pursue this debate for too long, but in Java, you don't have this issue, and you have null unsafety everywhere (or annotations everywhere), and that didn't prevent people from debugging nullability issues they may have encountered with code using Retrofit.

If you have reasons behind your opinion that I may not be aware of, I'm still interested to know about them.

JakeWharton commented 5 years ago

This is Kotlin and Kotlin tracks nullability

On Fri, Nov 2, 2018 at 9:20 AM Louis CAD notifications@github.com wrote:

I don't want to pursue this debate for too long, but in Java, you don't have this issue, and you have null unsafety everywhere (or annotations everywhere), and that didn't prevent people from debugging nullability issues they may have encountered with code using Retrofit.

If you have reasons behind your opinion that I may not be aware of, I'm still interested to know about them.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/JakeWharton/retrofit2-kotlin-coroutines-adapter/issues/5#issuecomment-435377894, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEfPxMybkpNgfBNhXGRAuFQEozJJ9ks5urEaNgaJpZM4SRG0u .

stravag commented 5 years ago

Well yes, that's true you end up with a null in a non-null type, didn't think of that. Only had the "Void" case in mind, and didn't want the call adapter to crash.

But I don't think you'll be able to solve it the way you want, and I honestly don't think you should. Since the Retrofit repsonse.body is annotated @Nullable you technically always have to expect a null body, and since you lose the generic type info at runtime there's no way to know if it's nullable or not.

If you know in advance that the server is not going to provide a body use Void response type to make it obvious. If you're not sure, you're better off checking the HTTP Status Code for 204/205 and/or use a Optional Response Type (which will also work mit my PR), see also: https://github.com/square/retrofit/blob/9a1b08e4fbb5cc56dd6e3f93ef4d99e386fa8064/retrofit/src/main/java/retrofit2/OkHttpCall.java#L216

josephbnb commented 5 years ago

@JakeWharton is there any possible workaround for this? I have a call that always returns HTTP/1.1 204 No Content with no response body and I'm getting kotlin.KotlinNullPointerException in CoroutineCallAdapterFactory.kt:102.

kevindmoore commented 5 years ago

I created a copy of the adapter to handle this. Seems to work: https://gist.github.com/kevindmoore/769328e5a256080706cd23029c622e7b