Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4.0.0 JAX-RS ExceptionMapper disrupts metrics, health endpoints #7959

Closed
tjquinno opened this issue Nov 6, 2023 · 5 comments
Closed

4.0.0 JAX-RS ExceptionMapper disrupts metrics, health endpoints #7959

tjquinno opened this issue Nov 6, 2023 · 5 comments
Assignees
Labels
4.x Version 4.x bug Something isn't working MP webserver wontfix This will not be worked on works as designed

Comments

@tjquinno
Copy link
Member

tjquinno commented Nov 6, 2023

Environment Details

  • Helidon Version: 4.0.0
  • Helidon SE or Helidon MP MP
  • JDK version:
  • OS:
  • Docker version (if applicable):

Problem Description

Adding a general, catch-all JAX-RS exception mapper to an app disrupts the metrics and health endpoints (and perhaps other observers' endpoints as well).

As coded in the repo, the MP QuickStart app correctly responds to the /health and /metrics endpoints.

If you add an exception mapper to the app, then it is invoked when the /health and /metrics endpoints are accessed.

Steps to reproduce

  1. cd examples/quickstarts/helidon-quickstart-mp
    mvn package

    The preceding commands work, including running unit tests which ping the health and metrics endpoints.

  2. Add the following class to the MP QuickStart app:

    package io.helidon.examples.quickstart.mp;
    
    import jakarta.ws.rs.core.Response;
    import jakarta.ws.rs.ext.ExceptionMapper;
    import jakarta.ws.rs.ext.Provider;
    
    @Provider
    public class GeneralExceptionMapper implements ExceptionMapper<Exception>  {
        @Override
        public Response toResponse(Exception e) {
            return Response.status(500).entity(e.getMessage()).build();
        }
    }
  3. Now mvn package fails during the /health or /metrics endpoint tests because the exception mapper returns 500 responses.

@tjquinno tjquinno added bug Something isn't working MP webserver 4.x Version 4.x labels Nov 6, 2023
@barchetta barchetta added this to the 4.0.1 milestone Nov 7, 2023
@barchetta barchetta removed this from the 4.0.1 milestone Nov 13, 2023
@tomas-langer
Copy link
Member

The exception handler defined in the example cannot work, as it transforms any kind of exception into an internal server error, including JAX-RS web excepetions that handle other states (such as 404 not found, 401 unauthorized, and 403 forbidden).

At least 404 MUST be correctly handled, as otherwise Helidon does not understand that the problem was that the endpoint was not found. When Jersey returns 500, we consider that final response and must return it to the caller.

Jersey has priority over other Helidon endpoints, so we first try to serve anything using Jersey, and only then we resolve Helidon WebServer routing.

The (minimal) correct handler:

    @Provider
    public static class GeneralExceptionMapper implements ExceptionMapper<Exception> {
        @Override
        public Response toResponse(Exception e) {
            if (e instanceof NotFoundException web) {
                return web.getResponse();
            }
            return Response.status(500).entity(e.getMessage()).build();
        }
    }

@tomas-langer tomas-langer added wontfix This will not be worked on works as designed labels Nov 20, 2023
@tomas-langer
Copy link
Member

I think that this works as designed - if exception handler tells us to return 500, we will return it.
The exception handler should be aware of JAX-RS jakarta.ws.rs.WebApplicationException that are used to return specific responses as part of control flow, and should not consider these to be internal server errors.

@m0mus m0mus closed this as completed Dec 7, 2023
@AshwinPrabhuB
Copy link

This is a definite change in behaviour between H3 and H4. The same exception mapper that is disrupting Health routing in H4 was not fouling in H3.

@FelTap23
Copy link

FelTap23 commented May 2, 2024

But why is it not able to identify metrics and health paths?

@shad0w-jo4n
Copy link

shad0w-jo4n commented Jun 4, 2024

Quite a strange behavior. For example, if I want to make a custom response for a 404 error, then I will have to get UriInfo from the context and check the path in it for compliance with the path of metrics, OpenAPI, etc.

It looks terrible:

var path = uriInfo.getPath();

if ((path.equals("metrics") || path.startsWith("openapi")) && e instanceof NotFoundException web) {
    return web.getResponse();
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x Version 4.x bug Something isn't working MP webserver wontfix This will not be worked on works as designed
Projects
Archived in project
Development

No branches or pull requests

7 participants