Skip to content

Commit

Permalink
[#6087] fix(CLI): Refactor the validation logic of catalog (#6104)
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 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]>
  • Loading branch information
@Abyss-lord @justinmclean
Abyss-lord and justinmclean authored Jan 6, 2025
1 parent 9d25109 commit 09ad370
Show file tree
Hide file tree
Showing 8 changed files with 142 additions and 14 deletions.
2 changes: 2 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 @@ -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: ";

Expand Down
26 changes: 15 additions & 11 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 @@ -268,9 +268,9 @@ private void handleCatalogCommand() {
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;

Expand All @@ -279,27 +279,29 @@ private void handleCatalogCommand() {
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:
Expand All @@ -309,19 +311,21 @@ private void handleCatalogCommand() {
}
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;

Expand Down
12 changes: 12 additions & 0 deletions clients/cli/src/main/java/org/apache/gravitino/cli/commands/Command.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,18 @@ public Command validate() {
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.
*
Expand Down
6 changes: 6 additions & 0 deletions clients/cli/src/main/java/org/apache/gravitino/cli/commands/CreateCatalog.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,4 +81,10 @@ public void handle() {

System.out.println(catalog + " catalog created");
}

@Override
public Command validate() {
if (provider == null) exitWithError(ErrorMessages.MISSING_PROVIDER);
return super.validate();
}
}
6 changes: 6 additions & 0 deletions clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveCatalogProperty.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,4 +66,10 @@ public void handle() {

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

@Override
public Command validate() {
if (property == null) exitWithError(ErrorMessages.MISSING_PROPERTY);
return super.validate();
}
}
6 changes: 6 additions & 0 deletions clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetCatalogProperty.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,4 +74,10 @@ public void handle() {

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

@Override
public Command validate() {
checkProperty(property, value);
return this;
}
}
4 changes: 1 addition & 3 deletions clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetMetalakeProperty.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,7 @@ public void handle() {

@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;
}
}
94 changes: 94 additions & 0 deletions clients/cli/src/test/java/org/apache/gravitino/cli/TestCatalogCommands.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ void testCatalogDetailsCommand() {
.when(commandLine)
.newCatalogDetails(
GravitinoCommandLine.DEFAULT_URL, false, null, "metalake_demo", "catalog");
doReturn(mockDetails).when(mockDetails).validate();
commandLine.handleCommandLine();
verify(mockDetails).handle();
}
Expand All @@ -131,6 +132,7 @@ void testCatalogAuditCommand() {
doReturn(mockAudit)
.when(commandLine)
.newCatalogAudit(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "catalog");
doReturn(mockAudit).when(mockAudit).validate();
commandLine.handleCommandLine();
verify(mockAudit).handle();
}
Expand Down Expand Up @@ -167,10 +169,30 @@ void testCreateCatalogCommand() {
"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);
Expand All @@ -186,6 +208,7 @@ void testDeleteCatalogCommand() {
.when(commandLine)
.newDeleteCatalog(
GravitinoCommandLine.DEFAULT_URL, false, false, "metalake_demo", "catalog");
doReturn(mockDelete).when(mockDelete).validate();
commandLine.handleCommandLine();
verify(mockDelete).handle();
}
Expand All @@ -206,6 +229,7 @@ void testDeleteCatalogForceCommand() {
.when(commandLine)
.newDeleteCatalog(
GravitinoCommandLine.DEFAULT_URL, false, true, "metalake_demo", "catalog");
doReturn(mockDelete).when(mockDelete).validate();
commandLine.handleCommandLine();
verify(mockDelete).handle();
}
Expand Down Expand Up @@ -234,10 +258,60 @@ void testSetCatalogPropertyCommand() {
"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);
Expand All @@ -255,10 +329,24 @@ void testRemoveCatalogPropertyCommand() {
.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);
Expand All @@ -274,6 +362,7 @@ void testListCatalogPropertiesCommand() {
.when(commandLine)
.newListCatalogProperties(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "catalog");
doReturn(mockListProperties).when(mockListProperties).validate();
commandLine.handleCommandLine();
verify(mockListProperties).handle();
}
Expand All @@ -295,6 +384,7 @@ void testUpdateCatalogCommentCommand() {
.when(commandLine)
.newUpdateCatalogComment(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "catalog", "new comment");
doReturn(mockUpdateComment).when(mockUpdateComment).validate();
commandLine.handleCommandLine();
verify(mockUpdateComment).handle();
}
Expand All @@ -317,6 +407,7 @@ void testUpdateCatalogNameCommand() {
.when(commandLine)
.newUpdateCatalogName(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "catalog", "new_name");
doReturn(mockUpdateName).when(mockUpdateName).validate();
commandLine.handleCommandLine();
verify(mockUpdateName).handle();
}
Expand Down Expand Up @@ -368,6 +459,7 @@ void testEnableCatalogCommand() {
.when(commandLine)
.newCatalogEnable(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "catalog", false);
doReturn(mockEnable).when(mockEnable).validate();
commandLine.handleCommandLine();
verify(mockEnable).handle();
}
Expand All @@ -390,6 +482,7 @@ void testEnableCatalogCommandWithRecursive() {
.when(commandLine)
.newCatalogEnable(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "catalog", true);
doReturn(mockEnable).when(mockEnable).validate();
commandLine.handleCommandLine();
verify(mockEnable).handle();
}
Expand All @@ -410,6 +503,7 @@ void testDisableCatalogCommand() {
doReturn(mockDisable)
.when(commandLine)
.newCatalogDisable(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "catalog");
doReturn(mockDisable).when(mockDisable).validate();
commandLine.handleCommandLine();
verify(mockDisable).handle();
}
Expand Down

0 comments on commit 09ad370

Please sign in to comment.