Skip to content

Commit

Permalink
[#5985] improvement(CLI): Fix role command that supports handling mul…
Browse files Browse the repository at this point in the history
…tiple values (#5988)

### What changes were proposed in this pull request?

In `GravitinoOptions`, the role option supports multiple values, but the
handleRoleCommand implementation currently only processes a single role
value. CLI should support create and delete multiple roles
simultaneously.


### Why are the changes needed?

Fix: #5985 

### Does this PR introduce _any_ user-facing change?

NO

### How was this patch tested?

local test + UT 

```bash
gcli role create -m demo_metalake --role role1 role2 role3
# role1, role2, role3 created

gcli role delete -m demo_metalake --role role1 role2 role3
# role1, role2, role3 deleted.

gcli role delete -m demo_metalake --role role1 role2 role3 unknown
# role1, role2, role3 deleted, but unknown is not deleted.

gcli role details -m demo_metalake
# Missing --role option.

gcli role details -m demo_metalake --role roleA roleB
# Exception in thread "main" java.lang.IllegalArgumentException: details requires only one role, but multiple are currently passed.
```
  • Loading branch information
Abyss-lord authored Dec 28, 2024
1 parent 486c042 commit f6e201b
Show file tree
Hide file tree
Showing 6 changed files with 184 additions and 38 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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_ROLE = "Missing --role 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.";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -720,17 +720,26 @@ protected void handleRoleCommand() {
String userName = line.getOptionValue(GravitinoOptions.LOGIN);
FullName name = new FullName(line);
String metalake = name.getMetalakeName();
String role = line.getOptionValue(GravitinoOptions.ROLE);
String[] privileges = line.getOptionValues(GravitinoOptions.PRIVILEGE);

Command.setAuthenticationMode(auth, userName);

String[] roles = line.getOptionValues(GravitinoOptions.ROLE);
if (roles == null && !CommandActions.LIST.equals(command)) {
System.err.println(ErrorMessages.MISSING_ROLE);
Main.exit(-1);
}

if (roles != null) {
roles = Arrays.stream(roles).distinct().toArray(String[]::new);
}

switch (command) {
case CommandActions.DETAILS:
if (line.hasOption(GravitinoOptions.AUDIT)) {
newRoleAudit(url, ignore, metalake, role).handle();
newRoleAudit(url, ignore, metalake, getOneRole(roles, CommandActions.DETAILS)).handle();
} else {
newRoleDetails(url, ignore, metalake, role).handle();
newRoleDetails(url, ignore, metalake, getOneRole(roles, CommandActions.DETAILS)).handle();
}
break;

Expand All @@ -739,20 +748,24 @@ protected void handleRoleCommand() {
break;

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

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

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

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

default:
Expand All @@ -762,6 +775,12 @@ protected void handleRoleCommand() {
}
}

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];
}

/**
* Handles the command execution for Columns based on command type and the command line options.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -468,13 +468,13 @@ protected RoleAudit newRoleAudit(String url, boolean ignore, String metalake, St
return new RoleAudit(url, ignore, metalake, role);
}

protected CreateRole newCreateRole(String url, boolean ignore, String metalake, String role) {
return new CreateRole(url, ignore, metalake, role);
protected CreateRole newCreateRole(String url, boolean ignore, String metalake, String[] roles) {
return new CreateRole(url, ignore, metalake, roles);
}

protected DeleteRole newDeleteRole(
String url, boolean ignore, boolean force, String metalake, String role) {
return new DeleteRole(url, ignore, force, metalake, role);
String url, boolean ignore, boolean force, String metalake, String[] roles) {
return new DeleteRole(url, ignore, force, metalake, roles);
}

protected TagDetails newTagDetails(String url, boolean ignore, String metalake, String tag) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package org.apache.gravitino.cli.commands;

import com.google.common.base.Joiner;
import java.util.Collections;
import org.apache.gravitino.cli.ErrorMessages;
import org.apache.gravitino.client.GravitinoClient;
Expand All @@ -27,28 +28,30 @@

public class CreateRole extends Command {
protected String metalake;
protected String role;
protected String[] roles;

/**
* Create a new role.
*
* @param url The URL of the Gravitino server.
* @param ignoreVersions If true don't check the client/server versions match.
* @param metalake The name of the metalake.
* @param role The name of the role.
* @param roles The array of roles.
*/
public CreateRole(String url, boolean ignoreVersions, String metalake, String role) {
public CreateRole(String url, boolean ignoreVersions, String metalake, String[] roles) {
super(url, ignoreVersions);
this.metalake = metalake;
this.role = role;
this.roles = roles;
}

/** Create a new role. */
@Override
public void handle() {
try {
GravitinoClient client = buildClient(metalake);
client.createRole(role, null, Collections.EMPTY_LIST);
for (String role : roles) {
client.createRole(role, null, Collections.EMPTY_LIST);
}
} catch (NoSuchMetalakeException err) {
exitWithError(ErrorMessages.UNKNOWN_METALAKE);
} catch (RoleAlreadyExistsException err) {
Expand All @@ -57,6 +60,6 @@ public void handle() {
exitWithError(exp.getMessage());
}

System.out.println(role + " created");
System.out.println(Joiner.on(", ").join(roles) + " created");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,19 @@

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 role;
protected String[] roles;
protected boolean force;

/**
Expand All @@ -38,28 +41,30 @@ public class DeleteRole extends Command {
* @param ignoreVersions If true don't check the client/server versions match.
* @param force Force operation.
* @param metalake The name of the metalake.
* @param role The name of the role.
* @param roles The name of the role.
*/
public DeleteRole(
String url, boolean ignoreVersions, boolean force, String metalake, String role) {
String url, boolean ignoreVersions, boolean force, String metalake, String[] roles) {
super(url, ignoreVersions);
this.metalake = metalake;
this.force = force;
this.role = role;
this.roles = roles;
}

/** Delete a role. */
@Override
public void handle() {
boolean deleted = false;

if (!AreYouSure.really(force)) {
return;
}
List<String> failedRoles = Lists.newArrayList();
List<String> successRoles = Lists.newArrayList();

try {
GravitinoClient client = buildClient(metalake);
deleted = client.deleteRole(role);
for (String role : roles) {
(client.deleteRole(role) ? successRoles : failedRoles).add(role);
}
} catch (NoSuchMetalakeException err) {
exitWithError(ErrorMessages.UNKNOWN_METALAKE);
} catch (NoSuchRoleException err) {
Expand All @@ -68,10 +73,15 @@ public void handle() {
exitWithError(exp.getMessage());
}

if (deleted) {
System.out.println(role + " deleted.");
if (failedRoles.isEmpty()) {
System.out.println(COMMA_JOINER.join(successRoles) + " deleted.");
} else {
System.out.println(role + " not deleted.");
System.err.println(
COMMA_JOINER.join(successRoles)
+ " deleted, "
+ "but "
+ COMMA_JOINER.join(failedRoles)
+ " is not deleted.");
}
}
}
Loading

0 comments on commit f6e201b

Please sign in to comment.