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

Bridge methods should be ignored for pointcut matching #255

Merged
merged 2 commits into from
Aug 21, 2023

Conversation

kriegaex
Copy link
Contributor

@kriegaex kriegaex commented Aug 20, 2023

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
Copy link
Contributor Author

kriegaex commented Aug 20, 2023

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 kriegaex requested a review from aclement August 21, 2023 10:27
@kriegaex kriegaex self-assigned this Aug 21, 2023
@kriegaex kriegaex added the bug Something isn't working label Aug 21, 2023
@kriegaex kriegaex added this to the 1.9.20.1 milestone Aug 21, 2023
Relates to spring-projects/spring-framework#27761.

The test needs an ASM-generated class file with reordered methods in
order to reproduce the issue in plain AspectJ. The test fails now, but
should pass after the fix.

Signed-off-by: Alexander Kriegisch <[email protected]>
@kriegaex
Copy link
Contributor Author

I finally managed to create a regression test for this issue with some ASM magic. See 65f3e2e 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.

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

Bridge methods are now ignored in favour of their overriding namesakes
during method matching.

Signed-off-by: Alexander Kriegisch <[email protected]>
@kriegaex kriegaex changed the title Experimental fix for spring-projects/spring-framework#27761 Bridge methods should be ignored for pointcut matching Aug 21, 2023
@kriegaex
Copy link
Contributor Author

kriegaex commented Aug 21, 2023

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.

@kriegaex kriegaex merged commit d2c3570 into master Aug 21, 2023
@kriegaex kriegaex deleted the spring-gh-27761 branch August 21, 2023 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bridge methods should be ignored for pointcut matching Spring AOP randomly fails when advising JpaRepository
1 participant