Skip to content
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

Expecting an empty observable results in JsonMappingException #1613

Closed
rtedin opened this issue Feb 18, 2016 · 7 comments
Closed

Expecting an empty observable results in JsonMappingException #1613

rtedin opened this issue Feb 18, 2016 · 7 comments

Comments

@rtedin
Copy link

rtedin commented Feb 18, 2016

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
Copy link
Author

rtedin commented Feb 18, 2016

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
Copy link
Contributor

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 [email protected]
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
#1613 (comment).

@artem_zin

@JakeWharton
Copy link
Collaborator

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
Copy link
Author

rtedin commented Feb 18, 2016

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
Copy link
Author

rtedin commented Feb 18, 2016

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
Copy link
Collaborator

By the way, here's an example of a null-to-empty delegating converter that doesn't require wrapping: #1554 (comment)

@rtedin
Copy link
Author

rtedin commented Feb 18, 2016

Thanks, seems a much cleaner solution than our workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants