Using Spring Boot 3.0.2, combining Problem Details (e.g., spring.mvc.problemdetails.enabled=true) with proxying (e.g., due to AOP) doesn't work since ProblemDetailsExceptionHandler classes (duplicated in both servlet and reactive) are final.

Use case

I want to perform certain operations (e.g., logging) on the Problem Details returned. For this purpose, I am intercepting ExceptionHandler return values using AspectJ.

Reproduction

Consider the following example trying to intercept ExceptionHandlers to log ErrorResponse details:

import org.aspectj.lang.JoinPoint;
import org.aspectj.lang.annotation.AfterReturning;
import org.aspectj.lang.annotation.Aspect;
import org.junit.jupiter.api.Test;
import org.springframework.boot.test.autoconfigure.web.servlet.WebMvcTest;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.EnableAspectJAutoProxy;
import org.springframework.http.ResponseEntity;
import org.springframework.lang.Nullable;
import org.springframework.web.ErrorResponse;

@EnableAspectJAutoProxy
@WebMvcTest(value = ProblemTest.TestConfig.class, properties = "spring.mvc.problemdetails.enabled=true")
class ProblemTest {

    @Test
    void contextLoads() {}

    @Configuration
    static class TestConfig {

        @Bean
        ProblemLoggingAspect problemLoggingAspect() {
            return new ProblemLoggingAspect();
        }

    }

    @Aspect
    static class ProblemLoggingAspect {

        @AfterReturning(
                pointcut = "@annotation(org.springframework.web.bind.annotation.ExceptionHandler)",
                returning = "returnValue")
        public void exceptionHandlerIntercept(JoinPoint joinPoint, Object returnValue) {
            ErrorResponse errorResponse = errorResponse(joinPoint, returnValue);
            if (errorResponse != null) {
                System.out.format("problem detail: %s%n", errorResponse.getBody().getDetail());
            }
        }

        @Nullable
        private ErrorResponse errorResponse(JoinPoint ignored, Object returnValue) {
            if (returnValue instanceof ErrorResponse errorResponse) {
                return errorResponse;
            } else if (returnValue instanceof ResponseEntity<?> responseEntity &&
                    responseEntity.getBody() instanceof ErrorResponse errorResponse) {
                return errorResponse;
            }
            return null;
        }

    }

}

Running this test results in the following failure:

...
org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'problemDetailsExceptionHandler' defined in class path resource [org/springframework/boot/autoconfigure/web/servlet/WebMvcAutoConfiguration$ProblemDetailsErrorHandlingConfiguration.class]: Could not generate CGLIB subclass of class org.springframework.boot.autoconfigure.web.servlet.ProblemDetailsExceptionHandler: Common causes of this problem include using a final class or a non-visible class
    ...
Caused by: org.springframework.aop.framework.AopConfigException: Could not generate CGLIB subclass of class org.springframework.boot.autoconfigure.web.servlet.ProblemDetailsExceptionHandler: Common causes of this problem include using a final class or a non-visible class
    ...
    at org.springframework.aop.framework.autoproxy.AbstractAutoProxyCreator.createProxy(AbstractAutoProxyCreator.java:464) ~[spring-aop-6.0.4.jar:6.0.4]
    at org.springframework.aop.framework.autoproxy.AbstractAutoProxyCreator.wrapIfNecessary(AbstractAutoProxyCreator.java:369) ~[spring-aop-6.0.4.jar:6.0.4]
    ...
Caused by: java.lang.IllegalArgumentException: Cannot subclass final class org.springframework.boot.autoconfigure.web.servlet.ProblemDetailsExceptionHandler
    at org.springframework.cglib.proxy.Enhancer.generateClass(Enhancer.java:653) ~[spring-core-6.0.4.jar:6.0.4]
    at org.springframework.cglib.core.DefaultGeneratorStrategy.generate(DefaultGeneratorStrategy.java:26) ~[spring-core-6.0.4.jar:6.0.4]
    ...

Notes

ProblemTest succeeds against Spring Boot 2.x and, when spring.mvc.problemdetails.enabled=false, 3.x. It only fails on 3.x when spring.mvc.problemdetails.enabled=true.

Comment From: sbrannen

combining Problem Details (e.g., spring.mvc.problemdetails.enabled=true) with proxying (e.g., due to AOP) doesn't work since ProblemDetailsExceptionHandler classes (duplicated in both servlet and reactive) are final.

That appears to be only the first hurdle. The respective handleException() methods are also final. So even if the ProblemDetailsExceptionHandler classes in Spring Boot were not final, Spring still could not create a CGLIB-based proxy that intercepts the handleException() methods in org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler and org.springframework.web.reactive.result.method.annotation.ResponseEntityExceptionHandler.

Thus, it seems that you will need to investigate a different approach.

Comment From: vy

@sbrannen, we weren't happy with AOP either, though it was working. Given your suggestion to look elsewhere, now we are out of options. We simply want to capture (and process, log, etc.) all ErrorResponses returned, what else can we do implement this functionality?

Comment From: sbrannen

we weren't happy with AOP either, though it was working.

Are you saying this is a regression and that the AspectJ approach previously worked for you?

Given your suggestion to look elsewhere, now we are out of options. We simply want to capture (and process, log, etc.) all ErrorResponses returned, what else can we do implement this functionality?

I will defer to @rstoyanchev to provide guidance there.

Comment From: vy

Are you saying this is a regression and that the AspectJ approach previously worked for you?

@sbrannen, yes. ProblemTest started failing with spring.mvc.problemdetails.enabled=true introduced in Spring Boot 3. Using AOP to intercept ExceptionHandler return values works in Spring Boot 2.x and, when spring.mvc.problemdetails.enabled=false, in 3.x. Hence, yes, it seems like a regression to me.

Comment From: sbrannen

Using AOP to intercept ExceptionHandler return values works in Spring Boot 2.x

Are you talking about intercepting @ExceptionHandler methods on your own non-final classes (intercepting non-final methods)?

Or are you talking about intercepting the @ExceptionHandler methods in subclasses of the two ResponseEntityExceptionHandler classes?

Comment From: vy

@AfterReturning(pointcut = "@annotation(org.springframework.web.bind.annotation.ExceptionHandler)", returning = "returnValue") is working against all ExceptionHandler beans (including the ones introduced by both Spring Framework 6, Spring Boot 3, and our custom ones), except when spring.mvc.problemdetails.enabled=true.

ResponseEntityExceptionHandler and ProblemDetailsExceptionHandler et al. are introduced with Spring Framework 6 and Spring Boot 3. These are the first occasions of non-final classes and methods breaking the AOP. I have the impression that removing final modifier from these classes and methods will solve the problem and comply with the old contract that all Spring-provided ExceptionHandler beans are proxy-able.

Comment From: sbrannen

These are the first occasions of non-final classes and methods breaking the AOP.

OK. Now I understand what you mean by a "regression".

It's not a technical regression in Spring Framework or Spring Boot's existing code per se, but... it is a "regression" in the sense that not all @ExceptionHandler methods published by Spring are capable of being proxied.

I have the impression that removing final modifier from these classes and methods will solve the problem and comply with the old contract that all Spring-provided ExceptionHandler beans are proxy-able.

Yes, that would likely suffice.

Comment From: rstoyanchev

Thanks for the report @vy.

You can check if removing final helps by setting spring.mvc.problemdetails.enabled=false and creating a class like ProblemDetailsExceptionHandler but without final.

I also expect that should work, in which case this issue does belong in Spring Boot after all.

Comment From: sbrannen

@rstoyanchev, please see my comments in https://github.com/spring-projects/spring-boot/issues/34426.

The handleException() methods in Spring Framwework's ResponseEntityExceptionHandler classes are final, which would also prevent CGLIB from being able to intercept those methods in a dynamic subclass.

Comment From: rstoyanchev

@sbrannen, that's certainly true. That is a template method, it's meant to be final and always has been. Changing it would open the class for extension where it's not meant to be extended. In that sense I still don't see anything we can do in Spring Framework.

By contrast I don't see a reason why the Boot classes can't be opened for extension, with this use case in mind.

Alternatively, as subclasses of ResponseEntityExceptionHandler are expected to override the protected method handleExceptionInternal for a single point of handling, the same should also be possible to intercept using an advice. I don't know if this is the exact syntax or not, but in any case it should be possible:

@AfterReturning(
    pointcut = "execution(protected * org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler.handleExceptionInternal(..)))",
    returning = "returnValue")

Comment From: sbrannen

@sbrannen, that's certainly true. That is a template method, it's meant to be final and always has been. Changing it would open the class for extension where it's not meant to be extended. In that sense I still don't see anything we can do in Spring Framework.

Right. I agree with you on those points.

By contrast I don't see a reason why the Boot classes can't be opened for extension, with this use case in mind.

I agree with that, too. Let's see what @bclozel has to say, since he authored those classes in Boot.

Alternatively, as subclasses of ResponseEntityExceptionHandler are expected to override the protected method handleExceptionInternal for a single point of handling, the same should also be possible to intercept using an advice. I don't know if this is the exact syntax or not, but in any case it should be possible:

java @AfterReturning( pointcut = "execution(protected * org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler.handleExceptionInternal(..)))", returning = "returnValue")

Yes, indeed, one could target other methods in the ResponseEntityExceptionHandler classes for the pointcuts.

However, if @vy wants to continue to apply his existing advice to @ExceptionHandler methods in other classes (that don't suffer from issues with final), he would need to put in a bit more work to get the pointcuts applied appropriately. Specifically, he'd need to introduce some shared @Pointcut methods to match against the two ResponseEntityExceptionHandler classes, and then he'd need to combine various pointcuts with boolean logic -- something to the effect of pointcut = "exceptionHandlerMethod() && !inResponseEntityExceptionHandler()" to continue to use his existing logic.

In summary, if Boot removes the final declarations on those two classes, @vy should be able to get his advice working but not without changing his pointcuts.

Comment From: bclozel

By contrast I don't see a reason why the Boot classes can't be opened for extension, with this use case in mind.

The main goal here is to enable ResponseEntityExceptionHandler support; Spring Boot doesn't intend to create new public contracts or extensions for this. The class is already package private so I guess we can remove the final keyword there. Note this won't prevent application developers from doing that exactly with custom handlers.

we weren't happy with AOP either, though it was working. Given your suggestion to look elsewhere, now we are out of options. We simply want to capture (and process, log, etc.) all ErrorResponses returned, what else can we do implement this functionality?

I don't know if there is a better way to achieve this. I'm happy to move the issue again to Spring Boot if this change looks good to you @vy.

Comment From: vy

@rstoyanchev, @sbrannen, @bclozel, thanks so much for the prompt responses. I will locally try out removing final modifiers and updating the aspect point-cut, and share the outcome here. Please allow me some time for this check.

Comment From: vy

I have confirmed that ProblemTest succeeds when final modifier is removed from ProblemDetailsExceptionHandler classes. Though the pointcut indeed misses the ResponseEntityExceptionHandler#handleException(). I can extend the pointcut to cover handleExceptionInternal() too, but that makes the approach pretty brittle:

Before

@AfterReturning(
        pointcut = "@annotation(org.springframework.web.bind.annotation.ExceptionHandler)",
        returning = "returnValue")

After

@AfterReturning(
        pointcut = "@annotation(org.springframework.web.bind.annotation.ExceptionHandler) || execution(protected * org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler.handleExceptionInternal(..)))",
        returning = "returnValue")

Before the intent was clear and compact. Now it appears like there are holes, we are covering them, and crossing our fingers that our coverage is still complete. Wouldn't it be possible to remove the final modifier from the handleException() method too?

Comment From: rstoyanchev

I'm afraid not. The class is a template class, with the parts of the template open for extension exposed as protected methods. It's just part of the design. The pointcut is more involved, but a test can be written to ensure it works as expected.

Comment From: vy

If handleException() cannot be opened, there is not much to be discussed. I think this issue can be moved back to Spring Boot and resolved by removing final modifier from ProblemDetailsExceptionHandler classes.

Comment From: philwebb

Closing in favor of PR #34503