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 09ad370429 [#6087] fix(CLI): Refactor the validation logic of catalog
(#6104)
09ad370429 is described below
commit 09ad370429b51333b0d1ae70b6dd939ee5f29424
Author: Lord of Abyss <[email protected]>
AuthorDate: Mon Jan 6 13:30:14 2025 +0800
[#6087] fix(CLI): Refactor the validation logic of catalog (#6104)
### What changes were proposed in this pull request?
Add `validate` method to Command, and refactor the validation code of
catalog.
### Why are the changes needed?
Fix: #6087
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
ut + local test
```bash
gcli catalog create -m demo_metalake --name test_catalog
# Missing --provider option.
gcli catalog set -m demo_metalake --name Hive_catalog
# Missing --property and --value options.
gcli catalog set -m demo_metalake --name Hive_catalog --property propertyA
# Missing --value option.
gcli catalog set -m demo_metalake --name Hive_catalog --value valA
# Missing --property option.
gcli catalog remove -m demo_metalake --name Hive_catalog
# Missing --property option.
```
---------
Co-authored-by: Justin Mclean <[email protected]>
---
.../org/apache/gravitino/cli/ErrorMessages.java | 2 +
.../apache/gravitino/cli/GravitinoCommandLine.java | 26 +++---
.../org/apache/gravitino/cli/commands/Command.java | 12 +++
.../gravitino/cli/commands/CreateCatalog.java | 6 ++
.../cli/commands/RemoveCatalogProperty.java | 6 ++
.../gravitino/cli/commands/SetCatalogProperty.java | 6 ++
.../cli/commands/SetMetalakeProperty.java | 4 +-
.../apache/gravitino/cli/TestCatalogCommands.java | 94 ++++++++++++++++++++++
8 files changed, 142 insertions(+), 14 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 10b1e9579a..c6c2a8d981 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
@@ -62,6 +62,8 @@ public class ErrorMessages {
public static final String MULTIPLE_TAG_COMMAND_ERROR =
"This command only supports one --tag option.";
+ public static final String MISSING_PROPERTY_AND_VALUE = "Missing --property
and --value options.";
+ public static final String MISSING_PROVIDER = "Missing --provider option.";
public static final String REGISTER_FAILED = "Failed to register model: ";
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 e19cc733f2..b3917c4f06 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
@@ -268,9 +268,9 @@ public class GravitinoCommandLine extends
TestableCommandLine {
switch (command) {
case CommandActions.DETAILS:
if (line.hasOption(GravitinoOptions.AUDIT)) {
- newCatalogAudit(url, ignore, metalake, catalog).handle();
+ newCatalogAudit(url, ignore, metalake, catalog).validate().handle();
} else {
- newCatalogDetails(url, ignore, outputFormat, metalake,
catalog).handle();
+ newCatalogDetails(url, ignore, outputFormat, metalake,
catalog).validate().handle();
}
break;
@@ -279,27 +279,29 @@ public class GravitinoCommandLine extends
TestableCommandLine {
String provider = line.getOptionValue(GravitinoOptions.PROVIDER);
String[] properties = line.getOptionValues(CommandActions.PROPERTIES);
Map<String, String> propertyMap = new Properties().parse(properties);
- newCreateCatalog(url, ignore, metalake, catalog, provider, comment,
propertyMap).handle();
+ newCreateCatalog(url, ignore, metalake, catalog, provider, comment,
propertyMap)
+ .validate()
+ .handle();
break;
case CommandActions.DELETE:
boolean force = line.hasOption(GravitinoOptions.FORCE);
- newDeleteCatalog(url, ignore, force, metalake, catalog).handle();
+ newDeleteCatalog(url, ignore, force, metalake,
catalog).validate().handle();
break;
case CommandActions.SET:
String property = line.getOptionValue(GravitinoOptions.PROPERTY);
String value = line.getOptionValue(GravitinoOptions.VALUE);
- newSetCatalogProperty(url, ignore, metalake, catalog, property,
value).handle();
+ newSetCatalogProperty(url, ignore, metalake, catalog, property,
value).validate().handle();
break;
case CommandActions.REMOVE:
property = line.getOptionValue(GravitinoOptions.PROPERTY);
- newRemoveCatalogProperty(url, ignore, metalake, catalog,
property).handle();
+ newRemoveCatalogProperty(url, ignore, metalake, catalog,
property).validate().handle();
break;
case CommandActions.PROPERTIES:
- newListCatalogProperties(url, ignore, metalake, catalog).handle();
+ newListCatalogProperties(url, ignore, metalake,
catalog).validate().handle();
break;
case CommandActions.UPDATE:
@@ -309,19 +311,21 @@ public class GravitinoCommandLine extends
TestableCommandLine {
}
if (line.hasOption(GravitinoOptions.ENABLE)) {
boolean enableMetalake = line.hasOption(GravitinoOptions.ALL);
- newCatalogEnable(url, ignore, metalake, catalog,
enableMetalake).handle();
+ newCatalogEnable(url, ignore, metalake, catalog,
enableMetalake).validate().handle();
}
if (line.hasOption(GravitinoOptions.DISABLE)) {
- newCatalogDisable(url, ignore, metalake, catalog).handle();
+ newCatalogDisable(url, ignore, metalake,
catalog).validate().handle();
}
if (line.hasOption(GravitinoOptions.COMMENT)) {
String updateComment = line.getOptionValue(GravitinoOptions.COMMENT);
- newUpdateCatalogComment(url, ignore, metalake, catalog,
updateComment).handle();
+ newUpdateCatalogComment(url, ignore, metalake, catalog,
updateComment)
+ .validate()
+ .handle();
}
if (line.hasOption(GravitinoOptions.RENAME)) {
String newName = line.getOptionValue(GravitinoOptions.RENAME);
- newUpdateCatalogName(url, ignore, metalake, catalog,
newName).handle();
+ newUpdateCatalogName(url, ignore, metalake, catalog,
newName).validate().handle();
}
break;
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 d881a1dbcd..98c4096cb0 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
@@ -111,6 +111,18 @@ public abstract class Command {
return this;
}
+ /**
+ * Validates that both property and value parameters are not null.
+ *
+ * @param property The property name to check
+ * @param value The value associated with the property
+ */
+ protected void checkProperty(String property, String value) {
+ if (property == null && value == null)
exitWithError(ErrorMessages.MISSING_PROPERTY_AND_VALUE);
+ if (property == null) exitWithError(ErrorMessages.MISSING_PROPERTY);
+ if (value == null) exitWithError(ErrorMessages.MISSING_VALUE);
+ }
+
/**
* Builds a {@link GravitinoClient} instance with the provided server URL
and metalake.
*
diff --git
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/CreateCatalog.java
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/CreateCatalog.java
index e0c11c1e04..2870dd7103 100644
---
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/CreateCatalog.java
+++
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/CreateCatalog.java
@@ -81,4 +81,10 @@ public class CreateCatalog extends Command {
System.out.println(catalog + " catalog created");
}
+
+ @Override
+ public Command validate() {
+ if (provider == null) exitWithError(ErrorMessages.MISSING_PROVIDER);
+ return super.validate();
+ }
}
diff --git
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveCatalogProperty.java
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveCatalogProperty.java
index a460d91b2f..c777ba1628 100644
---
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveCatalogProperty.java
+++
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveCatalogProperty.java
@@ -66,4 +66,10 @@ public class RemoveCatalogProperty extends Command {
System.out.println(property + " property removed.");
}
+
+ @Override
+ public Command validate() {
+ if (property == null) exitWithError(ErrorMessages.MISSING_PROPERTY);
+ return super.validate();
+ }
}
diff --git
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetCatalogProperty.java
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetCatalogProperty.java
index 21b1a6f1c9..8b511d7458 100644
---
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetCatalogProperty.java
+++
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetCatalogProperty.java
@@ -74,4 +74,10 @@ public class SetCatalogProperty extends Command {
System.out.println(catalog + " property set.");
}
+
+ @Override
+ public Command validate() {
+ checkProperty(property, value);
+ 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 71e5b55898..ff945cf742 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
@@ -66,9 +66,7 @@ public class SetMetalakeProperty extends Command {
@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);
+ checkProperty(property, value);
return this;
}
}
diff --git
a/clients/cli/src/test/java/org/apache/gravitino/cli/TestCatalogCommands.java
b/clients/cli/src/test/java/org/apache/gravitino/cli/TestCatalogCommands.java
index bd8f30b5ad..04c0dacc13 100644
---
a/clients/cli/src/test/java/org/apache/gravitino/cli/TestCatalogCommands.java
+++
b/clients/cli/src/test/java/org/apache/gravitino/cli/TestCatalogCommands.java
@@ -112,6 +112,7 @@ class TestCatalogCommands {
.when(commandLine)
.newCatalogDetails(
GravitinoCommandLine.DEFAULT_URL, false, null, "metalake_demo",
"catalog");
+ doReturn(mockDetails).when(mockDetails).validate();
commandLine.handleCommandLine();
verify(mockDetails).handle();
}
@@ -131,6 +132,7 @@ class TestCatalogCommands {
doReturn(mockAudit)
.when(commandLine)
.newCatalogAudit(GravitinoCommandLine.DEFAULT_URL, false,
"metalake_demo", "catalog");
+ doReturn(mockAudit).when(mockAudit).validate();
commandLine.handleCommandLine();
verify(mockAudit).handle();
}
@@ -167,10 +169,30 @@ class TestCatalogCommands {
"postgres",
"comment",
map);
+ doReturn(mockCreate).when(mockCreate).validate();
commandLine.handleCommandLine();
verify(mockCreate).handle();
}
+ @Test
+ void testCreateCatalogCommandWithoutProvider() {
+ Main.useExit = false;
+ CreateCatalog mockCreateCatalog =
+ spy(
+ new CreateCatalog(
+ GravitinoCommandLine.DEFAULT_URL,
+ false,
+ "metalake_demo",
+ "catalog",
+ null,
+ "comment",
+ null));
+
+ assertThrows(RuntimeException.class, mockCreateCatalog::validate);
+ String errOutput = new String(errContent.toByteArray(),
StandardCharsets.UTF_8).trim();
+ assertEquals(ErrorMessages.MISSING_PROVIDER, errOutput);
+ }
+
@Test
void testDeleteCatalogCommand() {
DeleteCatalog mockDelete = mock(DeleteCatalog.class);
@@ -186,6 +208,7 @@ class TestCatalogCommands {
.when(commandLine)
.newDeleteCatalog(
GravitinoCommandLine.DEFAULT_URL, false, false, "metalake_demo",
"catalog");
+ doReturn(mockDelete).when(mockDelete).validate();
commandLine.handleCommandLine();
verify(mockDelete).handle();
}
@@ -206,6 +229,7 @@ class TestCatalogCommands {
.when(commandLine)
.newDeleteCatalog(
GravitinoCommandLine.DEFAULT_URL, false, true, "metalake_demo",
"catalog");
+ doReturn(mockDelete).when(mockDelete).validate();
commandLine.handleCommandLine();
verify(mockDelete).handle();
}
@@ -234,10 +258,60 @@ class TestCatalogCommands {
"catalog",
"property",
"value");
+ doReturn(mockSetProperty).when(mockSetProperty).validate();
commandLine.handleCommandLine();
verify(mockSetProperty).handle();
}
+ @Test
+ void testSetCatalogPropertyCommandWithoutPropertyAndValue() {
+ Main.useExit = false;
+ SetCatalogProperty mockSetProperty =
+ spy(
+ new SetCatalogProperty(
+ GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo",
"catalog", null, null));
+
+ assertThrows(RuntimeException.class, mockSetProperty::validate);
+ String errOutput = new String(errContent.toByteArray(),
StandardCharsets.UTF_8).trim();
+ assertEquals("Missing --property and --value options.", errOutput);
+ }
+
+ @Test
+ void testSetCatalogPropertyCommandWithoutProperty() {
+ Main.useExit = false;
+ SetCatalogProperty mockSetProperty =
+ spy(
+ new SetCatalogProperty(
+ GravitinoCommandLine.DEFAULT_URL,
+ false,
+ "metalake_demo",
+ "catalog",
+ null,
+ "value"));
+
+ assertThrows(RuntimeException.class, mockSetProperty::validate);
+ String errOutput = new String(errContent.toByteArray(),
StandardCharsets.UTF_8).trim();
+ assertEquals(ErrorMessages.MISSING_PROPERTY, errOutput);
+ }
+
+ @Test
+ void testSetCatalogPropertyCommandWithoutValue() {
+ Main.useExit = false;
+ SetCatalogProperty mockSetProperty =
+ spy(
+ new SetCatalogProperty(
+ GravitinoCommandLine.DEFAULT_URL,
+ false,
+ "metalake_demo",
+ "catalog",
+ "property",
+ null));
+
+ assertThrows(RuntimeException.class, mockSetProperty::validate);
+ String errOutput = new String(errContent.toByteArray(),
StandardCharsets.UTF_8).trim();
+ assertEquals(ErrorMessages.MISSING_VALUE, errOutput);
+ }
+
@Test
void testRemoveCatalogPropertyCommand() {
RemoveCatalogProperty mockRemoveProperty =
mock(RemoveCatalogProperty.class);
@@ -255,10 +329,24 @@ class TestCatalogCommands {
.when(commandLine)
.newRemoveCatalogProperty(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo",
"catalog", "property");
+ doReturn(mockRemoveProperty).when(mockRemoveProperty).validate();
commandLine.handleCommandLine();
verify(mockRemoveProperty).handle();
}
+ @Test
+ void testRemoveCatalogPropertyCommandWithoutProperty() {
+ Main.useExit = false;
+ RemoveCatalogProperty mockRemoveProperty =
+ spy(
+ new RemoveCatalogProperty(
+ GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo",
"catalog", null));
+
+ assertThrows(RuntimeException.class, mockRemoveProperty::validate);
+ String errOutput = new String(errContent.toByteArray(),
StandardCharsets.UTF_8).trim();
+ assertEquals(ErrorMessages.MISSING_PROPERTY, errOutput);
+ }
+
@Test
void testListCatalogPropertiesCommand() {
ListCatalogProperties mockListProperties =
mock(ListCatalogProperties.class);
@@ -274,6 +362,7 @@ class TestCatalogCommands {
.when(commandLine)
.newListCatalogProperties(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo",
"catalog");
+ doReturn(mockListProperties).when(mockListProperties).validate();
commandLine.handleCommandLine();
verify(mockListProperties).handle();
}
@@ -295,6 +384,7 @@ class TestCatalogCommands {
.when(commandLine)
.newUpdateCatalogComment(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo",
"catalog", "new comment");
+ doReturn(mockUpdateComment).when(mockUpdateComment).validate();
commandLine.handleCommandLine();
verify(mockUpdateComment).handle();
}
@@ -317,6 +407,7 @@ class TestCatalogCommands {
.when(commandLine)
.newUpdateCatalogName(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo",
"catalog", "new_name");
+ doReturn(mockUpdateName).when(mockUpdateName).validate();
commandLine.handleCommandLine();
verify(mockUpdateName).handle();
}
@@ -368,6 +459,7 @@ class TestCatalogCommands {
.when(commandLine)
.newCatalogEnable(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo",
"catalog", false);
+ doReturn(mockEnable).when(mockEnable).validate();
commandLine.handleCommandLine();
verify(mockEnable).handle();
}
@@ -390,6 +482,7 @@ class TestCatalogCommands {
.when(commandLine)
.newCatalogEnable(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo",
"catalog", true);
+ doReturn(mockEnable).when(mockEnable).validate();
commandLine.handleCommandLine();
verify(mockEnable).handle();
}
@@ -410,6 +503,7 @@ class TestCatalogCommands {
doReturn(mockDisable)
.when(commandLine)
.newCatalogDisable(GravitinoCommandLine.DEFAULT_URL, false,
"metalake_demo", "catalog");
+ doReturn(mockDisable).when(mockDisable).validate();
commandLine.handleCommandLine();
verify(mockDisable).handle();
}
