WW-4105 Unwraps Spring proxy in actions chain #135
Conversation
| * </p> | ||
| * | ||
| */ | ||
| public class SpringUtils { |
There was a problem hiding this comment.
Maybe name it ProxyUtil or something like that. This class can be used to deal with known proxies and not only spring proxies.
There was a problem hiding this comment.
Thank you! I will do in next commit. However, I also try my effort to remove it to commons-lang if guys there accepted.
There was a problem hiding this comment.
SpringUtils doesn't sound good but I have no better name for now. Also it's the only case we have now so we can always refactor this code and introduce an interface with few implementation latter on.
For future reference: it would be good to have a ProxyAware interface in the core with a default implementation (that does nothing) and then in the Spring Plugin provide a real implementation.
There was a problem hiding this comment.
Thank you! Unfortunately I did not understand well. Do you mean that the user has to be careful to implement ProxyAware or inherit Spring Plugin's default implementation in his proxied actions? If so, there are two disadvantages, first he has to be careful about which actions may be proxied, and second he has to add Spring Plugin dependency or implement it manually.
There was a problem hiding this comment.
no, no ... it was just for a future reference when we will want turn this into a bean
| try { | ||
| if (isAopProxy(candidate) && | ||
| implementsInterface(candidate.getClass(), "org.springframework.aop.framework.Advised")) { | ||
| Object targetSource = MethodUtils.invokeMethod(candidate, "getTargetSource"); |
There was a problem hiding this comment.
What is targetSource here?
There was a problem hiding this comment.
It is what Spring himself does to unwrap it's proxies. Please see AopTestUtils.java
There was a problem hiding this comment.
Hmm... Never used AopTestUtils. But Advised extends TargetClassAware. Can getTargetClass just be invoked instead of two method calls? See AopUtils.
There was a problem hiding this comment.
getTargetClass just returns a Class but getting the target Object (i.e. actual action object) let us pass it directly to ReflectionProvider without any needed modification of ReflectionProvider codes.
| */ | ||
| public static boolean isAopProxy(Object object) { | ||
| Class<?> clazz = object.getClass(); | ||
| return (implementsInterface(clazz, "org.springframework.aop.SpringProxy") && |
There was a problem hiding this comment.
Extract strings to constants.
There was a problem hiding this comment.
Thank you! I will do in next commit.
| * </p> | ||
| * | ||
| */ | ||
| public class SpringUtils { |
There was a problem hiding this comment.
Another maybe. This can be turned to injectable bean so it can be easily overridable if needed. Wdyt?
There was a problem hiding this comment.
Why someone may need to override such specific methods?
As all methods could be static, I thought normally it should be a static utils class rather than a bean object.
There was a problem hiding this comment.
To support other possible proxies. Or in case something changes in spring-aop.
There was a problem hiding this comment.
I think we should update Struts itself in both cases when reported by a user.
|
@lukaszlenart , I updated with @aleksandr-m reviews. I hope it is ready for merge if you pals agree. |
|
I think with can go ahead and merge this, @aleksandr-m any objections? |
|
@lukaszlenart I think it is ok. |
Resolves WW-4105.