Response Body in ClientExceptionMapper is always null

[PaKu5]

Describe the bug

We implemented a REST-Client using the quarkus-rest-client-reactive dependency.
For this client we added custom exception handling using the @ClientExceptionMapper annotation.
There we could read the body of the response.
e.g.:

@ClientExceptionMapper
static ServiceException handle(Response response) {
    response.bufferEntity();
    Error error = response.readEntity(Error.class);
    return new ServiceException(error.getReason(), error.getMessage(), response.getStatus());
}

Now with the new version the entity object is always null. (So error.getMessag will throw a NullPointerException)
(Also if we jus handle the WebApplicationException we can't read the body because its null)
For the status everything works fine.

This works for quarkus-plattform version 2.13.3 but not in version 2.14.1.

Expected behavior

The error object should be the parsed (json-)-response-object.

Actual behavior

The error object is null because the entity of response is null.

How to Reproduce?

1. Create a rest-client
2. Make a request that will fail (400, 404, ...)
3. Handle the custom/WebApplicationException
4. read the response body from the exception

Output of uname -a or ver

No response

Output of java -version

openjdk version "17.0.2" 2022-01-18 OpenJDK Runtime Environment (build 17.0.2+8-86) OpenJDK 64-Bit Server VM (build 17.0.2+8-86, mixed mode, sharing)

GraalVM version (if different from Java)

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.8.2 (ea98e05a04480131370aa0c110b8c54cf726c06f)

Additional information

No response

[quarkus-bot]

/cc @Sgitario, @cescoffier

[geoand]

Can you please attach a sample project that exhibits this behavior?

[PaKu5]

Here is a link to an example project:
https://github.com/PaKu5/ExceptionMapperExample

You can just run the testcase. I printed the output of the response.
I used an example wiremock to create an 400 response with body.

Thank you!

[PaKu5]

[geoand]

@Sgitario any idea what changed in 2.13 and 2.14 that would cause this behavior?

[tisba29]

This fix breaks the possibilty to read the response body of an error sent by a server (ex: 400) in a ResponseExceptionMapper : #29119

[tisba29]

I also reproduce the problem in my project. Can't use 2.14.1 version, have to stay in 2.14.0 for now.

[geoand]

I can confirm that the problem does not occur in 2.14.0.Final but does occur in 2.14.1.Final and indeed #29119 looks like the culprit...

[geoand]

Actually I can no longer reproduce this problem with any version...

[tisba29]

In my test, sometimes i am able to get the body, sometimes not. How does the ClientRestHandler priority works ? If an handler reads the body before https://github.com/quarkusio/quarkus/blob/main/independent-projects/resteasy-reactive/client/runtime/src/main/java/org/jboss/resteasy/reactive/client/handlers/ClientSendRequestHandler.java attachSentHandlers -> sent.onSuccess is called, then I can read the response body.

[geoand]

If an handler reads the body before

Which handlers are you seeing doing this?

[geoand]

@Sgitario in any case https://github.com/quarkusio/quarkus/blob/2.14.1.Final/independent-projects/resteasy-reactive/client/runtime/src/main/java/org/jboss/resteasy/reactive/client/handlers/ClientSendRequestHandler.java#L247-L250 is wrong.

I am assuming you wanted to return after resuming, but if you do that, then the following test (which is similar to the reproducer in this issue) will fail:

package io.quarkus.rest.client.reactive.error.clientexceptionmapper;

import static io.quarkus.rest.client.reactive.RestClientTestUtil.setUrlForClass;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;

import javax.ws.rs.GET;
import javax.ws.rs.Path;
import javax.ws.rs.Produces;
import javax.ws.rs.core.Response;

import org.eclipse.microprofile.rest.client.inject.RegisterRestClient;
import org.eclipse.microprofile.rest.client.inject.RestClient;
import org.jboss.resteasy.reactive.RestResponse;
import org.jboss.shrinkwrap.api.asset.StringAsset;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.rest.client.reactive.ClientExceptionMapper;
import io.quarkus.test.QuarkusUnitTest;

public class ReadEntityClientExceptionMapperTest {

    @RegisterExtension
    static final QuarkusUnitTest TEST = new QuarkusUnitTest()
            .withApplicationRoot((jar) -> jar
                    .addClasses(DummyException.class, DummyClient.class, DummyResource.class)
                    .addAsResource(
                            new StringAsset(setUrlForClass(ReadEntityClientExceptionMapperTest.DummyClient.class)
                                    + "\nmicroprofile.rest.client.disable.default.mapper=true"),
                            "application.properties"));
    private static final String JSON = "{\"foo\": \"bar\"}";

    @RestClient
    DummyClient dummyClient;

    @Test
    void test() {
        DummyException dummyException = assertThrows(DummyException.class, dummyClient::get);
        assertThat(dummyException.getJson()).isEqualTo(JSON);
    }

    @Path("dummy")
    @RegisterRestClient
    public interface DummyClient {

        @GET
        Response get();

        @ClientExceptionMapper
        static DummyException toException(Response response) {
            return new DummyException(response.readEntity(String.class));
        }
    }

    @Path("dummy")
    public static class DummyResource {

        @GET
        @Produces("application/json")
        public RestResponse<String> respond() {
            return RestResponse.status(RestResponse.Status.BAD_REQUEST, JSON);
        }
    }

    public static class DummyException extends RuntimeException {

        private final String json;

        public DummyException(String json) {
            this.json = json;
            setStackTrace(new StackTraceElement[0]);
        }

        public String getJson() {
            return json;
        }
    }

}

If you don't resume and don't return, then this test passes, but I am not really sure it's the proper fix...

[Sgitario]

@Sgitario in any case https://github.com/quarkusio/quarkus/blob/2.14.1.Final/independent-projects/resteasy-reactive/client/runtime/src/main/java/org/jboss/resteasy/reactive/client/handlers/ClientSendRequestHandler.java#L247-L250 is wrong.

I am assuming you wanted to return after resuming, but if you do that, then the following test (which is similar to the reproducer in this issue) will fail:

package io.quarkus.rest.client.reactive.error.clientexceptionmapper;

import static io.quarkus.rest.client.reactive.RestClientTestUtil.setUrlForClass;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertThrows;

import javax.ws.rs.GET;
import javax.ws.rs.Path;
import javax.ws.rs.Produces;
import javax.ws.rs.core.Response;

import org.eclipse.microprofile.rest.client.inject.RegisterRestClient;
import org.eclipse.microprofile.rest.client.inject.RestClient;
import org.jboss.resteasy.reactive.RestResponse;
import org.jboss.shrinkwrap.api.asset.StringAsset;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.rest.client.reactive.ClientExceptionMapper;
import io.quarkus.test.QuarkusUnitTest;

public class ReadEntityClientExceptionMapperTest {

    @RegisterExtension
    static final QuarkusUnitTest TEST = new QuarkusUnitTest()
            .withApplicationRoot((jar) -> jar
                    .addClasses(DummyException.class, DummyClient.class, DummyResource.class)
                    .addAsResource(
                            new StringAsset(setUrlForClass(ReadEntityClientExceptionMapperTest.DummyClient.class)
                                    + "\nmicroprofile.rest.client.disable.default.mapper=true"),
                            "application.properties"));
    private static final String JSON = "{\"foo\": \"bar\"}";

    @RestClient
    DummyClient dummyClient;

    @Test
    void test() {
        DummyException dummyException = assertThrows(DummyException.class, dummyClient::get);
        assertThat(dummyException.getJson()).isEqualTo(JSON);
    }

    @Path("dummy")
    @RegisterRestClient
    public interface DummyClient {

        @GET
        Response get();

        @ClientExceptionMapper
        static DummyException toException(Response response) {
            return new DummyException(response.readEntity(String.class));
        }
    }

    @Path("dummy")
    public static class DummyResource {

        @GET
        @Produces("application/json")
        public RestResponse<String> respond() {
            return RestResponse.status(RestResponse.Status.BAD_REQUEST, JSON);
        }
    }

    public static class DummyException extends RuntimeException {

        private final String json;

        public DummyException(String json) {
            this.json = json;
            setStackTrace(new StackTraceElement[0]);
        }

        public String getJson() {
            return json;
        }
    }

}

If you don't resume and don't return, then this test passes, but I am not really sure it's the proper fix...

This block:

if (Response.Status.Family.familyOf(status) != Response.Status.Family.SUCCESSFUL) {
    httpClientRequest.connection().close();
    requestContext.resume();
}

Fixes this issue #28818, plus I had to add the requestContext.resume() because there were some tests failing. I will have another look today though.

[geoand]

I will have another look today though.

Thanks

[Sgitario]

After having another look at the changes in #29119, this fixes the following issue #28818 which only happens when the body is large (the server takes a time to fully read the large body and hence not releasing the client connection until is fully read).

Therefore, I think we should revert the changes in #29119 and update the issue #28818 to state that this happens when you use a large body.

Wdyt? @geoand

[geoand]

@Sgitario yes, I agree. The cure here is way worse than the disease

[knutwannheden]

We also encountered this issue and spent a lot of time trying to create a simple reproducer (at the time we first looked into it, this issue didn't exist yet). I am glad I now found this issue.

[felipelx07]

Any fix to 2.14.4-Final ?
It's still happen in that version