ResponseStatusException thrown from exception handler methods is no longer respected about spring-framework HOT 5 CLOSED

Before the change introduced in #32359, the following would happen:

1. the exception thrown by the controller would be handled first by the DispatcherHandler by calling the exception handler here.
2. the @ExceptionHandler rethrows another exception
3. 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.

from spring-framework.

@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!

from spring-framework.

Also see spring-projects/spring-boot#40148 (comment) for another report of this.

from spring-framework.

5f601ce handle exceptions from @ExceptionHandler regardless of whether
thrown immediately or via Publisher.

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 5f601ce 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!

from spring-framework.

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: spring-projects/spring-boot#40148 (comment)

I am closing my ticket.
And thank you again!

from spring-framework.