square / retrofit

A type-safe HTTP client for Android and the JVM
https://square.github.io/retrofit/
Apache License 2.0
43.1k stars 7.3k forks source link

Expecting an empty observable results in JsonMappingException #1613

Closed rtedin closed 8 years ago

rtedin commented 8 years ago

Hi.

We need to interpret an empty body response as an empty observable (completes, no errors, no items emitted). The response code from the server is HTTP 200. The current Retrofit implementation throws a JsonMappingException since it tries to deserialize the empty body. We suppose the implementation assumes that there must always be an item in the response body. However, no item is also a valid use case for an observable. The following test illustrates the issue.

import com.fasterxml.jackson.databind.ObjectMapper;
import okhttp3.HttpUrl;
import okhttp3.OkHttpClient;
import okhttp3.mockwebserver.MockResponse;
import okhttp3.mockwebserver.MockWebServer;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import retrofit2.Retrofit;
import retrofit2.adapter.rxjava.RxJavaCallAdapterFactory;
import retrofit2.converter.jackson.JacksonConverterFactory;
import retrofit2.http.GET;
import retrofit2.http.Headers;
import rx.Observable;
import rx.observers.TestSubscriber;

import java.net.HttpURLConnection;

public class EmptyObservableTest {

    public static final String BASE_URL = "/api/";
    private MockWebServer webServer;
    private Service service;

    public interface Service {

        @GET("person")
        @Headers("Accept-Encoding: application/json")
        Observable<Person> getPerson();
    }

    public static class Person {
        public String name;
        public int age;
    }

    @Before
    public void setUp() throws Exception {
        webServer = new MockWebServer();
        webServer.start();
        HttpUrl url = webServer.url(BASE_URL);

        Retrofit.Builder builder = new Retrofit.Builder();
        OkHttpClient httpClient = new OkHttpClient();
        ObjectMapper objectMapper = new ObjectMapper();

        builder.client(httpClient)
                .addCallAdapterFactory(RxJavaCallAdapterFactory.create())
                .addConverterFactory(JacksonConverterFactory.create(objectMapper))
                .baseUrl(url.toString());

        Retrofit retrofit = builder.build();
        service = retrofit.create(Service.class);
    }

    @After
    public void tearDown() throws Exception {
        webServer.shutdown();
    }

    @Test
    public void testReturnEmptyObservable() throws Exception {

        // Setup empty body response.
        webServer.enqueue(new MockResponse()
                .setHeader("Content-Type", "application/json")
                .setResponseCode(HttpURLConnection.HTTP_OK)
        );

        // Perform the request and subscribe to the observable.
        Observable<Person> observable = service.getPerson();
        TestSubscriber<Person> subscriber = new TestSubscriber<>();
        observable.subscribe(subscriber);

        // Expected: empty observable.
        subscriber.assertNoErrors();
        subscriber.assertNoValues();
        subscriber.assertCompleted();
    }
}

Note that we want to use Observable<Person> getPeople() for the service signature. Is there some workaround for this? Many thanks.

rtedin commented 8 years ago

The workaround we found until now: use a wrapper around a Call<ResponseBody>.

    // Service wrapper.
    public static class Service {
        private ServiceEndPoint endPoint;

        public Service(Retrofit retrofit) {
            endPoint = retrofit.create(ServiceEndPoint.class);
        }

        Observable<Person> getPerson() {
            return Observable.create(new Observable.OnSubscribe<Person>() {
                @Override
                public void call(Subscriber<? super Person> subscriber) {
                    if (!subscriber.isUnsubscribed()) {
                        try {
                            Response<ResponseBody> response =
                                    endPoint.getPerson().execute();

                            if(!response.isSuccess()) {
                                throw new HttpException(response);
                            }

                            String json = response.body().string();

                            if(json != null && json.length() > 0) {
                                Person person = new ObjectMapper()
                                        .readValue(json, Person.class);
                                if (!subscriber.isUnsubscribed()) {
                                    subscriber.onNext(person);
                                }
                            }

                            if (!subscriber.isUnsubscribed()) {
                                subscriber.onCompleted();
                            }
                        } catch (Exception ex) {
                            if (!subscriber.isUnsubscribed()) {
                                subscriber.onError(ex);
                            }
                        }
                    }
                }
            });
        }

        // Service definition.
        private interface ServiceEndPoint {
            @GET("person")
            @Headers("Accept-Encoding: application/json")
            Call<ResponseBody> getPerson();
        }
    }

A true solution would be to fix RxJavaCallAdapterFactory, if possible.

artem-zinnatullin commented 8 years ago

A true solution would be catch error and turn it into whatever you want via Rx operators.

If you expect empty body then your json entity should represent that.

Even if it's "valid" case for Observable, what will you do for rx.Single in the same case? Single can't be empty!

On Thu, 18 Feb 2016, 06:35 Rafael Tedin Alvarez notifications@github.com wrote:

The workaround we found until now: use a wrapper around a Call.

// Service wrapper.
public static class Service {
    private ServiceEndPoint endPoint;

    public Service(Retrofit retrofit) {
        endPoint = retrofit.create(ServiceEndPoint.class);
    }

    Observable<Person> getPerson() {
        return Observable.create(new Observable.OnSubscribe<Person>() {
            @Override
            public void call(Subscriber<? super Person> subscriber) {
                if (!subscriber.isUnsubscribed()) {
                    try {
                        Response<ResponseBody> response =
                                endPoint.getPerson().execute();

                        String json = response.body().string();

                        if(json != null && json.length() > 0) {
                            Person person = new ObjectMapper()
                                    .readValue(json, Person.class);
                            if (!subscriber.isUnsubscribed()) {
                                subscriber.onNext(person);
                            }
                        }

                        if (!subscriber.isUnsubscribed()) {
                            subscriber.onCompleted();
                        }
                    } catch (Exception ex) {
                        if (!subscriber.isUnsubscribed()) {
                            subscriber.onError(ex);
                        }
                    }
                }
            }
        });
    }

    // Service definition.
    private interface ServiceEndPoint {

@GET("person") @Headers("Accept-Encoding: application/json"

) Call getPerson(); } }

A true solution would be to fix RxJavaCallAdapterFactory, if possible.

— Reply to this email directly or view it on GitHub https://github.com/square/retrofit/issues/1613#issuecomment-185676324.

@artem_zin

JakeWharton commented 8 years ago

The current Retrofit implementation throws a JsonMappingException since it tries to deserialize the empty body.

Jackson throws this, not Retrofit.

We suppose the implementation assumes that there must always be an item in the response body.

This is an accurate statement for Jackson, yes. {} is a valid empty object. A 0-byte payload is not valid JSON and thus you get the exception thrown.

To support empty payloads you can wrap the JacksonConverterFactory in one which detects an empty payload (via body.contentLength() == 0) and returns null. Then you can either filter the resulting Observable stream for non-null elements or wrap the RxJavaCallAdapterFactory to do this automatically for you.

There's no action for Retrofit to take here, though.

rtedin commented 8 years ago

A true solution would be catch error and turn it into whatever you want via Rx operators.

Well, we were not considering it an error. With our workaround this is what we basically do: we catch the error before it occurs, i.e. we prevent it. But you're right, we should consider to represent a no-value as {}.

Even if it's "valid" case for Observable, what will you do for rx.Single in the same case? Single can't be empty!

Allow me to remove the "" around valid. It is a valid use case for Observable without a doubt. We weren't concerned with rx.Single since, well, as you said, Single can't be empty. If it were empty, Single would do what Single does when it finds himself in this case, report the error.

Your help was much appreciated! Your comment about Single made me realize that we could/should use it in other parts.

rtedin commented 8 years ago

To support empty payloads you can wrap the JacksonConverterFactory in one which detects an empty payload (via body.contentLength() == 0) and returns null. Then you can either filter the resulting Observable stream for non-null elements or wrap the RxJavaCallAdapterFactory to do this automatically for you.

Good suggestions. Many thanks!

JakeWharton commented 8 years ago

By the way, here's an example of a null-to-empty delegating converter that doesn't require wrapping: https://github.com/square/retrofit/issues/1554#issuecomment-178633697

rtedin commented 8 years ago

Thanks, seems a much cleaner solution than our workaround.