Copilot commented on code in PR #8028:
URL: https://github.com/apache/incubator-seata/pull/8028#discussion_r2999092178
##########
rm-datasource/src/test/java/org/apache/seata/rm/datasource/exec/MySQLInsertExecutorTest.java:
##########
@@ -881,4 +881,143 @@ 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);
Review Comment:
These new tests cover the hidden-parameter case where the PK placeholder is
the first column and the `SqlMethodExpr` is the last column. To prevent
regressions of the same bug when the PK column is not first, add a test where a
`SqlMethodExpr` column appears before the PK placeholder in the VALUES tuple
(so hidden `?`s precede the PK in JDBC parameter order).
##########
rm-datasource/src/main/java/org/apache/seata/rm/datasource/exec/BaseInsertExecutor.java:
##########
@@ -196,6 +213,9 @@ protected Map<String, List<Object>>
parsePkValuesFromStatement() {
pkValuesMap.put(ColumnUtils.delEscape(pkKey,
getDbType()), pkValues);
}
}
+ // Adjust totalPlaceholderNum to account for hidden
parameters inside
+ // function expressions, so the next row's parameter
offset is correct.
+ totalPlaceholderNum += hiddenParamsPerRow;
Review Comment:
`hiddenParamsPerRow` is only applied by incrementing `totalPlaceholderNum`
after finishing the row. If a function expression (represented by
`SqlMethodExpr`) appears *before* the PK placeholder within the same VALUES
tuple (i.e., PK column is not the first placeholder), the PK parameter index
calculation will still ignore the hidden `?`s inside that function and extract
the wrong PK. Consider accounting for hidden placeholders that occur before the
PK within the current row (e.g., by having the recognizer expose/count
placeholders inside `SQLMethodInvokeExpr`, or by including that count in the
row structure) rather than only adjusting the offset between rows.
--
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]
