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

Fix @Aspect perthis syntax in reference manual #29998

Closed
wants to merge 1 commit into from

Conversation

kriegaex
Copy link
Contributor

@kriegaex kriegaex commented Feb 20, 2023

In the Spring AOP manual, the sample code falsely reads in both Java and Kotlin examples as follows:

@Aspect("perthis(com.xyz.myapp.CommonPointcuts.businessService())")

But inside perthis must be a valid pointcut, not just a method name, as I explained in this StackOverflow answer.

Thus, the example code should be changed to something like:

@Aspect("perthis(execution(* com.xyz.myapp.CommonPointcuts.businessService()))")

@sbrannen, maybe you are the right person to review and merge this issue.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Feb 20, 2023
@sbrannen sbrannen added type: documentation A documentation task in: core Issues in core modules (aop, beans, core, context, expression) labels Feb 20, 2023
@sbrannen sbrannen changed the title Fix faulty 'perthis' aspect syntax in user manual Fix perthis aspect syntax in user manual Feb 20, 2023
@sbrannen sbrannen self-assigned this Feb 20, 2023
@sbrannen sbrannen added status: invalid An issue that we don't feel is valid and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Feb 20, 2023
@sbrannen sbrannen changed the title Fix perthis aspect syntax in user manual Fix perthis @Aspect syntax in reference manual Feb 20, 2023
@sbrannen sbrannen changed the title Fix perthis @Aspect syntax in reference manual Fix @Aspect perthis syntax in reference manual Feb 20, 2023
sbrannen added a commit that referenced this pull request Feb 20, 2023
This commit modifies PerThisAspect (which is used indirectly in
AspectJAutoProxyCreatorTests.perThisAspect()) so that it uses a shared
@pointcut for both the @aspect and @around declarations, in order to
refute claims made in gh-29998 that the documentation in the reference
manual is incorrect.
@sbrannen
Copy link
Member

sbrannen commented Feb 20, 2023

Hi @kriegaex,

In the Spring AOP manual, the sample code falsely reads in both Java and Kotlin examples as follows:

@Aspect("perthis(com.xyz.myapp.CommonPointcuts.businessService())")

It turns out that the examples are correct as-is. CommonPointcuts defines shared pointcuts as explained earlier in the documentation in the Sharing Common Pointcut Definitions section.

To ensure that this works as expected, I pushed some changes to the tests in commit b437b7b.

However, I can understand the confusion if one jumps to a particular example in the AOP chapter without having read the Sharing Common Pointcut Definitions section. So we may possibly reconsider our use of shared pointcuts in subsequent examples or possibly cross reference the Sharing Common Pointcut Definitions section. See #30003.

In light of that, I am closing this PR.

As a side note, your PR made me realize that our package statements are no longer displayed for those AOP/AspectJ examples and that inspired me to create #30001.

So thanks for bringing that to my attention! 👍

@kriegaex
Copy link
Contributor Author

kriegaex commented Feb 21, 2023

@sbrannen, I am sorry to have missed that. I even think, it happened for the second time within two or so years when trying to help someone on Stack Overflow. Actually, in my own experience it is a rather rare case to refer to pointcuts defined in a class other than the current aspect or one of its parents and requiring a fully qualified pointcut method name, even though perfectly fine as a development practice. Unfortunately, it makes reading the manual section in question misleading. When just looking for how to use this() and target() without also having read the section about sharing pointcuts the very same day, it is utterly non-intuitive, because the reader is lacking context, despite your best efforts to name the shared pointcuts class CommonPointcuts, which did not really register in my mind when reading this manual section in isolation.

I am not an AOP newbie. In fact, I am the current maintainer of AspectJ and still missed that. Chances are that other users will miss it, too. Would you mind me modifying the PR to mention the fact that you are referring to a pointcut defined in another class here, and maybe also provide a brief example of an inline pointcut?


Update: Having followed the links to the newly created issues, I am quite pleased with the fact that you agree that as-is the examples are counter-intuitive and you are planning to do something about it, especially with regard to the extensive use of shared pointcuts in introductory sample code. I really think that inline pointcuts should be used there, or maybe code comments mentioning the fact that you are referring to shared pointcuts would suffice. If you would like me to contribute in order to expedite an update, please let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: invalid An issue that we don't feel is valid type: documentation A documentation task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants