[#4968] improvement(api): Unify the modification behavior of the comm…

…ent field (#5288)

### What changes were proposed in this pull request?

- deprecated the removeComment change
- the new comment of updateComment supports null and empty string

### Why are the changes needed?

Fix: #4968 

### Does this PR introduce _any_ user-facing change?

N/A

### How was this patch tested?

Pass existing tests

Co-authored-by: Ricco Chen <[email protected]>
Co-authored-by: chenzeping.ricco <[email protected]>

api/src/main/java/org/apache/gravitino/file/FilesetChange.java

@@ -73,7 +73,9 @@ static FilesetChange removeProperty(String property) {
    * Creates a new fileset change to remove comment from the fileset.
    *
    * @return The fileset change.
+   * @deprecated Use {@link #updateComment(String)} with null value as the argument instead.
    */
+  @Deprecated
   static FilesetChange removeComment() {
     return RemoveComment.getInstance();
   }
@@ -310,7 +312,10 @@ public String toString() {
     }
   }
 
-  /** A fileset change to remove comment from the fileset. */
+  /**
+   * A fileset change to remove comment from the fileset. Use {@link UpdateFilesetComment} with null
+   * value as the argument instead.
+   */
   final class RemoveComment implements FilesetChange {
     private static final RemoveComment INSTANCE = new RemoveComment();
 

catalogs/catalog-hadoop/src/test/java/org/apache/gravitino/catalog/hadoop/TestHadoopCatalogOperations.java

@@ -770,7 +770,7 @@ public void testRemoveFilesetComment() throws IOException {
     String name = "fileset27";
     Fileset fileset = createFileset(name, schemaName, comment, Fileset.Type.MANAGED, null, null);
 
-    FilesetChange change1 = FilesetChange.removeComment();
+    FilesetChange change1 = FilesetChange.updateComment(null);
     try (SecureHadoopCatalogOperations ops = new SecureHadoopCatalogOperations(store)) {
       ops.initialize(Maps.newHashMap(), randomCatalogInfo(), HADOOP_PROPERTIES_METADATA);
       NameIdentifier filesetIdent = NameIdentifier.of("m1", "c1", schemaName, name);

catalogs/catalog-hadoop/src/test/java/org/apache/gravitino/catalog/hadoop/integration/test/HadoopCatalogIT.java

@@ -575,7 +575,7 @@ public void testFilesetRemoveComment() throws IOException {
         catalog
             .asFilesetCatalog()
             .alterFileset(
-                NameIdentifier.of(schemaName, filesetName), FilesetChange.removeComment());
+                NameIdentifier.of(schemaName, filesetName), FilesetChange.updateComment(null));
     assertFilesetExists(filesetName);
 
     // verify fileset is updated

catalogs/catalog-kafka/src/test/java/org/apache/gravitino/catalog/kafka/integration/test/CatalogKafkaIT.java

@@ -172,6 +172,10 @@ public void testCatalog() throws ExecutionException, InterruptedException {
     Assertions.assertEquals("new comment", alteredCatalog.comment());
     Assertions.assertFalse(alteredCatalog.properties().containsKey("key1"));
 
+    Catalog updateNullComment =
+        metalake.alterCatalog(catalogName, CatalogChange.updateComment(null));
+    Assertions.assertNull(updateNullComment.comment());
+
     // test drop catalog
     boolean dropped = metalake.dropCatalog(catalogName, true);
     Assertions.assertTrue(dropped);

clients/client-java/src/test/java/org/apache/gravitino/client/TestFilesetCatalog.java

@@ -378,7 +378,7 @@ public void testAlterFileset() throws JsonProcessingException {
     assertFileset(mockFileset3, res3);
 
     // Test remove fileset comment
-    FilesetUpdateRequest req4 = new FilesetUpdateRequest.RemoveFilesetCommentRequest();
+    FilesetUpdateRequest req4 = new FilesetUpdateRequest.UpdateFilesetCommentRequest(null);
     FilesetDTO mockFileset4 =
         mockFilesetDTO("new name", Fileset.Type.MANAGED, null, "mock location", ImmutableMap.of());
     FilesetResponse resp4 = new FilesetResponse(mockFileset4);

clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/CatalogIT.java

@@ -221,7 +221,7 @@ public void testCatalogAvailable() {
         CatalogNotInUseException.class,
         () ->
             filesetOps.alterFileset(
-                NameIdentifier.of("dummy", "dummy"), FilesetChange.removeComment()));
+                NameIdentifier.of("dummy", "dummy"), FilesetChange.updateComment(null)));
 
     Assertions.assertTrue(metalake.dropCatalog(catalogName), "catalog should be dropped");
     Assertions.assertFalse(metalake.dropCatalog(catalogName), "catalog should be non-existent");
@@ -399,6 +399,10 @@ void testUpdateCatalogWithNullableComment() {
         metalake.alterCatalog(catalogName, CatalogChange.updateComment("new catalog comment"));
     Assertions.assertEquals("new catalog comment", updatedCatalog.comment());
 
+    Catalog updateNullComment =
+        metalake.alterCatalog(catalogName, CatalogChange.updateComment(null));
+    Assertions.assertNull(updateNullComment.comment());
+
     metalake.disableCatalog(catalogName);
     metalake.dropCatalog(catalogName);
   }

clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/MetalakeIT.java

@@ -22,6 +22,7 @@
 import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
@@ -219,6 +220,11 @@ public void testUpdateMetalakeWithNullableComment() {
         new MetalakeChange[] {MetalakeChange.updateComment("new metalake comment")};
     GravitinoMetalake updatedMetalake = client.alterMetalake(metalakeNameA, changes);
     assertEquals("new metalake comment", updatedMetalake.comment());
+
+    GravitinoMetalake updateNullComment =
+        client.alterMetalake(metalakeNameA, MetalakeChange.updateComment(null));
+    assertNull(updateNullComment.comment());
+
     assertTrue(client.dropMetalake(metalakeNameA, true));
   }
 
@@ -317,7 +323,7 @@ public void testMetalakeAvailable() {
         MetalakeNotInUseException.class,
         () ->
             filesetOps.alterFileset(
-                NameIdentifier.of("dummy", "dummy"), FilesetChange.removeComment()));
+                NameIdentifier.of("dummy", "dummy"), FilesetChange.updateComment(null)));
 
     Assertions.assertThrows(
         NonEmptyMetalakeException.class, () -> client.dropMetalake(metalakeName));

clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/TagIT.java

@@ -197,6 +197,14 @@ public void testCreateGetAndListTag() {
     Assertions.assertEquals(tag2, tag3);
   }
 
+  @Test
+  public void testNullableComment() {
+    String tagName = GravitinoITUtils.genRandomName("tag_it_tag");
+    metalake.createTag(tagName, "comment", Collections.emptyMap());
+    Tag alteredTag6 = metalake.alterTag(tagName, TagChange.updateComment(null));
+    Assertions.assertNull(alteredTag6.comment());
+  }
+
   @Test
   public void testCreateAndAlterTag() {
     String tagName = GravitinoITUtils.genRandomName("tag_it_tag");

clients/client-python/gravitino/api/fileset_change.py

@@ -81,8 +81,11 @@ def remove_comment():
 
         Returns:
             The fileset change.
+
+        Deprecated:
+            Please use `update_comment(str)` with null value as the argument instead.
         """
-        return FilesetChange.RemoveComment()
+        return FilesetChange.UpdateFilesetComment(None)
 
     @dataclass
     class RenameFileset:
@@ -279,7 +282,11 @@ def __str__(self):
 
     @dataclass
     class RemoveComment:
-        """A fileset change to remove comment from the fileset."""
+        """A fileset change to remove comment from the fileset.
+
+        Deprecated:
+            Please use `UpdateFilesetComment(str)` with null value as the argument instead.
+        """
 
         def __eq__(self, other) -> bool:
             """Compares this RemoveComment instance with another object for equality.

clients/client-python/gravitino/catalog/fileset_catalog.py

@@ -319,5 +319,5 @@ def to_fileset_update_request(change: FilesetChange):
         if isinstance(change, FilesetChange.RemoveProperty):
             return FilesetUpdateRequest.RemoveFilesetPropertyRequest(change.property())
         if isinstance(change, FilesetChange.RemoveComment):
-            return FilesetUpdateRequest.RemoveFilesetCommentRequest()
+            return FilesetUpdateRequest.UpdateFilesetCommentRequest(None)
         raise ValueError(f"Unknown change type: {type(change).__name__}")

clients/client-python/gravitino/dto/requests/catalog_update_request.py

@@ -78,8 +78,8 @@ def catalog_change(self):
             return CatalogChange.update_comment(self._new_comment)
 
         def validate(self):
-            if not self._new_comment:
-                raise ValueError('"newComment" field is required and cannot be empty')
+            """Validates the fields of the request. Always pass."""
+            pass
 
     @dataclass
     class SetCatalogPropertyRequest(CatalogUpdateRequestBase):

clients/client-python/gravitino/dto/requests/fileset_update_request.py

@@ -79,13 +79,8 @@ def __init__(self, new_comment: str):
             self._new_comment = new_comment
 
         def validate(self):
-            """Validates the fields of the request.
-
-            Raises:
-                IllegalArgumentException if the new comment is not set.
-            """
-            if not self._new_comment:
-                raise ValueError('"new_comment" field is required and cannot be empty')
+            """Validates the fields of the request. Always pass."""
+            pass
 
         def fileset_change(self):
             """Returns the fileset change"""
@@ -149,7 +144,11 @@ def fileset_change(self):
 
     @dataclass
     class RemoveFilesetCommentRequest(FilesetUpdateRequestBase):
-        """Represents a request to remove comment from a Fileset."""
+        """Represents a request to remove comment from a Fileset.
+
+        Deprecated:
+            Please use `UpdateFilesetCommentRequest` with null value as the argument instead.
+        """
 
         def __init__(self):
             super().__init__("removeComment")

clients/client-python/gravitino/dto/requests/metalake_update_request.py

@@ -74,13 +74,8 @@ def __init__(self, new_comment: str):
             self._new_comment = new_comment
 
         def validate(self):
-            """Validates the fields of the request.
-
-            Raises:
-                IllegalArgumentException if the new comment is not set.
-            """
-            if not self._new_comment:
-                raise ValueError('"newComment" field is required and cannot be empty')
+            """Validates the fields of the request. Always pass."""
+            pass
 
         def metalake_change(self):
             return MetalakeChange.update_comment(self._new_comment)

clients/client-python/tests/integration/test_catalog.py

@@ -125,6 +125,14 @@ def test_failed_create_catalog(self):
         with self.assertRaises(CatalogAlreadyExistsException):
             _ = self.create_catalog(self.catalog_name)
 
+    def test_nullable_comment_catalog(self):
+        self.create_catalog(self.catalog_name)
+        changes = (CatalogChange.update_comment(None),)
+        null_comment_catalog = self.gravitino_client.alter_catalog(
+            self.catalog_name, *changes
+        )
+        self.assertIsNone(null_comment_catalog.comment())
+
     def test_alter_catalog(self):
         catalog = self.create_catalog(self.catalog_name)
 

clients/client-python/tests/integration/test_fileset_catalog.py

@@ -239,7 +239,7 @@ def test_alter_fileset(self):
         self.assertEqual(fileset_new.comment(), fileset_new_comment)
 
         fileset_comment_removed = catalog.as_fileset_catalog().alter_fileset(
-            self.fileset_ident, FilesetChange.remove_comment()
+            self.fileset_ident, FilesetChange.update_comment(None)
         )
         self.assertEqual(fileset_comment_removed.name(), self.fileset_name)
         self.assertIsNone(fileset_comment_removed.comment())

clients/client-python/tests/integration/test_metalake.py

@@ -90,6 +90,14 @@ def test_failed_create_metalake(self):
         with self.assertRaises(MetalakeAlreadyExistsException):
             _ = self.create_metalake(self.metalake_name)
 
+    def test_nullable_comment_metalake(self):
+        self.create_metalake(self.metalake_name)
+        changes = (MetalakeChange.update_comment(None),)
+        null_comment_metalake = self.gravitino_admin_client.alter_metalake(
+            self.metalake_name, *changes
+        )
+        self.assertIsNone(null_comment_metalake.comment())
+
     def test_alter_metalake(self):
         self.create_metalake(self.metalake_name)
 

common/src/main/java/org/apache/gravitino/dto/requests/CatalogUpdateRequest.java

@@ -117,17 +117,9 @@ public UpdateCatalogCommentRequest() {
       this(null);
     }
 
-    /**
-     * Validates the fields of the request.
-     *
-     * @throws IllegalArgumentException if the new comment is not set.
-     */
+    /** Validates the fields of the request. Always pass. */
     @Override
-    public void validate() throws IllegalArgumentException {
-      Preconditions.checkArgument(
-          StringUtils.isNotBlank(newComment),
-          "\"newComment\" field is required and cannot be empty");
-    }
+    public void validate() throws IllegalArgumentException {}
 
     @Override
     public CatalogChange catalogChange() {

common/src/main/java/org/apache/gravitino/dto/requests/FilesetUpdateRequest.java

@@ -109,17 +109,9 @@ public FilesetChange filesetChange() {
       return FilesetChange.updateComment(newComment);
     }
 
-    /**
-     * Validates the request.
-     *
-     * @throws IllegalArgumentException if the request is invalid.
-     */
+    /** Validates the fields of the request. Always pass. */
     @Override
-    public void validate() throws IllegalArgumentException {
-      Preconditions.checkArgument(
-          StringUtils.isNotBlank(newComment),
-          "\"newComment\" field is required and cannot be empty");
-    }
+    public void validate() throws IllegalArgumentException {}
   }
 
   /** The fileset update request for setting the properties of a fileset. */
@@ -194,7 +186,7 @@ class RemoveFilesetCommentRequest implements FilesetUpdateRequest {
     /** @return The fileset change. */
     @Override
     public FilesetChange filesetChange() {
-      return FilesetChange.removeComment();
+      return FilesetChange.updateComment(null);
     }
 
     /**

common/src/main/java/org/apache/gravitino/dto/requests/MetalakeUpdateRequest.java

@@ -117,17 +117,9 @@ public UpdateMetalakeCommentRequest() {
       this(null);
     }
 
-    /**
-     * Validates the fields of the request.
-     *
-     * @throws IllegalArgumentException if the new comment is not set.
-     */
+    /** Validates the fields of the request. Always pass. */
     @Override
-    public void validate() throws IllegalArgumentException {
-      Preconditions.checkArgument(
-          StringUtils.isNotBlank(newComment),
-          "\"newComment\" field is required and cannot be empty");
-    }
+    public void validate() throws IllegalArgumentException {}
 
     @Override
     public MetalakeChange metalakeChange() {

common/src/main/java/org/apache/gravitino/dto/requests/TableUpdateRequest.java

@@ -164,17 +164,9 @@ public UpdateTableCommentRequest() {
       this(null);
     }
 
-    /**
-     * Validates the request.
-     *
-     * @throws IllegalArgumentException If the request is invalid, this exception is thrown.
-     */
+    /** Validates the fields of the request. Always pass. */
     @Override
-    public void validate() throws IllegalArgumentException {
-      Preconditions.checkArgument(
-          StringUtils.isNotBlank(newComment),
-          "\"newComment\" field is required and cannot be empty");
-    }
+    public void validate() throws IllegalArgumentException {}
 
     /**
      * Returns the table change.

common/src/main/java/org/apache/gravitino/dto/requests/TagUpdateRequest.java

@@ -114,11 +114,9 @@ public TagChange tagChange() {
       return TagChange.updateComment(newComment);
     }
 
+    /** Validates the fields of the request. Always pass. */
     @Override
-    public void validate() throws IllegalArgumentException {
-      Preconditions.checkArgument(
-          StringUtils.isNotBlank(newComment), "\"newComment\" must not be blank");
-    }
+    public void validate() throws IllegalArgumentException {}
   }
 
   /** The tag update request for setting a tag property. */

common/src/main/java/org/apache/gravitino/dto/requests/TopicUpdateRequest.java

@@ -72,17 +72,9 @@ public UpdateTopicCommentRequest() {
       this(null);
     }
 
-    /**
-     * Validates the request.
-     *
-     * @throws IllegalArgumentException If the request is invalid, this exception is thrown.
-     */
+    /** Validates the fields of the request. Always pass. */
     @Override
-    public void validate() throws IllegalArgumentException {
-      Preconditions.checkArgument(
-          StringUtils.isNotBlank(newComment),
-          "\"newComment\" field is required and cannot be empty");
-    }
+    public void validate() throws IllegalArgumentException {}
 
     /**
      * Returns the topic change.

core/src/test/java/org/apache/gravitino/catalog/TestFilesetOperationDispatcher.java

@@ -143,7 +143,7 @@ public void testCreateAndAlterFileset() {
     Assertions.assertEquals(fileset1.name(), alteredFileset2.name());
     Assertions.assertEquals("new comment", alteredFileset2.comment());
 
-    FilesetChange[] changes3 = new FilesetChange[] {FilesetChange.removeComment()};
+    FilesetChange[] changes3 = new FilesetChange[] {FilesetChange.updateComment(null)};
 
     Fileset alteredFileset3 = filesetOperationDispatcher.alterFileset(filesetIdent1, changes3);
     Assertions.assertEquals(fileset1.name(), alteredFileset3.name());