[apache#5861] improvement(CLI): Refactor the validation logic in the …

…handle methods (apache#5972) ### What changes were proposed in this pull request? refactor the validation logic of all entities and add test case, just like validation of table command apache#5906 . A hint is provided when the user's output is missing the required arguments. for example: ```bash gcli column list -m demo_metalake, --name Hive_catalog # Malformed entity name. # Missing required argument(s): schema, table gcli column details -m demo_metalake, --name Hive_catalog --audit # Malformed entity name. # Missing required argument(s): schema, table, column gcli user delete -m demo_metalake Missing --user option. ``` Currently, the Role command needs to be refactored and opened as a separate issue ### Why are the changes needed? Fix: apache#5861 ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? local test
Abyss-lord · Dec 25, 2024 · 304ef70 · 304ef70
1 parent c2d1b1e
commit 304ef70
Show file tree

Hide file tree

Showing 11 changed files with 778 additions and 67 deletions.
diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/ErrorMessages.java b/clients/cli/src/main/java/org/apache/gravitino/cli/ErrorMessages.java
@@ -32,6 +32,7 @@ public class ErrorMessages {
   public static final String MISSING_NAME = "Missing --name option.";
   public static final String MISSING_GROUP = "Missing --group option.";
   public static final String MISSING_USER = "Missing --user option.";
+  public static final String MISSING_TAG = "Missing --tag option.";
   public static final String METALAKE_EXISTS = "Metalake already exists.";
   public static final String CATALOG_EXISTS = "Catalog already exists.";
   public static final String SCHEMA_EXISTS = "Schema already exists.";

diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java b/clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java
@@ -31,8 +31,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
 import org.apache.commons.cli.CommandLine;
 import org.apache.commons.cli.HelpFormatter;
 import org.apache.commons.cli.Options;
@@ -255,6 +253,7 @@ private void handleCatalogCommand() {
     String outputFormat = line.getOptionValue(GravitinoOptions.OUTPUT);
 
     Command.setAuthenticationMode(auth, userName);
+    List<String> missingEntities = Lists.newArrayList();
 
     // Handle the CommandActions.LIST action separately as it doesn't use `catalog`
     if (CommandActions.LIST.equals(command)) {
@@ -263,6 +262,8 @@ private void handleCatalogCommand() {
     }
 
     String catalog = name.getCatalogName();
+    if (catalog == null) missingEntities.add(CommandEntities.CATALOG);
+    checkEntities(missingEntities);
 
     switch (command) {
       case CommandActions.DETAILS:
@@ -343,29 +344,21 @@ private void handleSchemaCommand() {
     String catalog = name.getCatalogName();
 
     Command.setAuthenticationMode(auth, userName);
+
     List<String> missingEntities = Lists.newArrayList();
     if (metalake == null) missingEntities.add(CommandEntities.METALAKE);
     if (catalog == null) missingEntities.add(CommandEntities.CATALOG);
 
     // Handle the CommandActions.LIST action separately as it doesn't use `schema`
     if (CommandActions.LIST.equals(command)) {
-      if (!missingEntities.isEmpty()) {
-        System.err.println("Missing required argument(s): " + COMMA_JOINER.join(missingEntities));
-        Main.exit(-1);
-      }
+      checkEntities(missingEntities);
       newListSchema(url, ignore, metalake, catalog).handle();
       return;
     }
 
     String schema = name.getSchemaName();
-    if (schema == null) {
-      missingEntities.add(CommandEntities.SCHEMA);
-    }
-
-    if (!missingEntities.isEmpty()) {
-      System.err.println("Missing required argument(s): " + COMMA_JOINER.join(missingEntities));
-      Main.exit(-1);
-    }
+    if (schema == null) missingEntities.add(CommandEntities.SCHEMA);
+    checkEntities(missingEntities);
 
     switch (command) {
       case CommandActions.DETAILS:
@@ -421,33 +414,20 @@ private void handleTableCommand() {
     String schema = name.getSchemaName();
 
     Command.setAuthenticationMode(auth, userName);
-    List<String> missingEntities =
-        Stream.of(
-                catalog == null ? CommandEntities.CATALOG : null,
-                schema == null ? CommandEntities.SCHEMA : null)
-            .filter(Objects::nonNull)
-            .collect(Collectors.toList());
+    List<String> missingEntities = Lists.newArrayList();
+    if (catalog == null) missingEntities.add(CommandEntities.CATALOG);
+    if (schema == null) missingEntities.add(CommandEntities.SCHEMA);
 
     // Handle CommandActions.LIST action separately as it doesn't require the `table`
     if (CommandActions.LIST.equals(command)) {
-      if (!missingEntities.isEmpty()) {
-        System.err.println(
-            "Missing required argument(s): " + Joiner.on(", ").join(missingEntities));
-        Main.exit(-1);
-      }
+      checkEntities(missingEntities);
       newListTables(url, ignore, metalake, catalog, schema).handle();
       return;
     }
 
     String table = name.getTableName();
-    if (table == null) {
-      missingEntities.add(CommandEntities.TABLE);
-    }
-
-    if (!missingEntities.isEmpty()) {
-      System.err.println("Missing required argument(s): " + Joiner.on(", ").join(missingEntities));
-      Main.exit(-1);
-    }
+    if (table == null) missingEntities.add(CommandEntities.TABLE);
+    checkEntities(missingEntities);
 
     switch (command) {
       case CommandActions.DETAILS:
@@ -527,7 +507,7 @@ protected void handleUserCommand() {
 
     if (user == null && !CommandActions.LIST.equals(command)) {
       System.err.println(ErrorMessages.MISSING_USER);
-      return;
+      Main.exit(-1);
     }
 
     switch (command) {
@@ -588,7 +568,7 @@ protected void handleGroupCommand() {
 
     if (group == null && !CommandActions.LIST.equals(command)) {
       System.err.println(ErrorMessages.MISSING_GROUP);
-      return;
+      Main.exit(-1);
     }
 
     switch (command) {
@@ -647,6 +627,13 @@ protected void handleTagCommand() {
     Command.setAuthenticationMode(auth, userName);
 
     String[] tags = line.getOptionValues(GravitinoOptions.TAG);
+    if (tags == null
+        && !((CommandActions.REMOVE.equals(command) && line.hasOption(GravitinoOptions.FORCE))
+            || CommandActions.LIST.equals(command))) {
+      System.err.println(ErrorMessages.MISSING_TAG);
+      Main.exit(-1);
+    }
+
     if (tags != null) {
       tags = Arrays.stream(tags).distinct().toArray(String[]::new);
     }
@@ -790,12 +777,20 @@ private void handleColumnCommand() {
 
     Command.setAuthenticationMode(auth, userName);
 
+    List<String> missingEntities = Lists.newArrayList();
+    if (catalog == null) missingEntities.add(CommandEntities.CATALOG);
+    if (schema == null) missingEntities.add(CommandEntities.SCHEMA);
+    if (table == null) missingEntities.add(CommandEntities.TABLE);
+
     if (CommandActions.LIST.equals(command)) {
+      checkEntities(missingEntities);
       newListColumns(url, ignore, metalake, catalog, schema, table).handle();
       return;
     }
 
     String column = name.getColumnName();
+    if (column == null) missingEntities.add(CommandEntities.COLUMN);
+    checkEntities(missingEntities);
 
     switch (command) {
       case CommandActions.DETAILS:
@@ -965,12 +960,19 @@ private void handleTopicCommand() {
 
     Command.setAuthenticationMode(auth, userName);
 
+    List<String> missingEntities = Lists.newArrayList();
+    if (catalog == null) missingEntities.add(CommandEntities.CATALOG);
+    if (schema == null) missingEntities.add(CommandEntities.SCHEMA);
+
     if (CommandActions.LIST.equals(command)) {
+      checkEntities(missingEntities);
       newListTopics(url, ignore, metalake, catalog, schema).handle();
       return;
     }
 
     String topic = name.getTopicName();
+    if (topic == null) missingEntities.add(CommandEntities.TOPIC);
+    checkEntities(missingEntities);
 
     switch (command) {
       case CommandActions.DETAILS:
@@ -1040,12 +1042,20 @@ private void handleFilesetCommand() {
 
     Command.setAuthenticationMode(auth, userName);
 
+    List<String> missingEntities = Lists.newArrayList();
+    if (catalog == null) missingEntities.add(CommandEntities.CATALOG);
+    if (schema == null) missingEntities.add(CommandEntities.SCHEMA);
+
+    // Handle CommandActions.LIST action separately as it doesn't require the `fileset`
     if (CommandActions.LIST.equals(command)) {
+      checkEntities(missingEntities);
       newListFilesets(url, ignore, metalake, catalog, schema).handle();
       return;
     }
 
     String fileset = name.getFilesetName();
+    if (fileset == null) missingEntities.add(CommandEntities.FILESET);
+    checkEntities(missingEntities);
 
     switch (command) {
       case CommandActions.DETAILS:
@@ -1183,4 +1193,11 @@ public String getAuth() {
 
     return null;
   }
+
+  private void checkEntities(List<String> entities) {
+    if (!entities.isEmpty()) {
+      System.err.println("Missing required argument(s): " + COMMA_JOINER.join(entities));
+      Main.exit(-1);
+    }
+  }
 }
diff --git a/clients/cli/src/test/java/org/apache/gravitino/cli/TestCatalogCommands.java b/clients/cli/src/test/java/org/apache/gravitino/cli/TestCatalogCommands.java
@@ -19,6 +19,7 @@
 
 package org.apache.gravitino.cli;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertThrows;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.doReturn;
@@ -30,13 +31,15 @@
 
 import java.io.ByteArrayOutputStream;
 import java.io.PrintStream;
+import java.nio.charset.StandardCharsets;
 import java.util.HashMap;
 import org.apache.commons.cli.CommandLine;
 import org.apache.commons.cli.Options;
 import org.apache.gravitino.cli.commands.CatalogAudit;
 import org.apache.gravitino.cli.commands.CatalogDetails;
 import org.apache.gravitino.cli.commands.CatalogDisable;
 import org.apache.gravitino.cli.commands.CatalogEnable;
+import org.apache.gravitino.cli.commands.Command;
 import org.apache.gravitino.cli.commands.CreateCatalog;
 import org.apache.gravitino.cli.commands.DeleteCatalog;
 import org.apache.gravitino.cli.commands.ListCatalogProperties;
@@ -318,6 +321,36 @@ void testUpdateCatalogNameCommand() {
     verify(mockUpdateName).handle();
   }
 
+  @Test
+  @SuppressWarnings("DefaultCharset")
+  void testCatalogDetailsCommandWithoutCatalog() {
+    Main.useExit = false;
+    when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true);
+    when(mockCommandLine.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo");
+    when(mockCommandLine.hasOption(GravitinoOptions.NAME)).thenReturn(false);
+
+    GravitinoCommandLine commandLine =
+        spy(
+            new GravitinoCommandLine(
+                mockCommandLine, mockOptions, CommandEntities.CATALOG, CommandActions.DETAILS));
+
+    assertThrows(RuntimeException.class, commandLine::handleCommandLine);
+    verify(commandLine, never())
+        .newCatalogDetails(
+            GravitinoCommandLine.DEFAULT_URL,
+            false,
+            Command.OUTPUT_FORMAT_TABLE,
+            "metalake_demo",
+            "catalog");
+    String output = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim();
+    assertEquals(
+        output,
+        "Missing --name option."
+            + "\n"
+            + "Missing required argument(s): "
+            + CommandEntities.CATALOG);
+  }
+
   @Test
   void testEnableCatalogCommand() {
     CatalogEnable mockEnable = mock(CatalogEnable.class);