Propagate server response to exception#37
Conversation
|
Use case: |
| public UnsuccessfulResponseException(int code) { | ||
| super("Unsuccessful response code received from stream: " + code); | ||
| this.code = code; | ||
| this(buildSurrogateResponse(code)); |
There was a problem hiding this comment.
I would not do this; it'd be preferable to just let getResponse() return null in this case. The rationale can't be backward compatibility, since old code that uses this deprecated constructor couldn't be looking for a Response.
There was a problem hiding this comment.
Actually, I also realized that the whole PR didn't work.
I think due to the async fact that the whole response is not available yet when UnsuccessfulResponseException is created within the library's code (unlike the error code).
In other words, there are more changes needed (even if this constructor change is reverted).
So far, we happily used the library without this change anyway - @eli-darkly , I'm OK if it is rejected (when I need it badly, I'll propose proper change).
There was a problem hiding this comment.
Hmm. Can you say more about what specifically was not working? I would expect that trying to read the body would not work, since other code might already have read the body - that's just an expected issue whenever you share a Response around. But I would have expected the headers to be available.
eli-darkly
left a comment
eli-darkly
left a comment
There was a problem hiding this comment.
Sorry it took us a while to get to this. It seems like a potentially useful feature but I left one comment about an implementation concern.
(#2) add code coverage reports, improve CI
2 participants
It is often the case (especially for HTTP status 500) when many details are hidden in the headers/body of the HTTP server response.
Since
OkHttplibrary already constructs and provides theResponseobject, it makes sense to propagate it to the library's client code viaUnsuccessfulResponseException.