quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.84k stars 2.7k forks source link

Entity missing in case of WebApplicationException #44508

Closed FloGro3 closed 1 week ago

FloGro3 commented 1 week ago

Describe the bug

When using Quarkus-Rest-Client in Quarkus 3.15.1 and catching a WebApplicationException because e.g. the call results in an internal server error WebApplicationException is missing the entity from the response. The problem is that the Headers from the response are copied to WebApplicationException which means that a content-length header is present without any entity: Image

Expected behavior

When catching a WebApplicationException which results from a RestClient call and building a response from the webApplicationException via Response.fromResponse(webApplicationException.getResponse()).build(); then the response should include the response body from the failed RestClient call.

Actual behavior

The build response has not entity but not matching header fields (e.g. content-length): Image

How to Reproduce?

Here is a reproducer project: https://github.com/FloGro3/Minimal-Reproducer-Quarkus-3.15-LTS/tree/main

To simulate the issue call "/hello" endpoint. The endpoint itself calls another endpoint inside the application which returns an internal server error with a small entity.

Output of uname -a or ver

No response

Output of java -version

21.0.5

Quarkus version or git rev

3.15.1

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

Apache Maven 3.8.6

Additional information

No response

quarkus-bot[bot] commented 1 week ago

/cc @cescoffier (rest-client)

FloGro3 commented 1 week ago

Issue does not occur in Quarkus 3.8 LTS version.

geoand commented 1 week ago

I just tried 3.8 LTS and the corresponding client (quarkus-rest-client-reactive) does behave in the exact same way as it does in 3.15.

Now this is not something I really want to change, not least because you can easily work around the behavior by doing:

        try {
            return defaultApi.test();
        } catch (WebApplicationException webApplicationException) {
            webApplicationException.getResponse().readEntity(String.class); // force reading the body
            Response response = Response.fromResponse(webApplicationException.getResponse()).build();
            return response;
        }
cescoffier commented 1 week ago

Except if it's a regression, I don't believe we want to change that behavior (like @geoand said).

FloGro3 commented 1 week ago

@geoand You are right, the entity is also not read in by default in Quarkus 3.8 LTS (don't know why it does it anyway on our side). The big difference I see between 3.8 LTS and 3.15 LTS is that the header fields are not present in 3.8, but in 3.15. In both cases we don't have any entity by default which means that in 3.15 we send an invalid response, because header fields like content-length are set, but no body is returned (as entity is null by default). In 3.8 this header fields are not present.

Both simulated with my minimal reproducer. 3.8: Image

3.15: Image

cescoffier commented 1 week ago

Can you check the headers in both cases?

FloGro3 commented 1 week ago

So in case of 3.8 we have these headers in webApplicationException response (which are then also copied to the response that is build based on the webApplicationException response: Image

In case of 3.15 we have these headers: Image Content-length is set to "32" in this case even though we don't send any body by default.

geoand commented 1 week ago

@FloGro3 I checked and the behavior of quarkus-rest-client in 3.15 is the same as quarkus-rest-client-reactive in 3.8.

Having said that, I do of course need to point out that the quarkus-rest-client in 3.15 is not the same one as in 3.8 - the quarkus-rest-client of 3.8 is essentially the quarkus-resteasy-client in 3.15. We renamed the REST Client see this and this for more details.

FloGro3 commented 1 week ago

@geoand I see. I didn't take the renaming into consideration. Thanks for the hint. I assume as the behavior is then consistent across different Quarkus versions you will not change it, right? Even though I still find it weird that we have to force read the entity from the WebApplicationException response to generate valid responses out of it. But I will then apply your suggested workaround for all of our cases. Thanks for that :)

geoand commented 1 week ago

🙏🏽