Copilot commented on code in PR #9881:
URL: https://github.com/apache/gravitino/pull/9881#discussion_r2883502923
##########
common/src/test/java/org/apache/gravitino/dto/requests/TestTableUpdatesRequest.java:
##########
@@ -309,4 +309,37 @@ public void testOperationTableIndexRequest() throws
JsonProcessingException {
Assertions.assertEquals(
JsonUtils.objectMapper().readTree(expected),
JsonUtils.objectMapper().readTree(jsonString));
}
+
+ @Test
+ public void testUpdateColumnCommentWithEmptyString() {
+ TableUpdateRequest.UpdateTableColumnCommentRequest request =
+ new TableUpdateRequest.UpdateTableColumnCommentRequest(new String[]
{"column1"}, "");
+
+ request.validate();
+
+ Assertions.assertEquals("", request.getNewComment());
+ Assertions.assertArrayEquals(new String[] {"column1"},
request.getFieldName());
+ }
+
+ @Test
+ public void testUpdateColumnCommentWithNull() {
+ TableUpdateRequest.UpdateTableColumnCommentRequest request =
+ new TableUpdateRequest.UpdateTableColumnCommentRequest(new String[]
{"column1"}, null);
+
+ request.validate();
+
+ Assertions.assertNull(request.getNewComment());
+ Assertions.assertArrayEquals(new String[] {"column1"},
request.getFieldName());
+ }
+
+ @Test
+ public void testUpdateColumnCommentWithValidValue() {
+ TableUpdateRequest.UpdateTableColumnCommentRequest request =
+ new TableUpdateRequest.UpdateTableColumnCommentRequest(
+ new String[] {"column1"}, "This is a valid comment");
+
+ request.validate();
+
+ Assertions.assertEquals("This is a valid comment",
request.getNewComment());
+ }
Review Comment:
The `testUpdateColumnCommentWithValidValue` test is missing an assertion for
`request.getFieldName()`, while both sibling tests
(`testUpdateColumnCommentWithEmptyString` and
`testUpdateColumnCommentWithNull`) do assert the field name. For consistency
and completeness, `Assertions.assertArrayEquals(new String[] {"column1"},
request.getFieldName())` should be added.
##########
common/src/main/java/org/apache/gravitino/dto/requests/TableUpdateRequest.java:
##########
@@ -623,9 +623,6 @@ public void validate() throws IllegalArgumentException {
&& fieldName.length > 0
&& Arrays.stream(fieldName).allMatch(StringUtils::isNotBlank),
"\"fieldName\" field is required and cannot be empty");
Review Comment:
Removing the `newComment` non-blank validation here means null or empty
strings can now reach all catalog implementations via the REST API. While MySQL
and OceanBase handle null/empty gracefully (the `appendColumnDefinition` method
checks `StringUtils.isNotEmpty` before appending the COMMENT clause), Doris
(`DorisTableOperations.updateColumnCommentFieldDefinition`) uses
`String.format("MODIFY COLUMN \`%s\` COMMENT '%s'", col, newComment)` which
would produce `COMMENT 'null'` for a null value (setting the literal string
"null" as the comment rather than clearing it). Similarly, PostgreSQL's
`updateColumnCommentFieldDefinition` concatenates `newComment` directly into
the SQL string, which would produce `IS 'null';` for a null value instead of
the valid `IS NULL;` syntax needed to clear the comment.
These pre-existing issues in other catalog implementations are now exposed
by this DTO validation removal. Consider either fixing those implementations
alongside this change, or documenting which catalogs support null/empty comment
clearing.
```suggestion
"\"fieldName\" field is required and cannot be empty");
Preconditions.checkArgument(
StringUtils.isNotBlank(newComment),
"\"newComment\" field is required and cannot be null or blank");
```
--
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]
