-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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: Scan all interfaces implemented by the jdk proxy class #695
Conversation
There is a small part of the conflict that needs to be resolved. |
spring/src/main/java/com/alibaba/fescar/spring/util/SpringProxyUtils.java
Show resolved
Hide resolved
@XCXCXCXCX please resolve conflicts |
# Conflicts: # spring/src/main/java/com/alibaba/fescar/spring/util/SpringProxyUtils.java
Codecov Report
@@ Coverage Diff @@
## develop #695 +/- ##
=============================================
- Coverage 32.68% 32.65% -0.03%
- Complexity 894 898 +4
=============================================
Files 226 226
Lines 8792 8806 +14
Branches 1058 1063 +5
=============================================
+ Hits 2874 2876 +2
- Misses 5595 5602 +7
- Partials 323 328 +5
Continue to review full report at Codecov.
|
I can't see any changes in this PR. |
Sorry, in order to let CI pass, I restored the code, now I resubmit the code.Please see if there is any problem. |
What changed is this PR? |
Just for the supplement of pr#660, the previous implementation will only scan the first interface of the jdk dynamic proxy implementation. If the jdk dynamic proxy has multiple interfaces, it will probably ignore some objects that need to be intercepted by the GlobalTransactionalInterceptor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
return proxy.getClass(); | ||
} | ||
} | ||
|
||
public static Class<?>[] findInterfaces(Object proxy) throws Exception { | ||
if(AopUtils.isJdkDynamicProxy(proxy)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only check jdk proxy ? What to do if it's a cglib proxy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's a CGLIB proxy class, I don't think it's necessary to consider whether the interface it implements needs to be proxyed, otherwise we should refactor it, scan all normal classes, its parent class, and the interfaces it implements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why we need to scan the jdk proxy is that there are many such implementations :
only the interface exists, there is no specific implementation class, the user can only mark the implementation of the fescar interception on the interface.
Ⅰ. Describe what this PR did
pull request #660 for supplement
Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)?
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews