Add error response field to InvalidResponseCodeException

[chrisfillmore]

[REQUIRED] Use case description

I would like to include the HTTP response body in an InvalidResponseCodeException, so that I can read the response.

In particular, our license server sends back specific error codes which we need to triage.

Proposed solution

Add a new field to InvalidResponseCodeException which stores the value returned from HttpURLConnection#getErrorStream() (or string equivalent).

Alternatives considered

We could make our license request without using Exo's APIs. But I think making the change in Exo would be useful for everyone, and backwards-compatible.

I can make a pull request, with your support.

[ojw28]

#3920 was requesting this as well.

@chrisfillmore - Is there a reason to use 403 rather than 200 for this case (see the issue ref'd above for more information)?
@jrocharodrigues - Did you implement this already, or did you do it differently?

[chrisfillmore]

In our particular case we get HTTP 400 with an error code in the response body.

It's not feasible for us to change the HTTP status from the license proxy; this would have a far-reaching impact on other client platforms we support. But I also don't think that 200 is an appropriate status for this response. The server wants to indicate the request is bad, hence 400.

Is there a particular reason you'd prefer not to add this field? I'll admit, to me this seems a straightforward change with obvious benefit, without breaking anything. Am I missing something?

[ojw28]

Is there a particular reason you'd prefer not to add this field? I'll admit, to me this seems a straightforward change with obvious benefit, without breaking anything. Am I missing something?

Agreed it's not particularly difficult. There's a weird case in which there's an error reading the body though. What would you end up throwing in that case? To throw the InvalidResponseCodeException without the body in this case would be misleading, because calling code might be expecting the body to be present and you're not indicating that a failure occurred when reading it. Putting another exception inside the InvalidResponseCodeException to convey that failure would work, but is pretty weird and over complicated. Throwing the error that occurred when reading the body directly means you don't convey the response code to the caller in this case, but I suspect is probably the right thing to do.

[jrocharodrigues]

@ojw28 Hi,
Yes i've implemented it.
I've created a custom HttpDataSource with a custom HttpDataSourceException that includes the reponse code headers and responseBody.

  /**
   * Thrown when an attempt to open a connection results in a response code not in the 2xx range.
   */
  public static final class ExtendedInvalidResponseCodeException extends HttpDataSourceException {

    /**
     * The response code that was outside of the 2xx range.
     */
    public final int responseCode;

    /**
     * An unmodifiable map of the response header fields and values.
     */
    public final Map<String, List<String>> headerFields;

    /**
     * The response body.
     */
    public final String responseBody;

    public ExtendedInvalidResponseCodeException(int responseCode, Map<String, List<String>> headerFields,
                                                String responseBody, DataSpec dataSpec) {
      super("Response code: " + responseCode, dataSpec, TYPE_OPEN);
      this.responseCode = responseCode;
      this.headerFields = headerFields;
      this.responseBody = responseBody;
    }

  }

[ojw28]

Thanks for the information. How did you handle the case mentioned above (i.e. failure whilst reading the response body)?

[jrocharodrigues]

In that case the responseBody on the will be empty, and when handling the error i will look only at the responseCode, and treat the error as a generic.
It is not an ideal generic solution, but in my case it works :)

[chrisfillmore]

Thinking about it, there is not a great solution to the problem of "what happens if reading the error response fails". I think the right thing to do, in that case, is to set the new error response field to an empty string or byte array. It's not perfect but it is an incremental improvement. If users run into this particular problem, then they could investigate a better solution.

What do you think?

[ojw28]

It seems quite error prone to do that. If someone has a service where the empty body means something different to a non-empty body, they'll have no way to disambiguate whether an empty body is because the read failed, or because the body really was empty. So I think my preference would still be to throw "something else" in that case. Which is also consistent with what would happen if there was a problem reading the response headers, I think.

[jrocharodrigues]

@ojw28 i think that's a good idea. Throw "something else" when trying to read the responseBody from the InvalidResponseCodeException.
That way if you don't need the body you have the current behaviour, and if you need it you are "forced" to handle that case.

[chrisfillmore]

Some ideas:

1. Make the field nullable (null means failed to read, empty string means nothing there, otherwise the response body)
2. Add another field to InvalidResponseCodeException that signals whether the error response body was successfully read. (Something along the lines of C.RESULT_NOTHING_READ vs C.RESULT_END_OF_INPUT ... or something else, open to suggestions here.)
3. If reading the error stream results in an IOException (or other exception), attach that exception to the InvalidResponseCodeException, and expose it. (This seems like the most helpful to end users, since they could inspect the exception.)

What do you guys have in mind for throwing "something else"? I think we would still want to throw InvalidResponseCodeException, since this exception still applies.

[jrocharodrigues]

@chrisfillmore i was thinking in the same lines of your point 3.

The method getResponseBody() on the InvalidResponseCodeException would throw the exception originated from the parse of the error stream.

[chrisfillmore]

@jrocharodrigues ok I see what you mean now. That's a good idea; I'm on board. @ojw28 do we have your blessing?

[ojw28]

My suggestion was actually to throw something simpler. I'd be tempted just to treat it as a generic HttpDataSourceException with TYPE_OPEN, which is what gets thrown in the case that the response headers aren't read. The reasoning for this suggestion would be:

1. Nesting an optional non-cause exception inside an exception, to explain why the exception itself is incomplete, seems pretty weird.
2. The body should always be small in this case, so the probability of failing to read it having opened the connection is also small. Hence we don't expect this case to happen much. When it does happen, throwing what would have been thrown if what's presumably a network failure had occurred ever so slightly sooner shouldn't really make much difference.

Note that if the body isn't always small in this case, then it's flawed to be trying to read it into the exception in the first place.

[chrisfillmore]

Ok, I want to take a closer look at the behaviour of HttpURLConnection#getErrorStream(), because I'm getting conflicting information. Android docs say:

If the connection was not connected, or if the server did not have an error while connecting or if the server had an error but no error data was sent, this method will return null. This is the default.

But Android Studio tells me that getErrorStream() never returns null.

To be safe, I'm going to add a null check. I'll open a draft PR so you can give me some feedback.

[ojw28]

This is now implemented in dev-v2.