quarkiverse / quarkus-resteasy-problem

Unified error responses for Quarkus REST APIs via Problem Details for HTTP APIs (RFC9457 & RFC7807)
https://docs.quarkiverse.io/quarkus-resteasy-problem/dev
Apache License 2.0
69 stars 12 forks source link

MAJOR: Response is truncated when calling from one service to another and error happens on backend #429

Open tmulle opened 1 month ago

tmulle commented 1 month ago

I ran into an issue when developing a project for our company set to go live in a few days. So I'm hoping this is a simple issue.

I'm seeing the JSON output from resteasy-problem being truncated in the scenario when I make a rest call from one service to another service and that service throws an error (404 in this case).

For example, I have a reproducer at: https://github.com/tmulle/quarkus-resteasy-problem-test

I have a simulated gateway and server service in their respective projects.

To Test:

  1. Check out project
  2. Start both the server and gateway projects in separate terminals..I started my server on port 12000 and gateway on 12001. I hardcoded the backend in the RemoteService class in the gateway project. If you change ports you'll need to modify the code.
  3. Send a POST using curl or postman/insomnium to http://localhost:12000/hello/notFound and observe the correct JSON error response as shown below. This is hitting the backend server service directly.
  4. Send a POST using curl or postman/insomnium to http://localhost:12001/gateway/notFound and observe you get the truncated JSON error message as shown below. This is hitting the gateway which then calls the server and returns the response.

Correct Response - When direct call to the backend service

{
    "status": 404,
    "title": "Not Found",
    "detail": "HTTP 404 Not Found",
    "instance": "/hello/notFound"
}

Truncated Response - When called through gateway (the output is missing the rest of the structure)

{
    "status": 404,
    "title": "Not Found",
    "detail": "Received: 'Not Found, status code 404' when invoki

I don't get any other errors so I'm not sure why the json is being truncated. I found this issue because my Javascript web frontend I'm developing which is calling my GATEWAY->REMOTE_SERVICE threw an exception when parsing the JSON error from my Quarkus servers which then lead me to track down the issue to this library or something near it.

Here is the output I get from Insomnium when looking at the network request. Something looks wrong at the end of the stream.

* Preparing request to http://localhost:12001/gateway/notFound
* Current time is 2024-10-05T02:27:16.161Z
* Enable automatic URL encoding
* Using default HTTP version
* Enable timeout of 120000ms
* Enable SSL validation
* Enable cookie sending with jar of 6 cookies
* Found bundle for host localhost: 0x1cd002bb0f30 [serially]
* Can not multiplex, even if we wanted to!
* Re-using existing connection! (#135) with host localhost
* Connected to localhost (127.0.0.1) port 12001 (#135)

> POST /gateway/notFound HTTP/1.1
> Host: localhost:12001
> User-Agent: insomnium/0.2.3-a
> Accept: */*
> Content-Length: 0

* Mark bundle as not supporting multiuse

< HTTP/1.1 404 Not Found
< content-length: 94
< Content-Type: application/problem+json
< Content-Type: application/problem+json

* Received 185 B chunk
* Excess found in a read: excess = 91, size = 94, maxdownload = 94, bytecount = 0
* Closing connection 135

For comparison, here is the network request when calling the service directly.

* Preparing request to http://localhost:12000/hello/notFound
* Current time is 2024-10-05T03:12:31.574Z
* Enable automatic URL encoding
* Using default HTTP version
* Enable timeout of 120000ms
* Enable SSL validation
* Enable cookie sending with jar of 6 cookies
* Found bundle for host localhost: 0x1cd002bd1c20 [serially]
* Can not multiplex, even if we wanted to!
* Re-using existing connection! (#139) with host localhost
* Connected to localhost (127.0.0.1) port 12000 (#139)

> POST /hello/notFound HTTP/1.1
> Host: localhost:12000
> User-Agent: insomnium/0.2.3-a
> Accept: */*
> Content-Length: 0

* Mark bundle as not supporting multiuse

< HTTP/1.1 404 Not Found
< content-length: 93
< Content-Type: application/problem+json

* Received 93 B chunk
* Connection #139 to host localhost left intact

If you need anymore info, let me know..

pazkooda commented 1 month ago

Hi, I'm on mobile so haven't had possibility to run your project yet, but...

Have a feeling this truncation does not come from this extension.

Please see: https://github.com/quarkiverse/quarkus-resteasy-problem/blob/main/TROUBLESHOOTING.md#some-webapplicationexceptions-seems-not-to-be-handled-properly as your project fits exactly into this category.

Would suspect RestClient. Will try to look deeper in your case later on though

tmulle commented 1 month ago

Could the issue be when the server side is already returning an HttpProblem formatted message and the client is trying to parse that as an exception it doesn't know how? Because you're expecting a standard Jax-RS or Java exception format and not your HttpProblem format?

I would hope I use HttpProblem as the defacto error output both on one server and from server-server communication.

If I rebuild the http problem from the response and return it, it "sort of" works. I get a complete JSON response now but the detail of the problem is the unparsed json string of the incoming HttpProblem response.

 @ClientExceptionMapper
    static RuntimeException toException(Response response) {

        // REbuild the response
        HttpProblem problem = HttpProblem.builder()
        .withStatus(response.getStatus())
        .withDetail(response.readEntity(String.class))
        .withTitle(response.getStatusInfo().getReasonPhrase())
            .build();

        System.out.println("Got a client error " + response.readEntity(String.class));

        return problem;

    }

Output from curl

{
    "status": 400,
    "title": "Bad Request",
    "detail": "{\"status\":400,\"title\":\"Bad Request\",\"detail\":\"Your request was invalid\",\"instance\":\"/server/badRequest\"}",
    "instance": "/gateway/badRequest"
}

Funny enough, when you log out the error to the console it has all the information and isn't cut of like the response returned from the method call.

Got a client error {"status":400,"title":"Bad Request","detail":"Your request was invalid","instance":"/server/badRequest"}
2024-10-05 18:51:58,292 INFO  [http-problem] (executor-thread-1) status=400, title="Bad Request", detail="{"status":400,"title":"Bad Request","detail":"Your request was invalid","instance":"/server/badRequest"}"

I updated the test code with the new attempt.

tmulle commented 1 month ago

Ok another update to the code.. code is checked into my example

I think the reason is what I suggested is that the code isn't expecting an already formatted HttpProblem coming in.

I added this code to my gateway code as a proof of concept and it works. It's a hack but shows that if I check for an already formatted incoming message I can rebuild the original message and then return that, otherwise return a standard java exception.

 @ClientExceptionMapper
    static RuntimeException toException(Response response) {

        // Read the custom media type (if any)
        MediaType mediaType = response.getMediaType();

        // If we have a HttpProblem type response

        if (mediaType.equals(HttpProblem.MEDIA_TYPE)) {
            System.out.println("We have an HttpProblem format");

            // This would normally be passed into us by Quarkus
            ObjectMapper mapper = new ObjectMapper();

            // Holds the original
            HttpProblemRaw parsedOriginal;

            try {

                // Read the original HttpProblem
                // Normally this would be the real instance but it uses a Builder
                // and isn't parseable by Jackson at the moment
                parsedOriginal = mapper.readValue(response.readEntity(String.class), HttpProblemRaw.class);

                // Build the new message with the original fields
                HttpProblem newProblem = HttpProblem.builder()
                .withDetail(parsedOriginal.getDetail())
                .withInstance(parsedOriginal.getInstance())
                .withStatus(parsedOriginal.getStatusCode())
                .withTitle(parsedOriginal.getTitle())
                .withType(parsedOriginal.getType())
                .build();

                return newProblem;
            } catch (Exception e) {
                e.printStackTrace();
                return new RuntimeException(e);
            }
        }

        // This would be the normal handling of NON HttpProblem formatted messages
        switch (response.getStatus()) {
            case 404: return new NotFoundException(response);
            case 400: return new BadRequestException(response);
            default: return new ServerErrorException(response);
        }
    }
tmulle commented 1 month ago

@pazkooda

I made a final update to my test code.

I ended up adding a global HttpProblemExceptionMapper which will look for the http-problem header and if it finds it will reparse the response body into a new HttpProblem exception and copy all the fields from the original.

This seems to work and fixes my issue for the time being. I looked in the src of the extension and am not the exact location this code would go. I did see an HttpProblemMapper already there but it looks like it just returns the same incoming problem instance.

I didn't see anywhere where you're checking for an existing HttpProblem formatted response.

It would be nice to have this logic in the extension to handle the use cases where both the server and client are using the HttpProblem extension and I want to just pass through the backend formatted message.

/**
 * Provider which will look for a REST response formatted in the HttpProblem format
 * by looking for the MEDIA-TYPE header.
 * 
 * If it finds it, it will reparse it and rebuild the HttpProblem to rethrow.
 */
@Provider
public class HttpProblemExceptionMapper implements ResponseExceptionMapper<RuntimeException> {

    @Inject
    Logger log;

    @Inject
    ObjectMapper mapper;

    @Override
    public RuntimeException toThrowable(Response response) {

       // Read the custom media type (if any)
        MediaType mediaType = response.getMediaType();

        // If we have a HttpProblem type response

        if (mediaType != null && mediaType.equals(HttpProblem.MEDIA_TYPE)) {
            log.infof("We have an HttpProblem format");

            // Holds the original
            HttpProblemRaw parsedOriginal;

            try {

                // Read the original HttpProblem
                // Normally this would be the real instance but it uses a Builder
                // and isn't parseable by Jackson at the moment
                parsedOriginal = mapper.readValue(response.readEntity(String.class), HttpProblemRaw.class);

                // Build the new message with the original fields
                Builder newProblem = HttpProblem.builder()
                .withDetail(parsedOriginal.getDetail())
                .withInstance(parsedOriginal.getInstance())
                .withStatus(parsedOriginal.getStatusCode())
                .withTitle(parsedOriginal.getTitle())
                .withType(parsedOriginal.getType());

                // Add in the headers back in
                // Using for loop to get around lambda effectively final warning
                Map<String, Object>  headers = parsedOriginal.getHeaders();
                if (headers != null) {
                    for (Entry<String,Object> entry : headers.entrySet()) {
                        newProblem.withHeader(entry.getKey(), entry.getValue());
                    }
                }

                // Add params back in
                Map<String, Object>  params = parsedOriginal.getParameters();
                if (params != null) {
                    for (Entry<String,Object> entry : params.entrySet()) {
                        newProblem.with(entry.getKey(), entry.getValue());
                    }
                }

                return newProblem.build();

            } catch (Exception e) {
                log.error("Could not parse HttpProblem message", e);
                return new RuntimeException(e);
            }
        }

        // Let the others handle non-http formats
        return null;
    }

    /**
     * We want this to run first before the other HttpProblem provider 
     */
    @Override
    public int getPriority() {
        return Priorities.USER - 100;
    }
}
lwitkowski commented 1 month ago

@tmulle Thanks for the report and all the details, I've run some experiments and there's indeed something strange going on there.

It is related to how RestClient throws ClientWebApplicationExceptions which for some reason triggers our exception mappers, because Response's entity is not yet read (it's still in the buffer) and thus - is null. What is also weird: HttpProblem serialized by JacksonSerializer looks ok, not truncated, all the fields are properly pushed into output buffer.

I need more time to investigate it, and construct a full repeatable reproducer in the form of integration test - I have failed so far...

It would be nice to have this logic in the extension to handle the use cases where both the server and client are using the HttpProblem extension and I want to just pass through the backend formatted message.

That's something we have had on our list for a long time, but there was not enough motivation to do this.

tmulle commented 1 month ago

@lwitkowski Thanks for the verification that I am indeed seeing some strangeness :)

The code I have in my test repo with the HttpProblemExceptionMapper works for me right now and allows me to have one service call another, both using the HttpProblem extension.

That's something we have had on our list for a long time, but there was not enough motivation to do this. Haha, yeah I always seem to find edge cases in my projects...I would suspect more people would be doing this in the future especially if HttpProblem extension is trying to be a standard response format.

I'd be happy to help if I can and submit PRs or whatever in my spare time. This extension has replaced my custom APIError framework that myself and a buddy of mine were using to have a standard REST response. It would be nice to have this extension support all use cases.

It is related to how RestClient throws ClientWebApplicationExceptions which for some reason triggers our exception mappers, because Response's entity is not yet read (it's still in the buffer) and thus - is null

Dumb question, have you tried using Response.readEntity(String.class,..) and not .getEntity()? I know everytime I do .getEntity() it is always null. I'll take a closer look through your codebase.