Copilot commented on code in PR #12007:
URL: https://github.com/apache/gravitino/pull/12007#discussion_r3576837973
##########
catalogs/catalog-jdbc-mysql/src/main/java/org/apache/gravitino/catalog/mysql/operation/MysqlTableOperations.java:
##########
@@ -460,7 +461,10 @@ private String
addColumnFieldDefinition(TableChange.AddColumn addColumn) {
}
// Append comment if available
if (StringUtils.isNotEmpty(addColumn.getComment())) {
- columnDefinition.append("COMMENT
'").append(addColumn.getComment()).append("' ");
+ columnDefinition
+ .append("COMMENT '")
+ .append(escapeSqlLiteral(addColumn.getComment(), '\''))
+ .append("' ");
Review Comment:
This add-column path now escapes the column comment for SQL literals, but
there is no regression test covering an ADD COLUMN with a comment that contains
quotes/backslashes (current SQL-generation tests only cover create-table and
comment-update cases). Adding a focused unit test that builds a
TableChange.addColumn with a comment like `owner's \\ "comment"` and asserts
the generated ALTER TABLE SQL contains the properly escaped literal would
prevent regressions.
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java:
##########
@@ -873,7 +875,10 @@ private String
addColumnFieldDefinition(TableChange.AddColumn addColumn) {
}
// Append comment if available
if (StringUtils.isNotEmpty(addColumn.getComment())) {
- columnDefinition.append("COMMENT
'").append(addColumn.getComment()).append("' ");
+ columnDefinition
+ .append("COMMENT '")
+ .append(escapeSqlLiteral(addColumn.getComment(), '\''))
+ .append("' ");
Review Comment:
The ADD COLUMN DDL now escapes `addColumn.getComment()` for SQL literals,
but the new regression tests in this PR don’t cover an add-column change with
quotes/backslashes in the comment. Please add a unit test that generates ALTER
TABLE ... ADD COLUMN with a comment containing `'`, `"`, and `\\` and asserts
the emitted SQL keeps the comment inside the quoted literal (i.e.,
quote/backslash are escaped).
##########
catalogs/catalog-jdbc-starrocks/src/main/java/org/apache/gravitino/catalog/starrocks/operations/StarRocksTableOperations.java:
##########
@@ -481,7 +482,10 @@ private String
addColumnFieldDefinition(TableChange.AddColumn addColumn) {
// Append comment if available
if (StringUtils.isNotEmpty(addColumn.getComment())) {
- columnDefinition.append("COMMENT
'").append(addColumn.getComment()).append("' ");
+ columnDefinition
+ .append("COMMENT '")
+ .append(escapeSqlLiteral(addColumn.getComment(), '\''))
+ .append("' ");
Review Comment:
This method now escapes the comment for ADD COLUMN, but the StarRocks
SQL-generation tests added in this PR only cover create-table comments and
table-comment updates. Consider adding a regression test that exercises ALTER
TABLE ... ADD COLUMN with a comment containing quotes/backslashes and asserts
the generated SQL contains the escaped literal, so this new escaping logic is
covered.
##########
catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java:
##########
@@ -689,7 +697,7 @@ private List<String> addColumnFieldDefinition(
+ col
+ PG_QUOTE
+ IS
- + addColumn.getComment()
+ + escapeSqlLiteral(addColumn.getComment(), '\'')
+ "';");
Review Comment:
The PostgreSQL add-column path now escapes the generated COMMENT ON COLUMN
literal (via `escapeSqlLiteral(addColumn.getComment(), '\'')`), but there’s no
regression test that covers adding a column with a comment containing
quotes/backslashes. Please add a unit test that drives the AddColumn alter path
with a comment like `owner\\'s "comment"; --` and verifies the emitted SQL uses
`IS E'...'` with correct escaping, to keep this behavior from regressing.
--
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]
