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

…e comment field (apache#4968)
koonchen · Oct 20, 2024 · 3fae0cc · 3fae0cc
1 parent 6272303
commit 3fae0cc
Show file tree

Hide file tree

Showing 21 changed files with 55 additions and 80 deletions.
diff --git a/api/src/main/java/org/apache/gravitino/file/FilesetChange.java b/api/src/main/java/org/apache/gravitino/file/FilesetChange.java
@@ -74,6 +74,7 @@ static FilesetChange removeProperty(String property) {
    *
    * @return The fileset change.
    */
+  @Deprecated
   static FilesetChange removeComment() {
     return RemoveComment.getInstance();
   }
@@ -310,7 +311,11 @@ public String toString() {
     }
   }
 
-  /** A fileset change to remove comment from the fileset. */
+  /**
+   * A fileset change to remove comment from the fileset. The remove operation has been replaced by
+   * the update operation. Please use the update operation.
+   */
+  @Deprecated
   final class RemoveComment implements FilesetChange {
     private static final RemoveComment INSTANCE = new RemoveComment();
 

diff --git a/...log-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java b/...log-hadoop/src/main/java/org/apache/gravitino/catalog/hadoop/HadoopCatalogOperations.java
@@ -668,6 +668,7 @@ private SchemaEntity updateSchemaEntity(
         .build();
   }
 
+  @SuppressWarnings("deprecation")
   private FilesetEntity updateFilesetEntity(
       NameIdentifier ident, FilesetEntity filesetEntity, FilesetChange... changes) {
     Map<String, String> props =

diff --git a/...hadoop/src/test/java/org/apache/gravitino/catalog/hadoop/TestHadoopCatalogOperations.java b/...hadoop/src/test/java/org/apache/gravitino/catalog/hadoop/TestHadoopCatalogOperations.java
@@ -761,6 +761,7 @@ public void testUpdateFilesetComment() throws IOException {
   }
 
   @Test
+  @SuppressWarnings("deprecation")
   public void testRemoveFilesetComment() throws IOException {
     String schemaName = "schema27";
     String comment = "comment27";

diff --git a/...p/src/test/java/org/apache/gravitino/catalog/hadoop/integration/test/HadoopCatalogIT.java b/...p/src/test/java/org/apache/gravitino/catalog/hadoop/integration/test/HadoopCatalogIT.java
@@ -557,6 +557,7 @@ public void testFilesetRemoveProperties() throws IOException {
   }
 
   @Test
+  @SuppressWarnings("deprecation")
   public void testFilesetRemoveComment() throws IOException {
     // create fileset
     String filesetName = "test_remove_fileset_comment";

diff --git a/clients/client-java/src/main/java/org/apache/gravitino/client/DTOConverters.java b/clients/client-java/src/main/java/org/apache/gravitino/client/DTOConverters.java
@@ -205,6 +205,7 @@ static TableUpdateRequest toTableUpdateRequest(TableChange change) {
     }
   }
 
+  @SuppressWarnings("deprecation")
   static FilesetUpdateRequest toFilesetUpdateRequest(FilesetChange change) {
     if (change instanceof FilesetChange.RenameFileset) {
       return new FilesetUpdateRequest.RenameFilesetRequest(

diff --git a/clients/client-java/src/test/java/org/apache/gravitino/client/TestFilesetCatalog.java b/clients/client-java/src/test/java/org/apache/gravitino/client/TestFilesetCatalog.java
@@ -313,6 +313,7 @@ public void testDropFileset() throws JsonProcessingException {
   }
 
   @Test
+  @SuppressWarnings("deprecation")
   public void testAlterFileset() throws JsonProcessingException {
     NameIdentifier fileset = NameIdentifier.of("schema1", "fileset1");
     String filesetPath =

diff --git a/...nts/client-java/src/test/java/org/apache/gravitino/client/integration/test/CatalogIT.java b/...nts/client-java/src/test/java/org/apache/gravitino/client/integration/test/CatalogIT.java
@@ -156,6 +156,7 @@ public void testDropCatalog() {
   }
 
   @Test
+  @SuppressWarnings("deprecation")
   public void testCatalogAvailable() {
     String catalogName = GravitinoITUtils.genRandomName("test_catalog");
     Catalog catalog =

diff --git a/clients/client-python/gravitino/api/fileset_change.py b/clients/client-python/gravitino/api/fileset_change.py
@@ -81,6 +81,9 @@ def remove_comment():
 
         Returns:
             The fileset change.
+
+        Deprecated:
+            This class is deprecated and will be removed in future versions.
         """
         return FilesetChange.RemoveComment()
 
@@ -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:
+            This class is deprecated and will be removed in future versions.
+        """
 
         def __eq__(self, other) -> bool:
             """Compares this RemoveComment instance with another object for equality.

diff --git a/clients/client-python/gravitino/dto/requests/catalog_update_request.py b/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):

diff --git a/clients/client-python/gravitino/dto/requests/fileset_update_request.py b/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:
+            This class is deprecated and will be removed in future versions.
+        """
 
         def __init__(self):
             super().__init__("removeComment")

diff --git a/clients/client-python/gravitino/dto/requests/metalake_update_request.py b/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)

diff --git a/common/src/main/java/org/apache/gravitino/dto/requests/CatalogUpdateRequest.java b/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() {

diff --git a/common/src/main/java/org/apache/gravitino/dto/requests/FilesetUpdateRequest.java b/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. */
@@ -189,6 +181,7 @@ public void validate() throws IllegalArgumentException {
   @EqualsAndHashCode
   @NoArgsConstructor(force = true)
   @ToString
+  @Deprecated
   class RemoveFilesetCommentRequest implements FilesetUpdateRequest {
 
     /** @return The fileset change. */

diff --git a/common/src/main/java/org/apache/gravitino/dto/requests/MetalakeUpdateRequest.java b/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() {

diff --git a/common/src/main/java/org/apache/gravitino/dto/requests/TableUpdateRequest.java b/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.

diff --git a/common/src/main/java/org/apache/gravitino/dto/requests/TagUpdateRequest.java b/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. */

diff --git a/common/src/main/java/org/apache/gravitino/dto/requests/TopicUpdateRequest.java b/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.

diff --git a/core/src/test/java/org/apache/gravitino/catalog/TestFilesetOperationDispatcher.java b/core/src/test/java/org/apache/gravitino/catalog/TestFilesetOperationDispatcher.java
@@ -111,6 +111,7 @@ public void testCreateAndLoadFileset() {
   }
 
   @Test
+  @SuppressWarnings("deprecation")
   public void testCreateAndAlterFileset() {
     Namespace filesetNs = Namespace.of(metalake, catalog, "schema101");
     Map<String, String> props = ImmutableMap.of("k1", "v1", "k2", "v2");

diff --git a/core/src/test/java/org/apache/gravitino/connector/TestCatalogOperations.java b/core/src/test/java/org/apache/gravitino/connector/TestCatalogOperations.java
@@ -387,6 +387,7 @@ public Fileset createFileset(
   }
 
   @Override
+  @SuppressWarnings("deprecation")
   public Fileset alterFileset(NameIdentifier ident, FilesetChange... changes)
       throws NoSuchFilesetException, IllegalArgumentException {
     if (!filesets.containsKey(ident)) {

diff --git a/docs/manage-fileset-metadata-using-gravitino.md b/docs/manage-fileset-metadata-using-gravitino.md
@@ -389,13 +389,13 @@ fileset_new = catalog.as_fileset_catalog().alter_fileset(NameIdentifier.of("sche
 
 Currently, Gravitino supports the following changes to a fileset:
 
-| Supported modification     | JSON                                                         | Java                                          |
-|----------------------------|--------------------------------------------------------------|-----------------------------------------------|
-| Rename a fileset           | `{"@type":"rename","newName":"fileset_renamed"}`             | `FilesetChange.rename("fileset_renamed")`     |
-| Update a comment           | `{"@type":"updateComment","newComment":"new_comment"}`       | `FilesetChange.updateComment("new_comment")`  |
-| Set a fileset property     | `{"@type":"setProperty","property":"key1","value":"value1"}` | `FilesetChange.setProperty("key1", "value1")` |
-| Remove a fileset property  | `{"@type":"removeProperty","property":"key1"}`               | `FilesetChange.removeProperty("key1")`        |
-| Remove comment             | `{"@type":"removeComment"}`                                  | `FilesetChange.removeComment()`               |
+| Supported modification      | JSON                                                         | Java                                          |
+|-----------------------------|--------------------------------------------------------------|-----------------------------------------------|
+| Rename a fileset            | `{"@type":"rename","newName":"fileset_renamed"}`             | `FilesetChange.rename("fileset_renamed")`     |
+| Update a comment            | `{"@type":"updateComment","newComment":"new_comment"}`       | `FilesetChange.updateComment("new_comment")`  |
+| Set a fileset property      | `{"@type":"setProperty","property":"key1","value":"value1"}` | `FilesetChange.setProperty("key1", "value1")` |
+| Remove a fileset property   | `{"@type":"removeProperty","property":"key1"}`               | `FilesetChange.removeProperty("key1")`        |
+| Remove comment (deprecated) | `{"@type":"removeComment"}`                                  | `FilesetChange.removeComment()`               |
 
 ### Drop a fileset
 

diff --git a/server/src/test/java/org/apache/gravitino/server/web/rest/TestFilesetOperations.java b/server/src/test/java/org/apache/gravitino/server/web/rest/TestFilesetOperations.java
@@ -369,6 +369,7 @@ public void testUpdateFilesetComment() {
   }
 
   @Test
+  @SuppressWarnings("deprecation")
   public void testRemoveFilesetComment() {
     FilesetUpdateRequest req = new FilesetUpdateRequest.RemoveFilesetCommentRequest();
     Fileset fileset =
@@ -377,6 +378,7 @@ public void testRemoveFilesetComment() {
   }
 
   @Test
+  @SuppressWarnings("deprecation")
   public void testMultiUpdateRequest() {
     FilesetUpdateRequest req = new FilesetUpdateRequest.RenameFilesetRequest("new name");
     FilesetUpdateRequest req1 = new FilesetUpdateRequest.UpdateFilesetCommentRequest("new comment");