spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.65k stars 38.14k forks source link

Allow for external customization and i8n of the "detail" of an RFC 7807 response #28814

Closed rstoyanchev closed 2 years ago

rstoyanchev commented 2 years ago

After #27052, each Spring MVC / WebFlux exception implements ErrorResponse and exposes ProblemDetail for the response body. By default, the "detail" field is set to a minimal message (and not than the actual Exception message), to avoid leaking implementation details. An application can extend ResponseEntityExceptionHandler and override protected methods customize the details for each exception.

We should consider a way to allow customization of the error "detail" for each Spring MVC and WebFlux exception, as well as for any ErrorResponseException, for example by performing a lookup in a MessageSource with a message code derived from the exception name. That would allow an application to customize and internationalize exception messages through property files. This is already supported for @ResponseStatus exceptions, and we could support the same for ErrorResponse exceptions.

One specific challenge will be the ability to parameterize messages with information that is specific to each exception.

rstoyanchev commented 2 years ago

This is now largely done. Check the updated documentation section on Error Responses section and the sub-section on customization and internationalization.

biergit commented 2 years ago

Hi @rstoyanchev I was wondering how I as a user of Spring should go about providing i18n messages for my custom exceptions. It seems you are proposing to override the ResponseEntityExceptionHandler but I don't see as much support for reusing the default message source lookup mechanism as used for the Spring exceptions as I hoped for. Ideally I would like to provide my problemDetail.com.example.MyException message in my message.properties and write something like

@ControllerAdvice
class MyExceptionHandler extends ResponseEntityExceptionHandler {

  @ExceptionHandler
  public ResponseEntity<Object> handleMyException(MyException ex, NativeWebRequest request) {
    return handleExceptionInternal(ex, BAD_REQUEST, request, "My default message", new Object[] {ex.getParam1(), ex.getParam2()});
  }
}

Instead what I thought I had to do was something like this

@ControllerAdvice
public class MyExceptionHandler extends ResponseEntityExceptionHandler {
    @ExceptionHandler
    ResponseEntity<Object> handleMyException(MyException myException, NativeWebRequest nativeWebRequest) {
        ProblemDetail body = ProblemDetail.forStatusAndDetail(HttpStatus.BAD_REQUEST, myException.getMessage());
        ErrorResponseException mappedException = new ErrorResponseException(HttpStatus.BAD_REQUEST, body, myException,
            ErrorResponse.getDefaultDetailMessageCode(myOtherException.getClass(), null), null);
        return handleExceptionInternal(mappedException, null, null, HttpStatus.BAD_REQUEST,
            nativeWebRequest);
    }
}

This feels quite awkward and in finally getting to that solution I stumbled across an NPE here when I did ErrorResponseException mappedException = new ErrorResponseException(HttpStatus.BAD_REQUEST, !!null!!, myOtherException, ErrorResponse.getDefaultDetailMessageCode(myOtherException.getClass(), null), null); instead.

Does ProblemDetail have to be so weird to use? Why not go for a full builder?

Also the current implementation essentially only supports customizing the message detail via the message source for ErrorResponses. So problemDetail.org.springframework.web.servlet.NoHandlerFoundException=No handler works while problemDetail.org.springframework.beans.TypeMismatchException=Ooops does not. Would you agree that this is not ideal and also not clearly described in the documentation here?

rstoyanchev commented 2 years ago

@biergit, thanks for taking a look and for the feedback.

Generally, you don't need to extend ResponseEntityExceptionHandler. It can be any @ControllerAdvice, and it's easy for such a class to perform a lookup via MessageSource. That said, you're right that ResponseEntityExceptionHandler could make it easier to map custom exceptions.

I've created #29384 to make an improvement, and there is a specific proposal. Please, take a look and let's continue the discussion there.

As for TypeMismatchException, that should work, see here, but it's not actually passing the messageCode, so that's an oversight. I'll fix this that as part of #29384 since the handling for TypeMismatchException will take advantage of the same improvement.

NaitYoussef commented 1 year ago

This is now largely done. Check the updated documentation section on Error Responses section and the sub-section on customization and internationalization.

The link above is broken

bclozel commented 1 year ago

See https://docs.spring.io/spring-framework/reference/web/webmvc/mvc-ann-rest-exceptions.html#mvc-ann-rest-exceptions-i18n