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 1884df6790 [#6121] fix(CLI): Refactor the validation logic of topic
and fileset (#6122)
1884df6790 is described below
commit 1884df67906d2efd674fe40a5d3c263b81ca1de5
Author: Lord of Abyss <103809695+abyss-l...@users.noreply.github.com>
AuthorDate: Tue Jan 7 09:23:54 2025 +0800
[#6121] fix(CLI): Refactor the validation logic of topic and fileset (#6122)
### What changes were proposed in this pull request?
Refactor the validation logic of fileset and topic, meanwhile fix the
test case.
### Why are the changes needed?
Fix: #6121
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
ut + local test
Topic test
```bash
gcli topic set -m demo_metalake --name catalog.schema.topic
# Missing --property and --value options.
gcli topic set -m demo_metalake --name catalog.schema.topic --property
property
# Missing --value option.
gcli topic set -m demo_metalake --name catalog.schema.topic --value value
# Missing --property option.
gcli topic remove -m demo_metalake --name catalog.schema.topic
# Missing --property option.
```
Fileset test
```bash
gcli fileset set -m demo_metalake --name catalog.schema.fileset
# Missing --property and --value options.
gcli fileset set -m demo_metalake --name catalog.schema.fileset --property
property
# Missing --value option.
gcli fileset set -m demo_metalake --name catalog.schema.fileset --value
value
# Missing --property option.
gcli fileset remove -m demo_metalake --name catalog.schema.fileset
# Missing --property option.
```
---
.../apache/gravitino/cli/GravitinoCommandLine.java | 41 +++++++---
.../cli/commands/RemoveFilesetProperty.java | 6 ++
.../cli/commands/RemoveTopicProperty.java | 6 ++
.../gravitino/cli/commands/SetFilesetProperty.java | 6 ++
.../gravitino/cli/commands/SetTopicProperty.java | 6 ++
.../apache/gravitino/cli/TestFilesetCommands.java | 93 ++++++++++++++++++++++
.../apache/gravitino/cli/TestTopicCommands.java | 89 +++++++++++++++++++++
7 files changed, 235 insertions(+), 12 deletions(-)
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 589aa437df..f93da3003c 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
@@ -1015,7 +1015,7 @@ public class GravitinoCommandLine extends
TestableCommandLine {
if (CommandActions.LIST.equals(command)) {
checkEntities(missingEntities);
- newListTopics(url, ignore, metalake, catalog, schema).handle();
+ newListTopics(url, ignore, metalake, catalog,
schema).validate().handle();
return;
}
@@ -1025,20 +1025,22 @@ public class GravitinoCommandLine extends
TestableCommandLine {
switch (command) {
case CommandActions.DETAILS:
- newTopicDetails(url, ignore, metalake, catalog, schema,
topic).handle();
+ newTopicDetails(url, ignore, metalake, catalog, schema,
topic).validate().handle();
break;
case CommandActions.CREATE:
{
String comment = line.getOptionValue(GravitinoOptions.COMMENT);
- newCreateTopic(url, ignore, metalake, catalog, schema, topic,
comment).handle();
+ newCreateTopic(url, ignore, metalake, catalog, schema, topic,
comment)
+ .validate()
+ .handle();
break;
}
case CommandActions.DELETE:
{
boolean force = line.hasOption(GravitinoOptions.FORCE);
- newDeleteTopic(url, ignore, force, metalake, catalog, schema,
topic).handle();
+ newDeleteTopic(url, ignore, force, metalake, catalog, schema,
topic).validate().handle();
break;
}
@@ -1046,7 +1048,9 @@ public class GravitinoCommandLine extends
TestableCommandLine {
{
if (line.hasOption(GravitinoOptions.COMMENT)) {
String comment = line.getOptionValue(GravitinoOptions.COMMENT);
- newUpdateTopicComment(url, ignore, metalake, catalog, schema,
topic, comment).handle();
+ newUpdateTopicComment(url, ignore, metalake, catalog, schema,
topic, comment)
+ .validate()
+ .handle();
}
break;
}
@@ -1056,6 +1060,7 @@ public class GravitinoCommandLine extends
TestableCommandLine {
String property = line.getOptionValue(GravitinoOptions.PROPERTY);
String value = line.getOptionValue(GravitinoOptions.VALUE);
newSetTopicProperty(url, ignore, metalake, catalog, schema, topic,
property, value)
+ .validate()
.handle();
break;
}
@@ -1063,12 +1068,14 @@ public class GravitinoCommandLine extends
TestableCommandLine {
case CommandActions.REMOVE:
{
String property = line.getOptionValue(GravitinoOptions.PROPERTY);
- newRemoveTopicProperty(url, ignore, metalake, catalog, schema,
topic, property).handle();
+ newRemoveTopicProperty(url, ignore, metalake, catalog, schema,
topic, property)
+ .validate()
+ .handle();
break;
}
case CommandActions.PROPERTIES:
- newListTopicProperties(url, ignore, metalake, catalog, schema,
topic).handle();
+ newListTopicProperties(url, ignore, metalake, catalog, schema,
topic).validate().handle();
break;
default:
@@ -1098,7 +1105,7 @@ public class GravitinoCommandLine extends
TestableCommandLine {
// Handle CommandActions.LIST action separately as it doesn't require the
`fileset`
if (CommandActions.LIST.equals(command)) {
checkEntities(missingEntities);
- newListFilesets(url, ignore, metalake, catalog, schema).handle();
+ newListFilesets(url, ignore, metalake, catalog,
schema).validate().handle();
return;
}
@@ -1108,7 +1115,7 @@ public class GravitinoCommandLine extends
TestableCommandLine {
switch (command) {
case CommandActions.DETAILS:
- newFilesetDetails(url, ignore, metalake, catalog, schema,
fileset).handle();
+ newFilesetDetails(url, ignore, metalake, catalog, schema,
fileset).validate().handle();
break;
case CommandActions.CREATE:
@@ -1117,6 +1124,7 @@ public class GravitinoCommandLine extends
TestableCommandLine {
String[] properties =
line.getOptionValues(CommandActions.PROPERTIES);
Map<String, String> propertyMap = new Properties().parse(properties);
newCreateFileset(url, ignore, metalake, catalog, schema, fileset,
comment, propertyMap)
+ .validate()
.handle();
break;
}
@@ -1124,7 +1132,9 @@ public class GravitinoCommandLine extends
TestableCommandLine {
case CommandActions.DELETE:
{
boolean force = line.hasOption(GravitinoOptions.FORCE);
- newDeleteFileset(url, ignore, force, metalake, catalog, schema,
fileset).handle();
+ newDeleteFileset(url, ignore, force, metalake, catalog, schema,
fileset)
+ .validate()
+ .handle();
break;
}
@@ -1133,6 +1143,7 @@ public class GravitinoCommandLine extends
TestableCommandLine {
String property = line.getOptionValue(GravitinoOptions.PROPERTY);
String value = line.getOptionValue(GravitinoOptions.VALUE);
newSetFilesetProperty(url, ignore, metalake, catalog, schema,
fileset, property, value)
+ .validate()
.handle();
break;
}
@@ -1141,12 +1152,15 @@ public class GravitinoCommandLine extends
TestableCommandLine {
{
String property = line.getOptionValue(GravitinoOptions.PROPERTY);
newRemoveFilesetProperty(url, ignore, metalake, catalog, schema,
fileset, property)
+ .validate()
.handle();
break;
}
case CommandActions.PROPERTIES:
- newListFilesetProperties(url, ignore, metalake, catalog, schema,
fileset).handle();
+ newListFilesetProperties(url, ignore, metalake, catalog, schema,
fileset)
+ .validate()
+ .handle();
break;
case CommandActions.UPDATE:
@@ -1154,11 +1168,14 @@ public class GravitinoCommandLine extends
TestableCommandLine {
if (line.hasOption(GravitinoOptions.COMMENT)) {
String comment = line.getOptionValue(GravitinoOptions.COMMENT);
newUpdateFilesetComment(url, ignore, metalake, catalog, schema,
fileset, comment)
+ .validate()
.handle();
}
if (line.hasOption(GravitinoOptions.RENAME)) {
String newName = line.getOptionValue(GravitinoOptions.RENAME);
- newUpdateFilesetName(url, ignore, metalake, catalog, schema,
fileset, newName).handle();
+ newUpdateFilesetName(url, ignore, metalake, catalog, schema,
fileset, newName)
+ .validate()
+ .handle();
}
break;
}
diff --git
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveFilesetProperty.java
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveFilesetProperty.java
index 00deebe265..c443bf0fdf 100644
---
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveFilesetProperty.java
+++
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveFilesetProperty.java
@@ -86,4 +86,10 @@ public class RemoveFilesetProperty extends Command {
System.out.println(property + " property removed.");
}
+
+ @Override
+ public Command validate() {
+ validateProperty(property);
+ return super.validate();
+ }
}
diff --git
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveTopicProperty.java
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveTopicProperty.java
index a43820933e..51be0a139d 100644
---
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveTopicProperty.java
+++
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveTopicProperty.java
@@ -87,4 +87,10 @@ public class RemoveTopicProperty extends Command {
System.out.println(property + " property removed.");
}
+
+ @Override
+ public Command validate() {
+ validateProperty(property);
+ return super.validate();
+ }
}
diff --git
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetFilesetProperty.java
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetFilesetProperty.java
index 2c179db104..afafa3c9db 100644
---
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetFilesetProperty.java
+++
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetFilesetProperty.java
@@ -90,4 +90,10 @@ public class SetFilesetProperty extends Command {
System.out.println(schema + " property set.");
}
+
+ @Override
+ public Command validate() {
+ validatePropertyAndValue(property, value);
+ return super.validate();
+ }
}
diff --git
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetTopicProperty.java
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetTopicProperty.java
index 941c0b0321..2641259cdd 100644
---
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetTopicProperty.java
+++
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetTopicProperty.java
@@ -92,4 +92,10 @@ public class SetTopicProperty extends Command {
System.out.println(property + " property set.");
}
+
+ @Override
+ public Command validate() {
+ validatePropertyAndValue(property, value);
+ return super.validate();
+ }
}
diff --git
a/clients/cli/src/test/java/org/apache/gravitino/cli/TestFilesetCommands.java
b/clients/cli/src/test/java/org/apache/gravitino/cli/TestFilesetCommands.java
index 1e8c54124c..3529e60bf7 100644
---
a/clients/cli/src/test/java/org/apache/gravitino/cli/TestFilesetCommands.java
+++
b/clients/cli/src/test/java/org/apache/gravitino/cli/TestFilesetCommands.java
@@ -92,6 +92,7 @@ class TestFilesetCommands {
.when(commandLine)
.newListFilesets(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo",
"catalog", "schema");
+ doReturn(mockList).when(mockList).validate();
commandLine.handleCommandLine();
verify(mockList).handle();
}
@@ -117,6 +118,7 @@ class TestFilesetCommands {
"catalog",
"schema",
"fileset");
+ doReturn(mockDetails).when(mockDetails).validate();
commandLine.handleCommandLine();
verify(mockDetails).handle();
}
@@ -147,6 +149,7 @@ class TestFilesetCommands {
eq("fileset"),
eq("comment"),
any());
+ doReturn(mockCreate).when(mockCreate).validate();
commandLine.handleCommandLine();
verify(mockCreate).handle();
}
@@ -173,6 +176,7 @@ class TestFilesetCommands {
"catalog",
"schema",
"fileset");
+ doReturn(mockDelete).when(mockDelete).validate();
commandLine.handleCommandLine();
verify(mockDelete).handle();
}
@@ -200,6 +204,7 @@ class TestFilesetCommands {
"catalog",
"schema",
"fileset");
+ doReturn(mockDelete).when(mockDelete).validate();
commandLine.handleCommandLine();
verify(mockDelete).handle();
}
@@ -229,6 +234,7 @@ class TestFilesetCommands {
"schema",
"fileset",
"new_comment");
+ doReturn(mockUpdateComment).when(mockUpdateComment).validate();
commandLine.handleCommandLine();
verify(mockUpdateComment).handle();
}
@@ -258,6 +264,7 @@ class TestFilesetCommands {
"schema",
"fileset",
"new_name");
+ doReturn(mockUpdateName).when(mockUpdateName).validate();
commandLine.handleCommandLine();
verify(mockUpdateName).handle();
}
@@ -284,6 +291,7 @@ class TestFilesetCommands {
"catalog",
"schema",
"fileset");
+ doReturn(mockListProperties).when(mockListProperties).validate();
commandLine.handleCommandLine();
verify(mockListProperties).handle();
}
@@ -316,10 +324,74 @@ class TestFilesetCommands {
"fileset",
"property",
"value");
+ doReturn(mockSetProperties).when(mockSetProperties).validate();
commandLine.handleCommandLine();
verify(mockSetProperties).handle();
}
+ @Test
+ void testSetFilesetPropertyCommandWithoutPropertyAndValue() {
+ Main.useExit = false;
+ SetFilesetProperty spySetProperty =
+ spy(
+ new SetFilesetProperty(
+ GravitinoCommandLine.DEFAULT_URL,
+ false,
+ "metalake_demo",
+ "catalog",
+ "schema",
+ "fileset",
+ null,
+ null));
+
+ assertThrows(RuntimeException.class, spySetProperty::validate);
+ verify(spySetProperty, never()).handle();
+ String errOutput = new String(errContent.toByteArray(),
StandardCharsets.UTF_8).trim();
+ assertEquals(ErrorMessages.MISSING_PROPERTY_AND_VALUE, errOutput);
+ }
+
+ @Test
+ void testSetFilesetPropertyCommandWithoutProperty() {
+ Main.useExit = false;
+ SetFilesetProperty spySetProperty =
+ spy(
+ new SetFilesetProperty(
+ GravitinoCommandLine.DEFAULT_URL,
+ false,
+ "metalake_demo",
+ "catalog",
+ "schema",
+ "fileset",
+ null,
+ "value"));
+
+ assertThrows(RuntimeException.class, spySetProperty::validate);
+ verify(spySetProperty, never()).handle();
+ String errOutput = new String(errContent.toByteArray(),
StandardCharsets.UTF_8).trim();
+ assertEquals(ErrorMessages.MISSING_PROPERTY, errOutput);
+ }
+
+ @Test
+ void testSetFilesetPropertyCommandWithoutValue() {
+ Main.useExit = false;
+ SetFilesetProperty spySetProperty =
+ spy(
+ new SetFilesetProperty(
+ GravitinoCommandLine.DEFAULT_URL,
+ false,
+ "metalake_demo",
+ "catalog",
+ "schema",
+ "fileset",
+ "property",
+ null));
+
+ assertThrows(RuntimeException.class, spySetProperty::validate);
+ verify(spySetProperty, never()).handle();
+ String errOutput = new String(errContent.toByteArray(),
StandardCharsets.UTF_8).trim();
+ assertEquals(ErrorMessages.MISSING_VALUE, errOutput);
+ }
+
@Test
void testRemoveFilesetPropertyCommand() {
RemoveFilesetProperty mockSetProperties =
mock(RemoveFilesetProperty.class);
@@ -345,10 +417,31 @@ class TestFilesetCommands {
"schema",
"fileset",
"property");
+ doReturn(mockSetProperties).when(mockSetProperties).validate();
commandLine.handleCommandLine();
verify(mockSetProperties).handle();
}
+ @Test
+ void testRemoveFilesetPropertyCommandWithoutProperty() {
+ Main.useExit = false;
+ RemoveFilesetProperty spyRemoveProperty =
+ spy(
+ new RemoveFilesetProperty(
+ GravitinoCommandLine.DEFAULT_URL,
+ false,
+ "metalake_demo",
+ "catalog",
+ "schema",
+ "fileset",
+ null));
+
+ assertThrows(RuntimeException.class, spyRemoveProperty::validate);
+ verify(spyRemoveProperty, never()).handle();
+ String errOutput = new String(errContent.toByteArray(),
StandardCharsets.UTF_8).trim();
+ assertEquals(ErrorMessages.MISSING_PROPERTY, errOutput);
+ }
+
@Test
@SuppressWarnings("DefaultCharset")
void testListFilesetCommandWithoutCatalog() {
diff --git
a/clients/cli/src/test/java/org/apache/gravitino/cli/TestTopicCommands.java
b/clients/cli/src/test/java/org/apache/gravitino/cli/TestTopicCommands.java
index c886b4f8ed..31904b8856 100644
--- a/clients/cli/src/test/java/org/apache/gravitino/cli/TestTopicCommands.java
+++ b/clients/cli/src/test/java/org/apache/gravitino/cli/TestTopicCommands.java
@@ -89,6 +89,7 @@ class TestTopicCommands {
.when(commandLine)
.newListTopics(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo",
"catalog", "schema");
+ doReturn(mockList).when(mockList).validate();
commandLine.handleCommandLine();
verify(mockList).handle();
}
@@ -108,6 +109,7 @@ class TestTopicCommands {
.when(commandLine)
.newTopicDetails(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo",
"catalog", "schema", "topic");
+ doReturn(mockDetails).when(mockDetails).validate();
commandLine.handleCommandLine();
verify(mockDetails).handle();
}
@@ -136,6 +138,7 @@ class TestTopicCommands {
"schema",
"topic",
"comment");
+ doReturn(mockCreate).when(mockCreate).validate();
commandLine.handleCommandLine();
verify(mockCreate).handle();
}
@@ -161,6 +164,7 @@ class TestTopicCommands {
"catalog",
"schema",
"topic");
+ doReturn(mockDelete).when(mockDelete).validate();
commandLine.handleCommandLine();
verify(mockDelete).handle();
}
@@ -187,6 +191,7 @@ class TestTopicCommands {
"catalog",
"schema",
"topic");
+ doReturn(mockDelete).when(mockDelete).validate();
commandLine.handleCommandLine();
verify(mockDelete).handle();
}
@@ -215,6 +220,7 @@ class TestTopicCommands {
"schema",
"topic",
"new comment");
+ doReturn(mockUpdate).when(mockUpdate).validate();
commandLine.handleCommandLine();
verify(mockUpdate).handle();
}
@@ -235,6 +241,7 @@ class TestTopicCommands {
.when(commandLine)
.newListTopicProperties(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo",
"catalog", "schema", "topic");
+ doReturn(mockListProperties).when(mockListProperties).validate();
commandLine.handleCommandLine();
verify(mockListProperties).handle();
}
@@ -266,10 +273,71 @@ class TestTopicCommands {
"topic",
"property",
"value");
+ doReturn(mockSetProperties).when(mockSetProperties).validate();
commandLine.handleCommandLine();
verify(mockSetProperties).handle();
}
+ @Test
+ void testSetTopicPropertyCommandWithoutPropertyAndValue() {
+ Main.useExit = false;
+ SetTopicProperty spySetProperty =
+ spy(
+ new SetTopicProperty(
+ GravitinoCommandLine.DEFAULT_URL,
+ false,
+ "metalake_demo",
+ "catalog",
+ "schema",
+ "topic",
+ null,
+ null));
+ assertThrows(RuntimeException.class, spySetProperty::validate);
+ verify(spySetProperty, never()).handle();
+ String output = new String(errContent.toByteArray(),
StandardCharsets.UTF_8).trim();
+ assertEquals(ErrorMessages.MISSING_PROPERTY_AND_VALUE, output);
+ }
+
+ @Test
+ void testSetTopicPropertyCommandWithoutProperty() {
+ Main.useExit = false;
+ SetTopicProperty spySetProperty =
+ spy(
+ new SetTopicProperty(
+ GravitinoCommandLine.DEFAULT_URL,
+ false,
+ "metalake_demo",
+ "catalog",
+ "schema",
+ "topic",
+ null,
+ "value"));
+ assertThrows(RuntimeException.class, spySetProperty::validate);
+ verify(spySetProperty, never()).handle();
+ String output = new String(errContent.toByteArray(),
StandardCharsets.UTF_8).trim();
+ assertEquals(ErrorMessages.MISSING_PROPERTY, output);
+ }
+
+ @Test
+ void testSetTopicPropertyCommandWithoutValue() {
+ Main.useExit = false;
+ SetTopicProperty spySetProperty =
+ spy(
+ new SetTopicProperty(
+ GravitinoCommandLine.DEFAULT_URL,
+ false,
+ "metalake_demo",
+ "catalog",
+ "schema",
+ "topic",
+ "property",
+ null));
+ assertThrows(RuntimeException.class, spySetProperty::validate);
+ verify(spySetProperty, never()).handle();
+ String output = new String(errContent.toByteArray(),
StandardCharsets.UTF_8).trim();
+ assertEquals(ErrorMessages.MISSING_VALUE, output);
+ }
+
@Test
void testRemoveTopicPropertyCommand() {
RemoveTopicProperty mockSetProperties = mock(RemoveTopicProperty.class);
@@ -294,10 +362,31 @@ class TestTopicCommands {
"schema",
"topic",
"property");
+ doReturn(mockSetProperties).when(mockSetProperties).validate();
commandLine.handleCommandLine();
verify(mockSetProperties).handle();
}
+ @Test
+ void testRemoveTopicPropertyCommandWithoutProperty() {
+ Main.useExit = false;
+ RemoveTopicProperty spyRemoveProperty =
+ spy(
+ new RemoveTopicProperty(
+ GravitinoCommandLine.DEFAULT_URL,
+ false,
+ "metalake_demo",
+ "catalog",
+ "schema",
+ "topic",
+ null));
+
+ assertThrows(RuntimeException.class, spyRemoveProperty::validate);
+ verify(spyRemoveProperty, never()).handle();
+ String output = new String(errContent.toByteArray(),
StandardCharsets.UTF_8).trim();
+ assertEquals(ErrorMessages.MISSING_PROPERTY, output);
+ }
+
@Test
@SuppressWarnings("DefaultCharset")
void testListTopicCommandWithoutCatalog() {
