Hi, After upgrading from spring boot 3.2.3 (spring framework 6.1.4) to 3.2.4 (spring framework 6.1.5) I noticed that the status code for some validated rest endpoints changed to status code 500 - Internal Server Error when validation errors occur.
For example prior to spring boot 3.2.4 with below exception handler I would get status code 400 but in 3.2.4 it returns 500.
I suspect that it could be due to this change: https://github.com/spring-projects/spring-framework/commit/5f601ceb4585fbb2e868ac9a6c50789b0e8b8263
@RestControllerAdvice
internal class RestControllerExceptionHandler {
@ExceptionHandler(ConstraintViolationException::class)
fun exceptionHandler(e: ConstraintViolationException) {
throw ResponseStatusException(HttpStatus.BAD_REQUEST, e.message, e)
}
}
Comment From: injae-kim
https://github.com/spring-projects/spring-framework/commit/5f601ceb4585fbb2e868ac9a6c50789b0e8b8263 handle exceptions from @ExceptionHandler regardless of whether thrown immediately or via Publisher.
https://github.com/spring-projects/spring-framework/blob/186e70cd0490c1141dd936aac5bace6c0f9d6dc5/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerAdapter.java#L321
private static Mono<HandlerResult> handleExceptionHandlerFailure(
ServerWebExchange exchange, Throwable exception, Throwable invocationEx,
ArrayList<Throwable> exceptions, InvocableHandlerMethod invocable) {
if (disconnectedClientHelper.checkAndLogClientDisconnectedException(invocationEx)) {
return Mono.empty();
}
// Any other than the original exception (or a cause) is unintended here,
// probably an accident (e.g. failed assertion or the like).
if (!exceptions.contains(invocationEx) && logger.isWarnEnabled()) {
// ☝️☝️ if exception is 500 and invocationEx from ExceptionHandler is 400, intend to respect 500 here?
logger.warn(exchange.getLogPrefix() + "Failure in @ExceptionHandler " + invocable, invocationEx);
}
return Mono.error(exception); // ignore invocationEx and return origin exception!
}
Hmm maybe it's intended behavior from this https://github.com/spring-projects/spring-framework/commit/5f601ceb4585fbb2e868ac9a6c50789b0e8b8263 commit?
// Maybe we can fix it like this?
if (invocationEx != null && invocationEx instanceof ResponseStatusException) {
return Mono.error(invocationEx);
}
return Mono.error(exception);
Maybe we can fix like above code, let's wait maintainer's opinion~! If my suggestion makes sense, I'm ready to open fix PR :) thank you!
Comment From: bclozel
Also see https://github.com/spring-projects/spring-boot/issues/40148#issue-2220129300 for another report of this.
Comment From: bclozel
Before the change introduced in #32359, the following would happen:
- the exception thrown by the controller would be handled first by the
DispatcherHandler
by calling the exception handler here. - the
@ExceptionHandler
rethrows another exception - then the
RequestMappingHandlerAdapter
would handle this rethrown exception
I think this behavior is invalid and unintended for the following reasons:
- the same application in Spring MVC does not behave like this, rethrown exceptions are not processed by other handlers
- this behavior would only work once - rethrowing the exception another time would not work
- this has been the intended and documented behavior for
@ExceptionHandler
methods for a very long time, see https://docs.spring.io/spring-framework/reference/web/webmvc/mvc-controller/ann-exceptionhandler.html
Still, we can consider rolling back partially this change as this is disrupting applications in a maintenance release. But I do think that keeping this change around for Spring Framework 6.2 is a good choice.
Comment From: bclozel
@mbanaszkiewicz-vonage @beytularedzheb Thanks for raising this issue. I discussed this with @rstoyanchev and we confirmed that the original behavior was invalid in the first place and that exception handlers are supposed to fully handle the exception or re-throw the original exception.
I initially marked this as a bug for 6.1.x as I thought the the handling of exceptions in different phases was maybe a mistake, but it turns out this is by design in WebFlux and that we may consolidate things in the future.
In the meantime, can you let us know whether returning a ResponseEntity
would work for your application? We would like to know why you chose this setup in the first place. Thanks!
Comment From: beytularedzheb
Hi @bclozel, Thank you for checking it. It was just an easy & quick way of changing the status code and getting standard error response with already populated fields, like path, request id, etc. (DefaultErrorAttributes). Actually, as you mentioned ResponseEntity, I realized that I can still use ResponseStatusException and achieve the same effect. Instead of throwing an exception directly in the exception handler method, I can return it in a Mono:
@RestControllerAdvice
internal class RestControllerExceptionHandler {
@ExceptionHandler(ConstraintViolationException::class)
fun exceptionHandler(e: ConstraintViolationException): Mono<Throwable> {
return Mono.error(ResponseStatusException(HttpStatus.BAD_REQUEST, e.message, e))
}
}
This should also work for the case you shared above: https://github.com/spring-projects/spring-boot/issues/40148#issue-2220129300
I am closing my ticket. And thank you again!