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 d5a29a4161 [#6112] fix(CLI): Refactor the validation logic of column
and model (#6120)
d5a29a4161 is described below
commit d5a29a41615539cf77b82438338eeeeeff222d7d
Author: Lord of Abyss <103809695+abyss-l...@users.noreply.github.com>
AuthorDate: Tue Jan 7 09:25:04 2025 +0800
[#6112] fix(CLI): Refactor the validation logic of column and model (#6120)
### What changes were proposed in this pull request?
Refactor the validation logic of column and model.
### Why are the changes needed?
Fix: #6112
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
ut + local test
```bash
gcli model update -m demo_metalake --name catalog.schema.model
# Missing --uri option.
gcli column update -m demo_metalake --name
Hive_catalog.default.test_dates.id --default
# Missing --datatype option.
```
---
.../org/apache/gravitino/cli/ErrorMessages.java | 1 +
.../apache/gravitino/cli/GravitinoCommandLine.java | 35 ++++++++-------
.../apache/gravitino/cli/commands/LinkModel.java | 6 +++
.../cli/commands/UpdateColumnDefault.java | 6 +++
.../apache/gravitino/cli/TestCatalogCommands.java | 1 +
.../apache/gravitino/cli/TestColumnCommands.java | 33 ++++++++++++++
.../apache/gravitino/cli/TestModelCommands.java | 50 ++++++++++++----------
.../apache/gravitino/cli/TestSchemaCommands.java | 1 +
.../apache/gravitino/cli/TestTableCommands.java | 1 +
9 files changed, 96 insertions(+), 38 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 c839ad162b..abc6421d95 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
@@ -49,6 +49,7 @@ public class ErrorMessages {
public static final String MALFORMED_NAME = "Malformed entity name.";
public static final String MISSING_COLUMN_FILE = "Missing --columnfile
option.";
+ public static final String MISSING_DATATYPE = "Missing --datatype option.";
public static final String MISSING_ENTITIES = "Missing required entity
names: ";
public static final String MISSING_GROUP = "Missing --group option.";
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 f93da3003c..07a1ecd5b7 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
@@ -257,7 +257,7 @@ public class GravitinoCommandLine extends
TestableCommandLine {
// Handle the CommandActions.LIST action separately as it doesn't use
`catalog`
if (CommandActions.LIST.equals(command)) {
- newListCatalogs(url, ignore, outputFormat, metalake).handle();
+ newListCatalogs(url, ignore, outputFormat, metalake).validate().handle();
return;
}
@@ -356,7 +356,7 @@ public class GravitinoCommandLine extends
TestableCommandLine {
// Handle the CommandActions.LIST action separately as it doesn't use
`schema`
if (CommandActions.LIST.equals(command)) {
checkEntities(missingEntities);
- newListSchema(url, ignore, metalake, catalog).handle();
+ newListSchema(url, ignore, metalake, catalog).validate().handle();
return;
}
@@ -429,7 +429,7 @@ public class GravitinoCommandLine extends
TestableCommandLine {
// Handle CommandActions.LIST action separately as it doesn't require the
`table`
if (CommandActions.LIST.equals(command)) {
checkEntities(missingEntities);
- newListTables(url, ignore, metalake, catalog, schema).handle();
+ newListTables(url, ignore, metalake, catalog,
schema).validate().handle();
return;
}
@@ -833,7 +833,7 @@ public class GravitinoCommandLine extends
TestableCommandLine {
if (CommandActions.LIST.equals(command)) {
checkEntities(missingEntities);
- newListColumns(url, ignore, metalake, catalog, schema, table).handle();
+ newListColumns(url, ignore, metalake, catalog, schema,
table).validate().handle();
return;
}
@@ -844,7 +844,7 @@ public class GravitinoCommandLine extends
TestableCommandLine {
switch (command) {
case CommandActions.DETAILS:
if (line.hasOption(GravitinoOptions.AUDIT)) {
- newColumnAudit(url, ignore, metalake, catalog, schema, table,
column).handle();
+ newColumnAudit(url, ignore, metalake, catalog, schema, table,
column).validate().handle();
} else {
System.err.println(ErrorMessages.UNSUPPORTED_ACTION);
Main.exit(-1);
@@ -878,12 +878,13 @@ public class GravitinoCommandLine extends
TestableCommandLine {
nullable,
autoIncrement,
defaultValue)
+ .validate()
.handle();
break;
}
case CommandActions.DELETE:
- newDeleteColumn(url, ignore, metalake, catalog, schema, table,
column).handle();
+ newDeleteColumn(url, ignore, metalake, catalog, schema, table,
column).validate().handle();
break;
case CommandActions.UPDATE:
@@ -891,34 +892,40 @@ public class GravitinoCommandLine extends
TestableCommandLine {
if (line.hasOption(GravitinoOptions.COMMENT)) {
String comment = line.getOptionValue(GravitinoOptions.COMMENT);
newUpdateColumnComment(url, ignore, metalake, catalog, schema,
table, column, comment)
+ .validate()
.handle();
}
if (line.hasOption(GravitinoOptions.RENAME)) {
String newName = line.getOptionValue(GravitinoOptions.RENAME);
newUpdateColumnName(url, ignore, metalake, catalog, schema, table,
column, newName)
+ .validate()
.handle();
}
if (line.hasOption(GravitinoOptions.DATATYPE)
&& !line.hasOption(GravitinoOptions.DEFAULT)) {
String datatype = line.getOptionValue(GravitinoOptions.DATATYPE);
newUpdateColumnDatatype(url, ignore, metalake, catalog, schema,
table, column, datatype)
+ .validate()
.handle();
}
if (line.hasOption(GravitinoOptions.POSITION)) {
String position = line.getOptionValue(GravitinoOptions.POSITION);
newUpdateColumnPosition(url, ignore, metalake, catalog, schema,
table, column, position)
+ .validate()
.handle();
}
if (line.hasOption(GravitinoOptions.NULL)) {
boolean nullable =
line.getOptionValue(GravitinoOptions.NULL).equals("true");
newUpdateColumnNullability(
url, ignore, metalake, catalog, schema, table, column,
nullable)
+ .validate()
.handle();
}
if (line.hasOption(GravitinoOptions.AUTO)) {
boolean autoIncrement =
line.getOptionValue(GravitinoOptions.AUTO).equals("true");
newUpdateColumnAutoIncrement(
url, ignore, metalake, catalog, schema, table, column,
autoIncrement)
+ .validate()
.handle();
}
if (line.hasOption(GravitinoOptions.DEFAULT)) {
@@ -926,6 +933,7 @@ public class GravitinoCommandLine extends
TestableCommandLine {
String dataType = line.getOptionValue(GravitinoOptions.DATATYPE);
newUpdateColumnDefault(
url, ignore, metalake, catalog, schema, table, column,
defaultValue, dataType)
+ .validate()
.handle();
}
break;
@@ -1207,7 +1215,7 @@ public class GravitinoCommandLine extends
TestableCommandLine {
// Handle CommandActions.LIST action separately as it doesn't require the
`model`
if (CommandActions.LIST.equals(command)) {
checkEntities(missingEntities);
- newListModel(url, ignore, metalake, catalog, schema).handle();
+ newListModel(url, ignore, metalake, catalog, schema).validate().handle();
return;
}
@@ -1218,15 +1226,15 @@ public class GravitinoCommandLine extends
TestableCommandLine {
switch (command) {
case CommandActions.DETAILS:
if (line.hasOption(GravitinoOptions.AUDIT)) {
- newModelAudit(url, ignore, metalake, catalog, schema,
model).handle();
+ newModelAudit(url, ignore, metalake, catalog, schema,
model).validate().handle();
} else {
- newModelDetails(url, ignore, metalake, catalog, schema,
model).handle();
+ newModelDetails(url, ignore, metalake, catalog, schema,
model).validate().handle();
}
break;
case CommandActions.DELETE:
boolean force = line.hasOption(GravitinoOptions.FORCE);
- newDeleteModel(url, ignore, force, metalake, catalog, schema,
model).handle();
+ newDeleteModel(url, ignore, force, metalake, catalog, schema,
model).validate().handle();
break;
case CommandActions.CREATE:
@@ -1235,17 +1243,13 @@ public class GravitinoCommandLine extends
TestableCommandLine {
Map<String, String> createPropertyMap = new
Properties().parse(createProperties);
newCreateModel(
url, ignore, metalake, catalog, schema, model, createComment,
createPropertyMap)
+ .validate()
.handle();
break;
case CommandActions.UPDATE:
String[] alias = line.getOptionValues(GravitinoOptions.ALIAS);
String uri = line.getOptionValue(GravitinoOptions.URI);
- if (uri == null) {
- System.err.println(ErrorMessages.MISSING_URI);
- Main.exit(-1);
- }
-
String linkComment = line.getOptionValue(GravitinoOptions.COMMENT);
String[] linkProperties =
line.getOptionValues(CommandActions.PROPERTIES);
Map<String, String> linkPropertityMap = new
Properties().parse(linkProperties);
@@ -1260,6 +1264,7 @@ public class GravitinoCommandLine extends
TestableCommandLine {
alias,
linkComment,
linkPropertityMap)
+ .validate()
.handle();
break;
diff --git
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/LinkModel.java
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/LinkModel.java
index 6e8a4ffb76..cf34eae882 100644
--- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/LinkModel.java
+++ b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/LinkModel.java
@@ -103,4 +103,10 @@ public class LinkModel extends Command {
System.out.println(
"Linked model " + model + " to " + uri + " with aliases " +
Arrays.toString(alias));
}
+
+ @Override
+ public Command validate() {
+ if (uri == null) exitWithError(ErrorMessages.MISSING_URI);
+ return super.validate();
+ }
}
diff --git
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/UpdateColumnDefault.java
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/UpdateColumnDefault.java
index 7c7c2d3b40..976cf62305 100644
---
a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/UpdateColumnDefault.java
+++
b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/UpdateColumnDefault.java
@@ -103,4 +103,10 @@ public class UpdateColumnDefault extends Command {
System.out.println(column + " default changed.");
}
+
+ @Override
+ public Command validate() {
+ if (dataType == null) exitWithError(ErrorMessages.MISSING_DATATYPE);
+ return super.validate();
+ }
}
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 04c0dacc13..afa19b94c5 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
@@ -92,6 +92,7 @@ class TestCatalogCommands {
doReturn(mockList)
.when(commandLine)
.newListCatalogs(GravitinoCommandLine.DEFAULT_URL, false, null,
"metalake_demo");
+ doReturn(mockList).when(mockList).validate();
commandLine.handleCommandLine();
verify(mockList).handle();
}
diff --git
a/clients/cli/src/test/java/org/apache/gravitino/cli/TestColumnCommands.java
b/clients/cli/src/test/java/org/apache/gravitino/cli/TestColumnCommands.java
index 2d1e12debc..31a3139482 100644
--- a/clients/cli/src/test/java/org/apache/gravitino/cli/TestColumnCommands.java
+++ b/clients/cli/src/test/java/org/apache/gravitino/cli/TestColumnCommands.java
@@ -93,6 +93,7 @@ class TestColumnCommands {
.when(commandLine)
.newListColumns(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo",
"catalog", "schema", "users");
+ doReturn(mockList).when(mockList).validate();
commandLine.handleCommandLine();
verify(mockList).handle();
}
@@ -120,6 +121,7 @@ class TestColumnCommands {
"schema",
"users",
"name");
+ doReturn(mockAudit).when(mockAudit).validate();
commandLine.handleCommandLine();
verify(mockAudit).handle();
}
@@ -187,6 +189,7 @@ class TestColumnCommands {
true,
false,
null);
+ doReturn(mockAddColumn).when(mockAddColumn).validate();
commandLine.handleCommandLine();
verify(mockAddColumn).handle();
}
@@ -214,6 +217,7 @@ class TestColumnCommands {
"schema",
"users",
"name");
+ doReturn(mockDelete).when(mockDelete).validate();
commandLine.handleCommandLine();
verify(mockDelete).handle();
}
@@ -246,6 +250,7 @@ class TestColumnCommands {
"users",
"name",
"new comment");
+ doReturn(mockUpdateColumn).when(mockUpdateColumn).validate();
commandLine.handleCommandLine();
verify(mockUpdateColumn).handle();
}
@@ -278,6 +283,7 @@ class TestColumnCommands {
"users",
"name",
"renamed");
+ doReturn(mockUpdateName).when(mockUpdateName).validate();
commandLine.handleCommandLine();
verify(mockUpdateName).handle();
}
@@ -310,6 +316,7 @@ class TestColumnCommands {
"users",
"name",
"varchar(250)");
+ doReturn(mockUpdateDatatype).when(mockUpdateDatatype).validate();
commandLine.handleCommandLine();
verify(mockUpdateDatatype).handle();
}
@@ -342,6 +349,7 @@ class TestColumnCommands {
"users",
"name",
"first");
+ doReturn(mockUpdatePosition).when(mockUpdatePosition).validate();
commandLine.handleCommandLine();
verify(mockUpdatePosition).handle();
}
@@ -373,6 +381,7 @@ class TestColumnCommands {
"users",
"name",
true);
+ doReturn(mockUpdateNull).when(mockUpdateNull).validate();
commandLine.handleCommandLine();
verify(mockUpdateNull).handle();
}
@@ -404,6 +413,7 @@ class TestColumnCommands {
"users",
"name",
true);
+ doReturn(mockUpdateAuto).when(mockUpdateAuto).validate();
commandLine.handleCommandLine();
verify(mockUpdateAuto).handle();
}
@@ -439,10 +449,33 @@ class TestColumnCommands {
"name",
"Fred Smith",
"varchar(100)");
+ doReturn(mockUpdateDefault).when(mockUpdateDefault).validate();
commandLine.handleCommandLine();
verify(mockUpdateDefault).handle();
}
+ @Test
+ void testUpdateColumnDefaultWithoutDataType() {
+ Main.useExit = false;
+ UpdateColumnDefault spyUpdate =
+ spy(
+ new UpdateColumnDefault(
+ GravitinoCommandLine.DEFAULT_URL,
+ false,
+ "metalake_demo",
+ "catalog",
+ "schema",
+ "user",
+ "name",
+ "",
+ null));
+
+ assertThrows(RuntimeException.class, spyUpdate::validate);
+ verify(spyUpdate, never()).handle();
+ String output = new String(errContent.toByteArray(),
StandardCharsets.UTF_8).trim();
+ assertEquals(ErrorMessages.MISSING_DATATYPE, output);
+ }
+
@Test
@SuppressWarnings("DefaultCharset")
void testDeleteColumnCommandWithoutCatalog() {
diff --git
a/clients/cli/src/test/java/org/apache/gravitino/cli/TestModelCommands.java
b/clients/cli/src/test/java/org/apache/gravitino/cli/TestModelCommands.java
index 8d475d3625..b83cc3c313 100644
--- a/clients/cli/src/test/java/org/apache/gravitino/cli/TestModelCommands.java
+++ b/clients/cli/src/test/java/org/apache/gravitino/cli/TestModelCommands.java
@@ -94,6 +94,7 @@ public class TestModelCommands {
eq("metalake_demo"),
eq("catalog"),
eq("schema"));
+ doReturn(mockList).when(mockList).validate();
commandLine.handleCommandLine();
verify(mockList).handle();
}
@@ -176,6 +177,7 @@ public class TestModelCommands {
eq("catalog"),
eq("schema"),
eq("model"));
+ doReturn(mockList).when(mockList).validate();
commandLine.handleCommandLine();
verify(mockList).handle();
}
@@ -291,6 +293,7 @@ public class TestModelCommands {
.when(commandLine)
.newModelAudit(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo",
"catalog", "schema", "model");
+ doReturn(mockAudit).when(mockAudit).validate();
commandLine.handleCommandLine();
verify(mockAudit).handle();
}
@@ -320,6 +323,7 @@ public class TestModelCommands {
eq("model"),
isNull(),
argThat(Map::isEmpty));
+ doReturn(mockCreate).when(mockCreate).validate();
commandLine.handleCommandLine();
verify(mockCreate).handle();
}
@@ -349,6 +353,7 @@ public class TestModelCommands {
eq("model"),
eq("comment"),
argThat(Map::isEmpty));
+ doReturn(mockCreate).when(mockCreate).validate();
commandLine.handleCommandLine();
verify(mockCreate).handle();
}
@@ -384,6 +389,7 @@ public class TestModelCommands {
argument.size() == 2
&& argument.containsKey("key1")
&& argument.get("key1").equals("val1")));
+ doReturn(mockCreate).when(mockCreate).validate();
commandLine.handleCommandLine();
verify(mockCreate).handle();
}
@@ -420,6 +426,7 @@ public class TestModelCommands {
argument.size() == 2
&& argument.containsKey("key1")
&& argument.get("key1").equals("val1")));
+ doReturn(mockCreate).when(mockCreate).validate();
commandLine.handleCommandLine();
verify(mockCreate).handle();
}
@@ -446,6 +453,7 @@ public class TestModelCommands {
"catalog",
"schema",
"model");
+ doReturn(mockDelete).when(mockDelete).validate();
commandLine.handleCommandLine();
verify(mockDelete).handle();
}
@@ -478,6 +486,7 @@ public class TestModelCommands {
isNull(),
isNull(),
argThat(Map::isEmpty));
+ doReturn(linkModelMock).when(linkModelMock).validate();
commandLine.handleCommandLine();
verify(linkModelMock).handle();
}
@@ -516,6 +525,7 @@ public class TestModelCommands {
&& "aliasB".equals(argument[1])),
isNull(),
argThat(Map::isEmpty));
+ doReturn(linkModelMock).when(linkModelMock).validate();
commandLine.handleCommandLine();
verify(linkModelMock).handle();
}
@@ -523,30 +533,23 @@ public class TestModelCommands {
@Test
void testLinkModelCommandWithoutURI() {
Main.useExit = false;
-
when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true);
-
when(mockCommandLine.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo");
- when(mockCommandLine.hasOption(GravitinoOptions.NAME)).thenReturn(true);
-
when(mockCommandLine.getOptionValue(GravitinoOptions.NAME)).thenReturn("catalog.schema.model");
- when(mockCommandLine.hasOption(GravitinoOptions.URI)).thenReturn(false);
- when(mockCommandLine.hasOption(GravitinoOptions.ALIAS)).thenReturn(false);
- GravitinoCommandLine commandLine =
- spy(
- new GravitinoCommandLine(
- mockCommandLine, mockOptions, CommandEntities.MODEL,
CommandActions.UPDATE));
- assertThrows(RuntimeException.class, commandLine::handleCommandLine);
- verify(commandLine, never())
- .newLinkModel(
- eq(GravitinoCommandLine.DEFAULT_URL),
- eq(false),
- eq("metalake_demo"),
- eq("catalog"),
- eq("schema"),
- eq("model"),
- isNull(),
- isNull(),
- isNull(),
- argThat(Map::isEmpty));
+ LinkModel spyLinkModel =
+ spy(
+ new LinkModel(
+ GravitinoCommandLine.DEFAULT_URL,
+ false,
+ "metalake_demo",
+ "catalog",
+ "schema",
+ "model",
+ null,
+ new String[] {"aliasA", "aliasB"},
+ "comment",
+ Collections.EMPTY_MAP));
+
+ assertThrows(RuntimeException.class, spyLinkModel::validate);
+ verify(spyLinkModel, never()).handle();
String output = new String(errContent.toByteArray(),
StandardCharsets.UTF_8).trim();
assertEquals(ErrorMessages.MISSING_URI, output);
}
@@ -596,6 +599,7 @@ public class TestModelCommands {
&& argument.containsKey("key2")
&& "val1".equals(argument.get("key1"))
&& "val2".equals(argument.get("key2"))));
+ doReturn(linkModelMock).when(linkModelMock).validate();
commandLine.handleCommandLine();
verify(linkModelMock).handle();
}
diff --git
a/clients/cli/src/test/java/org/apache/gravitino/cli/TestSchemaCommands.java
b/clients/cli/src/test/java/org/apache/gravitino/cli/TestSchemaCommands.java
index 9059afeedb..6b8770d8ed 100644
--- a/clients/cli/src/test/java/org/apache/gravitino/cli/TestSchemaCommands.java
+++ b/clients/cli/src/test/java/org/apache/gravitino/cli/TestSchemaCommands.java
@@ -88,6 +88,7 @@ class TestSchemaCommands {
doReturn(mockList)
.when(commandLine)
.newListSchema(GravitinoCommandLine.DEFAULT_URL, false,
"metalake_demo", "catalog");
+ doReturn(mockList).when(mockList).validate();
commandLine.handleCommandLine();
verify(mockList).handle();
}
diff --git
a/clients/cli/src/test/java/org/apache/gravitino/cli/TestTableCommands.java
b/clients/cli/src/test/java/org/apache/gravitino/cli/TestTableCommands.java
index 0193c834a5..f068332045 100644
--- a/clients/cli/src/test/java/org/apache/gravitino/cli/TestTableCommands.java
+++ b/clients/cli/src/test/java/org/apache/gravitino/cli/TestTableCommands.java
@@ -95,6 +95,7 @@ class TestTableCommands {
.when(commandLine)
.newListTables(
GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo",
"catalog", "schema");
+ doReturn(mockList).when(mockList).validate();
commandLine.handleCommandLine();
verify(mockList).handle();
}
