From 506fbe5243af79b103ebd33c7b00f4ca20e099e1 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Tue, 1 Nov 2022 11:40:23 +0000 Subject: [PATCH] Improve mapping any Exception to ErrorResponse Add protected, convenience method in ResponseEntityExceptionHandler to create a ProblemDetail for any exception, along with a MessageSource lookup for the "detail" field. Closes gh-29384 --- .../springframework/web/ErrorResponse.java | 49 +++++++++++++ .../ResponseEntityExceptionHandler.java | 49 ++++++++----- .../ResponseEntityExceptionHandlerTests.java | 31 ++++++++- .../ResponseEntityExceptionHandler.java | 69 +++++++++---------- .../ResponseEntityExceptionHandlerTests.java | 25 +++++++ 5 files changed, 171 insertions(+), 52 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/ErrorResponse.java b/spring-web/src/main/java/org/springframework/web/ErrorResponse.java index c484f48f933c..86e66a2adac2 100644 --- a/spring-web/src/main/java/org/springframework/web/ErrorResponse.java +++ b/spring-web/src/main/java/org/springframework/web/ErrorResponse.java @@ -94,6 +94,24 @@ default Object[] getDetailMessageArguments(MessageSource messageSource, Locale l return getDetailMessageArguments(); } + /** + * Resolve the {@link #getDetailMessageCode() detailMessageCode} through the + * given {@link MessageSource}, and if found, update the "detail" field. + * @param messageSource the {@code MessageSource} to use for the lookup + * @param locale the {@code Locale} to use for the lookup + */ + default ProblemDetail updateAndGetBody(@Nullable MessageSource messageSource, Locale locale) { + if (messageSource != null) { + Object[] arguments = getDetailMessageArguments(messageSource, locale); + String detail = messageSource.getMessage(getDetailMessageCode(), arguments, null, locale); + if (detail != null) { + getBody().setDetail(detail); + } + } + return getBody(); + } + + /** * Build a message code for the given exception type, which consists of * {@code "problemDetail."} followed by the full {@link Class#getName() class name}. @@ -105,4 +123,35 @@ static String getDefaultDetailMessageCode(Class exceptionType, @Nullable Stri return "problemDetail." + exceptionType.getName() + (suffix != null ? "." + suffix : ""); } + /** + * Map the given Exception to an {@link ErrorResponse}. + * @param ex the Exception, mostly to derive message codes, if not provided + * @param status the response status to use + * @param headers optional headers to add to the response + * @param defaultDetail default value for the "detail" field + * @param detailMessageCode the code to use to look up the "detail" field + * through a {@code MessageSource}, falling back on + * {@link #getDefaultDetailMessageCode(Class, String)} + * @param detailMessageArguments the arguments to go with the detailMessageCode + * @return the created {@code ErrorResponse} instance + */ + static ErrorResponse createFor( + Exception ex, HttpStatusCode status, @Nullable HttpHeaders headers, + String defaultDetail, @Nullable String detailMessageCode, @Nullable Object[] detailMessageArguments) { + + if (detailMessageCode == null) { + detailMessageCode = ErrorResponse.getDefaultDetailMessageCode(ex.getClass(), null); + } + + ErrorResponseException errorResponse = new ErrorResponseException( + status, ProblemDetail.forStatusAndDetail(status, defaultDetail), null, + detailMessageCode, detailMessageArguments); + + if (headers != null) { + errorResponse.getHeaders().putAll(headers); + } + + return errorResponse; + } + } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ResponseEntityExceptionHandler.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ResponseEntityExceptionHandler.java index cd9e9d6f8fc6..61db9627eb8c 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ResponseEntityExceptionHandler.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/ResponseEntityExceptionHandler.java @@ -292,6 +292,35 @@ protected Mono> handleErrorResponseException( return handleExceptionInternal(ex, null, headers, status, exchange); } + /** + * Convenience method to create a {@link ProblemDetail} for any exception + * that doesn't implement {@link ErrorResponse}, also performing a + * {@link MessageSource} lookup for the "detail" field. + * @param ex the exception being handled + * @param status the status to associate with the exception + * @param defaultDetail default value for the "detail" field + * @param detailMessageCode the code to use to look up the "detail" field + * through a {@code MessageSource}, falling back on + * {@link ErrorResponse#getDefaultDetailMessageCode(Class, String)} + * @param detailMessageArguments the arguments to go with the detailMessageCode + * @return the created {@code ProblemDetail} instance + */ + protected ProblemDetail createProblemDetail( + Exception ex, HttpStatusCode status, @Nullable HttpHeaders headers, + String defaultDetail, @Nullable String detailMessageCode, @Nullable Object[] detailMessageArguments, + ServerWebExchange exchange) { + + ErrorResponse response = ErrorResponse.createFor( + ex, status, headers, defaultDetail, detailMessageCode, detailMessageArguments); + + return response.updateAndGetBody(this.messageSource, getLocale(exchange)); + } + + private static Locale getLocale(ServerWebExchange exchange) { + Locale locale = exchange.getLocaleContext().getLocale(); + return (locale != null ? locale : Locale.getDefault()); + } + /** * Internal handler method that all others in this class delegate to, for * common handling, and for the creation of a {@link ResponseEntity}. @@ -311,7 +340,7 @@ protected Mono> handleErrorResponseException( * @return a {@code Mono} with the {@code ResponseEntity} for the response */ protected Mono> handleExceptionInternal( - Exception ex, @Nullable Object body, HttpHeaders headers, HttpStatusCode status, + Exception ex, @Nullable Object body, @Nullable HttpHeaders headers, HttpStatusCode status, ServerWebExchange exchange) { if (exchange.getResponse().isCommitted()) { @@ -319,25 +348,12 @@ protected Mono> handleExceptionInternal( } if (body == null && ex instanceof ErrorResponse errorResponse) { - body = resolveDetailViaMessageSource(errorResponse, exchange.getLocaleContext().getLocale()); + body = errorResponse.updateAndGetBody(this.messageSource, getLocale(exchange)); } return createResponseEntity(body, headers, status, exchange); } - private ProblemDetail resolveDetailViaMessageSource(ErrorResponse response, @Nullable Locale locale) { - ProblemDetail body = response.getBody(); - if (this.messageSource != null) { - locale = (locale != null ? locale : Locale.getDefault()); - Object[] arguments = response.getDetailMessageArguments(this.messageSource, locale); - String detail = this.messageSource.getMessage(response.getDetailMessageCode(), arguments, null, locale); - if (detail != null) { - body.setDetail(detail); - } - } - return body; - } - /** * Create the {@link ResponseEntity} to use from the given body, headers, * and statusCode. Subclasses can override this method to inspect and possibly @@ -351,7 +367,8 @@ private ProblemDetail resolveDetailViaMessageSource(ErrorResponse response, @Nul * @since 6.0 */ protected Mono> createResponseEntity( - @Nullable Object body, HttpHeaders headers, HttpStatusCode status, ServerWebExchange exchange) { + @Nullable Object body, @Nullable HttpHeaders headers, HttpStatusCode status, + ServerWebExchange exchange) { return Mono.just(new ResponseEntity<>(body, headers, status)); } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ResponseEntityExceptionHandlerTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ResponseEntityExceptionHandlerTests.java index 28b58b5a58f2..d448b45b3830 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ResponseEntityExceptionHandlerTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/ResponseEntityExceptionHandlerTests.java @@ -59,7 +59,7 @@ */ public class ResponseEntityExceptionHandlerTests { - private final ResponseEntityExceptionHandler exceptionHandler = new GlobalExceptionHandler(); + private final GlobalExceptionHandler exceptionHandler = new GlobalExceptionHandler(); private final MockServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get("/").build()); @@ -149,6 +149,29 @@ void errorResponseProblemDetailViaMessageSource() { "Content-Type application/json not supported. Supported: [application/atom+xml, application/xml]"); } + @Test + void customExceptionToProblemDetailViaMessageSource() { + + Locale locale = Locale.UK; + LocaleContextHolder.setLocale(locale); + + StaticMessageSource messageSource = new StaticMessageSource(); + messageSource.addMessage( + "problemDetail." + IllegalStateException.class.getName(), locale, + "Invalid state: {0}"); + + this.exceptionHandler.setMessageSource(messageSource); + + MockServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get("/") + .acceptLanguageAsLocales(locale).build()); + + ResponseEntity responseEntity = + this.exceptionHandler.handleException(new IllegalStateException(), exchange).block(); + + ProblemDetail body = (ProblemDetail) responseEntity.getBody(); + assertThat(body.getDetail()).isEqualTo("Invalid state: A"); + } + @SuppressWarnings("unchecked") private ResponseEntity testException(ErrorResponseException exception) { @@ -247,6 +270,12 @@ protected Mono> handleErrorResponseException( return handleAndSetTypeToExceptionName(ex, headers, status, exchange); } + + public Mono> handleException(IllegalStateException ex, ServerWebExchange exchange) { + HttpStatus status = HttpStatus.INTERNAL_SERVER_ERROR; + ProblemDetail body = createProblemDetail(ex, status, null, ex.getMessage(), null, new Object[] {"A"}, exchange); + return handleExceptionInternal(ex, body, null, status, exchange); + } } } diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandler.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandler.java index a81798b4648a..ca596bb0c6f8 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandler.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandler.java @@ -16,8 +16,6 @@ package org.springframework.web.servlet.mvc.method.annotation; -import java.util.Locale; - import jakarta.servlet.http.HttpServletResponse; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -395,9 +393,8 @@ protected ResponseEntity handleConversionNotSupported( ConversionNotSupportedException ex, HttpHeaders headers, HttpStatusCode status, WebRequest request) { Object[] args = {ex.getPropertyName(), ex.getValue()}; - - ProblemDetail body = resolveDetailViaMessageSource( - status, args, "Failed to convert '" + args[0] + "' with value: '" + args[1] + "'"); + String defaultDetail = "Failed to convert '" + args[0] + "' with value: '" + args[1] + "'"; + ProblemDetail body = createProblemDetail(ex, status, headers, defaultDetail, null, args, request); return handleExceptionInternal(ex, body, headers, status, request); } @@ -420,9 +417,9 @@ protected ResponseEntity handleTypeMismatch( TypeMismatchException ex, HttpHeaders headers, HttpStatusCode status, WebRequest request) { Object[] args = {ex.getPropertyName(), ex.getValue()}; - - ProblemDetail body = resolveDetailViaMessageSource( - status, args, "Failed to convert '" + args[0] + "' with value: '" + args[1] + "'"); + String defaultDetail = "Failed to convert '" + args[0] + "' with value: '" + args[1] + "'"; + String messageCode = ErrorResponse.getDefaultDetailMessageCode(TypeMismatchException.class, null); + ProblemDetail body = createProblemDetail(ex, status, headers, defaultDetail, messageCode, args, request); return handleExceptionInternal(ex, body, headers, status, request); } @@ -444,7 +441,7 @@ protected ResponseEntity handleTypeMismatch( protected ResponseEntity handleHttpMessageNotReadable( HttpMessageNotReadableException ex, HttpHeaders headers, HttpStatusCode status, WebRequest request) { - ProblemDetail body = resolveDetailViaMessageSource(status, null, "Failed to read request"); + ProblemDetail body = createProblemDetail(ex, status, headers, "Failed to read request", null, null, request); return handleExceptionInternal(ex, body, headers, status, request); } @@ -465,7 +462,7 @@ protected ResponseEntity handleHttpMessageNotReadable( protected ResponseEntity handleHttpMessageNotWritable( HttpMessageNotWritableException ex, HttpHeaders headers, HttpStatusCode status, WebRequest request) { - ProblemDetail body = resolveDetailViaMessageSource(status, null, "Failed to write request"); + ProblemDetail body = createProblemDetail(ex, status, headers, "Failed to write request", null, null, request); return handleExceptionInternal(ex, body, headers, status, request); } @@ -492,6 +489,32 @@ protected ResponseEntity handleBindException( return handleExceptionInternal(ex, body, headers, status, request); } + /** + * Convenience method to create a {@link ProblemDetail} for any exception + * that doesn't implement {@link ErrorResponse}, also performing a + * {@link MessageSource} lookup for the "detail" field. + * @param ex the exception being handled + * @param status the status to associate with the exception + * @param defaultDetail default value for the "detail" field + * @param detailMessageCode the code to use to look up the "detail" field + * through a {@code MessageSource}, falling back on + * {@link ErrorResponse#getDefaultDetailMessageCode(Class, String)} + * @param detailMessageArguments the arguments to go with the detailMessageCode + * @param request the current request + * @return the created {@code ProblemDetail} instance + * @since 6.0 + */ + protected ProblemDetail createProblemDetail( + Exception ex, HttpStatusCode status, @Nullable HttpHeaders headers, + String defaultDetail, @Nullable String detailMessageCode, @Nullable Object[] detailMessageArguments, + WebRequest request) { + + ErrorResponse errorResponse = ErrorResponse.createFor( + ex, status, headers, defaultDetail, detailMessageCode, detailMessageArguments); + + return errorResponse.updateAndGetBody(this.messageSource, LocaleContextHolder.getLocale()); + } + /** * Internal handler method that all others in this class delegate to, for * common handling, and for the creation of a {@link ResponseEntity}. @@ -530,36 +553,12 @@ protected ResponseEntity handleExceptionInternal( } if (body == null && ex instanceof ErrorResponse errorResponse) { - body = resolveDetailViaMessageSource(errorResponse); + body = errorResponse.updateAndGetBody(this.messageSource, LocaleContextHolder.getLocale()); } return createResponseEntity(body, headers, statusCode, request); } - // For non-Web exceptions - private ProblemDetail resolveDetailViaMessageSource( - HttpStatusCode status, @Nullable Object[] arguments, String defaultDetail) { - - ProblemDetail body = ProblemDetail.forStatusAndDetail(status, defaultDetail); - ErrorResponseException errorResponseEx = new ErrorResponseException(status, body, null, null, arguments); - body = resolveDetailViaMessageSource(errorResponseEx); - return body; - } - - // For ErrorResponse exceptions - private ProblemDetail resolveDetailViaMessageSource(ErrorResponse response) { - ProblemDetail body = response.getBody(); - if (this.messageSource != null) { - Locale locale = LocaleContextHolder.getLocale(); - Object[] arguments = response.getDetailMessageArguments(this.messageSource, locale); - String detail = this.messageSource.getMessage(response.getDetailMessageCode(), arguments, null, locale); - if (detail != null) { - body.setDetail(detail); - } - } - return body; - } - /** * Create the {@link ResponseEntity} to use from the given body, headers, * and statusCode. Subclasses can override this method to inspect and possibly diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandlerTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandlerTests.java index f4370f86827b..dbdf0445919e 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandlerTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ResponseEntityExceptionHandlerTests.java @@ -16,6 +16,7 @@ package org.springframework.web.servlet.mvc.method.annotation; +import java.beans.PropertyChangeEvent; import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -192,6 +193,30 @@ public void typeMismatch() { testException(new TypeMismatchException("foo", String.class)); } + @Test + public void typeMismatchWithProblemDetailViaMessageSource() { + Locale locale = Locale.UK; + LocaleContextHolder.setLocale(locale); + + try { + StaticMessageSource messageSource = new StaticMessageSource(); + messageSource.addMessage( + "problemDetail." + TypeMismatchException.class.getName(), locale, + "Failed to set {0} to value: {1}"); + + this.exceptionHandler.setMessageSource(messageSource); + + ResponseEntity entity = testException( + new TypeMismatchException(new PropertyChangeEvent(this, "name", "John", "James"), String.class)); + + ProblemDetail body = (ProblemDetail) entity.getBody(); + assertThat(body.getDetail()).isEqualTo("Failed to set name to value: James"); + } + finally { + LocaleContextHolder.resetLocaleContext(); + } + } + @Test @SuppressWarnings("deprecation") public void httpMessageNotReadable() {