Copilot commented on code in PR #7966:
URL: https://github.com/apache/incubator-seata/pull/7966#discussion_r2731883951
##########
rm-datasource/src/main/java/org/apache/seata/rm/datasource/sql/struct/cache/KingbaseTableMetaCache.java:
##########
@@ -185,4 +204,12 @@ protected IndexMeta toIndexMeta(ResultSet rs, String
indexName, ColumnMeta colum
}
return result;
}
+
+ private static String getStringSafely(ResultSet rs, String columnLabel) {
+ try {
+ return rs.getString(columnLabel);
+ } catch (Exception e) {
Review Comment:
The getStringSafely method catches all exceptions and returns null. This is
overly broad and could hide important issues. Consider catching only
SQLException or specific exceptions that are expected when the column doesn't
exist. Other exceptions (like resource exhaustion or connection issues) should
be propagated.
```suggestion
} catch (SQLException e) {
```
##########
rm-datasource/src/test/java/org/apache/seata/rm/datasource/sql/struct/cache/KingbaseTableMetaCacheTest.java:
##########
@@ -332,4 +332,102 @@ public void testGetTableMetaOriginalTableNamePreserved()
throws SQLException {
Assertions.assertNotNull(tableMeta);
Assertions.assertEquals(originalName,
tableMeta.getOriginalTableName());
}
+
+ @Test
+ public void testCompositeIndexWithDifferentColumnOrder() throws
SQLException {
+ // Regression test for a bug fix: different column order caused
List.equals to fail, which could lead to
+ // failing to recognize a primary key index.
+ // Scenario: composite primary key (ID, USER_ID) while a unique index
lists the same columns in reverse order
+ // (USER_ID, ID).
+ // Before the fix: List.equals returned FALSE because of the different
order, causing the primary key index to
+ // be unrecognized.
+ // After the fix: using Set.equals should return TRUE and correctly
identify the index as PRIMARY.
+
+ Object[][] compositeIndexDiffOrder = new Object[][] {
+ new Object[] {"idx_id_userid", "id", false, "", 3, 1, "A", 34}, //
column order 1
+ new Object[] {"idx_id_userid", "user_id", false, "", 3, 2, "A",
34} // column order 2
+ };
+ Object[][] compositePKMetas = new Object[][] {
+ new Object[] {"id"}, // PK column order 1
+ new Object[] {"user_id"} // PK column order 2
Review Comment:
The test claims to verify "different column order" (line 338-341 mentions
"reverse order (USER_ID, ID)"), but the actual test data has both the index
columns and PK columns in the same order. The compositeIndexDiffOrder array has
columns in order (id, user_id), and compositePKMetas also has them in order
(id, user_id). To properly test the order-insensitivity fix, one of these
should be reversed, for example compositePKMetas should be {{"user_id"},
{"id"}}.
```suggestion
new Object[] {"user_id"}, // PK column order 1 (reverse of index)
new Object[] {"id"} // PK column order 2
```
##########
changes/zh-cn/2.6.0.md:
##########
@@ -75,6 +75,7 @@
- [[#7908](https://github.com/apache/incubator-seata/pull/7908)] 在 PostgreSQL
主键中处理带时区的时间戳
- [[#7938](https://github.com/apache/incubator-seata/pull/7938)]
保证Jakarta相关包路径正确
- [[#7959](https://github.com/apache/incubator-seata/pull/7959)]
修复consoleApiService bean未加载的问题
+- [[#7966](https://github.com/apache/incubator-seata/pull/7966)]
修复dm和kingbase中当没有将索引设置为主键索引
Review Comment:
The Chinese changelog entry appears incomplete or grammatically incorrect.
It reads "修复dm和kingbase中当没有将索引设置为主键索引" which translates roughly to "Fix when
index is not set as primary key index in dm and kingbase" but is missing
context. It should be something like "修复dm和kingbase中因列顺序不同导致索引未被识别为主键索引的问题"
(Fix issue where different column order causes index to not be recognized as
primary key index in dm and kingbase).
```suggestion
- [[#7966](https://github.com/apache/incubator-seata/pull/7966)]
修复dm和kingbase中因列顺序不同导致索引未被识别为主键索引的问题
```
##########
rm-datasource/src/test/java/org/apache/seata/rm/datasource/sql/struct/cache/DmTableMetaCacheTest.java:
##########
@@ -332,4 +332,102 @@ public void testGetTableMetaOriginalTableNamePreserved()
throws SQLException {
Assertions.assertNotNull(tableMeta);
Assertions.assertEquals(originalName,
tableMeta.getOriginalTableName());
}
+
+ @Test
+ public void testCompositeIndexWithDifferentColumnOrder() throws
SQLException {
+ // Regression test for a bug fix: different column order caused
List.equals to fail, which could lead to
+ // failing to recognize a primary key index.
+ // Scenario: composite primary key (ID, USER_ID) while a unique index
lists the same columns in reverse order
+ // (USER_ID, ID).
+ // Before the fix: List.equals returned FALSE because of the different
order, causing the primary key index to
+ // be unrecognized.
+ // After the fix: using Set.equals should return TRUE and correctly
identify the index as PRIMARY.
+
+ Object[][] compositeIndexDiffOrder = new Object[][] {
+ new Object[] {"idx_id_userid", "id", false, "", 3, 1, "A", 34}, //
column order 1
+ new Object[] {"idx_id_userid", "user_id", false, "", 3, 2, "A",
34} // column order 2
+ };
+ Object[][] compositePKMetas = new Object[][] {
+ new Object[] {"id"}, // PK column order 1
+ new Object[] {"user_id"} // PK column order 2
Review Comment:
The test claims to verify "different column order" (line 340-341 mentions
"reverse order (USER_ID, ID)"), but the actual test data has both the index
columns and PK columns in the same order. The compositeIndexDiffOrder array has
columns in order (id, user_id), and compositePKMetas also has them in order
(id, user_id). To properly test the order-insensitivity fix, one of these
should be reversed, for example compositePKMetas should be {{"user_id"},
{"id"}}.
```suggestion
new Object[] {"user_id"}, // PK column order 1 (intentionally
different from index order)
new Object[] {"id"} // PK column order 2
```
##########
rm-datasource/src/test/java/org/apache/seata/rm/datasource/sql/struct/cache/DmTableMetaCacheTest.java:
##########
@@ -332,4 +332,102 @@ public void testGetTableMetaOriginalTableNamePreserved()
throws SQLException {
Assertions.assertNotNull(tableMeta);
Assertions.assertEquals(originalName,
tableMeta.getOriginalTableName());
}
+
+ @Test
+ public void testCompositeIndexWithDifferentColumnOrder() throws
SQLException {
+ // Regression test for a bug fix: different column order caused
List.equals to fail, which could lead to
+ // failing to recognize a primary key index.
+ // Scenario: composite primary key (ID, USER_ID) while a unique index
lists the same columns in reverse order
+ // (USER_ID, ID).
+ // Before the fix: List.equals returned FALSE because of the different
order, causing the primary key index to
+ // be unrecognized.
+ // After the fix: using Set.equals should return TRUE and correctly
identify the index as PRIMARY.
+
+ Object[][] compositeIndexDiffOrder = new Object[][] {
+ new Object[] {"idx_id_userid", "id", false, "", 3, 1, "A", 34}, //
column order 1
+ new Object[] {"idx_id_userid", "user_id", false, "", 3, 2, "A",
34} // column order 2
+ };
+ Object[][] compositePKMetas = new Object[][] {
+ new Object[] {"id"}, // PK column order 1
+ new Object[] {"user_id"} // PK column order 2
+ };
+ Object[][] compositeColumnMetas = new Object[][] {
+ new Object[] {"", "", "dt2", "id", Types.INTEGER, "INTEGER", 64,
0, 10, 1, "", "", 0, 0, 64, 1, "NO", "YES"
+ },
+ new Object[] {
+ "", "", "dt2", "user_id", Types.INTEGER, "INTEGER", 64, 0, 10,
0, "", "", 0, 0, 64, 2, "YES", "NO"
+ }
+ };
+ Object[][] compositeTableMetas = new Object[][] {new Object[] {"",
"t", "dt2"}};
+
+ MockDriver mockDriver =
+ new MockDriver(compositeColumnMetas, compositeIndexDiffOrder,
compositePKMetas, compositeTableMetas);
+ DruidDataSource dataSource = new DruidDataSource();
+ dataSource.setUrl("jdbc:mock:dm");
+ dataSource.setDriver(mockDriver);
+
+ DataSourceProxy proxy =
DataSourceProxyTest.getDataSourceProxy(dataSource);
+ TableMetaCache tableMetaCache =
TableMetaCacheFactory.getTableMetaCache(JdbcConstants.DM);
+
+ TableMeta tableMeta =
tableMetaCache.getTableMeta(proxy.getPlainConnection(), "t.dt2",
proxy.getResourceId());
+
+ Assertions.assertNotNull(tableMeta);
+ IndexMeta compositeIndex =
tableMeta.getAllIndexes().get("idx_id_userid");
+ Assertions.assertNotNull(compositeIndex);
+ // Key assertion: should be recognized as PRIMARY (after the fix), not
mistakenly treated as UNIQUE
+ Assertions.assertEquals(
+ IndexType.PRIMARY,
+ compositeIndex.getIndextype(),
+ "Composite index with different column order should be
recognized as PRIMARY");
+ }
+
+ @Test
+ public void testCompositeIndexWithExtraColumnsNotMarkedAsPrimary() throws
SQLException {
+ // Regression test for a bug fix: a composite unique index that
contains primary key columns plus extra columns
+ // should NOT be marked as PRIMARY.
+ // Scenario: primary key (ID), unique index (ID, NAME) — unique index
contains PK column but has extra NAME.
+ // Before the fix: Oracle implementation matched by count (matchCols
== pkcol.size()) and could misidentify
Review Comment:
The comment mentions "Oracle implementation" but this is DmTableMetaCache,
not Oracle. The comment should refer to the "previous implementation" or "DM
implementation" to avoid confusion.
```suggestion
// Before the fix: the previous DM implementation matched by count
(matchCols == pkcol.size()) and could misidentify
```
##########
rm-datasource/src/main/java/org/apache/seata/rm/datasource/sql/struct/cache/DmTableMetaCache.java:
##########
@@ -186,4 +214,12 @@ protected IndexMeta toIndexMeta(ResultSet rs, String
indexName, ColumnMeta colum
}
return result;
}
+
+ private static String getStringSafely(ResultSet rs, String columnLabel) {
+ try {
+ return rs.getString(columnLabel);
+ } catch (Exception e) {
Review Comment:
The getStringSafely method catches all exceptions and returns null. This is
overly broad and could hide important issues. Consider catching only
SQLException or specific exceptions that are expected when the column doesn't
exist. Other exceptions (like resource exhaustion or connection issues) should
be propagated.
```suggestion
} catch (SQLException e) {
```
--
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]
