-
Notifications
You must be signed in to change notification settings - Fork 7.3k
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.
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
HTTP 204/205 bypass converter and unconditionally send null to adapter #2867
Comments
You should be able to add an OkHttp Interceptor that can convert original response code from |
If you need to error the request out, you can detect 204 in OkHttp Interceptor and throw a error, sorry I misinterpret the issue, I though you wanted to handle 204 as 200. |
@artem-zinnatullin Aha, thanks for your advice! I just tried the throwing-in-interceptor approach out, and it seems to satisfy my needs. Nonetheless, I am curious if Retrofit would ever allow to configure custom validation (similar to Alamofire) for response codes or if it's completely against the Retrofit's design ideas. |
Isn't this violating the
The way I read it, And because
So this does not work, which is rather unexpcted (empty body is a valid body after all, right?) |
I'm going to look at trying a PR for better handling of this, but part of the confusion is also that RxJava silently drops the |
This is a proposal implementation to address square#2867. The issue is two-fold 1 - Retrofit 2's RxJava 2 `BodyObservable` will, on 204/205 status codes, still try to send the `null` body into `Observer#onNext`, even though RxJava 2 does not allow nulls in the stream 2 - RxJava 2's `Single#fromObservable()` implementation does not null-check the onNext value, and instead just silently skips it. When an `onComplete` event comes from `BodyObserver` later, it then throws a `NoSuchElementException` This tries to solve it by being aware of 204/205 status codes in `BodyObservable` and sending a new `NoContentException` through `onError` in these events instead of the obscured `null` into RxJava. `NoContentException` is basically identical to `HttpException` except that it's only for 204/205 codes and used to indicate that a 204/205 event happened in a non-`Completable` environment. Other considerations made: - Converter - not feasible because converters are not given a chance at 204/205 responses - Forwarding call adapter - can work, but requires a lot of knowledge of how the underlying RxJava 2 adapter behaves and would (IMO) be something that's always required. Even if consumers before this had custom adapters to handle this, they can still lean on Retrofit and now catch a more concrete exception if they were trying to guess from an `onErrorResumeNext` or something similar.
I've put up a proposal PR in #3003, feedback welcome! |
Inspired from the snippet just above, I made a version that doesn't fail when the server responds with a content length of zero, and is lenient if there's actually some content: import okhttp3.Interceptor
import okhttp3.MediaType
import okhttp3.Response
import okhttp3.ResponseBody
object EmptyBodyInterceptor : Interceptor {
override fun intercept(chain: Interceptor.Chain): Response {
val response = chain.proceed(chain.request())
if (response.isSuccessful.not() || response.code().let { it != 204 && it != 205 }) {
return response
}
if ((response.body()?.contentLength() ?: -1) >= 0) {
return response.newBuilder().code(200).build()
}
val emptyBody = ResponseBody.create(MediaType.get("text/plain"), "")
return response
.newBuilder()
.code(200)
.body(emptyBody)
.build()
}
} |
Also inspired from the snippets above, here is my version: import okhttp3.Interceptor
import okhttp3.MediaType.Companion.toMediaType
import okhttp3.Response
import okhttp3.ResponseBody.Companion.toResponseBody
class NoContentInterceptor : Interceptor {
private val noContentHttpStatusCodes = arrayOf(204, 205)
override fun intercept(chain: Interceptor.Chain): Response {
val response = chain.proceed(chain.request())
if (!response.isSuccessful) {
return response
}
if (!noContentHttpStatusCodes.contains(response.code)) {
return response
}
return response
.newBuilder()
.code(200)
.body("".toResponseBody("text/plain".toMediaType()))
.build()
}
} |
You should still propagate headers |
Hi, can you tell me how to properly use My Retrofit version is 2.9.0. Thanks! |
It requires that you are using OkHttp 4.x |
Thanks for the reply! As far as I know, Retrofit is a wrapper around OkHttp, and I can find okhttp3 package in my project after I configured Retrofit 2.9.0. However, I don't know what version exactly Retrofit brought me... Should I configure OkHttp 4.x independently in build.gradle? |
Yes. Retrofit depend on OkHttp 3.14 and the latest binary compatible release is 4.9. |
Got it! You really saved my day. Thanks again! |
I've solved the issue by (in Interceptor):
|
This issue hasn't been resolved since the v2.10.0 version of Retrofit? |
The behavior has not changed, no. |
I stumbled upon this: #1554 and added a converter because I wanted to handle the empty response body and not pass it as null to Observable. That thread is talking about code 200 and empty body though.
Then I found out that in the current Retrofit version no converter is called at all for 204 and 205:
This forces me to use
Single<Response<SomeModel>>
instead ofSingle<SomeModel>
I was hoping to use...I understand that 204 stands for no content, so it's only logical to pass the response as null. But for my specific case, I would prefer to handle 204 in
onError
, so I hoped to handle the empty body in the converter and just throw a custom error there instead.Is there any chance that approach will be changed? I am relatively new to Retrofit but I am used to Alamofire on iOS and there it's possible to specify during configuration what is treated as a successful response code:
.validate(statusCode: 200)
So every code that is not on that list would be handled as an error.
The text was updated successfully, but these errors were encountered: