Copilot commented on code in PR #8050: URL: https://github.com/apache/incubator-seata/pull/8050#discussion_r3091884958
########## changes/zh-cn/2.x.md: ########## @@ -30,8 +30,10 @@ - [[#8002](https://github.com/apache/incubator-seata/pull/8002)] 为namingserver指标增加Grafana dashboard JSON - [[#8020](https://github.com/apache/incubator-seata/pull/8020)] 新增 UnregisterRM 协议，在客户端销毁时通知服务端 - [[#8044](https://github.com/apache/incubator-seata/pull/8044)] 为 UnregisterRM 协议添加 protobuf 序列化支持 +- [[#8041](https://github.com/apache/incubator-seata/pull/8041)] 新增SQL Server 多主键支持 - [[#8046](https://github.com/apache/incubator-seata/pull/8046)] 添加了 fastjson2 和 jackson3 + Review Comment: There is an extra blank line introduced after the new #8041 entry, which leaves unnecessary vertical spacing in the rendered changelog. Consider removing the additional empty line to keep formatting consistent with surrounding entries. ```suggestion ``` ########## rm-datasource/src/test/java/org/apache/seata/rm/datasource/exec/SqlServerInsertExecutorTest.java: ########## @@ -209,4 +209,101 @@ private void mockStatementInsertRows() { rows.add(Arrays.asList(Null.get(), "xx", "xx", "xx")); when(sqlInsertRecognizer.getInsertRows(pkIndexMap.values())).thenReturn(rows); } + + @Test + public void testGetPkValues_compositePrimaryKey_withAllPkInInsert() throws Exception { + // Mock composite primary key: id + user_id + List<String> compositePkList = Arrays.asList(ID_COLUMN, USER_ID_COLUMN); + when(tableMeta.getPrimaryKeyOnlyName()).thenReturn(compositePkList); + + doReturn(tableMeta).when(insertExecutor).getTableMeta(); + doReturn(true).when(insertExecutor).containsPK(); // All primary keys are in INSERT statement + + // Mock getPkValuesByColumn to return expected values directly + Map<String, List<Object>> expectedPkValues = new HashMap<>(); + expectedPkValues.put(ID_COLUMN, Arrays.asList(1, 2)); + expectedPkValues.put(USER_ID_COLUMN, Arrays.asList("user1", "user2")); + doReturn(expectedPkValues).when(insertExecutor).getPkValuesByColumn(); + + Map<String, List<Object>> pkValues = insertExecutor.getPkValues(); + + // Verify composite primary key values are correctly retrieved + Assertions.assertNotNull(pkValues); + Assertions.assertEquals(expectedPkValues, pkValues); + + // Verify that getPkValuesByColumn was called, confirming the code path for composite keys with manual values + verify(insertExecutor).getPkValuesByColumn(); + } + + @Test + public void testGetPkValues_compositePrimaryKey_withAutoIncrement() throws Exception { + // Mock composite primary key with auto-increment columns + List<String> compositePkList = Arrays.asList(ID_COLUMN, USER_ID_COLUMN); + when(tableMeta.getPrimaryKeyOnlyName()).thenReturn(compositePkList); + + Map<String, ColumnMeta> pkMap = new HashMap<>(); + ColumnMeta idMeta = mock(ColumnMeta.class); + when(idMeta.isAutoincrement()).thenReturn(true); // Auto-increment + pkMap.put(ID_COLUMN, idMeta); + + ColumnMeta userIdMeta = mock(ColumnMeta.class); + when(userIdMeta.isAutoincrement()).thenReturn(true); // Auto-increment + pkMap.put(USER_ID_COLUMN, userIdMeta); + when(tableMeta.getPrimaryKeyMap()).thenReturn(pkMap); + + doReturn(tableMeta).when(insertExecutor).getTableMeta(); + doReturn(false).when(insertExecutor).containsPK(); // Primary keys not in INSERT + doReturn(Arrays.asList(PK_VALUE)).when(insertExecutor).getGeneratedKeys(); + + Map<String, List<Object>> pkValues = insertExecutor.getPkValues(); + + // Verify auto-increment column gets value from generated keys + Assertions.assertEquals(Arrays.asList(PK_VALUE), pkValues.get(ID_COLUMN)); + Assertions.assertEquals(Arrays.asList(PK_VALUE), pkValues.get(USER_ID_COLUMN)); + } + + @Test Review Comment: The new composite-PK tests cover (1) all PK columns provided and (2) all PK columns marked autoincrement, but they don't cover the common mixed case where some PK columns are provided in the INSERT and the remaining PK column is identity/auto-generated. Adding a test for that scenario would catch the current behavior where `getPkValues()` throws instead of merging manual + generated PK values. ```suggestion @Test public void testGetPkValues_compositePrimaryKey_withPartialPkInInsertAndAutoIncrement() throws Exception { // Mock composite primary key where one PK is provided in INSERT and the other is auto-increment List<String> compositePkList = Arrays.asList(ID_COLUMN, USER_ID_COLUMN); when(tableMeta.getPrimaryKeyOnlyName()).thenReturn(compositePkList); Map<String, ColumnMeta> pkMap = new HashMap<>(); ColumnMeta idMeta = mock(ColumnMeta.class); when(idMeta.isAutoincrement()).thenReturn(false); // Provided in INSERT pkMap.put(ID_COLUMN, idMeta); ColumnMeta userIdMeta = mock(ColumnMeta.class); when(userIdMeta.isAutoincrement()).thenReturn(true); // Generated by database pkMap.put(USER_ID_COLUMN, userIdMeta); when(tableMeta.getPrimaryKeyMap()).thenReturn(pkMap); mockParametersForCompositePk(); doReturn(tableMeta).when(insertExecutor).getTableMeta(); doReturn(true).when(insertExecutor).containsPK(); // One primary key column is present in INSERT doReturn(Arrays.asList(PK_VALUE)).when(insertExecutor).getGeneratedKeys(); Map<String, List<Object>> pkValues = insertExecutor.getPkValues(); // Verify manual and generated PK values are merged correctly Assertions.assertEquals(Arrays.asList(1), pkValues.get(ID_COLUMN)); Assertions.assertEquals(Arrays.asList(PK_VALUE), pkValues.get(USER_ID_COLUMN)); } @Test ``` ########## rm-datasource/src/main/java/org/apache/seata/rm/datasource/SqlGenerateUtils.java: ########## @@ -78,6 +79,11 @@ public static List<WhereSql> buildWhereConditionListByPKs(List<String> pkNameLis */ public static List<WhereSql> buildWhereConditionListByPKs( List<String> pkNameList, int rowSize, String dbType, int maxInSize) { + // SQL Server does not support tuple IN syntax: (col1,col2) IN ((?,?),(?,?)) + // Use AND/OR syntax instead + if (JdbcConstants.SQLSERVER.equalsIgnoreCase(dbType)) { Review Comment: The SQL Server special-case in `buildWhereConditionListByPKs` triggers for all PK sizes, including single-column PKs. This changes SQL Server WHERE generation from the existing tuple-`IN` form to an `OR` chain (e.g. `(id=?) OR (id=?)`), which is longer and can be less efficient for large batches. If the limitation only applies to composite PKs, gate this branch on `pkNameList.size() > 1` and let the existing implementation handle single-column PKs. ```suggestion // Use AND/OR syntax instead for composite primary keys if (JdbcConstants.SQLSERVER.equalsIgnoreCase(dbType) && pkNameList.size() > 1) { ``` ########## rm-datasource/src/test/java/org/apache/seata/rm/datasource/exec/SqlServerInsertExecutorTest.java: ########## @@ -209,4 +209,101 @@ private void mockStatementInsertRows() { rows.add(Arrays.asList(Null.get(), "xx", "xx", "xx")); when(sqlInsertRecognizer.getInsertRows(pkIndexMap.values())).thenReturn(rows); } + + @Test + public void testGetPkValues_compositePrimaryKey_withAllPkInInsert() throws Exception { + // Mock composite primary key: id + user_id + List<String> compositePkList = Arrays.asList(ID_COLUMN, USER_ID_COLUMN); + when(tableMeta.getPrimaryKeyOnlyName()).thenReturn(compositePkList); + + doReturn(tableMeta).when(insertExecutor).getTableMeta(); + doReturn(true).when(insertExecutor).containsPK(); // All primary keys are in INSERT statement + + // Mock getPkValuesByColumn to return expected values directly + Map<String, List<Object>> expectedPkValues = new HashMap<>(); + expectedPkValues.put(ID_COLUMN, Arrays.asList(1, 2)); + expectedPkValues.put(USER_ID_COLUMN, Arrays.asList("user1", "user2")); + doReturn(expectedPkValues).when(insertExecutor).getPkValuesByColumn(); + + Map<String, List<Object>> pkValues = insertExecutor.getPkValues(); + + // Verify composite primary key values are correctly retrieved + Assertions.assertNotNull(pkValues); + Assertions.assertEquals(expectedPkValues, pkValues); + + // Verify that getPkValuesByColumn was called, confirming the code path for composite keys with manual values + verify(insertExecutor).getPkValuesByColumn(); + } + + @Test + public void testGetPkValues_compositePrimaryKey_withAutoIncrement() throws Exception { + // Mock composite primary key with auto-increment columns + List<String> compositePkList = Arrays.asList(ID_COLUMN, USER_ID_COLUMN); + when(tableMeta.getPrimaryKeyOnlyName()).thenReturn(compositePkList); + + Map<String, ColumnMeta> pkMap = new HashMap<>(); + ColumnMeta idMeta = mock(ColumnMeta.class); + when(idMeta.isAutoincrement()).thenReturn(true); // Auto-increment + pkMap.put(ID_COLUMN, idMeta); + + ColumnMeta userIdMeta = mock(ColumnMeta.class); + when(userIdMeta.isAutoincrement()).thenReturn(true); // Auto-increment + pkMap.put(USER_ID_COLUMN, userIdMeta); + when(tableMeta.getPrimaryKeyMap()).thenReturn(pkMap); + + doReturn(tableMeta).when(insertExecutor).getTableMeta(); + doReturn(false).when(insertExecutor).containsPK(); // Primary keys not in INSERT + doReturn(Arrays.asList(PK_VALUE)).when(insertExecutor).getGeneratedKeys(); + + Map<String, List<Object>> pkValues = insertExecutor.getPkValues(); + + // Verify auto-increment column gets value from generated keys + Assertions.assertEquals(Arrays.asList(PK_VALUE), pkValues.get(ID_COLUMN)); + Assertions.assertEquals(Arrays.asList(PK_VALUE), pkValues.get(USER_ID_COLUMN)); + } + + @Test + public void testGetPkValues_compositePrimaryKey_nonAutoIncrementThrowsException() throws Exception { + // Mock composite primary key with non-auto-increment column not in INSERT + List<String> compositePkList = Arrays.asList(ID_COLUMN, USER_ID_COLUMN); + when(tableMeta.getPrimaryKeyOnlyName()).thenReturn(compositePkList); + + Map<String, ColumnMeta> pkMap = new HashMap<>(); + ColumnMeta idMeta = mock(ColumnMeta.class); + when(idMeta.isAutoincrement()).thenReturn(false); // Not auto-increment + pkMap.put(ID_COLUMN, idMeta); + + ColumnMeta userIdMeta = mock(ColumnMeta.class); + when(userIdMeta.isAutoincrement()).thenReturn(false); + pkMap.put(USER_ID_COLUMN, userIdMeta); + when(tableMeta.getPrimaryKeyMap()).thenReturn(pkMap); + + doReturn(tableMeta).when(insertExecutor).getTableMeta(); + doReturn(false).when(insertExecutor).containsPK(); // Primary keys not in INSERT + + // Should throw exception for non-auto-increment composite primary key + Assertions.assertThrows(NotSupportYetException.class, () -> insertExecutor.getPkValues()); + } + + private void mockParametersForCompositePk() { + Map<Integer, ArrayList<Object>> parameters = new HashMap<>(4); + ArrayList<Object> arrayList0 = new ArrayList<>(); + arrayList0.add(1); // id value + ArrayList<Object> arrayList1 = new ArrayList<>(); + arrayList1.add("userId1"); + ArrayList<Object> arrayList2 = new ArrayList<>(); + arrayList2.add("userName1"); + ArrayList<Object> arrayList3 = new ArrayList<>(); + arrayList3.add("userStatus1"); + parameters.put(1, arrayList0); + parameters.put(2, arrayList1); + parameters.put(3, arrayList2); + parameters.put(4, arrayList3); + PreparedStatementProxy psp = (PreparedStatementProxy) this.statementProxy; + when(psp.getParameters()).thenReturn(parameters); + + List<List<Object>> rows = new ArrayList<>(); + rows.add(Arrays.asList("?", "?", "?", "?")); + when(sqlInsertRecognizer.getInsertRows(pkIndexMap.values())).thenReturn(rows); + } Review Comment: `mockParametersForCompositePk()` is added but never called in this test class. Unused helpers make the test harder to maintain; either use it in the composite PK tests or remove it. ```suggestion ``` ########## rm-datasource/src/main/java/org/apache/seata/rm/datasource/exec/sqlserver/SqlServerInsertExecutor.java: ########## @@ -84,8 +86,29 @@ public Map<String, List<Object>> getPkValues() throws SQLException { pkValuesMap = getPkValuesWithNoColumn(); } } else { - // when there is a composite primary key - throw new NotSupportYetException("composite primary key is not supported in sqlserver"); + // when there is a composite primary key - Fix: Support SQL Server composite primary keys. + if (isContainsPk) { + // All primary key columns are manually assigned in the INSERT statement. + pkValuesMap = getPkValuesByColumn(); + } else { + // Some or all primary key columns are auto-generated. + // Get metadata for all primary key columns. + Map<String, ColumnMeta> primaryKeyMap = getTableMeta().getPrimaryKeyMap(); + + // Iterate over each primary key column. + for (String pkColumnName : pkColumnNameList) { + ColumnMeta pkMeta = primaryKeyMap.get(pkColumnName); + if (pkMeta.isAutoincrement()) { + // Auto-increment column: get from generated keys. + pkValuesMap.put(pkColumnName, getGeneratedKeys()); + } else { + // Non-auto-increment column: requires manual assignment, throw exception if not supported. + throw new NotSupportYetException( + "composite primary key with non-autoincrement column is not supported in sqlserver: " + + pkColumnName); + } + } Review Comment: `getPkValues()` for SQL Server composite PKs incorrectly assumes that when `containsPK()` is false, *none* of the PK columns are provided. For a common composite PK case where one PK column is provided in the INSERT and another is identity/auto-generated, this branch will throw for the provided non-autoincrement column instead of merging manual values with generated keys. Consider following the MySQLInsertExecutor pattern: start from `getPkValuesByColumn()` (for any PKs present) and then fill any missing PK columns that are autoincrement from `getGeneratedKeys()`; only throw if a required PK column is still missing and not autoincrement. ########## changes/en-us/2.x.md: ########## @@ -29,8 +29,10 @@ Add changes here for all PR submitted to the 2.x branch. - [[#8002](https://github.com/apache/incubator-seata/pull/8002)] add Grafana dashboard JSON for NamingServer metrics - [[#8020](https://github.com/apache/incubator-seata/pull/8020)] add UnregisterRM protocol to notify server on client destroy - [[#8044](https://github.com/apache/incubator-seata/pull/8044)] add protobuf serialization support for UnregisterRM protocol +- [[#8041](https://github.com/apache/incubator-seata/pull/8041)] add SQL Server composite primary keys - [[#8046](https://github.com/apache/incubator-seata/pull/8046)] add fastjson2 and jackson3 + Review Comment: There is an extra blank line introduced after the new #8041 entry, which leaves an empty list item spacing in the rendered changelog. Consider removing the additional empty line to keep formatting consistent with surrounding entries. ```suggestion ``` ########## rm-datasource/src/main/java/org/apache/seata/rm/datasource/AbstractConnectionProxy.java: ########## @@ -121,7 +122,14 @@ public PreparedStatement prepareStatement(String sql) throws SQLException { String[] pkNameArray = new String[tableMeta.getPrimaryKeyOnlyName().size()]; tableMeta.getPrimaryKeyOnlyName().toArray(pkNameArray); - targetPreparedStatement = getTargetConnection().prepareStatement(sql, pkNameArray); + // Fix: SQL Server does not support array of column names for getGeneratedKeys, use + // RETURN_GENERATED_KEYS instead. + if (JdbcConstants.SQLSERVER.equalsIgnoreCase(dbType)) { + targetPreparedStatement = + getTargetConnection().prepareStatement(sql, Statement.RETURN_GENERATED_KEYS); + } else { + targetPreparedStatement = getTargetConnection().prepareStatement(sql, pkNameArray); + } Review Comment: `pkNameArray` is built unconditionally, but for SQL Server the code path now ignores it and uses `Statement.RETURN_GENERATED_KEYS`. Consider moving the `pkNameArray` allocation/population into the non-SQLServer branch to avoid unnecessary work on SQL Server inserts. -- 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]
