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

Response status lost when error body parse fail #23

Closed
DIG- opened this issue Sep 15, 2020 · 5 comments · Fixed by #26
Closed

Response status lost when error body parse fail #23

DIG- opened this issue Sep 15, 2020 · 5 comments · Fixed by #26
Labels
bug Something isn't working

Comments

@DIG-
Copy link

DIG- commented Sep 15, 2020

When receiving an error response, if body parse fails we lose the reponse status code.
Since it is really a valid response, I think it is plausible to return as ServerError with empty body instead of UnknownError.

@haroldadmin
Copy link
Owner

There's no way for NetworkResponseAdapter to know if the received response was valid or not. That's for your JSON deserialization library to handle.

If you can provide a small example here to illustrate your usecase, I can look into it. Otherwise, this is working as intended.

@haroldadmin haroldadmin added the question Further information is requested label Sep 27, 2020
@Frank1234
Copy link

Frank1234 commented Oct 6, 2020

I have the same problem.

As an example, my server sends json error responses that we can parse, but for a 429 Too Many Requests response, AWS sends the error response, and not my server. That error response is not parsable, because the json differs from what our own server sends.

In this case, we get an "UnknownError" with a json parse exception. This doesn't tell us anything about the problem, and I can only show a general message to the user.

If UnknownError would have an optional status code, we can at least know what went wrong and present something useful to the user.

@haroldadmin
Copy link
Owner

Makes sense. I'll try to experiment with a design in which UnknownResponse contains nullable headers and response code. Thanks for the example of your usecase!

@ThanosFisherman
Copy link

I'm in an exact same situation here. Our Rest api throws different Error Json responses differentiated by their status code. I do not want to parse all of the error messages I just need to know the 4xx status code to act accordingly.

@haroldadmin
Copy link
Owner

Sorry for the long wait on this issue. I spent some time investigating it today. Here are my findings:

  1. It is possible to extract the status code and headers for a network request that results in NetworkResponse.UnknownError, but only if the status code lies outside 200-300 range.
  2. If the status code indicates a successful response (200 <= code < 300), Retrofit tries to parse the response body using a registered response converter. If the response converter throws an exception while parsing the body, Retrofit decides that the call failed and only forwards the original call and the raised exception to the registered call adapter. This means that any information related to headers and status code is lost (see here).
  3. If the status code indicates an unsuccessful response, Retrofit does not try to parse the body and forwards the response to the registered call adapter. In this case, I can manually parse the body and do the right thing.
  4. Therefore, it is possible to extract the response headers and status code from failed request, but not always. With this in mind, I will update the NetworkResponse.UnknownError class to include nullable fields for headers and status code. These fields will be populated only if they can be extracted from the failed request.

The fields will only be unpopulated in a weird scenario where the server responds with a success code and an invalid response body, so I don't think it could be a major problem.

Expect a new release with these fixes soon. Thank you everyone for reporting it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants