Copilot commented on code in PR #7908:
URL: https://github.com/apache/incubator-seata/pull/7908#discussion_r2652099522
##########
rm-datasource/pom.xml:
##########
@@ -70,6 +70,14 @@
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-databind</artifactId>
</dependency>
+ <dependency>
+ <groupId>com.fasterxml.jackson.datatype</groupId>
+ <artifactId>jackson-datatype-jdk8</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>com.fasterxml.jackson.datatype</groupId>
Review Comment:
The jackson-datatype-jdk8 dependency appears to be unused in the codebase.
This module provides support for JDK 8 features like Optional, OptionalInt,
etc., but there's no code that explicitly uses these Jackson JDK 8 serializers.
If this dependency is not needed, consider removing it to keep dependencies
minimal. Only jackson-datatype-jsr310 is needed for OffsetDateTime and other
java.time types.
```suggestion
```
##########
rm-datasource/src/main/java/org/apache/seata/rm/datasource/undo/parser/JacksonUndoLogParser.java:
##########
@@ -185,7 +186,8 @@ public void init() {
module.addSerializer(SerialArray.class, serialArraySerializer);
module.addDeserializer(SerialArray.class, serialArrayDeserializer);
registerDmdbTimestampModuleIfPresent();
- mapper.registerModule(module);
+ JavaTimeModule javaTimeModule = new JavaTimeModule();
+ mapper.registerModules(module, javaTimeModule);
Review Comment:
The JavaTimeModule already provides serializers and deserializers for
LocalDateTime and other Java 8 date/time types. Adding it alongside custom
LocalDateTime serializers (localDateTimeSerializer and
localDateTimeDeserializer) may cause conflicts or unexpected behavior. Consider
removing the custom LocalDateTime serializer/deserializer registration on lines
184-185, or verify that the custom implementation is intentionally overriding
the default JavaTimeModule behavior.
##########
rm-datasource/src/test/java/org/apache/seata/rm/datasource/sql/struct/TableRecordsTest.java:
##########
@@ -292,4 +293,86 @@ public void testEmpty() {
Assertions.assertThrows(UnsupportedOperationException.class, () ->
empty.add(new Row()));
Assertions.assertThrows(UnsupportedOperationException.class,
empty::getTableMeta);
}
+
+ private static Object[][] columnMetasOffsetDateTime = new Object[][] {
+ new Object[] {
+ "", "", "table_records_test", "id", Types.INTEGER, "INTEGER", 64,
0, 10, 1, "", "", 0, 0, 64, 1, "NO", "YES"
+ },
+ new Object[] {
+ "",
+ "",
+ "table_records_test",
+ "time_col",
+ Types.TIMESTAMP_WITH_TIMEZONE,
+ "TIMESTAMP_WITH_TIMEZONE",
+ 64,
+ 0,
+ 10,
+ 0,
+ "",
+ "",
+ 0,
+ 0,
+ 64,
+ 2,
+ "YES",
+ "NO"
+ },
+ };
+
+ private static List<String> returnValueColumnLabelsOffsetDateTime =
Lists.newArrayList("id", "time_col");
+
+ private static Object[][] returnValueOffsetDateTime = new Object[][] {
+ new Object[] {1, OffsetDateTime.now()},
+ };
Review Comment:
Using OffsetDateTime.now() creates a non-deterministic test value which can
make test results harder to reproduce and debug. Consider using a fixed
OffsetDateTime value instead, such as OffsetDateTime.of(2024, 1, 15, 10, 30,
45, 0, ZoneOffset.UTC) to ensure consistent and reproducible test behavior.
##########
rm-datasource/src/main/java/org/apache/seata/rm/datasource/undo/parser/JacksonUndoLogParser.java:
##########
@@ -185,7 +186,8 @@ public void init() {
module.addSerializer(SerialArray.class, serialArraySerializer);
module.addDeserializer(SerialArray.class, serialArrayDeserializer);
registerDmdbTimestampModuleIfPresent();
- mapper.registerModule(module);
+ JavaTimeModule javaTimeModule = new JavaTimeModule();
+ mapper.registerModules(module, javaTimeModule);
Review Comment:
Consider adding a test case in JacksonUndoLogParserTest to verify that
OffsetDateTime values can be properly serialized and deserialized. While
there's a test for LocalDateTime (lines 114-125), there's no test for
OffsetDateTime which is now supported through the JavaTimeModule. This would
ensure that undo logs containing PostgreSQL TIMESTAMP WITH TIMEZONE fields can
be correctly persisted and restored.
##########
rm-datasource/src/test/java/org/apache/seata/rm/datasource/sql/struct/TableRecordsTest.java:
##########
@@ -292,4 +293,86 @@ public void testEmpty() {
Assertions.assertThrows(UnsupportedOperationException.class, () ->
empty.add(new Row()));
Assertions.assertThrows(UnsupportedOperationException.class,
empty::getTableMeta);
}
+
+ private static Object[][] columnMetasOffsetDateTime = new Object[][] {
+ new Object[] {
+ "", "", "table_records_test", "id", Types.INTEGER, "INTEGER", 64,
0, 10, 1, "", "", 0, 0, 64, 1, "NO", "YES"
+ },
+ new Object[] {
+ "",
+ "",
+ "table_records_test",
+ "time_col",
+ Types.TIMESTAMP_WITH_TIMEZONE,
+ "TIMESTAMP_WITH_TIMEZONE",
+ 64,
+ 0,
+ 10,
+ 0,
+ "",
+ "",
+ 0,
+ 0,
+ 64,
+ 2,
+ "YES",
+ "NO"
+ },
+ };
+
+ private static List<String> returnValueColumnLabelsOffsetDateTime =
Lists.newArrayList("id", "time_col");
+
+ private static Object[][] returnValueOffsetDateTime = new Object[][] {
+ new Object[] {1, OffsetDateTime.now()},
+ };
+
+ @Test
+ public void testBuildRecordsWithOffsetDateTime() throws SQLException {
+ MockDriver mockDriver = new MockDriver(
+ returnValueColumnLabelsOffsetDateTime,
+ returnValueOffsetDateTime,
+ columnMetasOffsetDateTime,
+ indexMetas);
+
+ DruidDataSource dataSource = new DruidDataSource();
+ dataSource.setUrl("jdbc:mock:offset");
+ dataSource.setDriver(mockDriver);
+
+ MockStatementBase mockStatement = new
MockStatement(getPhysicsConnection(dataSource));
+ DataSourceProxy proxy =
DataSourceProxyTest.getDataSourceProxy(dataSource);
+
+ TableMetaCacheFactory.getTableMetaCache(JdbcConstants.MYSQL)
+ .refresh(proxy.getPlainConnection(), proxy.getResourceId());
+
+ TableMeta tableMeta =
TableMetaCacheFactory.getTableMetaCache(JdbcConstants.MYSQL)
+ .getTableMeta(proxy.getPlainConnection(),
"table_records_test", proxy.getResourceId());
+
+ ResultSet originalResultSet = mockDriver.executeQuery(mockStatement,
"select * from table_records_test");
+
+ ResultSet proxyResultSet = (ResultSet)
java.lang.reflect.Proxy.newProxyInstance(
+ TableRecordsTest.class.getClassLoader(), new Class[]
{ResultSet.class}, (p, method, args) -> {
+ if ("getObject".equals(method.getName()) && args.length ==
2 && args[1] == OffsetDateTime.class) {
+ return originalResultSet.getObject((Integer) args[0]);
+ }
+ try {
+ return method.invoke(originalResultSet, args);
+ } catch (java.lang.reflect.InvocationTargetException e) {
+ throw e.getTargetException();
+ }
+ });
+
+ TableRecords tableRecords = TableRecords.buildRecords(tableMeta,
proxyResultSet);
+
+ Assertions.assertNotNull(tableRecords);
+ Assertions.assertEquals(1, tableRecords.size());
+
+ Row row = tableRecords.getRows().get(0);
+ Field timeField = row.getFields().stream()
+ .filter(f -> "time_col".equalsIgnoreCase(f.getName()))
+ .findFirst()
+ .orElseThrow(() -> new RuntimeException("time_col not found"));
+
+ Assertions.assertEquals(Types.TIMESTAMP_WITH_TIMEZONE,
timeField.getType());
+ Assertions.assertTrue(timeField.getValue() instanceof OffsetDateTime);
Review Comment:
This MockStatement is not always closed on method exit.
```suggestion
try (MockStatementBase mockStatement = new
MockStatement(getPhysicsConnection(dataSource))) {
DataSourceProxy proxy =
DataSourceProxyTest.getDataSourceProxy(dataSource);
TableMetaCacheFactory.getTableMetaCache(JdbcConstants.MYSQL)
.refresh(proxy.getPlainConnection(),
proxy.getResourceId());
TableMeta tableMeta =
TableMetaCacheFactory.getTableMetaCache(JdbcConstants.MYSQL)
.getTableMeta(proxy.getPlainConnection(),
"table_records_test", proxy.getResourceId());
ResultSet originalResultSet =
mockDriver.executeQuery(mockStatement, "select * from table_records_test");
ResultSet proxyResultSet = (ResultSet)
java.lang.reflect.Proxy.newProxyInstance(
TableRecordsTest.class.getClassLoader(), new Class[]
{ResultSet.class}, (p, method, args) -> {
if ("getObject".equals(method.getName()) &&
args.length == 2 && args[1] == OffsetDateTime.class) {
return originalResultSet.getObject((Integer)
args[0]);
}
try {
return method.invoke(originalResultSet, args);
} catch (java.lang.reflect.InvocationTargetException
e) {
throw e.getTargetException();
}
});
TableRecords tableRecords = TableRecords.buildRecords(tableMeta,
proxyResultSet);
Assertions.assertNotNull(tableRecords);
Assertions.assertEquals(1, tableRecords.size());
Row row = tableRecords.getRows().get(0);
Field timeField = row.getFields().stream()
.filter(f -> "time_col".equalsIgnoreCase(f.getName()))
.findFirst()
.orElseThrow(() -> new RuntimeException("time_col not
found"));
Assertions.assertEquals(Types.TIMESTAMP_WITH_TIMEZONE,
timeField.getType());
Assertions.assertTrue(timeField.getValue() instanceof
OffsetDateTime);
}
```
--
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]
