Skip to content

Commit

Permalink
[apache#6123] fix(CLI): Refactor the validation logic of tag and role
Browse files Browse the repository at this point in the history 
Refactor the validation logic of the Role and fix the test case.
  • Loading branch information
@Abyss-lord
Abyss-lord committed Jan 7, 2025
1 parent 8e48de3 commit dbd50ec
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 14 deletions.
3 changes: 3 additions & 0 deletions clients/cli/src/main/java/org/apache/gravitino/cli/ErrorMessages.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ public class ErrorMessages {
public static final String MISSING_GROUP = "Missing --group option.";
public static final String MISSING_METALAKE = "Missing --metalake option.";
public static final String MISSING_NAME = "Missing --name option.";
public static final String MISSING_PRIVILEGES = "Missing --privilege option.";
public static final String MISSING_PROPERTY = "Missing --property option.";
public static final String MISSING_PROPERTY_AND_VALUE = "Missing --property and --value options.";
public static final String MISSING_ROLE = "Missing --role option.";
Expand All @@ -63,6 +64,8 @@ public class ErrorMessages {
public static final String MISSING_USER = "Missing --user option.";
public static final String MISSING_VALUE = "Missing --value option.";

public static final String MULTIPLE_ROLE_COMMAND_ERROR =
"This command only supports one --role option.";
public static final String MULTIPLE_TAG_COMMAND_ERROR =
"This command only supports one --tag option.";
public static final String MISSING_PROVIDER = "Missing --provider option.";
Expand Down
28 changes: 15 additions & 13 deletions clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
package org.apache.gravitino.cli;

import com.google.common.base.Joiner;
import com.google.common.base.Preconditions;
import com.google.common.collect.Lists;
import java.io.BufferedReader;
import java.io.IOException;
Expand Down Expand Up @@ -767,34 +766,34 @@ protected void handleRoleCommand() {
switch (command) {
case CommandActions.DETAILS:
if (line.hasOption(GravitinoOptions.AUDIT)) {
newRoleAudit(url, ignore, metalake, getOneRole(roles, CommandActions.DETAILS)).handle();
newRoleAudit(url, ignore, metalake, getOneRole(roles)).validate().handle();
} else {
newRoleDetails(url, ignore, metalake, getOneRole(roles, CommandActions.DETAILS)).handle();
newRoleDetails(url, ignore, metalake, getOneRole(roles)).validate().handle();
}
break;

case CommandActions.LIST:
newListRoles(url, ignore, metalake).handle();
newListRoles(url, ignore, metalake).validate().handle();
break;

case CommandActions.CREATE:
newCreateRole(url, ignore, metalake, roles).handle();
newCreateRole(url, ignore, metalake, roles).validate().handle();
break;

case CommandActions.DELETE:
boolean forceDelete = line.hasOption(GravitinoOptions.FORCE);
newDeleteRole(url, ignore, forceDelete, metalake, roles).handle();
newDeleteRole(url, ignore, forceDelete, metalake, roles).validate().handle();
break;

case CommandActions.GRANT:
newGrantPrivilegesToRole(
url, ignore, metalake, getOneRole(roles, CommandActions.GRANT), name, privileges)
newGrantPrivilegesToRole(url, ignore, metalake, getOneRole(roles), name, privileges)
.validate()
.handle();
break;

case CommandActions.REVOKE:
newRevokePrivilegesFromRole(
url, ignore, metalake, getOneRole(roles, CommandActions.REMOVE), name, privileges)
newRevokePrivilegesFromRole(url, ignore, metalake, getOneRole(roles), name, privileges)
.validate()
.handle();
break;

Expand All @@ -805,9 +804,12 @@ url, ignore, metalake, getOneRole(roles, CommandActions.REMOVE), name, privilege
}
}

private String getOneRole(String[] roles, String command) {
Preconditions.checkArgument(
roles.length == 1, command + " requires only one role, but multiple are currently passed.");
private String getOneRole(String[] roles) {
if (roles == null || roles.length != 1) {
System.err.println(ErrorMessages.MULTIPLE_ROLE_COMMAND_ERROR);
Main.exit(-1);
}

return roles[0];
}

Expand Down
6 changes: 6 additions & 0 deletions clients/cli/src/main/java/org/apache/gravitino/cli/commands/GrantPrivilegesToRole.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,4 +103,10 @@ public void handle() {
String all = String.join(",", privileges);
System.out.println(role + " granted " + all + " on " + entity.getName());
}

@Override
public Command validate() {
if (privileges == null) exitWithError(ErrorMessages.MISSING_PRIVILEGES);
return super.validate();
}
}
6 changes: 6 additions & 0 deletions clients/cli/src/main/java/org/apache/gravitino/cli/commands/RevokePrivilegesFromRole.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,4 +103,10 @@ public void handle() {
String all = String.join(",", privileges);
System.out.println(role + " revoked " + all + " on " + entity.getName());
}

@Override
public Command validate() {
if (privileges == null) exitWithError(ErrorMessages.MISSING_PRIVILEGES);
return super.validate();
}
}
39 changes: 38 additions & 1 deletion clients/cli/src/test/java/org/apache/gravitino/cli/TestRoleCommands.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ void testListRolesCommand() {
doReturn(mockList)
.when(commandLine)
.newListRoles(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo");
doReturn(mockList).when(mockList).validate();
commandLine.handleCommandLine();
verify(mockList).handle();
}
Expand All @@ -98,12 +99,14 @@ void testRoleDetailsCommand() {
doReturn(mockDetails)
.when(commandLine)
.newRoleDetails(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "admin");
doReturn(mockDetails).when(mockDetails).validate();
commandLine.handleCommandLine();
verify(mockDetails).handle();
}

@Test
void testRoleDetailsCommandWithMultipleRoles() {
Main.useExit = false;
when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true);
when(mockCommandLine.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo");
when(mockCommandLine.hasOption(GravitinoOptions.ROLE)).thenReturn(true);
Expand All @@ -114,7 +117,7 @@ void testRoleDetailsCommandWithMultipleRoles() {
new GravitinoCommandLine(
mockCommandLine, mockOptions, CommandEntities.ROLE, CommandActions.DETAILS));

assertThrows(IllegalArgumentException.class, commandLine::handleCommandLine);
assertThrows(RuntimeException.class, commandLine::handleCommandLine);
verify(commandLine, never())
.newRoleDetails(
eq(GravitinoCommandLine.DEFAULT_URL), eq(false), eq("metalake_demo"), any());
Expand All @@ -135,6 +138,7 @@ void testRoleAuditCommand() {
doReturn(mockAudit)
.when(commandLine)
.newRoleAudit(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "group");
doReturn(mockAudit).when(mockAudit).validate();
commandLine.handleCommandLine();
verify(mockAudit).handle();
}
Expand All @@ -154,6 +158,7 @@ void testCreateRoleCommand() {
.when(commandLine)
.newCreateRole(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", new String[] {"admin"});
doReturn(mockCreate).when(mockCreate).validate();
commandLine.handleCommandLine();
verify(mockCreate).handle();
}
Expand All @@ -178,6 +183,7 @@ void testCreateRolesCommand() {
eq(false),
eq("metalake_demo"),
eq(new String[] {"admin", "engineer", "scientist"}));
doReturn(mockCreate).when(mockCreate).validate();
commandLine.handleCommandLine();
verify(mockCreate).handle();
}
Expand All @@ -201,6 +207,7 @@ void testDeleteRoleCommand() {
false,
"metalake_demo",
new String[] {"admin"});
doReturn(mockDelete).when(mockDelete).validate();
commandLine.handleCommandLine();
verify(mockDelete).handle();
}
Expand All @@ -227,6 +234,7 @@ void testDeleteRolesCommand() {
eq(false),
eq("metalake_demo"),
eq(new String[] {"admin", "engineer", "scientist"}));
doReturn(mockDelete).when(mockDelete).validate();
commandLine.handleCommandLine();
verify(mockDelete).handle();
}
Expand All @@ -247,6 +255,7 @@ void testDeleteRoleForceCommand() {
.when(commandLine)
.newDeleteRole(
GravitinoCommandLine.DEFAULT_URL, false, true, "metalake_demo", new String[] {"admin"});
doReturn(mockDelete).when(mockDelete).validate();
commandLine.handleCommandLine();
verify(mockDelete).handle();
}
Expand Down Expand Up @@ -276,10 +285,24 @@ void testGrantPrivilegesToRole() {
eq("admin"),
any(),
eq(privileges));
doReturn(mockGrant).when(mockGrant).validate();
commandLine.handleCommandLine();
verify(mockGrant).handle();
}

@Test
void testGrantPrivilegesToRoleWithoutPrivileges() {
Main.useExit = false;
GrantPrivilegesToRole spyGrantRole =
spy(
new GrantPrivilegesToRole(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "admin", null, null));
assertThrows(RuntimeException.class, spyGrantRole::validate);
verify(spyGrantRole, never()).handle();
String errOutput = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim();
assertEquals(ErrorMessages.MISSING_PRIVILEGES, errOutput);
}

@Test
void testRevokePrivilegesFromRole() {
RevokePrivilegesFromRole mockRevoke = mock(RevokePrivilegesFromRole.class);
Expand All @@ -305,10 +328,24 @@ void testRevokePrivilegesFromRole() {
eq("admin"),
any(),
eq(privileges));
doReturn(mockRevoke).when(mockRevoke).validate();
commandLine.handleCommandLine();
verify(mockRevoke).handle();
}

@Test
void testRevokePrivilegesFromRoleWithoutPrivileges() {
Main.useExit = false;
RevokePrivilegesFromRole spyGrantRole =
spy(
new RevokePrivilegesFromRole(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "admin", null, null));
assertThrows(RuntimeException.class, spyGrantRole::validate);
verify(spyGrantRole, never()).handle();
String errOutput = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim();
assertEquals(ErrorMessages.MISSING_PRIVILEGES, errOutput);
}

@Test
void testDeleteRoleCommandWithoutRole() {
Main.useExit = false;
Expand Down

0 comments on commit dbd50ec

Please sign in to comment.