Copilot commented on code in PR #7867:
URL: https://github.com/apache/incubator-seata/pull/7867#discussion_r2633308201
##########
integration-tx-api/src/main/java/org/apache/seata/integration/tx/api/interceptor/parser/GlobalTransactionalInterceptorParser.java:
##########
@@ -89,17 +90,29 @@ 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);
+ for (Method method : allMethods) {
Review Comment:
The approach of combining getMethods() and getDeclaredMethods() results in
duplicate entries for public methods. The getMethods() call already returns all
public methods including those declared in the class, and getDeclaredMethods()
returns all methods declared in the class regardless of access modifier. This
means public methods will appear twice in the allMethods array.
Since methodsToProxy is a Set, duplicates won't cause functional issues, but
this creates unnecessary iterations and checks. Consider using only
getDeclaredMethods() and filtering by modifier instead, or add a check to skip
duplicates when processing the combined array.
```suggestion
System.arraycopy(declaredMethods, 0, allMethods,
methods.length, declaredMethods.length);
Set<Method> processedMethods = new HashSet<>();
for (Method method : allMethods) {
// skip methods already processed (avoids duplicates
from combining getMethods and getDeclaredMethods)
if (!processedMethods.add(method)) {
continue;
}
```
##########
integration-tx-api/src/test/java/org/apache/seata/integration/tx/api/interceptor/parser/MethodImpl.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.seata.integration.tx.api.interceptor.parser;
+
+import org.apache.seata.spring.annotation.GlobalTransactional;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class MethodImpl {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(MethodImpl.class);
+
+ @GlobalTransactional(timeoutMills = 300000)
+ String dftMethod() {
+ LOGGER.info("default method");
+ return "default method";
+ }
+
+ @GlobalTransactional(timeoutMills = 300000)
+ protected String protectedMethod() {
+ LOGGER.info("protected method");
+ return "protected method";
+ }
+}
Review Comment:
The test class should include a private method with the GlobalTransactional
annotation to verify that private methods are correctly excluded from proxying.
Currently, the test only validates that non-private methods (default and
protected) are included, but doesn't test the exclusion logic for private
methods.
##########
integration-tx-api/src/main/java/org/apache/seata/integration/tx/api/interceptor/parser/GlobalTransactionalInterceptorParser.java:
##########
@@ -89,17 +90,29 @@ 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);
Review Comment:
The manual array concatenation using System.arraycopy is verbose and
error-prone. Consider using a more maintainable approach such as streaming both
arrays or using a list-based approach to combine the methods. This would
improve code readability and reduce the risk of errors in calculating array
lengths.
--
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]
