Skip to content

Commit

Permalink
[#6086] fix(CLI): Refactor the validation logic of Metalake (#6091)
Browse files Browse the repository at this point in the history
### What changes were proposed in this pull request?

Add `validate` method to Command, and refactor the validation code.

### Why are the changes needed?

Fix: #6086 

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

No.

### How was this patch tested?

#### UT

local ut

#### bash

```bash
gcli metalake set
# Missing --metalake option.

gcli metalake details
# Missing --metalake option.

gcli metalake set -m demo_metalake
# Missing --property and --value options.

gcli metalake set -m demo_metalake --property propertyA
# Missing --value option.

gcli metalake set -m demo_metalake --value valA
# Missing --property option.

gcli metalake details --audit
# Missing --metalake option.

gcli metalake remove -m demo_metalake
# Missing --property option.

gcli metalake update -m demo_metalake
# The command does nothing.
```

---------

Co-authored-by: roryqi <[email protected]>
Co-authored-by: Yuhui <[email protected]>
Co-authored-by: Qiming Teng <[email protected]>
  • Loading branch information
4 people authored Jan 5, 2025
1 parent 4c886eb commit c16f595
Show file tree
Hide file tree
Showing 8 changed files with 126 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ public class ErrorMessages {
public static final String MISSING_ROLE = "Missing --role option.";
public static final String MISSING_TAG = "Missing --tag option.";
public static final String MISSING_URI = "Missing --uri option.";
public static final String MISSING_PROPERTY = "Missing --property option.";
public static final String MISSING_VALUE = "Missing --value 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.";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ public String getMetalakeName() {
}

System.err.println(ErrorMessages.MISSING_METALAKE);
Main.exit(-1);

return null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import org.apache.commons.cli.CommandLine;
import org.apache.commons.cli.HelpFormatter;
import org.apache.commons.cli.Options;
Expand Down Expand Up @@ -167,51 +166,49 @@ private void handleMetalakeCommand() {
String auth = getAuth();
String userName = line.getOptionValue(GravitinoOptions.LOGIN);
FullName name = new FullName(line);
String metalake = name.getMetalakeName();
String outputFormat = line.getOptionValue(GravitinoOptions.OUTPUT);

Command.setAuthenticationMode(auth, userName);

if (CommandActions.LIST.equals(command)) {
newListMetalakes(url, ignore, outputFormat).validate().handle();
return;
}

String metalake = name.getMetalakeName();

switch (command) {
case CommandActions.DETAILS:
if (line.hasOption(GravitinoOptions.AUDIT)) {
newMetalakeAudit(url, ignore, metalake).handle();
newMetalakeAudit(url, ignore, metalake).validate().handle();
} else {
newMetalakeDetails(url, ignore, outputFormat, metalake).handle();
newMetalakeDetails(url, ignore, outputFormat, metalake).validate().handle();
}
break;

case CommandActions.LIST:
newListMetalakes(url, ignore, outputFormat).handle();
break;

case CommandActions.CREATE:
if (Objects.isNull(metalake)) {
System.err.println(CommandEntities.METALAKE + " is not defined");
Main.exit(-1);
}
String comment = line.getOptionValue(GravitinoOptions.COMMENT);
newCreateMetalake(url, ignore, metalake, comment).handle();
newCreateMetalake(url, ignore, metalake, comment).validate().handle();
break;

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

case CommandActions.SET:
String property = line.getOptionValue(GravitinoOptions.PROPERTY);
String value = line.getOptionValue(GravitinoOptions.VALUE);
newSetMetalakeProperty(url, ignore, metalake, property, value).handle();
newSetMetalakeProperty(url, ignore, metalake, property, value).validate().handle();
break;

case CommandActions.REMOVE:
property = line.getOptionValue(GravitinoOptions.PROPERTY);
newRemoveMetalakeProperty(url, ignore, metalake, property).handle();
newRemoveMetalakeProperty(url, ignore, metalake, property).validate().handle();
break;

case CommandActions.PROPERTIES:
newListMetalakeProperties(url, ignore, metalake).handle();
newListMetalakeProperties(url, ignore, metalake).validate().handle();
break;

case CommandActions.UPDATE:
Expand All @@ -221,21 +218,22 @@ private void handleMetalakeCommand() {
}
if (line.hasOption(GravitinoOptions.ENABLE)) {
boolean enableAllCatalogs = line.hasOption(GravitinoOptions.ALL);
newMetalakeEnable(url, ignore, metalake, enableAllCatalogs).handle();
newMetalakeEnable(url, ignore, metalake, enableAllCatalogs).validate().handle();
}
if (line.hasOption(GravitinoOptions.DISABLE)) {
newMetalakeDisable(url, ignore, metalake).handle();
newMetalakeDisable(url, ignore, metalake).validate().handle();
}

if (line.hasOption(GravitinoOptions.COMMENT)) {
comment = line.getOptionValue(GravitinoOptions.COMMENT);
newUpdateMetalakeComment(url, ignore, metalake, comment).handle();
newUpdateMetalakeComment(url, ignore, metalake, comment).validate().handle();
}
if (line.hasOption(GravitinoOptions.RENAME)) {
String newName = line.getOptionValue(GravitinoOptions.RENAME);
force = line.hasOption(GravitinoOptions.FORCE);
newUpdateMetalakeName(url, ignore, force, metalake, newName).handle();
newUpdateMetalakeName(url, ignore, force, metalake, newName).validate().handle();
}

break;

default:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

import static org.apache.gravitino.client.GravitinoClientBase.Builder;

import com.google.common.base.Joiner;
import java.io.File;
import org.apache.gravitino.cli.GravitinoConfig;
import org.apache.gravitino.cli.KerberosData;
Expand All @@ -39,14 +40,14 @@
public abstract class Command {
public static final String OUTPUT_FORMAT_TABLE = "table";
public static final String OUTPUT_FORMAT_PLAIN = "plain";
public static final Joiner COMMA_JOINER = Joiner.on(", ").skipNulls();

protected static String authentication = null;
protected static String userName = null;

private static final String SIMPLE_AUTH = "simple";
private static final String OAUTH_AUTH = "oauth";
private static final String KERBEROS_AUTH = "kerberos";

private final String url;
private final boolean ignoreVersions;
private final String outputFormat;
Expand Down Expand Up @@ -99,6 +100,16 @@ public static void setAuthenticationMode(String authentication, String userName)

/** All commands have a handle method to handle and run the required command. */
public abstract void handle();

/**
* verify the arguments. All commands have a verify method to verify the arguments.
*
* @return Returns itself via argument validation, otherwise exits.
*/
public Command validate() {
return this;
}

/**
* Builds a {@link GravitinoClient} instance with the provided server URL and metalake.
*
Expand Down Expand Up @@ -192,4 +203,8 @@ protected <T> void output(T entity) {
throw new IllegalArgumentException("Unsupported output format");
}
}

protected String getMissingEntitiesInfo(String... entities) {
return "Missing required argument(s): " + COMMA_JOINER.join(entities);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -60,4 +60,10 @@ public void handle() {

System.out.println(property + " property removed.");
}

@Override
public Command validate() {
if (property == null) exitWithError(ErrorMessages.MISSING_PROPERTY);
return this;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,12 @@ public void handle() {

System.out.println(metalake + " property set.");
}

@Override
public Command validate() {
if (property == null && value == null) exitWithError("Missing --property and --value options.");
if (property == null) exitWithError(ErrorMessages.MISSING_PROPERTY);
if (value == null) exitWithError(ErrorMessages.MISSING_VALUE);
return this;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ public class TestFulllName {

@BeforeEach
public void setUp() {
Main.useExit = false;
options = new GravitinoOptions().options();
System.setOut(new PrintStream(outContent));
System.setErr(new PrintStream(errContent));
Expand Down Expand Up @@ -82,8 +83,7 @@ public void entityNotFound() throws Exception {
CommandLine commandLine = new DefaultParser().parse(options, args);
FullName fullName = new FullName(commandLine);

String metalakeName = fullName.getMetalakeName();
assertNull(metalakeName);
assertThrows(RuntimeException.class, fullName::getMetalakeName);
}

@Test
Expand Down Expand Up @@ -231,8 +231,7 @@ public void testGetMetalakeWithoutMetalakeOption() throws ParseException {
String[] args = {"table", "list", "-i", "--name", "Hive_catalog.default"};
CommandLine commandLine = new DefaultParser().parse(options, args);
FullName fullName = new FullName(commandLine);
String metalakeName = fullName.getMetalakeName();
assertNull(metalakeName);
assertThrows(RuntimeException.class, fullName::getMetalakeName);
String errOutput = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim();
assertEquals(errOutput, ErrorMessages.MISSING_METALAKE);
}
Expand Down
Loading

0 comments on commit c16f595

Please sign in to comment.