Retrofit: RxJava 2 Adapter: null may slip into RxJava

[akarnokd]

There was a question on Stackoverflow recently which reported an unexpected NullPointerException caught by the one of the internal safeguards of RxJava 2.

Looks like retrofit2.Response.body() may return null which is handed over to RxJava 2 without validation in BodyObservable.

@Test
public void responseBodyNull() throws Exception {

    Response<Integer> response = Response.success(null);

    Assert.assertNull(response.body());

    Class<?> clazz = Class.forName("retrofit2.adapter.rxjava2.BodyObservable");
    Constructor<?> c = clazz.getDeclaredConstructor(Observable.class);
    c.setAccessible(true);
    Observable<Integer> o = (Observable)c.newInstance(Observable.just(response));

    o.test()
    .assertNever(r -> r == null)
    .assertTerminated();
}

I don't know what the reasonably response to a null body could be:

• complete the Observable without value or
• signal a NoSuchElementException.

[naturalwarren]

I think it makes sense to complete the Observable. It's not really an error and if you've written a custom converter to return null when you've received an empty body (as JW described in #1554) this seems like the right behavior.

I'd love to help out with a PR here if you have any interest Jake. I've had trouble with backends returning empty response bodies and am currently use a delegating converter to return null. This is going to cause troubles now that I'm moving my Retrofit APIs to RxJava 2.

[JakeWharton]

I don't think there's any action to take here. A null body in the success (HTTP 2xx) case should warrant the use of Observable<Response<Integer>> in the Retrofit interface signature.

Completing the stream without a value is definitely the wrong solution as it means the successful-but-empty response will go unprocessed due to the lack of actionable notification downstream onto which operators can be applied.

This is most strongly exemplified by the Single<Integer> case which more accurately represents an HTTP request. Observable<Integer> is just a variant of this where onSuccess is turned into the pair of onNext/onComplete calls (despite the implementation details actually being the reverse of this).

So there's three ways to handle this case:

• Use Observable<BodyType> which indicates you expect to never receive null in the success case.
• Use Observable<Response<BodyType>> which allows null bodies to be emitted inside the Response<> wrapper and handled accordingly.
• Write a delegating Converter which transforms a null body into some other type which can abstract it (like Optional<T> such that you can use Observable<Optional<BodyType>>).

[JakeWharton]

We may want to add a first-party delegating converter that supports Optional...

[JakeWharton]

Optional converters released in 2.3.0

[iflove]

thanks

[sheaye]

@JakeWharton : Excuse me. You means that in release 2.3.0 we can solve the problem with the inner BuiltInConverters.VoidResponseBodyConverter ? But it seemes that the responseBodyConverter do not work when the status code is 204, I saw the code in the OkhttpCall:

At last, forgive me for my English.

[JakeWharton]

Converters are ResponseBody --> T and we don't want them to have to understand null. What are you trying to solve?

[davidbilik]

I have a one thing to note here, we needed to fork BodyObserver into our lib and we've converted it to Kotlin. If we use Completable type as return type in Retrofit interface, on this line https://github.com/square/retrofit/blob/master/retrofit-adapters/rxjava2/src/main/java/retrofit2/adapter/rxjava2/BodyObservable.java#L51 is the response.body() null and if we double bang it, it will throw an exception. I don't have such a deep knowledge of RxJava to know why it doesn't matter, its probably not checked inside onNext method and .ignoreElements() called later will hide this fact. We've solved it by sending info to BodyObservable if the type is Completable and if so, we call observer.onComplete() instead of onNext. Reference https://github.com/AckeeCZ/rxoauth/blob/master/retrofitadapter/src/main/java/cz/ackee/rxoauth/adapter/BodyObservable.kt#L42