Copilot commented on code in PR #7996:
URL: https://github.com/apache/incubator-seata/pull/7996#discussion_r2813301006
##########
namingserver/pom.xml:
##########
@@ -40,15 +40,15 @@
<dependencyManagement>
<dependencies>
- <!-- Spring Boot -->
+ <!-- Spring Boot -->
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-dependencies</artifactId>
<version>${spring-boot-for-server.version}</version>
<type>pom</type>
<scope>import</scope>
</dependency>
- <!-- spring-framework-->
+ <!-- spring-framework -->
Review Comment:
Trailing whitespace has been added to comments on lines 43 and 51. While
this doesn't affect functionality, it's generally better to avoid trailing
whitespace as it can cause unnecessary diff noise in future changes and may be
flagged by some linters or editor configurations.
##########
console/pom.xml:
##########
@@ -33,10 +33,10 @@
<properties>
<spring-boot-for-server.version>3.5.2</spring-boot-for-server.version>
+
<spring-framework-for-server.version>6.2.8</spring-framework-for-server.version>
<snakeyaml-for-server.version>2.0</snakeyaml-for-server.version>
<tomcat-embed.version>11.0.12</tomcat-embed.version>
<spring-ai.version>1.1.0</spring-ai.version>
-
<spring-framework-for-server.version>6.2.8</spring-framework-for-server.version>
</properties>
Review Comment:
The property `spring-framework-for-server.version` is being moved from line
39 to line 36, appearing to be a simple reordering for better organization
(grouping Spring-related properties together). However, this creates a minor
inconsistency in the diff where the property appears to be both added and
removed. While this is just a reordering and functionally correct, consider
whether this reordering provides value or could be avoided to reduce diff noise.
##########
sqlparser/seata-sqlparser-druid/src/test/java/org/apache/seata/sqlparser/druid/DruidSQLRecognizerFactoryTest.java:
##########
@@ -221,6 +222,28 @@ public void testIsSqlSyntaxSupports() {
NotSupportYetException.class, () ->
recognizerFactory.create(sql9, JdbcConstants.KINGBASE));
}
+ @EnabledIfSystemProperty(
+ named = "druid.version",
+ matches =
"(1\\.[3-9]\\..*)|(2\\..*)|(1\\.2\\.[5-9].*)|(1\\.2\\.[1-9][0-9].*)")
Review Comment:
The regex pattern in the `@EnabledIfSystemProperty` annotation appears
overly complex and may not work as intended.
Breaking down the pattern
`(1\\.[3-9]\\..*)|(2\\..*)|(1\\.2\\.[5-9].*)|(1\\.2\\.[1-9][0-9].*)`:
- `1\\.[3-9]\\.*` matches 1.3.x through 1.9.x
- `2\\.*` matches any 2.x version
- `1\\.2\\.[5-9].*` matches 1.2.5 through 1.2.9
- `1\\.2\\.[1-9][0-9].*` matches 1.2.10 through 1.2.99
However, this pattern has issues:
1. The last part `1\\.2\\.[1-9][0-9].*` would match "1.2.10" through
"1.2.99" but NOT "1.2.25" onwards correctly due to the `[1-9]` constraint on
the tens digit.
2. The description says the test requires Druid >= 1.2.25, but the pattern
would actually enable it for versions >= 1.2.5 OR >= 1.2.10, which is broader
than intended.
A clearer pattern matching "1.2.25+" would be:
`(1\\.2\\.(2[5-9]|[3-9][0-9]))|(1\\.[3-9]\\..*))|(2\\..*)`
##########
rm-datasource/src/test/java/org/apache/seata/rm/datasource/undo/oscar/OscarUndoLogManagerTest.java:
##########
@@ -51,6 +51,10 @@
import java.util.Date;
import java.util.List;
+@DisabledIfSystemProperty(
+ named = "druid.version",
+ matches = "[0-1].[1-2].[0-24]",
+ disabledReason = "druid 1.2.24 correct support oscar")
Review Comment:
The regex pattern in the `@DisabledIfSystemProperty` annotation is
incorrect. The pattern `[0-1].[1-2].[0-24]` uses character classes instead of
capturing groups, which matches single digits/characters rather than version
ranges.
For example:
- `[0-1]` matches only "0" or "1", not "0.x" or "1.x"
- `[1-2]` matches only "1" or "2"
- `[0-24]` matches "0", "2", or "4" (not "0-24" as a range)
The pattern should be: `1\\.2\\.(([0-9]|1[0-9]|2[0-4]))` to match versions
1.2.0 through 1.2.24, or simply `1\\.2\\.([0-9]|1[0-9]|2[0-4])` without the
extra capturing group.
The current pattern would match strings like "0.1.0" or "1.2.4" but not
proper version strings like "1.2.24".
--
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]
