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

Add system that can globally convert any exception to an ErrorResponse #30567

Closed
mzeijen opened this issue May 31, 2023 · 6 comments
Closed

Add system that can globally convert any exception to an ErrorResponse #30567

mzeijen opened this issue May 31, 2023 · 6 comments
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: superseded An issue that has been superseded by another

Comments

@mzeijen
Copy link

mzeijen commented May 31, 2023

As a user I would like a system that can convert any exception that is thrown during request processing to an ErrorResponse. This also includes exceptions that can be thrown before the @ExceptionHandler methods can handle exceptions. This should allow the server to always respond with an ErrorResponse for any exception.

To make this possible, I propose creating a system where converters can be registered that take an exception (and possibly any other relevant HTTP request information) and convert them to an ErrorResponse instance. Spring can itself provide default converters for all its exceptions, but users should also be able to provide their own converters. Either to override the default converters from Spring or to be able to convert their own exceptions globally.

The converters should be ordered, and the converter with the highest precedence should be used when multiple converters can be applied to a specific exception.

By providing a converter that can convert Throwable to an ErrorResponse, there can be a catch-all converter for those exceptions for which no converter or higher-level @ExceptionHandler was defined.

This system can be a replacement of the ResponseEntityExceptionHandler class, although it probably should still stick around for a while for backwards compatible reasons.

Note: I have discussed this feature request with @rstoyanchev at Spring I/O 2023

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label May 31, 2023
@rstoyanchev
Copy link
Contributor

Thanks for raising this.

Generally, I understand the goal is to convert Throwable to ErrorResponse, but we already have such a mechanism. @ExceptionHandler methods on Controller or ControllerAdvice can map Throwable to ErrorResponse.

Note that all Spring Web exceptions are implementations of ErrorResponse. So all that ResponseEntityExceptionHandler does is to return them from an @ExceptionHandler method, without conversion. Its secondary purpose is to allow customization either in code through protected methods, or through a MessageSource.

In summary, exceptions can implement ErrorResponse and be handled automatically in ResponseEntityExceptionHandler. For others, use @ExceptionHandler methods to map Throwable to ErrorResponse.

So I'm wondering what's actually missing?

@rstoyanchev rstoyanchev added status: waiting-for-feedback We need additional information before we can continue in: web Issues in web modules (web, webmvc, webflux, websocket) labels Jun 15, 2023
@spring-projects-issues
Copy link
Collaborator

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@spring-projects-issues spring-projects-issues added the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Jun 22, 2023
@mzeijen
Copy link
Author

mzeijen commented Jun 23, 2023

Dear spring-projects-issues bot, I am still intended to look at this, but I first need to do some other things ;).

The following I already e-mailed to Rossen directly:

Regarding on the conversion from Throwable to ErrorResponse I agree it needs some more clarification. In its current form it is just an alternative to the ResponseEntityExceptionHandler implementation. If that is only what it is, then I agree it might not be worth the effort. However, I think there is possible still a reason to consider changing it a bit. It might not need to be as flexible as the system I propose though. The reason is a suspicion I have: A while ago, we also considered getting rid of our own conversion system in our problem implementation, in favor of using just exception handlers to do all conversions. However, we hit some limitations that it wasn’t possible to apply it for every type of common web exception and that is why we kept the current “flexible” implementation. I am not sure though if the ResponseEntityExceptionHandler has the same limitations, so I need to do some investigation in to that.

If I find any issues then I will either create new issues for them, or if I believe it requires the conversion system to be overhauled, I will add it to the existing issue on exception conversion. If I don’t see any reason for the conversion system to be overhauled, then I will close the ticket.

It might take me a couple of weeks before I have time to do the investigation, so a response might take a while.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue status: feedback-reminder We've sent a reminder that we need additional information before we can continue labels Jun 23, 2023
@rstoyanchev
Copy link
Contributor

@mzeijen, no worries at all, take your time!

@mzeijen
Copy link
Author

mzeijen commented Jul 10, 2023

I am closing this issue, because there is no real need for it. I have done extensive testing, and the situations for which we needed a more complicated implementation in our own custom ProblemDetail implementation, are not a problem with this implementation. I think the reason because changes are made in several core Spring components to facilitate ProblemDetail support, that solve the issues we ourselves encountered with our custom implementation.

There are two situations thought which changes in the Spring Framework and Spring Boot would be appreciated, to make it easier for us to switch from our custom implementation to the default implementation:

@mzeijen mzeijen closed this as completed Jul 10, 2023
@sbrannen sbrannen closed this as not planned Won't fix, can't repro, duplicate, stale Jul 10, 2023
@sbrannen sbrannen added status: invalid An issue that we don't feel is valid and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Jul 10, 2023
@bclozel bclozel added status: superseded An issue that has been superseded by another and removed status: invalid An issue that we don't feel is valid labels Jul 10, 2023
@rstoyanchev
Copy link
Contributor

Thanks for the feedback @mzeijen. The 404 handling was addressed for 6.1 with #29491 and is available in M2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: superseded An issue that has been superseded by another
Projects
None yet
Development

No branches or pull requests

5 participants