[#5831] fix(CLI): Fix CLi gives unexpected output when input tag set …

…command (#5897) ### What changes were proposed in this pull request? Running the command `gcli tag set --metalake metalake_demo` or `gcli tag remove --metalake metalake_demo` gives unexpected output.it should give some help information. ### Why are the changes needed? Fix: #5831 ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? local test + UT. ```bash gcli tag set -m demo_metalake # Missing --tag option. gcli tag set -m demo_metalake --tag tagA # Missing --name option. gcli tag remove -m demo_metalake # Missing --tag option. gcli tag remove -m demo_metalake --tag tagA # Missing --name option. gcli tag set --tag tagA --property test # Command cannot be executed. The set command only supports configuring tag properties or attaching tags to entity. gcli tag set --tag tagA --value val1 # Command cannot be executed. The set command only supports configuring tag properties or attaching tags to entity. gcli tag set --metalake demo_metalake --name Hive_catalog --tag tagB tagC # Hive_catalog now tagged with tagA,tagB,tagC gcli tag remove --metalake demo_metalake --name Hive_catalog --tag tagA tagC # Hive_catalog removed tag(s): [tagA, tagC], now tagged with [tagB] gcli tag remove --metalake demo_metalake --name Hive_catalog --tag tagA tagB tagC # Hive_catalog removed tag(s): [tagA, tagC], now tagged with [] ``` --------- Co-authored-by: Justin Mclean <[email protected]> Co-authored-by: Shaofeng Shi <[email protected]> Co-authored-by: roryqi <[email protected]> Co-authored-by: Xun <[email protected]> Co-authored-by: JUN <[email protected]> Co-authored-by: fsalhi2 <[email protected]> Co-authored-by: Jerry Shao <[email protected]> Co-authored-by: Cheng-Yi Shih <[email protected]> Co-authored-by: Qiming Teng <[email protected]> Co-authored-by: FANNG <[email protected]> Co-authored-by: Eric Chang <[email protected]> Co-authored-by: cai can <[email protected]> Co-authored-by: caican <[email protected]> Co-authored-by: Qi Yu <[email protected]> Co-authored-by: Jimmy Lee <[email protected]>
apache · Dec 31, 2024 · 9a5fef9 · 9a5fef9
1 parent 8273276
commit 9a5fef9
Show file tree

Hide file tree

Showing 5 changed files with 177 additions and 1 deletion.
diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/FullName.java b/clients/cli/src/main/java/org/apache/gravitino/cli/FullName.java
@@ -148,6 +148,15 @@ public String getName() {
     return null;
   }
 
+  /**
+   * Are there any names that can be retrieved?
+   *
+   * @return True if the name exists, or false if it does not.
+   */
+  public Boolean hasName() {
+    return line.hasOption(GravitinoOptions.NAME);
+  }
+
   /**
    * Helper method to retrieve a specific part of the full name based on the position of the part.
    *

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
@@ -667,7 +667,14 @@ protected void handleTagCommand() {
         if (propertySet != null && valueSet != null) {
           newSetTagProperty(url, ignore, metalake, getOneTag(tags), propertySet, valueSet).handle();
         } else if (propertySet == null && valueSet == null) {
+          if (!name.hasName()) {
+            System.err.println(ErrorMessages.MISSING_NAME);
+            Main.exit(-1);
+          }
           newTagEntity(url, ignore, metalake, name, tags).handle();
+        } else {
+          System.err.println("The set command only supports tag properties or attaching tags.");
+          Main.exit(-1);
         }
         break;
 
@@ -681,6 +688,10 @@ protected void handleTagCommand() {
           if (propertyRemove != null) {
             newRemoveTagProperty(url, ignore, metalake, getOneTag(tags), propertyRemove).handle();
           } else {
+            if (!name.hasName()) {
+              System.err.println(ErrorMessages.MISSING_NAME);
+              Main.exit(-1);
+            }
             newUntagEntity(url, ignore, metalake, name, tags).handle();
           }
         }

diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/UntagEntity.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/UntagEntity.java
@@ -19,6 +19,7 @@
 
 package org.apache.gravitino.cli.commands;
 
+import com.google.common.base.Joiner;
 import org.apache.gravitino.Catalog;
 import org.apache.gravitino.NameIdentifier;
 import org.apache.gravitino.Schema;
@@ -32,6 +33,7 @@
 import org.apache.gravitino.rel.Table;
 
 public class UntagEntity extends Command {
+  public static final Joiner COMMA_JOINER = Joiner.on(", ").skipNulls();
   protected final String metalake;
   protected final FullName name;
   protected final String[] tags;
@@ -91,7 +93,7 @@ public void handle() {
     } catch (NoSuchCatalogException err) {
       exitWithError(ErrorMessages.UNKNOWN_CATALOG);
     } catch (NoSuchSchemaException err) {
-      exitWithError(ErrorMessages.UNKNOWN_TABLE);
+      exitWithError(ErrorMessages.UNKNOWN_SCHEMA);
     } catch (NoSuchTableException err) {
       exitWithError(ErrorMessages.UNKNOWN_TABLE);
     } catch (Exception exp) {

diff --git a/clients/cli/src/test/java/org/apache/gravitino/cli/TestFulllName.java b/clients/cli/src/test/java/org/apache/gravitino/cli/TestFulllName.java
@@ -111,6 +111,21 @@ public void missingArgs() throws Exception {
     assertNull(namePart);
   }
 
+  @Test
+  public void hasPartName() throws ParseException {
+    String[] argsWithoutName = {"catalog", "details", "--metalake", "metalake"};
+    CommandLine commandLineWithoutName = new DefaultParser().parse(options, argsWithoutName);
+    FullName fullNameWithoutName = new FullName(commandLineWithoutName);
+    assertFalse(fullNameWithoutName.hasName());
+
+    String[] argsWithName = {
+      "catalog", "details", "--metalake", "metalake", "--name", "Hive_catalog"
+    };
+    CommandLine commandLineWithName = new DefaultParser().parse(options, argsWithName);
+    FullName fullNameWithName = new FullName(commandLineWithName);
+    assertTrue(fullNameWithName.hasName());
+  }
+
   @Test
   public void hasPartNameMetalake() throws Exception {
     String[] args = {"metalake", "details", "--metalake", "metalake"};

diff --git a/clients/cli/src/test/java/org/apache/gravitino/cli/TestTagCommands.java b/clients/cli/src/test/java/org/apache/gravitino/cli/TestTagCommands.java
@@ -22,6 +22,7 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertThrows;
 import static org.mockito.ArgumentMatchers.argThat;
+import static org.mockito.ArgumentMatchers.isNull;
 import static org.mockito.Mockito.any;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.eq;
@@ -141,6 +142,30 @@ void testCreateTagCommand() {
     verify(mockCreate).handle();
   }
 
+  @Test
+  void testCreateCommandWithoutTagOption() {
+    Main.useExit = false;
+    when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true);
+    when(mockCommandLine.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo");
+    when(mockCommandLine.hasOption(GravitinoOptions.TAG)).thenReturn(false);
+
+    GravitinoCommandLine commandLine =
+        spy(
+            new GravitinoCommandLine(
+                mockCommandLine, mockOptions, CommandEntities.TAG, CommandActions.CREATE));
+
+    assertThrows(RuntimeException.class, commandLine::handleCommandLine);
+    verify(commandLine, never())
+        .newCreateTags(
+            eq(GravitinoCommandLine.DEFAULT_URL),
+            eq(false),
+            eq("metalake_demo"),
+            isNull(),
+            isNull());
+    String output = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim();
+    assertEquals(output, ErrorMessages.MISSING_TAG);
+  }
+
   @Test
   void testCreateTagsCommand() {
     CreateTag mockCreate = mock(CreateTag.class);
@@ -272,6 +297,62 @@ void testSetTagPropertyCommand() {
     verify(mockSetProperty).handle();
   }
 
+  @Test
+  void testSetTagPropertyCommandWithoutPropertyOption() {
+    Main.useExit = false;
+    when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true);
+    when(mockCommandLine.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo");
+    when(mockCommandLine.hasOption(GravitinoOptions.TAG)).thenReturn(true);
+    when(mockCommandLine.getOptionValues(GravitinoOptions.TAG)).thenReturn(new String[] {"tagA"});
+    when(mockCommandLine.hasOption(GravitinoOptions.PROPERTY)).thenReturn(false);
+    when(mockCommandLine.hasOption(GravitinoOptions.VALUE)).thenReturn(true);
+    when(mockCommandLine.getOptionValue(GravitinoOptions.VALUE)).thenReturn("value");
+    GravitinoCommandLine commandLine =
+        spy(
+            new GravitinoCommandLine(
+                mockCommandLine, mockOptions, CommandEntities.TAG, CommandActions.SET));
+
+    assertThrows(RuntimeException.class, commandLine::handleCommandLine);
+    verify(commandLine, never())
+        .newSetTagProperty(
+            eq(GravitinoCommandLine.DEFAULT_URL),
+            eq(false),
+            eq("metalake_demo"),
+            eq("tagA"),
+            isNull(),
+            eq("value"));
+    String output = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim();
+    assertEquals(output, "The set command only supports tag properties or attaching tags.");
+  }
+
+  @Test
+  void testSetTagPropertyCommandWithoutValueOption() {
+    Main.useExit = false;
+    when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true);
+    when(mockCommandLine.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo");
+    when(mockCommandLine.hasOption(GravitinoOptions.TAG)).thenReturn(true);
+    when(mockCommandLine.getOptionValues(GravitinoOptions.TAG)).thenReturn(new String[] {"tagA"});
+    when(mockCommandLine.hasOption(GravitinoOptions.PROPERTY)).thenReturn(true);
+    when(mockCommandLine.getOptionValue(GravitinoOptions.PROPERTY)).thenReturn("property");
+    when(mockCommandLine.hasOption(GravitinoOptions.VALUE)).thenReturn(false);
+    GravitinoCommandLine commandLine =
+        spy(
+            new GravitinoCommandLine(
+                mockCommandLine, mockOptions, CommandEntities.TAG, CommandActions.SET));
+
+    assertThrows(RuntimeException.class, commandLine::handleCommandLine);
+    verify(commandLine, never())
+        .newSetTagProperty(
+            eq(GravitinoCommandLine.DEFAULT_URL),
+            eq(false),
+            eq("metalake_demo"),
+            eq("tagA"),
+            eq("property"),
+            isNull());
+    String output = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim();
+    assertEquals(output, "The set command only supports tag properties or attaching tags.");
+  }
+
   @Test
   void testSetMultipleTagPropertyCommandError() {
     when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true);
@@ -339,6 +420,7 @@ void testDeleteAllTagCommand() {
     when(mockCommandLine.hasOption(GravitinoOptions.TAG)).thenReturn(false);
     when(mockCommandLine.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo");
     when(mockCommandLine.hasOption(GravitinoOptions.FORCE)).thenReturn(true);
+    when(mockCommandLine.hasOption(GravitinoOptions.NAME)).thenReturn(true);
     when(mockCommandLine.getOptionValue(GravitinoOptions.NAME)).thenReturn("catalog.schema.table");
     GravitinoCommandLine commandLine =
         spy(
@@ -447,6 +529,32 @@ public boolean matches(String[] argument) {
     verify(mockTagEntity).handle();
   }
 
+  @Test
+  void testTagEntityCommandWithoutName() {
+    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);
+    when(mockCommandLine.hasOption(GravitinoOptions.TAG)).thenReturn(true);
+    when(mockCommandLine.getOptionValues(GravitinoOptions.TAG)).thenReturn(new String[] {"tagA"});
+    GravitinoCommandLine commandLine =
+        spy(
+            new GravitinoCommandLine(
+                mockCommandLine, mockOptions, CommandEntities.TAG, CommandActions.SET));
+
+    assertThrows(RuntimeException.class, commandLine::handleCommandLine);
+    verify(commandLine, never())
+        .newTagEntity(
+            eq(GravitinoCommandLine.DEFAULT_URL),
+            eq(false),
+            eq("metalake_demo"),
+            isNull(),
+            argThat(
+                argument -> argument != null && argument.length > 0 && "tagA".equals(argument[0])));
+    String output = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim();
+    assertEquals(output, ErrorMessages.MISSING_NAME);
+  }
+
   @Test
   void testTagsEntityCommand() {
     TagEntity mockTagEntity = mock(TagEntity.class);
@@ -516,6 +624,37 @@ public boolean matches(String[] argument) {
     verify(mockUntagEntity).handle();
   }
 
+  @Test
+  void testUntagEntityCommandWithoutName() {
+    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);
+    when(mockCommandLine.hasOption(GravitinoOptions.TAG)).thenReturn(true);
+    when(mockCommandLine.getOptionValues(GravitinoOptions.TAG))
+        .thenReturn(new String[] {"tagA", "tagB"});
+    GravitinoCommandLine commandLine =
+        spy(
+            new GravitinoCommandLine(
+                mockCommandLine, mockOptions, CommandEntities.TAG, CommandActions.REMOVE));
+
+    assertThrows(RuntimeException.class, commandLine::handleCommandLine);
+    verify(commandLine, never())
+        .newUntagEntity(
+            eq(GravitinoCommandLine.DEFAULT_URL),
+            eq(false),
+            eq("metalake_demo"),
+            isNull(),
+            argThat(
+                argument ->
+                    argument != null
+                        && argument.length > 0
+                        && "tagA".equals(argument[0])
+                        && "tagB".equals(argument[1])));
+    String output = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim();
+    assertEquals(output, ErrorMessages.MISSING_NAME);
+  }
+
   @Test
   void testUntagsEntityCommand() {
     UntagEntity mockUntagEntity = mock(UntagEntity.class);