This is an automated email from the ASF dual-hosted git repository.
jmclean pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new c16f5955d6 [#6086] fix(CLI): Refactor the validation logic of Metalake
(#6091)
c16f5955d6 is described below
commit c16f5955d64208da5f9b09c5ebc33471f56bfaef
Author: Lord of Abyss <[email protected]>
AuthorDate: Mon Jan 6 06:09:53 2025 +0800
[#6086] fix(CLI): Refactor the validation logic of Metalake (#6091)
### 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]>
---
.../org/apache/gravitino/cli/ErrorMessages.java | 2 +
.../java/org/apache/gravitino/cli/FullName.java | 1 +
.../apache/gravitino/cli/GravitinoCommandLine.java | 40 ++++++------
.../org/apache/gravitino/cli/commands/Command.java | 17 ++++-
.../cli/commands/RemoveMetalakeProperty.java | 6 ++
.../cli/commands/SetMetalakeProperty.java | 8 +++
.../org/apache/gravitino/cli/TestFulllName.java | 7 +--
.../apache/gravitino/cli/TestMetalakeCommands.java | 72 +++++++++++++++++++++-
8 files changed, 126 insertions(+), 27 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
index e90c525963..7fd4e27221 100644
--- a/clients/cli/src/main/java/org/apache/gravitino/cli/ErrorMessages.java
+++ b/clients/cli/src/main/java/org/apache/gravitino/cli/ErrorMessages.java
@@ -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.";
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
index a3b206dfdd..7a9481cb95 100644
--- a/clients/cli/src/main/java/org/apache/gravitino/cli/FullName.java
+++ b/clients/cli/src/main/java/org/apache/gravitino/cli/FullName.java
@@ -74,6 +74,7 @@ public class FullName {
}
System.err.println(ErrorMessages.MISSING_METALAKE);
+ Main.exit(-1);
return null;
}
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
index 3a9322d010..f8347dfe1f 100644
---
a/clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java
+++
b/clients/cli/src/main/java/org/apache/gravitino/cli/GravitinoCommandLine.java
@@ -30,7 +30,6 @@ import java.nio.charset.StandardCharsets;
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;
@@ -167,51 +166,49 @@ public class GravitinoCommandLine extends
TestableCommandLine {
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:
@@ -221,21 +218,22 @@ public class GravitinoCommandLine extends
TestableCommandLine {
}
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:
diff --git
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/Command.java
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/Command.java
index f91dae4042..cb11d7dfce 100644
--- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/Command.java
+++ b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/Command.java
@@ -21,6 +21,7 @@ package org.apache.gravitino.cli.commands;
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;
@@ -39,6 +40,7 @@ import
org.apache.gravitino.exceptions.NoSuchMetalakeException;
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;
@@ -46,7 +48,6 @@ public abstract class Command {
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;
@@ -99,6 +100,16 @@ public abstract class Command {
/** 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.
*
@@ -192,4 +203,8 @@ public abstract class Command {
throw new IllegalArgumentException("Unsupported output format");
}
}
+
+ protected String getMissingEntitiesInfo(String... entities) {
+ return "Missing required argument(s): " + COMMA_JOINER.join(entities);
+ }
}
diff --git
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveMetalakeProperty.java
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveMetalakeProperty.java
index 9642456f37..0664ddaad1 100644
---
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveMetalakeProperty.java
+++
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveMetalakeProperty.java
@@ -60,4 +60,10 @@ public class RemoveMetalakeProperty extends Command {
System.out.println(property + " property removed.");
}
+
+ @Override
+ public Command validate() {
+ if (property == null) exitWithError(ErrorMessages.MISSING_PROPERTY);
+ return this;
+ }
}
diff --git
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetMetalakeProperty.java
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetMetalakeProperty.java
index 817beaec91..71e5b55898 100644
---
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetMetalakeProperty.java
+++
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetMetalakeProperty.java
@@ -63,4 +63,12 @@ public class SetMetalakeProperty extends Command {
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;
+ }
}
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
index 48ee79cfcc..f13d6e0920 100644
--- a/clients/cli/src/test/java/org/apache/gravitino/cli/TestFulllName.java
+++ b/clients/cli/src/test/java/org/apache/gravitino/cli/TestFulllName.java
@@ -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));
@@ -82,8 +83,7 @@ public class TestFulllName {
CommandLine commandLine = new DefaultParser().parse(options, args);
FullName fullName = new FullName(commandLine);
- String metalakeName = fullName.getMetalakeName();
- assertNull(metalakeName);
+ assertThrows(RuntimeException.class, fullName::getMetalakeName);
}
@Test
@@ -231,8 +231,7 @@ public class TestFulllName {
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);
}
diff --git
a/clients/cli/src/test/java/org/apache/gravitino/cli/TestMetalakeCommands.java
b/clients/cli/src/test/java/org/apache/gravitino/cli/TestMetalakeCommands.java
index 01eebb6dab..7df08b8ada 100644
---
a/clients/cli/src/test/java/org/apache/gravitino/cli/TestMetalakeCommands.java
+++
b/clients/cli/src/test/java/org/apache/gravitino/cli/TestMetalakeCommands.java
@@ -19,6 +19,8 @@
package org.apache.gravitino.cli;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThrows;
import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
@@ -29,6 +31,7 @@ import static org.mockito.Mockito.when;
import java.io.ByteArrayOutputStream;
import java.io.PrintStream;
+import java.nio.charset.StandardCharsets;
import org.apache.commons.cli.CommandLine;
import org.apache.commons.cli.Options;
import org.apache.gravitino.cli.commands.CreateMetalake;
@@ -87,6 +90,7 @@ class TestMetalakeCommands {
doReturn(mockList)
.when(commandLine)
.newListMetalakes(GravitinoCommandLine.DEFAULT_URL, false, null);
+ doReturn(mockList).when(mockList).validate();
commandLine.handleCommandLine();
verify(mockList).handle();
}
@@ -104,6 +108,7 @@ class TestMetalakeCommands {
doReturn(mockDetails)
.when(commandLine)
.newMetalakeDetails(GravitinoCommandLine.DEFAULT_URL, false, null,
"metalake_demo");
+ doReturn(mockDetails).when(mockDetails).validate();
commandLine.handleCommandLine();
verify(mockDetails).handle();
}
@@ -121,6 +126,7 @@ class TestMetalakeCommands {
doReturn(mockAudit)
.when(commandLine)
.newMetalakeAudit(GravitinoCommandLine.DEFAULT_URL, false,
"metalake_demo");
+ doReturn(mockAudit).when(mockAudit).validate();
commandLine.handleCommandLine();
verify(mockAudit).handle();
}
@@ -139,6 +145,7 @@ class TestMetalakeCommands {
doReturn(mockCreate)
.when(commandLine)
.newCreateMetalake(GravitinoCommandLine.DEFAULT_URL, false,
"metalake_demo", "comment");
+ doReturn(mockCreate).when(mockCreate).validate();
commandLine.handleCommandLine();
verify(mockCreate).handle();
}
@@ -155,6 +162,7 @@ class TestMetalakeCommands {
doReturn(mockCreate)
.when(commandLine)
.newCreateMetalake(GravitinoCommandLine.DEFAULT_URL, false,
"metalake_demo", null);
+ doReturn(mockCreate).when(mockCreate).validate();
commandLine.handleCommandLine();
verify(mockCreate).handle();
}
@@ -171,6 +179,7 @@ class TestMetalakeCommands {
doReturn(mockDelete)
.when(commandLine)
.newDeleteMetalake(GravitinoCommandLine.DEFAULT_URL, false, false,
"metalake_demo");
+ doReturn(mockDelete).when(mockDelete).validate();
commandLine.handleCommandLine();
verify(mockDelete).handle();
}
@@ -188,6 +197,7 @@ class TestMetalakeCommands {
doReturn(mockDelete)
.when(commandLine)
.newDeleteMetalake(GravitinoCommandLine.DEFAULT_URL, false, true,
"metalake_demo");
+ doReturn(mockDelete).when(mockDelete).validate();
commandLine.handleCommandLine();
verify(mockDelete).handle();
}
@@ -209,10 +219,50 @@ class TestMetalakeCommands {
.when(commandLine)
.newSetMetalakeProperty(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo",
"property", "value");
+ doReturn(mockSetProperty).when(mockSetProperty).validate();
commandLine.handleCommandLine();
verify(mockSetProperty).handle();
}
+ @Test
+ void testSetMetalakePropertyCommandWithoutPropertyAndValue() {
+ Main.useExit = false;
+ SetMetalakeProperty metalakeProperty =
+ spy(
+ new SetMetalakeProperty(
+ GravitinoCommandLine.DEFAULT_URL, false, "demo_metalake",
null, null));
+
+ assertThrows(RuntimeException.class, metalakeProperty::validate);
+ String errOutput = new String(errContent.toByteArray(),
StandardCharsets.UTF_8).trim();
+ assertEquals("Missing --property and --value options.", errOutput);
+ }
+
+ @Test
+ void testSetMetalakePropertyCommandWithoutProperty() {
+ Main.useExit = false;
+ SetMetalakeProperty metalakeProperty =
+ spy(
+ new SetMetalakeProperty(
+ GravitinoCommandLine.DEFAULT_URL, false, "demo_metalake",
null, "val1"));
+
+ assertThrows(RuntimeException.class, metalakeProperty::validate);
+ String errOutput = new String(errContent.toByteArray(),
StandardCharsets.UTF_8).trim();
+ assertEquals(ErrorMessages.MISSING_PROPERTY, errOutput);
+ }
+
+ @Test
+ void testSetMetalakePropertyCommandWithoutValue() {
+ Main.useExit = false;
+ SetMetalakeProperty metalakeProperty =
+ spy(
+ new SetMetalakeProperty(
+ GravitinoCommandLine.DEFAULT_URL, false, "demo_metalake",
"property1", null));
+
+ assertThrows(RuntimeException.class, metalakeProperty::validate);
+ String errOutput = new String(errContent.toByteArray(),
StandardCharsets.UTF_8).trim();
+ assertEquals(ErrorMessages.MISSING_VALUE, errOutput);
+ }
+
@Test
void testRemoveMetalakePropertyCommand() {
RemoveMetalakeProperty mockRemoveProperty =
mock(RemoveMetalakeProperty.class);
@@ -228,10 +278,24 @@ class TestMetalakeCommands {
.when(commandLine)
.newRemoveMetalakeProperty(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo",
"property");
+ doReturn(mockRemoveProperty).when(mockRemoveProperty).validate();
commandLine.handleCommandLine();
verify(mockRemoveProperty).handle();
}
+ @Test
+ void testRemoveMetalakePropertyCommandWithoutProperty() {
+ Main.useExit = false;
+ RemoveMetalakeProperty mockRemoveProperty =
+ spy(
+ new RemoveMetalakeProperty(
+ GravitinoCommandLine.DEFAULT_URL, false, "demo_metalake",
null));
+
+ assertThrows(RuntimeException.class, mockRemoveProperty::validate);
+ String errOutput = new String(errContent.toByteArray(),
StandardCharsets.UTF_8).trim();
+ assertEquals(ErrorMessages.MISSING_PROPERTY, errOutput);
+ }
+
@Test
void testListMetalakePropertiesCommand() {
ListMetalakeProperties mockListProperties =
mock(ListMetalakeProperties.class);
@@ -244,6 +308,7 @@ class TestMetalakeCommands {
doReturn(mockListProperties)
.when(commandLine)
.newListMetalakeProperties(GravitinoCommandLine.DEFAULT_URL, false,
"metalake_demo");
+ doReturn(mockListProperties).when(mockListProperties).validate();
commandLine.handleCommandLine();
verify(mockListProperties).handle();
}
@@ -263,6 +328,7 @@ class TestMetalakeCommands {
.when(commandLine)
.newUpdateMetalakeComment(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "new
comment");
+ doReturn(mockUpdateComment).when(mockUpdateComment).validate();
commandLine.handleCommandLine();
verify(mockUpdateComment).handle();
}
@@ -282,6 +348,7 @@ class TestMetalakeCommands {
.when(commandLine)
.newUpdateMetalakeName(
GravitinoCommandLine.DEFAULT_URL, false, false, "metalake_demo",
"new_name");
+ doReturn(mockUpdateName).when(mockUpdateName).validate();
commandLine.handleCommandLine();
verify(mockUpdateName).handle();
}
@@ -302,6 +369,7 @@ class TestMetalakeCommands {
.when(commandLine)
.newUpdateMetalakeName(
GravitinoCommandLine.DEFAULT_URL, false, true, "metalake_demo",
"new_name");
+ doReturn(mockUpdateName).when(mockUpdateName).validate();
commandLine.handleCommandLine();
verify(mockUpdateName).handle();
}
@@ -319,6 +387,7 @@ class TestMetalakeCommands {
doReturn(mockEnable)
.when(commandLine)
.newMetalakeEnable(GravitinoCommandLine.DEFAULT_URL, false,
"metalake_demo", false);
+ doReturn(mockEnable).when(mockEnable).validate();
commandLine.handleCommandLine();
verify(mockEnable).handle();
}
@@ -337,6 +406,7 @@ class TestMetalakeCommands {
doReturn(mockEnable)
.when(commandLine)
.newMetalakeEnable(GravitinoCommandLine.DEFAULT_URL, false,
"metalake_demo", true);
+ doReturn(mockEnable).when(mockEnable).validate();
commandLine.handleCommandLine();
verify(mockEnable).handle();
}
@@ -355,7 +425,7 @@ class TestMetalakeCommands {
doReturn(mockDisable)
.when(commandLine)
.newMetalakeDisable(GravitinoCommandLine.DEFAULT_URL, false,
"metalake_demo");
-
+ doReturn(mockDisable).when(mockDisable).validate();
commandLine.handleCommandLine();
verify(mockDisable).handle();
}
