Improve ControlFlowPointcut extensibility

[kriegaex]

I am starting with an issue instead of a PR, because I want to find out if the maintainers are open to this suggestion and discuss how it best be done.

ControlFlowPointcut does not support method name patterns like

• test* (AspectJ-like syntax) or
• test.*, test[1-3] (regex syntax).

While the class is extensible and sports two methods to be overridden for improved filtering and it would be possible to override public boolean matches(Method, Class<?>) or public boolean matches(Method, Class<?>, Object...) in order to add the extra functionality, subclasses do not have access to fields like private Class<?> clazz, private String methodName and private volatile int evaluations. I.e., instead of extending existing functionality users would end up duplicating some things or even copying and extending the whole class like I did when answering StackOverflow #68431056 - please read and feel free to copy the source code.

I see several options here:

• Extend ControlFlowPointcut directly in order to allow for more flexible method name matching, similar to how I did it. Sub-options:
  • Add a new Pattern field and corresponding constructor, like I did, in order to retain exact String matching mode for best performance.
  • Always interpret the existing String parameter as a regex.
• Improve ControlFlowPointcut extensibility by making the fields mentioned above protected instead of private.

After we have come to a common understanding how this should best be done, the implementation as such should be pretty trivial. I can then create a PR, but the maintainers could also quickly do it by themselves, depending on their preference.

[kriegaex]

I would be glad to receive some feedback after 3 months. I am not sure what there is to triage in this case.

[snicoll]

@kriegaex sorry for the time it took to come to this. As I am sure you are aware, Spring Framework is touching a lot of ground and the team is limited. I don't know why this particular request took a lot of time, but there hasn't been a lot of interest by the community either (votes, comments, etc).

ControlFlowPointcut states in its class javadoc that it is a matcher for "simple cflow-style pointcut". I don't know if what you are suggesting to do stays in that category, but I'd rather provide some more extension rather than implementing these ourselves.

[kriegaex]

@snicoll, I am not sure I understand what exactly you are suggesting. If you have seen my code, you know that it would be easy to add more power and flexibility to this pointcut type with almost trivial changes.

If you think, that this makes things too complicated, I am failing to see why. But as maintainer of AspectJ, I can always welcome more Spring AOP users in need of better aspect functionality there, if you prefer.

[sbrannen]

Hi @kriegaex,

Improve ControlFlowPointcut extensibility by making the fields mentioned above protected instead of private.

Let's go with that option.

Do you want to submit a PR?

If not, we can make the necessary changes.

Cheers,

Sam

[kriegaex]

@sbrannen, assuming that I provide PR A to make the fields protected, would you also accept PR B which adds a subclass extending the simple parent class with more powerful pattern matching functionality? It would be backwards compatible, leaving the original untouched, but also enable Spring AOP users to utilise a more powerful solution out of the box, not everyone having to implement it by themselves. I am assuming, that pattern matching is what most users are looking for, expecting something like it, because other pointcut designators also support it.

What would it take for you to accept that into Spring Core? Or asking the other way around, what would be the problem in accepting it? The prospect to have one more class to maintain? To have to document it? Test coverage? Maybe we can address your concerns for the benefit of the Spring users community.

Just to be clear here: I personally would not benefit from it. I am not even an active Spring user. I am a native AspectJ guy. I simply mean to help to improve Spring AOP, raising the threshold for users for having to switch to full-blown AspectJ just because they need a control flow pointcut with a pattern.

[sbrannen]

Hi @kriegaex,

@jhoeller and I took a closer look at our options and decided to focus this issue on the basic extensibility needs in ControlFlowPointcut.

In addition, we've created #31435 to provide a built-in mechanism for pattern matching which simultaneously allows for extension -- for example, to use regular expressions instead of PatternMatchUtils.simpleMatch(...).

I'll take care of both of these issues.

In light of that, there's no need for a PR.

Cheers,

Sam

[kriegaex]

Thanks, @sbrannen and @jhoeller. I have subscribed to the other issue, too. Looking forward to seeing this implemented.

[sbrannen]

In addition, we've created #31435 to provide a built-in mechanism for pattern matching which simultaneously allows for extension -- for example, to use regular expressions instead of PatternMatchUtils.simpleMatch(...).

For anyone interested, this is now quite easy to achieve, as can be seen in the RegExControlFlowPointcut I implemented as part of that closing commit.

spring-framework/spring-aop/src/test/java/org/springframework/aop/support/ControlFlowPointcutTests.java

Lines 263 to 276 in f0c6fab

	private static class RegExControlFlowPointcut extends ControlFlowPointcut {
	private final List<Pattern> compiledPatterns;
	RegExControlFlowPointcut(Class<?> clazz, String... methodNamePatterns) {
	super(clazz, methodNamePatterns);
	this.compiledPatterns = super.methodNamePatterns.stream().map(Pattern::compile).toList();
	}
	@Override
	protected boolean isMatch(String methodName, int patternIndex) {
	return this.compiledPatterns.get(patternIndex).matcher(methodName).matches();
	}
	}

I've also posted an answer on that Stack Overflow discussion to point this out.

[kriegaex]

@sbrannen, LGTM. Thank you so much for addressing this issue.