Kong / unirest-java

Unirest in Java: Simplified, lightweight HTTP client library.
http://kong.github.io/unirest-java/
MIT License
2.58k stars 591 forks source link

Cannot use response.mapError with ByteResponse #424

Closed andrei-ivanov closed 2 years ago

andrei-ivanov commented 2 years ago

Using a HttpResponse<byte[]> response, I cannot use mapError on it because it converts the byte array, like [97, 98, 99] into a literal String representation of the array, like '[97, 98, 99]', instead of abc, in getErrorBody.

To Reproduce Steps to reproduce the behavior:

  1. Have an endpoint that would return an octet/stream in case of success, but a JSON in case of failure, like a Spring REST controller would do, with the default error handling:
    {
    "timestamp": "2022-01-05T16:56:26.920+0000",
    "status": 500,
    "error": "Internal Server Error",
    "message": "Some message",
    "path": "/getContent"
    }
    
    static class Content {
    String error;
    String message;
    }

Unirest.get(url) .asBytes() .ifFailure((HttpResponse<byte[]> httpResponse) -> { // this will invoke BaseResponse.getErrorBody // which will invoke getBody() that returns the byte array // and the byte array passed through config.getObjectMapper().writeValue(body) // which returns something like Arrays.toString(byteArray) Content error = response.mapError(Content.class); response.getParsingError() // no parsing error here actually, the object will be empty .ifPresent(e -> logger.error("Unable to parse body", e)); if (error.message == null) { // this will have the same result as Arrays.toString(body) // not the actual string representation constructed from the byte array, like new String(body) System.out.println(response.mapError(String.class)); } });

Since I can't find such a public endpoint, I can offer a test that proves just the conversion to `String`:
```java
String url = "https://httpbin.org/bytes/-1";
Unirest.get(url)
    .asBytes()
    .ifFailure((HttpResponse<byte[]> httpResponse) -> {
        // we don't really know the response encoding, since it's stored in the request, doh, assuming UTF-8
        String body = httpResponse.mapBody(bytes -> new String(bytes, StandardCharsets.UTF_8));
        System.out.println(body);
        // this should have printed the same HTML as above
        System.out.println(httpResponse.mapError(String.class));
    });

Expected behavior I expected both mapError to be able to return an object instance populated with the values from the response, but since the byte array gets converted to String using Arrays.toString(..), it will fail.

Screenshots If applicable, add screenshots to help explain your problem.

Environmental Data:

ryber commented 2 years ago

Wow, what is this JavaScript 🤣

This is actually even weirder than it looks, the final string conversion is done by the ObjectMapper which is being asked to encode the body to a string and does so literally, I've bypassed that for the case of byte[] to go to a string(byte[]) but I'm not sure this is the fully correct answer, we don't actually know fully if the byte[] is in fact a string. I've patched it to pass the tests in 3.13.6. I think it's more right than it was before, but probably not 100% right, need to mull on that a bit.

At the very least it still needs to look at the response body for the encoding (which should in fact be there), it's assuming UTF-8 right now.

andrei-ivanov commented 2 years ago

Thanks for the fix 🙂 Should I create a ticket for the HTTP response encoding to be available in the response object? It seems weird that it's in the request...

ryber commented 2 years ago

The request contains the encoding that you asked for (or a default of UTF-8) which might not be what you got. We have to look at the response Content-Type, which in the case you cited should be application/json or application/json;charset=utf-8 or whatever the encoding it. It could ALSO come from the Content-Encoding header which might tell us that the byte[] is gziped (in which case these tests would probably fail maybe...although it might already be deflated by the time we get here).

If you want to open a new issue feel free otherwise I will.

ryber commented 2 years ago

well, I guess we do use the request one, sometimes, eh, I think we just assume the response is what we asked for. So that might be another issue, if the response says something that contradicts the request. Pretty edge case.