Affects: Framework 6.1.10 (most likely also 6.1.9)

Disclaimer: I have a hard time digging into this issue, because most of the concepts here are new to me. I might use the terminology wrong or be way off with my assessment.

We experienced a change with Framework 6.1.8 where AspectJ related Mockito tests started failing, most likely related to https://github.com/spring-projects/spring-framework/issues/32970 However, instead of being executed twice there were now 3 calls for one method. Besides the actual delegate and the cglib proxy (as described in the issue) there was another call from a Mockito mock instance with a cglib proxy - at least I think that is what the debugger indicates: a call from MyService$MockitoMock$<somerandomstuff>$$SpringCGLIB$$0

Since we only manage the spring-boot version directly, we tested the backported fix (I think it is this one: https://github.com/spring-projects/spring-framework/commit/628b0504ec64b8ac7412fc71097c490a0ca653ac) using framework version 6.1.10 (spring-boot update to 3.2.7). The tests still fail because there are still two calls. The CTW cglib proxy call has been fixed, but there is still the Mockito mock related one. At least that is what it looks like to me.

Comment From: jhoeller

Thanks for raising this, I can imagine a side effect in a Mockito scenario indeed.

Am I understanding correctly that you got an AspectJ aspect configured as a bean and applied with compile-time weaving, and the Spring AOP auto-proxying accidentally picked it up for building a CGLIB proxy - and that latter part got fixed by #32970?

So what remains is an accidental extra call for a Mockito mock that's set up as a Spring bean as well? Is there still a CGLIB proxy being built for that Mockito mock where you do not intend to have a CGLIB proxy as all?

I assume that Mockito mock is of the same type as your regular target bean and therefore matched by auto-proxying as well, and not excluded by #32970 either since the Mockito mock class itself is not compiled by ajc.

What is your test actually trying to assert there? Is it expecting any interactions between the aspect and the Mockito mock at all?

Comment From: jhoeller

Is your Mockito mock effectively extending the class that the aspect actually targets? Is its superclass compiled by ajc and we are not detecting that yet? Any other indication why it is not passing the compiledByAjc(targetClass) check in AspectJExpressionPointcut:286 which is meant to find out about pre-weaved classes that we need to skip for proxying?

Comment From: jhoeller

Trying various Mockito scenarios, your MyService$MockitoMock class name above clearly indicates a subclass generated by Mockito (in case of extra interfaces or extra settings - otherwise Mockito just transforms the existing class), with the original MyService class as its superclass. Assuming this is registered as a bean and that the original MyService class is compile-time weaved, it looks we should simply check for ajc markers in the superclass as well in such a scenario.

I've tried that with a patch in AspectJExpressionPointcut.compiledByAjc where it recurses through the given class hierarchy now - which seems to work fine for me so far. @andrethiel1und1 if this seems to match your scenario, I'll commit the patch so that you could try a 6.1.11 snapshot with it. If there is anything else to consider for your scenario, let me know.

Comment From: andrethiel1und1

Thanks for raising this, I can imagine a side effect in a Mockito scenario indeed.

Am I understanding correctly that you got an AspectJ aspect configured as a bean and applied with compile-time weaving, and the Spring AOP auto-proxying accidentally picked it up for building a CGLIB proxy - and that latter part got fixed by #32970?

The project configuration is all over the place, so I have a hard time evaluating this. I might outline what I find in more detail later. An a quick initial response, I can outline how the tests work.

We definitely have compile time weaving set up in the project. That much I know.

What is your test actually trying to assert there? Is it expecting any interactions between the aspect and the Mockito mock at all?

There is a LoggingAspect where I set a breakpoint in the log Method to see where it is called from in the stack:

@Aspect
@Slf4j
public class LoggingAspect {

    ...

    private String log(...) {
        return myService.log(le);
    }

    ...

This is a rough outline of a test that fails:

        InOrder inOrder = inOrder(myServiceMock, myAdminServiceMock);
        myAdminService.cancelSomething(getInfo(), SOME_ID);
        inOrder.verify(myServiceMock).log(isCancellation(...));

Here the inOrder.verify() call indirectly expects only one call to log and with 6.1.8 there were 3 calls to log instead of one, now there are 2 calls:

 // I think the Delegate classes are Camunda Steps.

 3.2.6 (6.1.8)
 MyAdminServiceAspectDelegate$$SpringCGLIB$$0 -> cancelSomething
 MyAdminServiceAspectDelegate -> cancelSomething
 MyAdminService$MockitoMock$w7AKM2b0$$SpringCGLIB$$0 -> cancelSomething

 3.2.7 (6.1.10)
 MyAdminServiceAspectDelegate -> cancelSomething
 MyAdminService$MockitoMock$xYX7lgEd$$SpringCGLIB$$0 -> cancelSomething

I think up to 6.1.8 there was only one call from MyAdminServiceAspectDelegate, none from a Mockito mock.

So I think what you are writing is basically correct!?

So what remains is an accidental extra call for a Mockito mock that's set up as a Spring bean as well? Is there still a CGLIB proxy being built for that Mockito mock where you do not intend to have a CGLIB proxy as all?

I think this is the case and debugging confirmed it - not sure if I understand the nuances here, though.

I assume that Mockito mock is of the same type as your regular target bean and therefore matched by auto-proxying as well, and not excluded by #32970 either since the Mockito mock class itself is not compiled by ajc.

This is where I have a hard time wrapping my head around the configuration. I'm still looking for the code that defines how the Mockito mock instance is defined. But that seems to make sense. If LTW for example has changed in 6.1.8 and is not covered by the fix, something like that might explain things.

Comment From: andrethiel1und1

Trying various Mockito scenarios, your MyService$MockitoMock class name above clearly indicates a subclass generated by Mockito (in case of extra interfaces or extra settings - otherwise Mockito just transforms the existing class), with the original MyService class as its superclass. Assuming this is registered as a bean and that the original MyService class is compile-time weaved, it looks we should simply check for ajc markers in the superclass as well in such a scenario.

I've tried that with a patch in AspectJExpressionPointcut.compiledByAjc where it recurses through the given class hierarchy now - which seems to work fine for me so far. @andrethiel1und1 if this seems to match your scenario, I'll commit the patch so that you could try a 6.1.11 snapshot with it. If there is anything else to consider for your scenario, let me know.

Purely intuitively speaking I think checking for ajc markers in the superclass is worth a shot and nothing else comes to mind immediately that needs to be taken into consideration. It is worth a shot, I think.

Comment From: jhoeller

Alright, thanks for the feedback! I've pushed the superclass check to 6.1.x, this will be available in the upcoming 6.1.11-SNAPSHOT from https://repo.spring.io/snapshot in a couple of minutes. Please give it a try and let me know whether it makes any difference...

Comment From: andrethiel1und1

Alright, thanks for the feedback! I've pushed the superclass check to 6.1.x, this will be available in the upcoming 6.1.11-SNAPSHOT from https://repo.spring.io/snapshot in a couple of minutes. Please give it a try and let me know whether it makes any difference...

Thank you for looking into this! I have tested it, but it didn't seem to make a difference.

I imported the 6.1.11-SNAPSHOT version of the spring-framework-bom to override the spring-boot 3.2.7 bom versions and checked the Maven tree to verify that the expected spring libraries have been resolved properly. Basically I see the same behavior as before.

Comment From: jhoeller

Hmm, could you try to step through AspectJExpressionPointcut.matches(Class) line 286 and into the compiledByAjc method from there - when it is being called with the MyService$MockitoMock$... class? For some reason, compiledByAjc seems to return false there when it should really identify it as pre-weaved (now even with a superclass check).

Another possible explanation is that your Mockito mock is somehow running with the pre-compiled AspectJ aspect in the application context but not with compile-time weaving applied to the corresponding application classes. We would have simply ignored the ajc-compiled aspect in that case before, whereas we are actually considering the aspect for proxying now if the target classes are not weaved. Your test setup might have accidentally relied on this before. In order to arrive at consistent behavior in that case, you'd have to run against the ajc-compiled application classes even in such a test scenario - or to remove the aspect bean from your test application context if it is not meant to be used at all.

Comment From: andrethiel1und1

Hmm, could you try to step through AspectJExpressionPointcut.matches(Class) line 286 and into the compiledByAjc method from there - when it is being called with the MyService$MockitoMock$... class? For some reason, compiledByAjc seems to return false there when it should really identify it as pre-weaved (now even with a superclass check).

Another possible explanation is that your Mockito mock is somehow running with the pre-compiled AspectJ aspect in the application context but not with compile-time weaving applied to the corresponding application classes. We would have simply ignored the ajc-compiled aspect in that case before, whereas we are actually considering the aspect for proxying now if the target classes are not weaved. Your test setup might have accidentally relied on this before. In order to arrive at consistent behavior in that case, you'd have to run against the ajc-compiled application classes even in such a test scenario - or to remove the aspect bean from your test application context if it is not meant to be used at all.

I tried to step through using the debugger. I added a breakpoint condition at 286: targetClass.toString().contains("AdminService$MockitoMock")

There are no Fields that start with ajc$ and the parent is simply java.lang.Object. The field this.aspectCompiledByAjc is set to true. I suppose that is what we would see in the "another explanation" case?

I might have to look into the suggested changes with a colleague next week. With my current understanding of the configuration I wouldn't know where to start changing things ^^. Thanks you for the pointers and the time you put into this.

Comment From: jhoeller

That makes sense, AdminService is an interface then? The same Mockito class naming pattern applies there as well. In that case, the base class would indeed be java.lang.Object. So I suppose this interface-based mock object completely replaces the regular AdminService implementation in your system, with only that regular implementation class being ajc-compiled. As a consequence, the AdminService mock is not weaved in your test setup, and the aspect is therefore applied via proxying.

Now I'm wondering why the aspect is present in your test configuration to begin with... when it is effectively not expected to apply to the mocked AdminService. Maybe it is there for some other non-mocked service class that it is expected to apply to, possibly even a separate instance of your regular AdminService implementation class? If so, the easiest way out of this would be to adapt the test assertions, expecting the aspect to apply to the mocked AdminService as well. Or otherwise, if possible, to completely remove the aspect bean from the test configuration so that it does not apply to any variant of your test services.

Comment From: jhoeller

At this point, the superclass check seems worth fixing and backporting in any case since it covers superclass-derived mocks where the superclass has the aspect behavior weaved in already.

For scenarios where the target bean does not have the aspect behavior yet, it arguably is correct to apply it via CGLIB then. The general goal is to consistently apply aspects but prevent double execution of aspects against the same target bean, and as far as I understand the scenario above, that is the case here. The test assertions should expect the aspect behavior to be applied to the Mockito mock as well: for every matching target object, but just once per target object.

Comment From: jhoeller

Closing this issue based on the reasoning above, to be reopened if any further steps on our side are being identified before the release next week.