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

Specify support for Jakarta REST ExceptionMapper<T> #89

Open
erdlet opened this issue Sep 27, 2022 · 8 comments
Open

Specify support for Jakarta REST ExceptionMapper<T> #89

erdlet opened this issue Sep 27, 2022 · 8 comments

Comments

@erdlet
Copy link
Contributor

erdlet commented Sep 27, 2022

I recognized that we're not specifying exception handling in MVC applications or, at least, how the existing approaches shall be used. In my opinion, we should add some section about how MVC implementations should handle the response from jakarta.ws.rs.ext.ExceptionMapper implementations, because at the moment there is the need to use Krazo-specific API (Viewable).

@Provider
@Priority(Interceptor.Priority.APPLICATION + 999)
public class GeneralExceptionMapper implements ExceptionMapper<Exception> {

    @Override
    public Response toResponse(final Exception exception) {
        return Response.serverError().entity(new org.eclipse.krazo.engine.Viewable("serverError.jsp")).build();
    }
}

I think the implementations should ensure, that we can return the same values as in @Controller annotated resources, so we have a consistent and clear behavior. The example posted above would then look like this:

@Provider
@Priority(Interceptor.Priority.APPLICATION + 999)
public class GeneralExceptionMapper implements ExceptionMapper<Exception> {

    @Override
    public Response toResponse(final Exception exception) {
        return Response.serverError().entity("serverError.jsp").build();
    }
}

Please add your feedback or possible Jakarta REST limitations which I'm not aware of.

@chkal
Copy link
Contributor

chkal commented Oct 15, 2022

I agree that this is an aspect which may need clarification.

I see two options for this:

  • We could make Viewable part of the spec. In this case, we simply need to describe that the exception handler has to return a Viewable entity for rendering to work correctly.
  • We keep Viewable as an implementation detail. But in this case we will have to support that exception handlers return a view name (simple string as shown above) which is AFAIK currently not possible, because all the HTML rendering happens in a MBW for the Viewable type.

I'm not completely sure how to proceed here. Actually, I really like the fact the Krazo currently simply transforms the String return type of the controller to a Viewable to let the MBW for Viewable do the rendering. So if we go with the second approach, we may have to change this.

@erdlet
Copy link
Contributor Author

erdlet commented Oct 23, 2022

I'd prefer the first option, as this seems to be the easiest and most transparent way of handling exceptions in this case.

Sure, we need to make Viewable more than a implementation detail, but on the other hand other implementations have a consistent way of handling exceptions too.

@erdlet erdlet added this to the 3.0 milestone Oct 23, 2022
@erdlet
Copy link
Contributor Author

erdlet commented Oct 23, 2022

Added this to the upcoming milestone as this seems like a important feature to me, because a good exception handling is important for UX and security.

@chkal
Copy link
Contributor

chkal commented Oct 29, 2022

I agree that option 1 could be the best way to go. But maybe Viewable should be an interface in this case? Not sure yet to be honest.

@erdlet
Copy link
Contributor Author

erdlet commented Oct 30, 2022

I think it should be an interface too. So it is easier to integrate it in existing other MVC implementations which are not based on Jakarta MVC but maybe want to be in the future.

@chkal
Copy link
Contributor

chkal commented Oct 30, 2022

But in this case users would have to create an instance of some MVC implementation class to actually use this feature, correct? Not sure how it could work without this.

@erdlet
Copy link
Contributor Author

erdlet commented Oct 30, 2022

Ah right. Hmm... probably we can do something with default methods inside the interface to work around this issue? Just some "stupid" factory methods creating anonymous instances of the interface?🤔 Don't know if this works out or is something we should do inside the API🤔

@chkal
Copy link
Contributor

chkal commented Oct 30, 2022

Yeah, possible. But I wonder if providing an actual class instead of an interface in the API would be simpler? Just thinking out loudly. Not sure which way I prefer TBH.

@erdlet erdlet removed this from the 3.0 milestone Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants