eclipse-aspectj / aspectj

Other
303 stars 86 forks source link

Bridge methods should be ignored for pointcut matching #255

Closed kriegaex closed 1 year ago

kriegaex commented 1 year ago

Locally, it seems to fix the problem in the sample project. But does it make any existing tests fail? Answer: no.

@aclement, this method is not covered by any tests, which is kind of unusual for AspectJ. I guess, it is indirectly tested by dozens of other tests, but I am not confident that my naive approach to simply exclude bridge methods is actually correct, or if it might break real-world use cases with specific pointcuts. Can you please comment on this change? It would make me feel much better to merge with your consent than without it. Thank you.

Fixes #256. Fixes spring-projects/spring-framework#27761.

kriegaex commented 1 year ago

At least, I have established that in addition to

@Around("execution(* com.example.demo.BusinessRelationshipRepository.save*(..))")

or the more specific

@Around("execution(java.util.List com.example.demo.BusinessRelationshipRepository.save*(..))")

also this one still matches when excluding bridge methods:

@Around("execution(java.lang.Iterable org.springframework.data.repository.CrudRepository.save*(..))")

That is kind of encouraging.

kriegaex commented 1 year ago

I finally managed to create a regression test for this issue with some ASM magic. See 65f3e2e4 for details.

The bugfix commit was temporarily dropped, so I could force-push the regression test and see it fail in the CI build once, before recreating the bugfix and subsequently see the same test pass.

kriegaex commented 1 year ago

As expected, the regression test running before the bugfix reproduces the problem:

Error:  Failures: 
Error:    Bugs1920Tests.test_Spring_GitHub_27761:79->XMLBasedAjcTestCase.runTest:171->XMLBasedAjcTestCase.runTest:157->AjcTestCase.assertMessages:479 test "do not match bridge methods" failed'
test "do not match bridge methods" failed
Unexpected warning messages:
    warning at /tmp/ajcSandbox/aspectj/ajcTest4097072875510523219.tmp/RepositoryAspect.aj:6::0 advice defined in RepositoryAspect has not been applied [Xlint:adviceDidNotMatch]

Update: Also as expected, the build containing the fix passed.