From d2dcf586ed9540e40d9c3dae0d33e157b6d18b11 Mon Sep 17 00:00:00 2001 From: pancx Date: Mon, 6 Jan 2025 21:11:33 +0800 Subject: [PATCH 1/2] [#6121] fix(CLI): Refactor the validation logic of topic and fileset Refactor the validation logic of topic and fix the test case. --- .../gravitino/cli/GravitinoCommandLine.java | 21 +++-- .../cli/commands/RemoveTopicProperty.java | 6 ++ .../cli/commands/SetTopicProperty.java | 6 ++ .../gravitino/cli/TestTopicCommands.java | 89 +++++++++++++++++++ 4 files changed, 115 insertions(+), 7 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 589aa437df5..2bcdbf6e50b 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 @@ private void handleTopicCommand() { 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 @@ private void handleTopicCommand() { 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 @@ private void handleTopicCommand() { { 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 @@ private void handleTopicCommand() { 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 @@ private void handleTopicCommand() { 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: 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 a43820933e8..51be0a139d9 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 void handle() { 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/SetTopicProperty.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetTopicProperty.java index 941c0b0321e..2641259cdde 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 void handle() { 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/TestTopicCommands.java b/clients/cli/src/test/java/org/apache/gravitino/cli/TestTopicCommands.java index c886b4f8ede..31904b88563 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 @@ void testListTopicsCommand() { .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 @@ void testTopicDetailsCommand() { .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 @@ void testCreateTopicCommand() { "schema", "topic", "comment"); + doReturn(mockCreate).when(mockCreate).validate(); commandLine.handleCommandLine(); verify(mockCreate).handle(); } @@ -161,6 +164,7 @@ void testDeleteTopicCommand() { "catalog", "schema", "topic"); + doReturn(mockDelete).when(mockDelete).validate(); commandLine.handleCommandLine(); verify(mockDelete).handle(); } @@ -187,6 +191,7 @@ void testDeleteTopicForceCommand() { "catalog", "schema", "topic"); + doReturn(mockDelete).when(mockDelete).validate(); commandLine.handleCommandLine(); verify(mockDelete).handle(); } @@ -215,6 +220,7 @@ void testUpdateCommentTopicCommand() { "schema", "topic", "new comment"); + doReturn(mockUpdate).when(mockUpdate).validate(); commandLine.handleCommandLine(); verify(mockUpdate).handle(); } @@ -235,6 +241,7 @@ void testListTopicPropertiesCommand() { .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 @@ void testSetTopicPropertyCommand() { "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 @@ void testRemoveTopicPropertyCommand() { "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() { From 2a9843d3a414763a4cc9da39150b7432157d2d96 Mon Sep 17 00:00:00 2001 From: pancx Date: Tue, 7 Jan 2025 00:30:12 +0800 Subject: [PATCH 2/2] [#6121] fix(CLI): Refactor the validation logic of topic and fileset Refactor the validation logic of fileset and fix the test case. --- .../gravitino/cli/GravitinoCommandLine.java | 20 +++- .../cli/commands/RemoveFilesetProperty.java | 6 ++ .../cli/commands/SetFilesetProperty.java | 6 ++ .../gravitino/cli/TestFilesetCommands.java | 93 +++++++++++++++++++ 4 files changed, 120 insertions(+), 5 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 2bcdbf6e50b..f93da3003cc 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 @@ -1105,7 +1105,7 @@ private void handleFilesetCommand() { // 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; } @@ -1115,7 +1115,7 @@ private void handleFilesetCommand() { 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: @@ -1124,6 +1124,7 @@ private void handleFilesetCommand() { String[] properties = line.getOptionValues(CommandActions.PROPERTIES); Map propertyMap = new Properties().parse(properties); newCreateFileset(url, ignore, metalake, catalog, schema, fileset, comment, propertyMap) + .validate() .handle(); break; } @@ -1131,7 +1132,9 @@ private void handleFilesetCommand() { 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; } @@ -1140,6 +1143,7 @@ private void handleFilesetCommand() { String property = line.getOptionValue(GravitinoOptions.PROPERTY); String value = line.getOptionValue(GravitinoOptions.VALUE); newSetFilesetProperty(url, ignore, metalake, catalog, schema, fileset, property, value) + .validate() .handle(); break; } @@ -1148,12 +1152,15 @@ private void handleFilesetCommand() { { 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: @@ -1161,11 +1168,14 @@ private void handleFilesetCommand() { 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 00deebe265a..c443bf0fdfe 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 void handle() { 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 2c179db104c..afafa3c9dbd 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 void handle() { System.out.println(schema + " 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 1e8c54124c1..3529e60bf77 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 @@ void testListFilesetsCommand() { .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 @@ void testFilesetDetailsCommand() { "catalog", "schema", "fileset"); + doReturn(mockDetails).when(mockDetails).validate(); commandLine.handleCommandLine(); verify(mockDetails).handle(); } @@ -147,6 +149,7 @@ void testCreateFilesetCommand() { eq("fileset"), eq("comment"), any()); + doReturn(mockCreate).when(mockCreate).validate(); commandLine.handleCommandLine(); verify(mockCreate).handle(); } @@ -173,6 +176,7 @@ void testDeleteFilesetCommand() { "catalog", "schema", "fileset"); + doReturn(mockDelete).when(mockDelete).validate(); commandLine.handleCommandLine(); verify(mockDelete).handle(); } @@ -200,6 +204,7 @@ void testDeleteFilesetForceCommand() { "catalog", "schema", "fileset"); + doReturn(mockDelete).when(mockDelete).validate(); commandLine.handleCommandLine(); verify(mockDelete).handle(); } @@ -229,6 +234,7 @@ void testUpdateFilesetCommentCommand() { "schema", "fileset", "new_comment"); + doReturn(mockUpdateComment).when(mockUpdateComment).validate(); commandLine.handleCommandLine(); verify(mockUpdateComment).handle(); } @@ -258,6 +264,7 @@ void testUpdateFilesetNameCommand() { "schema", "fileset", "new_name"); + doReturn(mockUpdateName).when(mockUpdateName).validate(); commandLine.handleCommandLine(); verify(mockUpdateName).handle(); } @@ -284,6 +291,7 @@ void testListFilesetPropertiesCommand() { "catalog", "schema", "fileset"); + doReturn(mockListProperties).when(mockListProperties).validate(); commandLine.handleCommandLine(); verify(mockListProperties).handle(); } @@ -316,10 +324,74 @@ void testSetFilesetPropertyCommand() { "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 @@ void testRemoveFilesetPropertyCommand() { "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() {