[apache#5985] improvement(CLI): Fix role command that supports handli…

…ng multiple values Fix some issue due to reviewer's opinion.
Abyss-lord · Dec 26, 2024 · 22d34a1 · 22d34a1
1 parent b96f648
commit 22d34a1
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 20 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
@@ -43,8 +43,6 @@ public class ErrorMessages {
   public static final String UNKNOWN_TAG = "Unknown tag.";
   public static final String MULTIPLE_TAG_COMMAND_ERROR =
       "Error: The current command only supports one --tag option.";
-  public static final String MULTIPLE_ROLE_COMMAND_ERROR =
-      "Error: The current command only supports one --role option.";
   public static final String TAG_EXISTS = "Tag already exists.";
   public static final String UNKNOWN_COLUMN = "Unknown column.";
   public static final String COLUMN_EXISTS = "Column 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
@@ -750,9 +750,9 @@ protected void handleRoleCommand() {
     switch (command) {
       case CommandActions.DETAILS:
         if (line.hasOption(GravitinoOptions.AUDIT)) {
-          newRoleAudit(url, ignore, metalake, getOneRole(roles)).handle();
+          newRoleAudit(url, ignore, metalake, getOneRole(roles, CommandActions.DETAILS)).handle();
         } else {
-          newRoleDetails(url, ignore, metalake, getOneRole(roles)).handle();
+          newRoleDetails(url, ignore, metalake, getOneRole(roles, CommandActions.DETAILS)).handle();
         }
         break;
 
@@ -770,12 +770,14 @@ protected void handleRoleCommand() {
         break;
 
       case CommandActions.GRANT:
-        newGrantPrivilegesToRole(url, ignore, metalake, getOneRole(roles), name, privileges)
+        newGrantPrivilegesToRole(
+                url, ignore, metalake, getOneRole(roles, CommandActions.GRANT), name, privileges)
             .handle();
         break;
 
       case CommandActions.REVOKE:
-        newRevokePrivilegesFromRole(url, ignore, metalake, getOneRole(roles), name, privileges)
+        newRevokePrivilegesFromRole(
+                url, ignore, metalake, getOneRole(roles, CommandActions.REMOVE), name, privileges)
             .handle();
         break;
 
@@ -786,8 +788,9 @@ protected void handleRoleCommand() {
     }
   }
 
-  private String getOneRole(String[] roles) {
-    Preconditions.checkArgument(roles.length <= 1, ErrorMessages.MULTIPLE_ROLE_COMMAND_ERROR);
+  private String getOneRole(String[] roles, String command) {
+    Preconditions.checkArgument(
+        roles.length == 1, command + " requires only one role, but multiple are currently passed.");
     return roles[0];
   }
 

diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/DeleteRole.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/DeleteRole.java
@@ -20,14 +20,16 @@
 package org.apache.gravitino.cli.commands;
 
 import com.google.common.base.Joiner;
+import com.google.common.collect.Lists;
+import java.util.List;
 import org.apache.gravitino.cli.AreYouSure;
 import org.apache.gravitino.cli.ErrorMessages;
 import org.apache.gravitino.client.GravitinoClient;
 import org.apache.gravitino.exceptions.NoSuchMetalakeException;
 import org.apache.gravitino.exceptions.NoSuchRoleException;
 
 public class DeleteRole extends Command {
-
+  public static final Joiner COMMA_JOINER = Joiner.on(", ").skipNulls();
   protected String metalake;
   protected String[] roles;
   protected boolean force;
@@ -52,16 +54,16 @@ public DeleteRole(
   /** Delete a role. */
   @Override
   public void handle() {
-    boolean deleted = true;
-
     if (!AreYouSure.really(force)) {
       return;
     }
+    List<String> failedRoles = Lists.newArrayList();
+    List<String> successRoles = Lists.newArrayList();
 
     try {
       GravitinoClient client = buildClient(metalake);
       for (String role : roles) {
-        deleted &= client.deleteRole(role);
+        (client.deleteRole(role) ? successRoles : failedRoles).add(role);
       }
     } catch (NoSuchMetalakeException err) {
       exitWithError(ErrorMessages.UNKNOWN_METALAKE);
@@ -71,10 +73,15 @@ public void handle() {
       exitWithError(exp.getMessage());
     }
 
-    if (deleted) {
-      System.out.println(Joiner.on(", ").join(roles) + " deleted.");
+    if (failedRoles.isEmpty()) {
+      System.out.println(COMMA_JOINER.join(successRoles) + " deleted.");
     } else {
-      System.out.println(Joiner.on(", ").join(roles) + " not deleted.");
+      System.err.println(
+          COMMA_JOINER.join(successRoles)
+              + " deleted, "
+              + "but "
+              + COMMA_JOINER.join(failedRoles)
+              + " is not deleted.");
     }
   }
 }
diff --git a/clients/cli/src/test/java/org/apache/gravitino/cli/TestRoleCommands.java b/clients/cli/src/test/java/org/apache/gravitino/cli/TestRoleCommands.java
@@ -62,11 +62,6 @@ void setUp() {
     System.setErr(new PrintStream(errContent));
   }
 
-  @AfterEach
-  void restoreExitFlg() {
-    Main.useExit = true;
-  }
-
   @AfterEach
   public void restoreStreams() {
     System.setOut(originalOut);
@@ -107,6 +102,24 @@ void testRoleDetailsCommand() {
     verify(mockDetails).handle();
   }
 
+  @Test
+  void testRoleDetailsCommandWithMultipleRoles() {
+    when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true);
+    when(mockCommandLine.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo");
+    when(mockCommandLine.hasOption(GravitinoOptions.ROLE)).thenReturn(true);
+    when(mockCommandLine.getOptionValues(GravitinoOptions.ROLE))
+        .thenReturn(new String[] {"admin", "roleA", "roleB"});
+    GravitinoCommandLine commandLine =
+        spy(
+            new GravitinoCommandLine(
+                mockCommandLine, mockOptions, CommandEntities.ROLE, CommandActions.DETAILS));
+
+    assertThrows(IllegalArgumentException.class, commandLine::handleCommandLine);
+    verify(commandLine, never())
+        .newRoleDetails(
+            eq(GravitinoCommandLine.DEFAULT_URL), eq(false), eq("metalake_demo"), any());
+  }
+
   @Test
   void testRoleAuditCommand() {
     RoleAudit mockAudit = mock(RoleAudit.class);