Copilot commented on code in PR #8050:
URL: https://github.com/apache/incubator-seata/pull/8050#discussion_r3107830022
##########
changes/en-us/2.x.md:
##########
@@ -1,4 +1,4 @@
-<!--
+<!--
Licensed to the Apache Software Foundation (ASF) under one or more
contributor license agreements. See the NOTICE file distributed with
Review Comment:
The very first character on this file appears to be a UTF-8 BOM / invisible
character (line starts with "\uFEFF<!--"). This can create noisy diffs and
occasionally breaks tooling; please remove the BOM so the file starts with a
plain "<!--" like the other changelog files.
##########
rm-datasource/src/main/java/org/apache/seata/rm/datasource/SqlGenerateUtils.java:
##########
@@ -115,6 +121,45 @@ public static List<WhereSql> buildWhereConditionListByPKs(
return whereSqls;
}
+ /**
+ * Build where condition list by PKs for SQL Server.
+ * SQL Server does not support tuple IN syntax: (col1,col2) IN
((?,?),(?,?))
+ * Use AND/OR syntax instead: (col1=? AND col2=?) OR (col1=? AND col2=?)
+ *
+ * @param pkNameList pk column name list
+ * @param rowSize the row size of records
+ * @param maxInSize the max in size
+ * @return where condition sql list for SQL Server
+ */
+ private static List<WhereSql> buildWhereConditionListByPKsForSqlServer(
+ List<String> pkNameList, int rowSize, int maxInSize, String
dbType) {
+ List<WhereSql> whereSqls = new ArrayList<>();
Review Comment:
Javadoc for buildWhereConditionListByPKsForSqlServer() is missing an
`@param` entry for the dbType argument, even though it is part of the signature
and used for escaping. Please either document dbType or remove it from the
signature (since this method is SQL Server-specific).
##########
changes/zh-cn/2.x.md:
##########
@@ -31,6 +31,7 @@
- [[#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 序列化支持
+- [[#8050](https://github.com/apache/incubator-seata/pull/8050)] 新增SQL Server
多主键支持
- [[#8046](https://github.com/apache/incubator-seata/pull/8046)] 添加了 fastjson2
和 jackson3
Review Comment:
The changelog entry links to PR #8050, but this PR/issue context is #8041.
Please update the PR number/link in the changelog entry so it points to the
correct pull request.
##########
changes/en-us/2.x.md:
##########
@@ -30,6 +30,7 @@ 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
+- [[#8050](https://github.com/apache/incubator-seata/pull/8050)] add SQL
Server composite primary keys
- [[#8046](https://github.com/apache/incubator-seata/pull/8046)] add fastjson2
and jackson3
Review Comment:
The changelog entry links to PR #8050, but this PR/issue context is #8041.
Please update the PR number/link in the changelog entry so it points to the
correct pull request.
##########
rm-datasource/src/test/java/org/apache/seata/rm/datasource/exec/SqlServerInsertExecutorTest.java:
##########
@@ -209,4 +209,114 @@ 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(pkIndexMap).when(insertExecutor).getPkIndex(); // PK columns
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(new HashMap<String,
Integer>()).when(insertExecutor).getPkIndex(); // No PK columns 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));
Review Comment:
This test models a composite PK where *both* columns are autoincrement and
expects the same generated key to be applied to both. SQL Server allows only
one IDENTITY column per table (and the production code comment states that), so
this scenario is not valid and could mask real behavior. Please adjust the test
to a realistic composite-PK case (e.g., one provided PK + one IDENTITY, which
you already test) or assert that multiple autoincrement PK columns are rejected.
```suggestion
public void
testGetPkValues_compositePrimaryKey_withOneAutoIncrementAndOnePkInInsert()
throws Exception {
// Mock realistic SQL Server composite primary key: one IDENTITY
column + one provided PK column
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);
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();
Map<String, Integer> pkIndex = new HashMap<>();
pkIndex.put(USER_ID_COLUMN, 0);
doReturn(pkIndex).when(insertExecutor).getPkIndex();
Map<String, List<Object>> pkValuesByColumn = new HashMap<>();
pkValuesByColumn.put(USER_ID_COLUMN, Arrays.asList("user1"));
doReturn(pkValuesByColumn).when(insertExecutor).getPkValuesByColumn();
doReturn(Arrays.asList(PK_VALUE)).when(insertExecutor).getGeneratedKeys();
Map<String, List<Object>> pkValues = insertExecutor.getPkValues();
Assertions.assertEquals(Arrays.asList(PK_VALUE),
pkValues.get(ID_COLUMN));
Assertions.assertEquals(Arrays.asList("user1"),
pkValues.get(USER_ID_COLUMN));
```
##########
rm-datasource/src/main/java/org/apache/seata/rm/datasource/exec/sqlserver/SqlServerInsertExecutor.java:
##########
@@ -84,8 +86,44 @@ 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.
+ // SQL Server allows only one IDENTITY column per table.
+ // So composite PK can have at most one auto-increment column.
+ // Strategy: parse PK values from INSERT columns, then fill
missing auto-increment PK from generated keys.
+ if (!getPkIndex().isEmpty()) {
+ // At least one PK column is in the INSERT statement.
+ pkValuesMap = getPkValuesByColumn();
+ Map<String, ColumnMeta> primaryKeyMap =
getTableMeta().getPrimaryKeyMap();
+
+ // Fill any missing auto-increment PK columns from generated
keys.
+ for (String pkColumnName : pkColumnNameList) {
+ if (!pkValuesMap.containsKey(pkColumnName)) {
+ ColumnMeta pkMeta = primaryKeyMap.get(pkColumnName);
+ if (pkMeta.isAutoincrement()) {
+ pkValuesMap.put(pkColumnName, getGeneratedKeys());
+ } else {
+ throw new NotSupportYetException(
+ "composite primary key with
non-autoincrement column not in INSERT is not supported in sqlserver: "
+ + pkColumnName);
+ }
+ }
+ }
+ } else {
+ // No PK columns in INSERT statement.
+ // For composite PK, this means all PK columns must have
values from elsewhere.
+ // Since SQL Server only supports one IDENTITY column,
non-identity PK columns would fail.
+ Map<String, ColumnMeta> primaryKeyMap =
getTableMeta().getPrimaryKeyMap();
+ for (String pkColumnName : pkColumnNameList) {
+ ColumnMeta pkMeta = primaryKeyMap.get(pkColumnName);
+ if (pkMeta.isAutoincrement()) {
+ pkValuesMap.put(pkColumnName, getGeneratedKeys());
+ } else {
Review Comment:
In composite-PK branches, this calls getGeneratedKeys() inside a loop and
may invoke it multiple times (e.g., when more than one PK column is missing or
marked autoincrement). SqlServerInsertExecutor.getGeneratedKeys() consumes the
generated-keys ResultSet and relies on beforeFirst(), which may fail on some
drivers; repeated calls can therefore return empty and throw
NotSupportYetException or produce inconsistent results. Consider fetching
generated keys once and reusing the list, and/or explicitly enforcing that at
most one PK column can be autoincrement (otherwise fail fast).
--
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]
