Copilot commented on code in PR #8028:
URL: https://github.com/apache/incubator-seata/pull/8028#discussion_r3062632179
##########
rm-datasource/src/test/java/org/apache/seata/rm/datasource/exec/MySQLInsertExecutorTest.java:
##########
@@ -881,4 +881,209 @@ private void mockInsertRows() {
rows.add(Arrays.asList("?", "?", "?", "?"));
when(sqlInsertRecognizer.getInsertRows(pkIndexMap.values())).thenReturn(rows);
}
+
+ /**
+ * Test that batch INSERT with function expressions containing hidden JDBC
parameters
+ * correctly extracts PK values for all rows.
+ *
+ * Simulates: INSERT INTO t(id, name, location) VALUES
+ * (?, ?, ST_GeomFromText(CONCAT('POINT(', ?, ' ', ?, ')'))),
+ * (?, ?, ST_GeomFromText(CONCAT('POINT(', ?, ' ', ?, ')'))),
+ * (?, ?, ST_GeomFromText(CONCAT('POINT(', ?, ' ', ?, ')')))
+ *
+ * Each row has 2 visible placeholders + 2 hidden placeholders inside the
function = 4 actual JDBC params per row.
+ * See: https://github.com/apache/incubator-seata/issues/6941
+ */
+ @Test
+ public void testGetPkValuesByColumn_BatchInsertWithFunctionExpression()
throws SQLException {
Review Comment:
The PR description says 'Added 3 test cases', but this diff adds 4 new
`@Test` methods (batch with function, batch without function, batch with
function-before-PK, and single-row with function). Please update the PR
description to match the actual test additions.
##########
rm-datasource/src/test/java/org/apache/seata/rm/datasource/exec/MySQLInsertExecutorTest.java:
##########
@@ -881,4 +881,209 @@ private void mockInsertRows() {
rows.add(Arrays.asList("?", "?", "?", "?"));
when(sqlInsertRecognizer.getInsertRows(pkIndexMap.values())).thenReturn(rows);
}
+
+ /**
+ * Test that batch INSERT with function expressions containing hidden JDBC
parameters
+ * correctly extracts PK values for all rows.
+ *
+ * Simulates: INSERT INTO t(id, name, location) VALUES
+ * (?, ?, ST_GeomFromText(CONCAT('POINT(', ?, ' ', ?, ')'))),
+ * (?, ?, ST_GeomFromText(CONCAT('POINT(', ?, ' ', ?, ')'))),
+ * (?, ?, ST_GeomFromText(CONCAT('POINT(', ?, ' ', ?, ')')))
+ *
+ * Each row has 2 visible placeholders + 2 hidden placeholders inside the
function = 4 actual JDBC params per row.
+ * See: https://github.com/apache/incubator-seata/issues/6941
+ */
+ @Test
+ public void testGetPkValuesByColumn_BatchInsertWithFunctionExpression()
throws SQLException {
+ // Setup: 3 columns (id, name, location), PK is id at index 0
+ List<String> columns = new ArrayList<>();
+ columns.add(ID_COLUMN);
+ columns.add(USER_NAME_COLUMN);
+ columns.add("location");
+ when(sqlInsertRecognizer.getInsertColumns()).thenReturn(columns);
Review Comment:
The newly added tests replicate a lot of setup boilerplate
(columns/rows/parameters, stubbing `getTableMeta()`/`getPkIndex()`, and PK
assertions). Consider extracting small private helpers (e.g.,
`stubInsertColumns(...)`, `stubInsertRows(...)`, `stubParameters(...)`,
`assertPkValues(...)`) to reduce duplication and make it easier to add future
INSERT-shape cases.
##########
rm-datasource/src/test/java/org/apache/seata/rm/datasource/exec/MySQLInsertExecutorTest.java:
##########
@@ -881,4 +881,209 @@ private void mockInsertRows() {
rows.add(Arrays.asList("?", "?", "?", "?"));
when(sqlInsertRecognizer.getInsertRows(pkIndexMap.values())).thenReturn(rows);
}
+
+ /**
+ * Test that batch INSERT with function expressions containing hidden JDBC
parameters
+ * correctly extracts PK values for all rows.
+ *
+ * Simulates: INSERT INTO t(id, name, location) VALUES
+ * (?, ?, ST_GeomFromText(CONCAT('POINT(', ?, ' ', ?, ')'))),
+ * (?, ?, ST_GeomFromText(CONCAT('POINT(', ?, ' ', ?, ')'))),
+ * (?, ?, ST_GeomFromText(CONCAT('POINT(', ?, ' ', ?, ')')))
+ *
+ * Each row has 2 visible placeholders + 2 hidden placeholders inside the
function = 4 actual JDBC params per row.
+ * See: https://github.com/apache/incubator-seata/issues/6941
+ */
+ @Test
+ public void testGetPkValuesByColumn_BatchInsertWithFunctionExpression()
throws SQLException {
+ // Setup: 3 columns (id, name, location), PK is id at index 0
+ List<String> columns = new ArrayList<>();
+ columns.add(ID_COLUMN);
+ columns.add(USER_NAME_COLUMN);
+ columns.add("location");
+ when(sqlInsertRecognizer.getInsertColumns()).thenReturn(columns);
+
+ // Row structure: ["?", "?", SqlMethodExpr] — function hides 2 extra
params
+ List<List<Object>> rows = new ArrayList<>();
+ rows.add(Arrays.asList("?", "?", SqlMethodExpr.get()));
+ rows.add(Arrays.asList("?", "?", SqlMethodExpr.get()));
+ rows.add(Arrays.asList("?", "?", SqlMethodExpr.get()));
+
when(sqlInsertRecognizer.getInsertRows(pkIndexMap.values())).thenReturn(rows);
+
+ // JDBC parameters: 4 per row (id, name, x, y), 12 total
+ Map<Integer, ArrayList<Object>> parameters = new HashMap<>(12);
+ // Row 1: id=1, name="name1", x=10.0, y=20.0
+ parameters.put(1, new ArrayList<>(Arrays.asList(1)));
+ parameters.put(2, new ArrayList<>(Arrays.asList("name1")));
+ parameters.put(3, new ArrayList<>(Arrays.asList(10.0)));
+ parameters.put(4, new ArrayList<>(Arrays.asList(20.0)));
+ // Row 2: id=2, name="name2", x=30.0, y=40.0
+ parameters.put(5, new ArrayList<>(Arrays.asList(2)));
+ parameters.put(6, new ArrayList<>(Arrays.asList("name2")));
+ parameters.put(7, new ArrayList<>(Arrays.asList(30.0)));
+ parameters.put(8, new ArrayList<>(Arrays.asList(40.0)));
+ // Row 3: id=3, name="name3", x=50.0, y=60.0
+ parameters.put(9, new ArrayList<>(Arrays.asList(3)));
+ parameters.put(10, new ArrayList<>(Arrays.asList("name3")));
+ parameters.put(11, new ArrayList<>(Arrays.asList(50.0)));
+ parameters.put(12, new ArrayList<>(Arrays.asList(60.0)));
+ PreparedStatementProxy psp = (PreparedStatementProxy)
this.statementProxy;
+ when(psp.getParameters()).thenReturn(parameters);
+
+ doReturn(tableMeta).when(insertExecutor).getTableMeta();
+ when(tableMeta.getPrimaryKeyOnlyName()).thenReturn(Arrays.asList(new
String[] {ID_COLUMN}));
+ doReturn(pkIndexMap).when(insertExecutor).getPkIndex();
+
+ Map<String, List<Object>> pkValuesList =
insertExecutor.getPkValuesByColumn();
+ List<Object> idValues = pkValuesList.get(ID_COLUMN);
+
+ Assertions.assertNotNull(idValues);
+ Assertions.assertEquals(3, idValues.size());
+ Assertions.assertEquals(1, idValues.get(0));
+ Assertions.assertEquals(2, idValues.get(1));
+ Assertions.assertEquals(3, idValues.get(2));
Review Comment:
The newly added tests replicate a lot of setup boilerplate
(columns/rows/parameters, stubbing `getTableMeta()`/`getPkIndex()`, and PK
assertions). Consider extracting small private helpers (e.g.,
`stubInsertColumns(...)`, `stubInsertRows(...)`, `stubParameters(...)`,
`assertPkValues(...)`) to reduce duplication and make it easier to add future
INSERT-shape cases.
```suggestion
private void stubInsertColumns(String... columns) {
when(sqlInsertRecognizer.getInsertColumns()).thenReturn(Arrays.asList(columns));
}
private void stubInsertRows(List<List<Object>> rows) {
when(sqlInsertRecognizer.getInsertRows(pkIndexMap.values())).thenReturn(rows);
}
private void stubParameters(Object... parameterValues) {
Map<Integer, ArrayList<Object>> parameters = new
HashMap<>(parameterValues.length);
for (int i = 0; i < parameterValues.length; i++) {
parameters.put(i + 1, new
ArrayList<>(Arrays.asList(parameterValues[i])));
}
PreparedStatementProxy psp = (PreparedStatementProxy)
this.statementProxy;
when(psp.getParameters()).thenReturn(parameters);
}
private void stubPkMetadata() throws SQLException {
doReturn(tableMeta).when(insertExecutor).getTableMeta();
when(tableMeta.getPrimaryKeyOnlyName()).thenReturn(Arrays.asList(new
String[] {ID_COLUMN}));
doReturn(pkIndexMap).when(insertExecutor).getPkIndex();
}
private void assertPkValues(Map<String, List<Object>> pkValuesList,
String pkColumn, Object... expectedValues) {
List<Object> actualValues = pkValuesList.get(pkColumn);
Assertions.assertNotNull(actualValues);
Assertions.assertEquals(expectedValues.length, actualValues.size());
for (int i = 0; i < expectedValues.length; i++) {
Assertions.assertEquals(expectedValues[i], actualValues.get(i));
}
}
public void testGetPkValuesByColumn_BatchInsertWithFunctionExpression()
throws SQLException {
stubInsertColumns(ID_COLUMN, USER_NAME_COLUMN, "location");
List<List<Object>> rows = new ArrayList<>();
rows.add(Arrays.asList("?", "?", SqlMethodExpr.get()));
rows.add(Arrays.asList("?", "?", SqlMethodExpr.get()));
rows.add(Arrays.asList("?", "?", SqlMethodExpr.get()));
stubInsertRows(rows);
stubParameters(
1, "name1", 10.0, 20.0,
2, "name2", 30.0, 40.0,
3, "name3", 50.0, 60.0
);
stubPkMetadata();
Map<String, List<Object>> pkValuesList =
insertExecutor.getPkValuesByColumn();
assertPkValues(pkValuesList, ID_COLUMN, 1, 2, 3);
```
##########
rm-datasource/src/main/java/org/apache/seata/rm/datasource/exec/BaseInsertExecutor.java:
##########
@@ -151,6 +151,16 @@ protected Map<String, List<Object>>
parsePkValuesFromStatement() {
Map<Integer, ArrayList<Object>> parameters =
preparedStatementProxy.getParameters();
final int rowSize = insertRows.size();
int totalPlaceholderNum = -1;
+ // Calculate the number of hidden JDBC parameters per row
caused by function
+ // expressions (e.g. ST_GeomFromText(CONCAT('POINT(', ?, ' ',
?, ')'))) that
+ // contain parameter placeholders not visible in the parsed
row structure.
+ int nonEmptyRowCount = 0;
+ for (List<Object> r : insertRows) {
+ if (!r.isEmpty()) {
+ nonEmptyRowCount++;
+ }
+ }
+ int hiddenParamsPerRow = 0;
for (List<Object> row : insertRows) {
// oracle insert sql statement specify
RETURN_GENERATED_KEYS will append :rowid on sql end
// insert parameter count will than the actual +1
Review Comment:
The inferred `actualParamsPerRow = parameters.size() / nonEmptyRowCount` can
be wrong when `parameters.size()` includes non-row parameters (the comment
above already calls out Oracle `RETURN_GENERATED_KEYS` adding an extra
binding). This is especially problematic when `nonEmptyRowCount == 1`, where
the extra binding is not eliminated by integer division and can incorrectly set
`hiddenParamsPerRow`, shifting PK indices. A safer approach is to account for a
remainder explicitly (e.g., when `insertRows` contains an appended empty row
and `parameters.size() % nonEmptyRowCount == 1`, compute `(parameters.size() -
1) / nonEmptyRowCount`), and/or only derive hidden params when the
row-parameter total cleanly matches the row count.
##########
rm-datasource/src/main/java/org/apache/seata/rm/datasource/exec/BaseInsertExecutor.java:
##########
@@ -164,6 +174,13 @@ protected Map<String, List<Object>>
parsePkValuesFromStatement() {
currentRowPlaceholderNum += 1;
}
}
+ if (hiddenParamsPerRow == 0 && nonEmptyRowCount > 0) {
+ int visiblePlaceholdersPerRow =
currentRowPlaceholderNum + 1;
+ int actualParamsPerRow = parameters.size() /
nonEmptyRowCount;
+ if (actualParamsPerRow > visiblePlaceholdersPerRow) {
+ hiddenParamsPerRow = actualParamsPerRow -
visiblePlaceholdersPerRow;
+ }
+ }
Review Comment:
The inferred `actualParamsPerRow = parameters.size() / nonEmptyRowCount` can
be wrong when `parameters.size()` includes non-row parameters (the comment
above already calls out Oracle `RETURN_GENERATED_KEYS` adding an extra
binding). This is especially problematic when `nonEmptyRowCount == 1`, where
the extra binding is not eliminated by integer division and can incorrectly set
`hiddenParamsPerRow`, shifting PK indices. A safer approach is to account for a
remainder explicitly (e.g., when `insertRows` contains an appended empty row
and `parameters.size() % nonEmptyRowCount == 1`, compute `(parameters.size() -
1) / nonEmptyRowCount`), and/or only derive hidden params when the
row-parameter total cleanly matches the row count.
--
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]
