Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle ResponseStatusException thrown by MVC functional endpoints #32689

Closed
DanielLiu1123 opened this issue Apr 22, 2024 · 6 comments
Closed
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@DanielLiu1123
Copy link

I am customizing HandlerFunction:

For web MVC:

public class ServletHandlerFunction implements HandlerFunction<ServerResponse> {
    @Override
    public ServerResponse handle(ServerRequest request) throws Exception {
        throw new ResponseStatusException(HttpStatus.NOT_IMPLEMENTED, "Not implemented yet!");
    }
}

For web Flux:

public class ReactiveHandlerFunction implements HandlerFunction<ServerResponse> {
    @Override
    public Mono<ServerResponse> handle(ServerRequest request) {
        return Mono.error(new ResponseStatusException(HttpStatus.NOT_IMPLEMENTED, "Not implemented yet!"));
    }
}

I expect that the ResponseStatusException should be handled by ResponseEntityExceptionHandler (when setting spring.{mvc,webflux}.problemdetails.enabled to true), but the result is that the exception in ReactiveHandlerFunction can be handled, while the one in ServletHandlerFunction cannot. The reason is found here (org.springframework.web.servlet.mvc.method.annotation.ExceptionHandlerExceptionResolver#shouldApplyTo), where exceptions thrown from HandlerFunction are not processed.

This is somewhat frustrating, especially when developing frameworks. I hope that both webmvc and webflux can have the same behavior for exception handling. It would be really helpful if ExceptionHandlerExceptionResolver could handle ResponseStatusException thrown by HandlerFunction.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 22, 2024
@jhoeller jhoeller added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Apr 22, 2024
@DanielLiu1123
Copy link
Author

Any Progress? I really hope to see this problem being fixed in 6.2.0.

@bclozel bclozel self-assigned this Jun 4, 2024
@bclozel
Copy link
Member

bclozel commented Jun 4, 2024

This topic has been worked on in the past already.

With #22619 in 5.3, we have made it possible to handle exceptions from any handler if you configure the mappedHandlerClasses property - as of this enhancement, it is possible for non-annotated controllers to handle exceptions. But this has some limitations, since functional endpoints can be lambdas and making a decision based on the enclosing type is not really possible.

Later with #26772 in 6.1.2, we added a mappedHandler Predicate that you can use to make a decision based on the handler instance - this makes it possible to configure error handling for functional endpoints.

Here, this issue points to the fact that while this is now possible to handle exceptions from a function handler in MVC, it's not consistent with the behavior in WebFlux.

With WebFlux, exceptions raised by functional handlers are processed by the org.springframework.web.reactive.result.method.annotation.ResponseEntityExceptionHandler. This exception handler is meant for @ExceptionHandler annotated classes. As pointed by @poutsma in his comment, maybe we shouldn't use this infrastructure directly for handler functions?

If the handler function is registered as a lambda, the resolver will not be able to find the enclosing type. On the other hand, I'm wondering if declaring the handler function as a type itself would mean that the infrastructure would look for annotated methods on that type directly? That could be indeed strange.

You can opt-in for this behavior quite easily in Spring Boot with the following:

@Bean
WebMvcRegistrations webMvcRegistrations() {
	return new WebMvcRegistrations() {
		@Override
		public ExceptionHandlerExceptionResolver getExceptionHandlerExceptionResolver() {
			ExceptionHandlerExceptionResolver exceptionResolver = new ExceptionHandlerExceptionResolver();
			exceptionResolver.setMappedHandlerPredicate(handler -> handler instanceof HandlerFunction<?>);
			return exceptionResolver;
		}
	};
}

If we want behavior parity with WebFlux, I guess we could update ExceptionHandlerExceptionResolver#shouldApplyTo to apply when handler instanceof HandlerFunction<?>. WDYT @rstoyanchev @poutsma

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jun 4, 2024

Spring MVC and WebFlux evolved in slightly different ways. The ExceptionHandlerExceptionResolver on the WebMvc side has had the mappedHandlerClasses property for a long time, and when the feature to handle exceptions from any handler via @ControllerAdvice was added, it made sense to do it on an opt-in basis through that property. By contrast, WebFlux doesn't have that.

So, both already support use of @ControllerAdvice for exceptions from any handler, including for functional handlers, but for Spring MVC it requires a bit of extra configuration. I think it would be good to enable it out of the box for functional endpoints, which are relatively more recent. The goal of having one way to handle annotated and functional handlers does make sense, especially in the context of the WebMvc config where such exception handling is already configured for use. It's also important because ResponseEntityExceptionHandler is how we provide support for handling internal exception.

@bclozel bclozel added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jun 5, 2024
@bclozel bclozel added this to the 6.2.0-M4 milestone Jun 5, 2024
@bclozel bclozel changed the title Inconsistent Exception Handling Between WebMVC and WebFlux Handle ResponseStatusException thrown by MVC functional endpoints Jun 6, 2024
@bclozel bclozel closed this as completed in 52af43d Jun 6, 2024
@thiagolocatelli
Copy link

thiagolocatelli commented Jun 6, 2024

@bclozel I followed your suggestion and while your registration works, the controller advice stopped handling exceptions for annotated controllers.

The application has both annotated controllers and router functions (non annotated controllers) and without the custom registration @ControllerAdvice doesn't work for non annotated controllers, but it works annotated Controllers. With your registration @ControllerAdvice stopped working for annotated controllers and started working for RF. Any suggestion?

@bclozel
Copy link
Member

bclozel commented Jun 6, 2024

@thiagolocatelli Yes, I believe the following would work:

@Bean
WebMvcRegistrations webMvcRegistrations() {
	return new WebMvcRegistrations() {
		@Override
		public ExceptionHandlerExceptionResolver getExceptionHandlerExceptionResolver() {
			ExceptionHandlerExceptionResolver exceptionResolver = new ExceptionHandlerExceptionResolver();
			exceptionResolver.setMappedHandlerPredicate(handler -> {
				return handler instanceof HandlerFunction<?> || handler instanceof HandlerMethod;
			});
			return exceptionResolver;
		}
	};
}

@bclozel
Copy link
Member

bclozel commented Jun 6, 2024

@thiagolocatelli Right, sorry I mislead you. This feature is indeed built to give you finer control. In your case, chances are returning true all the time will work out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

6 participants