Questions for clarification

[kriegaex]

Hi.

Let's talk here, StackOverflow is not the right place for discussions.

Your approach seems to be quite contrived and over-engineered with lots of reflection based on magic class names. The sibling class approach is also not nice, it is not type-safe. I know you wish to avoid that, but for now I can only see your curent implementation.

Questions:

• Why do you put the annotations on methods instead of interface classes? Are there cases in which for some interfaces only a subset of its methods are subject to the strategy pattern?
• Is class name StrategyPattern meant to signify some kind of base class for any type of strategy pattern or is is just an example for one possible strategy pattern and there could be many such classes or interfaces?
• I am also trying to understand which basic problem you are trying to solve by using AOP in order to implement the strategy pattern. A strategy pattern is a means to an end and not the end itself. On SO you mention that you want to run different implementations based on security role. This example project rather looks like you select the strategy based on some company name, whatever "product company" means in your context. So this rather looks like you want to implement a multi-tenant system.

I might have other questions later, but let's start here. I wish to understand your situation better before making suggestions.

[nvanhoeck]

• We have found method based annotations more flexible on service/business classes. We only use class annotations on entities/dto's/objects... We've had a situation where an older similar implementation was class based and the scope changed in a way we suddenly had to do method based annotations for our code to be more flexible and clean.
• This is just an example. To give some context, our company works with different divisions/product companies and all of them have seperate database, queries, logic, .... Your security keeps track of your division your are logged into. So this annotation should be used across repo's, services, "query builders" .... So quite generic actually.
• I think above answer can be used here as well. I'm not a native speaker of English and I only know the meaning of multi-tenant from security context 😀. If you mean multiple types of logics needs to be executed by applying polymorphism, then yes.

I can see your concern about name based reflection, but this is based on Spring's bean declaration with a name. I believe it to be the same principle, only on class level. Spring chooses an implementation of an interface based on its bean name and the variable name you declare. Like with qualifiers.

You could say: why not an interface and have multiple implementations of it and declare them with those bean names. That causes issues, because you need to declare that interface multiple times with a different bean name in a service that uses those implementations. And if you have ten divisions, you'll need to declare it thenfold, each with a seperate variable/bean name. And if you choose a single interface, you get the "multiple beans found" error. Choose @Primary or @Qualifier.

So we build this. So that when running code which comes across this annotation, the interceptor looks for a bean with a classname similar to this, but with the division code as a prefix.

Sadly this isn't an interface, otherwise we could implement it and force those methods to be implemented so they always will be the same, also with the same method parameters and types.

[kriegaex]

OK, so which degree of freedom do I have when suggesting an alternative solution?

• Let's say we stick with the method annotations and the reflective approach for determining the strategy class name.
• Can we refactor the template class with siblings into an interface with implementations that have magic class name prefixes? I think this is what you want.
• Can we refactor the method interceptor and bean factory method with extra pointcut into a classical aspect so as to have all AOP information in one place and enjoy component scan auto-detection? This might just be cosmetics to you, but I would like to use a more canonical AOP approach.
• Is is acceptable that the aspect (just like the method interceptor would) would be triggered would be triggered for too many methods and during runtime we would determine if the intercepted method has a strategy marker annotation on its interface?
  • If yes, we can stick with Spring AOP at some performance cost.
  • If no, we need to switch to native AspectJ and use ITD in order to emulate method annotation inheritance.
    • This would either be done via load-time weaving (one-time performance cost during container start-up, but better performance due to no longer needing proxies for AOP), also meaning that all aspects would use LTW instead of Spring AOP
    • or compile-time weaving, meaning no additional start-up cost but having to use AspectJ Maven Plugin for weaving the strategy annotations into the interface-implementing target classes. But in this case, the native aspects would be in the byte code already and for the "normal" Spring aspects you can continue using Spring AOP.

[kriegaex]

BTW, this issue is really kind of complex. Feel free to suggest to your company to hire me as a remote consultant. I can sign NDAs, see your real code and we can iteratively find just the right approach for you instead of playing with the little MCVE here.

[kriegaex]

I am also wondering why you need this code block:

Class<?>[] paramClasses = new Class<?>[method.getParameterTypes().length];
for (int paramIndex = 0; paramIndex < method.getParameterTypes().length; paramIndex++) {
  Class<?> parameterType = method.getParameterTypes()[paramIndex];
  if (parameterType.isPrimitive()) {
    paramClasses[paramIndex] = parameterType;
  }
  else {
    Class<?> paramClass = Class.forName(parameterType.getName());
    paramClasses[paramIndex] = paramClass;
  }
}

Method productCompanySpecificMethod = productCompanySpecificClass.getMethod(methodName, paramClasses);

Why would you convert Class instances to strings and the back into classes via Class.forName(String)? Instead you could IMO simply write:

Method productCompanySpecificMethod = productCompanySpecificClass.getMethod(methodName, method.getParameterTypes());

[kriegaex]

I took the liberty to refactor your existing solution into an aspect-based one, but without changing the basic functionality, see this tree and this commit.

I suggest to start enhancing the solution starting from this point according to your feedback to https://github.com/nvanhoeck/strategy-pattern-aop/issues/1#issuecomment-950131607.

[nvanhoeck]

@kriegaex thank you so far for your support. Once I have the spare-time to take a look at your comments & suggestions & code changes I'll come back to this (hopefully tomorrow). Enjoy your day.

[nvanhoeck]

@kriegaex i'm gonna answer to all your comments and code changes in one message (italic is your text).

- Let's say we stick with the method annotations and the reflective approach for determining the strategy class name. Can we refactor the template class with siblings into an interface with implementations that have magic class name prefixes? I think this is what you want.

Yes, this is totally what we want.

-Can we refactor the method interceptor and bean factory method with extra pointcut into a classical aspect so as to have all AOP information in one place and enjoy component scan auto-detection? This might just be cosmetics to you, but I would like to use a more canonical AOP approach.

I have seen the enhancements in the branch. That looks way better indeed.

- Is is acceptable that the aspect (just like the method interceptor would) would be triggered for too many methods and during runtime we would determine if the intercepted method has a strategy marker annotation on its interface?

Yes, it is acceptable. However, I'll take a look at AspectJ and ITD for further investigation, should the calls become indeed to heavy.

- If yes, we can stick with Spring AOP at some performance cost.

See answer above.

- If no, we need to switch to native AspectJ and use ITD in order to emulate method annotation inheritance.

See answer above.

- This would either be done via load-time weaving (one-time performance cost during container start-up, but better performance due to no longer needing proxies for AOP), also meaning that all aspects would use LTW instead of Spring AOP or compile-time weaving, meaning no additional start-up cost but having to use AspectJ Maven Plugin for weaving the strategy annotations into the interface-implementing target classes. But in this case, the native aspects would be in the byte code already and for the "normal" Spring aspects you can continue using Spring AOP.

---

BTW, this issue is really kind of complex. Feel free to suggest to your company to hire me as a remote consultant. I can sign NDAs, see your real code and we can iteratively find just the right approach for you instead of playing with the little MCVE here.

That's gonna be a hard one, i'm afraid. The setup for onboarding will take two weeks at least and my manager only allows long-term consultants. I'll ask him tomorrow when i'm at the office, but I believe I know the answer already (especially if I tell him it's for refactoring or code improvements.)

I am also wondering why you need this code block.

Indeed, unnecessary after a second look.

_I took the liberty to refactor your existing solution into an aspect-based one, but without changing the basic functionality, see this tree and this commit.

I suggest to start enhancing the solution starting from this point according to your feedback to #1 (comment)._

Yes, thank you for your work so far. I hope the feedback provided will be enough. We are already grateful at our team for your support so far. (Is there some way to give you medals or upvotes or something on SO, like they have with reddit?)

[kriegaex]

We are already grateful at our team for your support so far. (Is there some way to give you medals or upvotes or something on SO, like they have with reddit?)

Of course you can accept and upvote my future answer on SO after we will have sorted out this problem here on GitHub. Accepted answers yield 15 reputation points, upvotes 10. I currently have ~53,000, so you see it does not make a big difference, no matter how complex this problem is and how much effort I am investing. Another way to reward people on SO is to put a bounty of N points (e.g. 50, 100, 500, whatever) on a question and then decide to give it away to an answer you appreciate. You only have 63 points, i.e. you cannot give a way much. Sometimes, money is just unbeatable, because I can buy things with it. 😉

So much for the meta topic. I am going to write another comment later concerning the technical problem we are discussing here. I got to do some other work first.

[kriegaex]

OK, I did the following:

• Method annotation @ProductCompanyImplSelection can now be used on both interfaces and base classes, depending on your preference. You can also mix the approach for different types of strategies. The aspect will detect the annotation anywhere in the parent class or interface hierarchy, utilising Spring's AnnotationUtils.
• A new type annotation @DefaultStrategyPattern now marks the default (possibly dummy) strategy implementations you use when wiring the application. This not only documents their role, but also has the additional benefit of making pointcut matching in the aspect more efficient via @within(*..ProductCompanyDefaultImpl) pointcut. Otherwise, we would have to manually avoid recursive pointcut matching on both parent and child class methods, because now @ProductCompanyImplSelection annotations are detected throughout the class hierarchy, see above. By limiting matching to only default classes, strategy classes will not be intercepted by ProductCompanyBoundImplSelectionAspect.

You can see a version using both an interface and a default implementation in branch refactor-to-interface. Both the default implementation and all product company strategies implement the same interface StrategyPattern. See commit https://github.com/kriegaex/strategy-pattern-aop/commit/8fa0429f38e7266f893a61702289227f57800059.

Alternatively, I added one more commit https://github.com/kriegaex/strategy-pattern-aop/commit/2901587d52b91915c472443e16e54c56a4e79cbe in branch refactor-to-base-class, removing the interface and moving the method annotation directly into the default strategy implementation. Now the product company strategy classes directly extend the default class.

Both of the above approaches work with the exact same aspect. Just select the one you like best. Let me know if you want me to create PRs for either or both of the two branches.

[nvanhoeck]

I have one remark for your improvement on the refactor-to-interface code. Is it possible to define StrategyPattern interface as the variable class instead of DefaultStrategyPattern?

[kriegaex]

I do not understand your question, and the screenshot is not helping either. Please elaborate.

[nvanhoeck]

As is now we declare an default class (DefaultStrategyPattern) to use the strategy pattern. But it would be cleaner if you just declare the interface (StrategyPattern) so the DefaultStrategyPattern class would not be needed anymore.

Flow:

Interface --> has Strategy annotation --> determine at runtime which class implementation is needed to run.

[kriegaex]

I already explained why I did it that way: If we would simply rely on the interface, we would have to explicitly check if the target class itself has the strategy annotation on its method and exclude those classes which do not have it, because Spring's annotation utils would always find it, which again would lead to endless recursion of aspects being triggered for each strategy implementation. That would be slow, because 99% of aspect advice method executions would be false positives. So the additional annotation is actually a performance booster - by how much, I do not know.

So yes, an implementation without the additional annotation is possible, but slow if you want to stick with Spring AOP and avoid using AspectJ. IMO it would be "cleaner" on the surface only, but "dirtier" under the hood. Do you really want Spring to create a proxy for each single strategy implementation, just so as to execute code which finally skips aspect execution?

[nvanhoeck]

Ok, thank you for the clarification. A PR for both branches would be nice.

[kriegaex]

One more question that just occurred to me: If we only had the interface and no default implementation anymore, what would you wire into your application? And if we would keep the default implementation, how would we identify it? Only by name matching? If you answer these questions, I can prepare a 3rd branch for you, then you can see how ugly it actually is inside the aspect. In order to see the full scope of ugliness, you would have to count the number of proxies in comparison to the other solution, though.

[kriegaex]

OK, I simply assumed magical default class names, see branch refactor-to-interface-slow. BTW, in this case it should work, no matter if the actual strategies are sibling or child classes of the default strategy. I kept them siblings for now, but you can easily make them extend the default class, whatever best matches your own idea of a class hierarchy.

[kriegaex]

As for PRs, you only have one branch I could select as a target for all 3 of my own branches, so instead of creating 3 conflicting PRs, I simply recommend you to add my repository as an additional remote and merge whichever branch you like best by yourself.

[nvanhoeck]

Ok great. I'll take a look at the latest one and then I'll add yours repo as a remote one for the PR's.

[nvanhoeck]

I added your branches and remote.

I changed to see if the interface alone could work, but I got the multiple beans implementations as an error:

This is not something that we can bypass probably, like jut declare the interface and let the annotation at runtime make the bean selection?

And I see you had some questions as well. Are these questions belonging to the branch you created (slow) or is this another topic?

[kriegaex]

I cannot reproduce your problem. Which branch did you check out?

[nvanhoeck]

The branch refactor-to-interface-slow.

[nvanhoeck]

Closing issue since three alternative solutions has been provided. @kriegaex thank you for your help! Much appreciated!

[kriegaex]

The branch refactor-to-interface-slow.

But for me it just works. Did you somehow modify my code or merge it into yours, ending up with a different way of fetching beans from the application context?

[nvanhoeck]

I renamed the variable from DefaultStrategyPattern to StrategyPattern, to skip the default implementation and hoping to call the interface, but I don't think that would be possible, since spring gonna need at runtime an already defined implementation.

[kriegaex]

I renamed the variable from DefaultStrategyPattern to StrategyPattern

You mean here?

    @Autowired
    private DefaultStrategyPattern defaultStrategyPattern;

Yeah well, of course you need to auto-wire something concrete or alternatively configure a default implementation for the interface somehow. If you would not need to switch strategies during runtime and could just decide which strategy to wire during application start-up, it would be easier and you would not even need the dynamic reflective aspect. Anyway, I am happy your problem is solved.