Copilot commented on code in PR #7867:
URL: https://github.com/apache/incubator-seata/pull/7867#discussion_r2633446019
##########
integration-tx-api/src/main/java/org/apache/seata/integration/tx/api/interceptor/parser/GlobalTransactionalInterceptorParser.java:
##########
@@ -89,17 +94,40 @@ protected boolean existsAnnotation(Class<?>... classes) {
return true;
}
Method[] methods = clazz.getMethods();
- for (Method method : methods) {
- trxAnno = method.getAnnotation(GlobalTransactional.class);
- if (trxAnno != null) {
- methodsToProxy.add(method.getName());
- result = true;
- }
+ Method[] declaredMethods = clazz.getDeclaredMethods();
+ int arrayLength = methods.length + declaredMethods.length;
+ if (arrayLength > 0) {
+ Method[] allMethods = new Method[arrayLength];
+ System.arraycopy(methods, 0, allMethods, 0,
methods.length);
+ System.arraycopy(declaredMethods, 0, allMethods,
methods.length, declaredMethods.length);
+ Set<Method> processedMethods = new HashSet<>();
+ for (Method method : allMethods) {
+ if (!processedMethods.add(method)) {
+ continue;
+ }
+ trxAnno =
method.getAnnotation(GlobalTransactional.class);
+ if (trxAnno != null) {
+ methodsToProxy.add(method.getName());
+ if (!Modifier.isPrivate(method.getModifiers())) {
+ result = true;
+ } else {
+ LOGGER.warn(
+ "GlobalTransactional annotation found
on private method {}, which will be ignored.",
+ method.getName());
+ }
+ }
- GlobalLock lockAnno =
method.getAnnotation(GlobalLock.class);
- if (lockAnno != null) {
- methodsToProxy.add(method.getName());
- result = true;
+ GlobalLock lockAnno =
method.getAnnotation(GlobalLock.class);
+ if (lockAnno != null) {
+ methodsToProxy.add(method.getName());
+ if (!Modifier.isPrivate(method.getModifiers())) {
+ result = true;
+ } else {
+ LOGGER.warn(
+ "GlobalLock annotation found on
private method {}, which will be ignored.",
+ method.getName());
+ }
Review Comment:
Private methods with GlobalLock annotation are still being added to the
methodsToProxy set even though they should be ignored. The check on line 123
only affects the result boolean but doesn't prevent adding the method name to
methodsToProxy on line 122. This means private methods will be included in the
proxy set, which contradicts the intent of filtering them out. Consider moving
line 122 inside the non-private check block so that only non-private annotated
methods are added to methodsToProxy.
##########
integration-tx-api/src/main/java/org/apache/seata/integration/tx/api/interceptor/parser/GlobalTransactionalInterceptorParser.java:
##########
@@ -89,17 +94,40 @@ protected boolean existsAnnotation(Class<?>... classes) {
return true;
}
Method[] methods = clazz.getMethods();
- for (Method method : methods) {
- trxAnno = method.getAnnotation(GlobalTransactional.class);
- if (trxAnno != null) {
- methodsToProxy.add(method.getName());
- result = true;
- }
+ Method[] declaredMethods = clazz.getDeclaredMethods();
+ int arrayLength = methods.length + declaredMethods.length;
+ if (arrayLength > 0) {
+ Method[] allMethods = new Method[arrayLength];
+ System.arraycopy(methods, 0, allMethods, 0,
methods.length);
+ System.arraycopy(declaredMethods, 0, allMethods,
methods.length, declaredMethods.length);
+ Set<Method> processedMethods = new HashSet<>();
+ for (Method method : allMethods) {
+ if (!processedMethods.add(method)) {
+ continue;
+ }
+ trxAnno =
method.getAnnotation(GlobalTransactional.class);
+ if (trxAnno != null) {
+ methodsToProxy.add(method.getName());
+ if (!Modifier.isPrivate(method.getModifiers())) {
+ result = true;
+ } else {
+ LOGGER.warn(
+ "GlobalTransactional annotation found
on private method {}, which will be ignored.",
+ method.getName());
+ }
Review Comment:
Private methods are still being added to the methodsToProxy set even though
they should be ignored. The check on line 112 only affects the result boolean
but doesn't prevent adding the method name to methodsToProxy on line 111. This
means private methods will be included in the proxy set, which contradicts the
intent of filtering them out. Consider moving line 111 inside the non-private
check block so that only non-private annotated methods are added to
methodsToProxy.
##########
integration-tx-api/src/main/java/org/apache/seata/integration/tx/api/interceptor/parser/GlobalTransactionalInterceptorParser.java:
##########
@@ -89,17 +94,40 @@ protected boolean existsAnnotation(Class<?>... classes) {
return true;
}
Method[] methods = clazz.getMethods();
- for (Method method : methods) {
- trxAnno = method.getAnnotation(GlobalTransactional.class);
- if (trxAnno != null) {
- methodsToProxy.add(method.getName());
- result = true;
- }
+ Method[] declaredMethods = clazz.getDeclaredMethods();
+ int arrayLength = methods.length + declaredMethods.length;
+ if (arrayLength > 0) {
+ Method[] allMethods = new Method[arrayLength];
+ System.arraycopy(methods, 0, allMethods, 0,
methods.length);
+ System.arraycopy(declaredMethods, 0, allMethods,
methods.length, declaredMethods.length);
+ Set<Method> processedMethods = new HashSet<>();
Review Comment:
Creating a combined array of methods and declaredMethods using
System.arraycopy is inefficient when both arrays could be processed separately
in consecutive loops. Consider processing methods array first, then
declaredMethods array, using a single Set to track processed methods across
both loops. This would avoid the unnecessary array allocation and copying.
##########
integration-tx-api/src/main/java/org/apache/seata/integration/tx/api/interceptor/parser/GlobalTransactionalInterceptorParser.java:
##########
@@ -89,17 +94,40 @@ protected boolean existsAnnotation(Class<?>... classes) {
return true;
}
Method[] methods = clazz.getMethods();
- for (Method method : methods) {
- trxAnno = method.getAnnotation(GlobalTransactional.class);
- if (trxAnno != null) {
- methodsToProxy.add(method.getName());
- result = true;
- }
+ Method[] declaredMethods = clazz.getDeclaredMethods();
+ int arrayLength = methods.length + declaredMethods.length;
+ if (arrayLength > 0) {
+ Method[] allMethods = new Method[arrayLength];
+ System.arraycopy(methods, 0, allMethods, 0,
methods.length);
+ System.arraycopy(declaredMethods, 0, allMethods,
methods.length, declaredMethods.length);
+ Set<Method> processedMethods = new HashSet<>();
+ for (Method method : allMethods) {
+ if (!processedMethods.add(method)) {
+ continue;
+ }
+ trxAnno =
method.getAnnotation(GlobalTransactional.class);
+ if (trxAnno != null) {
+ methodsToProxy.add(method.getName());
+ if (!Modifier.isPrivate(method.getModifiers())) {
+ result = true;
+ } else {
+ LOGGER.warn(
+ "GlobalTransactional annotation found
on private method {}, which will be ignored.",
+ method.getName());
+ }
+ }
- GlobalLock lockAnno =
method.getAnnotation(GlobalLock.class);
- if (lockAnno != null) {
- methodsToProxy.add(method.getName());
- result = true;
+ GlobalLock lockAnno =
method.getAnnotation(GlobalLock.class);
+ if (lockAnno != null) {
+ methodsToProxy.add(method.getName());
+ if (!Modifier.isPrivate(method.getModifiers())) {
+ result = true;
+ } else {
+ LOGGER.warn(
+ "GlobalLock annotation found on
private method {}, which will be ignored.",
+ method.getName());
+ }
Review Comment:
The logic for checking GlobalTransactional and GlobalLock annotations is
duplicated. Both blocks check for the annotation, add the method to
methodsToProxy, verify it's not private, and log a warning if it is private.
Consider extracting this repeated logic into a helper method that takes the
annotation and annotation name as parameters to reduce code duplication and
improve maintainability.
##########
integration-tx-api/src/main/java/org/apache/seata/integration/tx/api/interceptor/parser/GlobalTransactionalInterceptorParser.java:
##########
@@ -26,13 +26,18 @@
import org.apache.seata.spring.annotation.GlobalLock;
import org.apache.seata.spring.annotation.GlobalTransactional;
import org.apache.seata.tm.api.FailureHandlerHolder;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
import java.util.HashSet;
Review Comment:
The Arrays import is added but never used in the code. Consider removing
this unused import to keep the code clean.
##########
integration-tx-api/src/test/java/org/apache/seata/integration/tx/api/interceptor/parser/GlobalTransactionalInterceptorParserTest.java:
##########
@@ -38,4 +38,19 @@ void parserInterfaceToProxy() throws Exception {
// then
Assertions.assertNotNull(proxyInvocationHandler);
}
+
+ @Test
+ void parserInterfaceToProxyNonPrivateMethod() throws Exception {
+ MethodImpl method = new MethodImpl();
+
+ GlobalTransactionalInterceptorParser
globalTransactionalInterceptorParser =
+ new GlobalTransactionalInterceptorParser();
+
+ ProxyInvocationHandler proxyInvocationHandler =
globalTransactionalInterceptorParser.parserInterfaceToProxy(
+ method, method.getClass().getName());
+
+ Assertions.assertNotNull(proxyInvocationHandler);
+
+ Assertions.assertEquals(2,
proxyInvocationHandler.getMethodsToProxy().size());
Review Comment:
The test expects 2 methods in methodsToProxy, but due to a bug in the
implementation where private methods are still added to the set (despite being
ignored for proxying), this will actually be 3. The MethodImpl class has three
annotated methods: dftMethod (package-private), protectedMethod (protected),
and privateMethod (private). Once the bug is fixed to exclude private methods
from methodsToProxy, this assertion will be correct, but currently it will fail.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
