Copilot commented on code in PR #7664:
URL: https://github.com/apache/incubator-seata/pull/7664#discussion_r2390306968
##########
sqlparser/seata-sqlparser-druid/src/test/java/org/apache/seata/sqlparser/druid/oscar/OscarInsertRecognizerTest.java:
##########
@@ -99,18 +96,17 @@ public void testGetInsertRows() {
final int pkIndex = 0;
// test for null value
String sql = "insert into t(id, no, name, age, time) values
(id_seq.nextval, null, 'a', ?, now())";
- List<SQLStatement> asts = SQLUtils.parseStatements(sql, DB_TYPE);
+ SQLStatement sqlStatement = getSQLStatement(sql);
- OscarInsertRecognizer recognizer = new OscarInsertRecognizer(sql,
asts.get(0));
+ OscarInsertRecognizer recognizer = new OscarInsertRecognizer(sql,
sqlStatement);
List<List<Object>> insertRows =
recognizer.getInsertRows(Collections.singletonList(pkIndex));
Assertions.assertEquals(1, insertRows.size());
// test for exception
Assertions.assertThrows(SQLParsingException.class, () -> {
String s = "insert into t(a) values (?)";
- List<SQLStatement> sqlStatements = SQLUtils.parseStatements(s,
DB_TYPE);
- SQLInsertStatement sqlInsertStatement = (SQLInsertStatement)
sqlStatements.get(0);
- sqlInsertStatement.getValuesList().get(0).getValues().set(pkIndex,
new OracleBinaryDoubleExpr());
+ SQLInsertStatement sqlInsertStatement = (SQLInsertStatement)
getSQLStatement(s);
+ sqlInsertStatement.getValuesList().get(0).getValues().set(pkIndex,
new OracleIntervalExpr());
Review Comment:
[nitpick] The change from `OracleBinaryDoubleExpr` to `OracleIntervalExpr`
should be consistent across all similar test cases. This appears to be done to
fix compatibility issues with the Druid upgrade, but consider using a more
generic expression type that's less likely to have compatibility issues in
future Druid versions.
##########
rm-datasource/src/test/java/org/apache/seata/rm/datasource/xa/DataSourceProxyXATest.java:
##########
@@ -167,11 +138,14 @@ public void testGetKingbaseXaConnection() throws
SQLException, ClassNotFoundExce
Assertions.assertTrue(wrappedConnection instanceof PooledConnection);
Connection wrappedPhysicalConn = ((PooledConnection)
wrappedConnection).getConnection();
+ if (wrappedPhysicalConn instanceof DruidStatementConnection) {
+ wrappedPhysicalConn = ((DruidStatementConnection)
wrappedPhysicalConn).getConnection();
+ }
Review Comment:
This pattern of unwrapping DruidStatementConnection is repeated across many
test files. Consider extracting this logic into a shared utility method in a
test base class to reduce code duplication and improve maintainability.
##########
sqlparser/seata-sqlparser-druid/src/main/java/org/apache/seata/sqlparser/druid/oscar/OscarOperateRecognizerHolder.java:
##########
@@ -47,8 +48,19 @@ public SQLRecognizer getUpdateRecognizer(String sql,
SQLStatement ast) {
@Override
public SQLRecognizer getSelectForUpdateRecognizer(String sql, SQLStatement
ast) {
- if (((SQLSelectStatement)
ast).getSelect().getFirstQueryBlock().isForUpdate()) {
- return new OscarSelectForUpdateRecognizer(sql, ast);
+ if ((ast instanceof SQLSelectStatement)) {
+ SQLSelectStatement selectStatement = (SQLSelectStatement) ast;
+ if (selectStatement.getSelect() != null) {
+ if (selectStatement.getSelect().getFirstQueryBlock() != null) {
+ OscarSelectQueryBlock queryBlock =
+ (OscarSelectQueryBlock)
selectStatement.getSelect().getFirstQueryBlock();
+ if (queryBlock.getForClause() != null) {
+ if (queryBlock.getForClause().getOption() ==
OscarSelectQueryBlock.ForClause.Option.UPDATE) {
+ return new OscarSelectForUpdateRecognizer(sql,
ast);
+ }
+ }
+ }
+ }
}
return null;
Review Comment:
[nitpick] The nested if statements create deep nesting that reduces
readability. Consider using early returns or extracting validation logic into
separate methods to flatten the structure and improve code clarity.
```suggestion
if (!(ast instanceof SQLSelectStatement)) {
return null;
}
SQLSelectStatement selectStatement = (SQLSelectStatement) ast;
if (selectStatement.getSelect() == null) {
return null;
}
if (selectStatement.getSelect().getFirstQueryBlock() == null) {
return null;
}
OscarSelectQueryBlock queryBlock =
(OscarSelectQueryBlock)
selectStatement.getSelect().getFirstQueryBlock();
if (queryBlock.getForClause() == null) {
return null;
}
if (queryBlock.getForClause().getOption() !=
OscarSelectQueryBlock.ForClause.Option.UPDATE) {
return null;
}
return new OscarSelectForUpdateRecognizer(sql, ast);
```
--
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]
