Read nullability information of response body type.
Opened this issue · 17 comments
Without depending on a ridiculously massive library...
Will this handle kotlin.KotlinNullPointerException in CoroutineCallAdapterFactory.kt:102 in case of a non-existent body?
We're seeing this issue when sending HTTP DELETE request that is returning a 204 with no response body.
@AndrewWestberg @itsmortoncornelius do you have any workaround for this?
@davyskiba I don't use this library in this case but return a call which I by myself wrap in a GlobalScope.async.
@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.
@itsmortoncornelius @AndrewWestberg thanks, will try this approach
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.
Your PR ignores any nullability declaration and allows the propagation of null into a non-null type.
@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.
Of course, #36 doesn't really fixes this issue since it does not read nullability, but I think it's a pragmatic solution.
I don't
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.
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
@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.
I created a copy of the adapter to handle this. Seems to work:
https://gist.github.com/kevindmoore/769328e5a256080706cd23029c622e7b