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

Why use RetrofitError #789

Closed
dominicfarr opened this issue Mar 20, 2015 · 9 comments
Closed

Why use RetrofitError #789

dominicfarr opened this issue Mar 20, 2015 · 9 comments

Comments

@dominicfarr
Copy link

Calling a Service that returns, for example, HTTP 404, causes the Service to throw an exception: RetrofitError. Is this really an exception?

Granted in HTTP it is an error code, but it is an standard response and not unexpected per se?

Throwing an exception means you have to catch it if you need to handle them and that makes code more complex.

I would like to understand the rational behind this choice.

Thanks

@casidiablo
Copy link

I think the rational behind this choice is that errors usually return bodies that are not necessarily parseables to the expected type. For instance, let's say you want to get a list of users:

@GET("/users")
List<User> listUsers();

If the server responds with a, let's say a 401 like this:

{"error": "unauthorized"}

There's no way for you to parse this message into a list, which means it will anyway throw at converting time.

@JakeWharton
Copy link
Member

Yep. That's pretty much it. In v2 we'll have some kind of wrapper object (e.g., Response<List<User>>) that you can opt into and check .success() or something on it. Network errors might still throw.

@JakeWharton
Copy link
Member

There's also a lot of historical baggage inside of Retrofit since it's nearly a 4-year-old project. It had a long pre-1.x lifetime and the move to 1.x was not very drastic. In the next few months a lot of these things will be changed in what has been learned in the last 2-3 years of real, hard use.

@dominicfarr
Copy link
Author

I suppose that was what i was hinting at; Response that wraps the type is cleaner.

Please don't get me wrong, I'm not here to criticise, Retrofit is excellent. Happy hear these things are being considered for V2.

As for 50x and exceptions, I'm still thinking it should follow the other HTTP codes with Response wrapped types. Retrofit didn't throw an exception, the calling service isn't available, which is 1 of many expected responses.

Not throwing an exception leaves it open to perform retries on error codes; retries with different wait or back off strategies. See https://github.com/rholder/guava-retrying This is powerful and flexible.

@crossle
Copy link

crossle commented Mar 21, 2015

@JakeWharton when v2 release?

@JakeWharton
Copy link
Member

Right now it's looking to drop the same day as Half-Life 3...

On Sat, Mar 21, 2015 at 2:05 AM Crossle Song notifications@github.com
wrote:

@JakeWharton https://github.com/JakeWharton What time v2 release?


Reply to this email directly or view it on GitHub
#789 (comment).

@dominicfarr
Copy link
Author

oh dear...that far out, eh.

@JakeWharton
Copy link
Member

Also worth considering, we need an unchecked mechanism for propagating information. By specifying List<User> in the above example you are opting in to this mechanism because we have nothing else to do but throw.

In any case, this issue isn't specific and it's something we are aware of and will fix.

@dominicfarr
Copy link
Author

Okay - look forward to seeing V2.

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

4 participants