Spring ResponseEntityExceptionHandler error handling improvements

I'm implementing a REST API with a ControllerAdvice and planning to use RFC 9457-compliant error responses. I see what Spring Framework 6+ provides.

In Spring 6 documentation I read this: https://docs.spring.io/spring-framework/docs/6.0.0/reference/html/web.html#mvc-ann-rest-exceptions-non-standard

You can also extend ProblemDetail to add dedicated non-standard properties. The copy constructor in ProblemDetail allows a subclass to make it easy to be created from an existing ProblemDetail. This could be done centrally, e.g. from an @ControllerAdvice such as ResponseEntityExceptionHandler that re-creates the ProblemDetail of an exception into a subclass with the additional non-standard fields.

Sounds good: the ResponseEntityExceptionHandler provides this method which might be overridden to create my own extension instances of ProblemDetail from the framework-provided ones:

protected ProblemDetail createProblemDetail(
            Exception ex, HttpStatusCode status, String defaultDetail, @Nullable String detailMessageCode,
            @Nullable Object[] detailMessageArguments, WebRequest request) {
  // ...
}

However this method is invoked "for any exception that doesn't implement ErrorResponse", as confirmed by its Javadoc. When, however, the intercepted exception implements ErrorResponse, the ProblemDetail is built inside the handleExceptionInternal method directly, by using ErrorResponse.updateAndGetBody, with no call to the above method and so no hook available to transform it into my own problem detail extension.

Perhaps some protected method that gets the ProblemDetail, the originating exception, the web request, and so on, and returns another ProblemDetail, invoked in both the above places, could make the job. Default implementation could just return the ProblemDetail instance as-is, while an extension could decorate it and return a suitable ProblemDetail extension.

The second doubt I have is this: why does createProblemDetail receive an Exception and not a Throwable? If my ControllerAdvice extending ResponseEntityExceptionHandler wants to also handle Errors (or a generic Throwable), it can't use it to create a ProblemDetail. This is in contrast with ErrorResponse.builder() method, which does accept a Throwable.

Comment From: rstoyanchev

ResponseEntityExceptionHandler has one @ExceptionHandler that handles built-in Exceptions that may be thrown, and for each exception there is a dedicated protected method. That's one opportunity to enrich the body (ProblemDetail) per exception. For a single place, you can also override handleExceptionInternal, check if the exception is ErrorResponseException and re-create its ProblemDetail with additional properties.

As for createProblemDetail taking Exception, the method helps with built-in, non-web exceptions of our own, and it's sufficient for that purpose. For any other (application) exceptions, using ErrorResponse.Builder directly might be more straight forward, especially if you don't need to support customization via MessageSource.

What do you actually want to add to responses for built-in exceptions?

Comment From: mauromol

What I mean is: if handleExceptionInternal were somehow delegating to createProblemDetail (*), and if the latter were accepting Throwable, we would have a single point of extension were we could inject our logic to further decorate the standard ProblemDetail.

Right now, to have ALL error responses be an extension of ProblemDetail (suppose I want to add a timestamp to it, or any other kind of additional information), I have to add the code to extend the ProblemDetail creation in three different points: - in handleExceptionInternal, because otherwise for Spring exceptions implementing ErrorResponse I would get plain ProblemDetails - in createProblemDetail, so that I can intercept any ProblemDetail creation for built-in exceptions not implementing ErrorResponse, and/or I can then delegate to this method the creation of problem details while implementing my own custom exception handler methods - in a newly added createProblemDetail copy, which supports Throwable instead of Exception, if I want to write a "catch all" handler for any other throwable or if I want to write a handler for some Error, for example

We may discuss whether handling Throwables that are not Exceptions is really meaningful (but since ErrorResponse.builder() does support Throwable, I would say: why not?), but anyway at least the first two points remain valid for sure.

Not a huge problem, but given the elegance of the Spring Framework in many other places where similar problems were brilliantly solved, it surprised me to see such an inconsistency here, especially because the documentation seems to suggest that this is indeed one possible use case that was expected from the beginning.

(*) when I talk about delegation from handleExceptionInternal to createProblemDetail for ErrorResponses implementation, I do not mean that handleExceptionInternal should refrain from using the built-in support given by ErrorResponse interface to create the ProblemDetail, I mean that we could have a single extension point/overridable method were a ProblemDetail (either created by the ErrorResponse or by the createProblemDetail method) could be decorated and possibly extended, so that you just need to override that point and nothing else. So one single extension point vs 2 or 3.

Hope I've explained it better, sorry if I've been confusing.

Comment From: rstoyanchev

I get what you're after, but you're missing some parts.

createProblemDetail is to create a ProblemDetail from scratch for exceptions that don't expose this, so you need to pass detail, detailMessageCode, and detailMessageArguments into it. An ErrorResponse already encapsulates the ProblemDetail and additional details, and so it doesn't make sense to re-create it from scratch.

Let's leave that aside, and focus on what you're after. If you want to add a property from a single place, the easiest thing you can do is:

@ControllerAdvice
public class MyExceptionHandler extends ResponseEntityExceptionHandler {

    @Override
    protected ResponseEntity<Object> createResponseEntity(
            @Nullable Object body, HttpHeaders headers, HttpStatusCode statusCode, 
            WebRequest request) {

        if (body instanceof ProblemDetail problemDetail) {
            problemDetail.setProperty("timestamp", System.currentTimeMillis());
        }

        return super.createResponseEntity(body, headers, statusCode, request);
    }
}

Comment From: spring-projects-issues

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.

Comment From: mauromol

Ok, thanks Rossen, I see it now. I find it a bit counter-intuitive that I have to customize the body creation to customize the ProblemDetail creation, instead of just having a hook where I can enrich or post-process a previously built ProblemDetail, but the createResponseEntity can serve the purpose.

Comment From: rstoyanchev

Indeed this has evolved a bit. Originally, there was no ProblemDetail and no specific format to use, and the class always aimed to be as open for extension as possible. The RFC support made it a little more involved, but it's still quite open with multiple possibilities, which is also part of the issue. Hence my hesitation to add yet another option.

Comment From: rstoyanchev

Closing for now, but thanks for the feedback.