From 6ad3d3bf9c6db1c67ad7ccfd24d5ebfd2d06454a Mon Sep 17 00:00:00 2001 From: Lord of Abyss <103809695+Abyss-lord@users.noreply.github.com> Date: Tue, 7 Jan 2025 14:37:40 +0800 Subject: [PATCH] [#6123] fix(CLI): Refactor the validation logic of tag and role (#6127) ### What changes were proposed in this pull request? Refactor the validation logic of the Tag and Role, meantime fix the test case. ### Why are the changes needed? Fix: #6123 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? ut + local test **Role test** ```bash gcli role grant -m demo_metalake --role admin # Missing --privilege option. gcli role revoke -m demo_metalake --role admin # Missing --privilege option. ``` **Tag test** ```bash gcli tag set -m demo_metalake # Missing --name option. gcli tag set -m demo_metalake --name catalog.schema.table --property property --tag tagA # Missing --value option. gcli tag set -m demo_metalake --name catalog.schema.table --value value --tag tagA # Missing --property option. gcli tag remove -m demo_metalake --tag tagA # Missing --name option. gcli tag remove -m demo_metalake # Missing --name option. gcli tag delete -m demo_metalake # Missing --tag option. gcli tag create -m demo_metalake # Missing --tag option. ``` --- .../apache/gravitino/cli/ErrorMessages.java | 3 + .../gravitino/cli/GravitinoCommandLine.java | 81 +++--- .../gravitino/cli/commands/CreateTag.java | 6 + .../gravitino/cli/commands/DeleteTag.java | 6 + .../cli/commands/GrantPrivilegesToRole.java | 6 + .../gravitino/cli/commands/RemoveAllTags.java | 6 + .../commands/RevokePrivilegesFromRole.java | 6 + .../cli/commands/SetTagProperty.java | 6 + .../gravitino/cli/commands/TagEntity.java | 6 + .../gravitino/cli/commands/UntagEntity.java | 6 + .../gravitino/cli/TestRoleCommands.java | 39 ++- .../apache/gravitino/cli/TestTagCommands.java | 242 ++++++++++-------- 12 files changed, 256 insertions(+), 157 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 abc6421d955..ecf1dbff4c7 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 @@ -55,6 +55,7 @@ public class ErrorMessages { public static final String MISSING_GROUP = "Missing --group option."; public static final String MISSING_METALAKE = "Missing --metalake option."; public static final String MISSING_NAME = "Missing --name option."; + public static final String MISSING_PRIVILEGES = "Missing --privilege option."; public static final String MISSING_PROPERTY = "Missing --property option."; public static final String MISSING_PROPERTY_AND_VALUE = "Missing --property and --value options."; public static final String MISSING_ROLE = "Missing --role option."; @@ -63,6 +64,8 @@ public class ErrorMessages { public static final String MISSING_USER = "Missing --user option."; public static final String MISSING_VALUE = "Missing --value option."; + public static final String MULTIPLE_ROLE_COMMAND_ERROR = + "This command only supports one --role option."; public static final String MULTIPLE_TAG_COMMAND_ERROR = "This command only supports one --tag option."; public static final String MISSING_PROVIDER = "Missing --provider 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 07a1ecd5b7f..442ec2d1c33 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 @@ -20,7 +20,6 @@ package org.apache.gravitino.cli; import com.google.common.base.Joiner; -import com.google.common.base.Preconditions; import com.google.common.collect.Lists; import java.io.BufferedReader; import java.io.IOException; @@ -643,12 +642,6 @@ protected void handleTagCommand() { Command.setAuthenticationMode(auth, userName); String[] tags = line.getOptionValues(GravitinoOptions.TAG); - if (tags == null - && !((CommandActions.REMOVE.equals(command) && line.hasOption(GravitinoOptions.FORCE)) - || CommandActions.LIST.equals(command))) { - System.err.println(ErrorMessages.MISSING_TAG); - Main.exit(-1); - } if (tags != null) { tags = Arrays.stream(tags).distinct().toArray(String[]::new); @@ -656,41 +649,36 @@ protected void handleTagCommand() { switch (command) { case CommandActions.DETAILS: - newTagDetails(url, ignore, metalake, getOneTag(tags)).handle(); + newTagDetails(url, ignore, metalake, getOneTag(tags)).validate().handle(); break; case CommandActions.LIST: if (!name.hasCatalogName()) { - newListTags(url, ignore, metalake).handle(); + newListTags(url, ignore, metalake).validate().handle(); } else { - newListEntityTags(url, ignore, metalake, name).handle(); + newListEntityTags(url, ignore, metalake, name).validate().handle(); } break; case CommandActions.CREATE: String comment = line.getOptionValue(GravitinoOptions.COMMENT); - newCreateTags(url, ignore, metalake, tags, comment).handle(); + newCreateTags(url, ignore, metalake, tags, comment).validate().handle(); break; case CommandActions.DELETE: boolean forceDelete = line.hasOption(GravitinoOptions.FORCE); - newDeleteTag(url, ignore, forceDelete, metalake, tags).handle(); + newDeleteTag(url, ignore, forceDelete, metalake, tags).validate().handle(); break; case CommandActions.SET: String propertySet = line.getOptionValue(GravitinoOptions.PROPERTY); String valueSet = line.getOptionValue(GravitinoOptions.VALUE); - if (propertySet != null && valueSet != null) { - newSetTagProperty(url, ignore, metalake, getOneTag(tags), propertySet, valueSet).handle(); - } else if (propertySet == null && valueSet == null) { - if (!name.hasName()) { - System.err.println(ErrorMessages.MISSING_NAME); - Main.exit(-1); - } - newTagEntity(url, ignore, metalake, name, tags).handle(); + if (propertySet == null && valueSet == null) { + newTagEntity(url, ignore, metalake, name, tags).validate().handle(); } else { - System.err.println(ErrorMessages.INVALID_SET_COMMAND); - Main.exit(-1); + newSetTagProperty(url, ignore, metalake, getOneTag(tags), propertySet, valueSet) + .validate() + .handle(); } break; @@ -698,33 +686,33 @@ protected void handleTagCommand() { boolean isTag = line.hasOption(GravitinoOptions.TAG); if (!isTag) { boolean forceRemove = line.hasOption(GravitinoOptions.FORCE); - newRemoveAllTags(url, ignore, metalake, name, forceRemove).handle(); + newRemoveAllTags(url, ignore, metalake, name, forceRemove).validate().handle(); } else { String propertyRemove = line.getOptionValue(GravitinoOptions.PROPERTY); if (propertyRemove != null) { - newRemoveTagProperty(url, ignore, metalake, getOneTag(tags), propertyRemove).handle(); + newRemoveTagProperty(url, ignore, metalake, getOneTag(tags), propertyRemove) + .validate() + .handle(); } else { - if (!name.hasName()) { - System.err.println(ErrorMessages.MISSING_NAME); - Main.exit(-1); - } - newUntagEntity(url, ignore, metalake, name, tags).handle(); + newUntagEntity(url, ignore, metalake, name, tags).validate().handle(); } } break; case CommandActions.PROPERTIES: - newListTagProperties(url, ignore, metalake, getOneTag(tags)).handle(); + newListTagProperties(url, ignore, metalake, getOneTag(tags)).validate().handle(); break; case CommandActions.UPDATE: if (line.hasOption(GravitinoOptions.COMMENT)) { String updateComment = line.getOptionValue(GravitinoOptions.COMMENT); - newUpdateTagComment(url, ignore, metalake, getOneTag(tags), updateComment).handle(); + newUpdateTagComment(url, ignore, metalake, getOneTag(tags), updateComment) + .validate() + .handle(); } if (line.hasOption(GravitinoOptions.RENAME)) { String newName = line.getOptionValue(GravitinoOptions.RENAME); - newUpdateTagName(url, ignore, metalake, getOneTag(tags), newName).handle(); + newUpdateTagName(url, ignore, metalake, getOneTag(tags), newName).validate().handle(); } break; @@ -736,7 +724,7 @@ protected void handleTagCommand() { } private String getOneTag(String[] tags) { - if (tags.length > 1) { + if (tags == null || tags.length > 1) { System.err.println(ErrorMessages.MULTIPLE_TAG_COMMAND_ERROR); Main.exit(-1); } @@ -767,34 +755,34 @@ protected void handleRoleCommand() { switch (command) { case CommandActions.DETAILS: if (line.hasOption(GravitinoOptions.AUDIT)) { - newRoleAudit(url, ignore, metalake, getOneRole(roles, CommandActions.DETAILS)).handle(); + newRoleAudit(url, ignore, metalake, getOneRole(roles)).validate().handle(); } else { - newRoleDetails(url, ignore, metalake, getOneRole(roles, CommandActions.DETAILS)).handle(); + newRoleDetails(url, ignore, metalake, getOneRole(roles)).validate().handle(); } break; case CommandActions.LIST: - newListRoles(url, ignore, metalake).handle(); + newListRoles(url, ignore, metalake).validate().handle(); break; case CommandActions.CREATE: - newCreateRole(url, ignore, metalake, roles).handle(); + newCreateRole(url, ignore, metalake, roles).validate().handle(); break; case CommandActions.DELETE: boolean forceDelete = line.hasOption(GravitinoOptions.FORCE); - newDeleteRole(url, ignore, forceDelete, metalake, roles).handle(); + newDeleteRole(url, ignore, forceDelete, metalake, roles).validate().handle(); break; case CommandActions.GRANT: - newGrantPrivilegesToRole( - url, ignore, metalake, getOneRole(roles, CommandActions.GRANT), name, privileges) + newGrantPrivilegesToRole(url, ignore, metalake, getOneRole(roles), name, privileges) + .validate() .handle(); break; case CommandActions.REVOKE: - newRevokePrivilegesFromRole( - url, ignore, metalake, getOneRole(roles, CommandActions.REMOVE), name, privileges) + newRevokePrivilegesFromRole(url, ignore, metalake, getOneRole(roles), name, privileges) + .validate() .handle(); break; @@ -805,9 +793,12 @@ url, ignore, metalake, getOneRole(roles, CommandActions.REMOVE), name, privilege } } - private String getOneRole(String[] roles, String command) { - Preconditions.checkArgument( - roles.length == 1, command + " requires only one role, but multiple are currently passed."); + private String getOneRole(String[] roles) { + if (roles == null || roles.length != 1) { + System.err.println(ErrorMessages.MULTIPLE_ROLE_COMMAND_ERROR); + Main.exit(-1); + } + return roles[0]; } diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/CreateTag.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/CreateTag.java index 87ab0da779d..dabf34c8b1b 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/CreateTag.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/CreateTag.java @@ -103,4 +103,10 @@ private void handleMultipleTags() { System.out.println("Tags " + String.join(",", remaining) + " not created"); } } + + @Override + public Command validate() { + if (tags == null) exitWithError(ErrorMessages.MISSING_TAG); + return super.validate(); + } } diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/DeleteTag.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/DeleteTag.java index 1e05292c82a..26919e06acf 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/DeleteTag.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/DeleteTag.java @@ -116,4 +116,10 @@ private void handleOnlyOneTag() { System.out.println("Tag " + tags[0] + " not deleted."); } } + + @Override + public Command validate() { + if (tags == null) exitWithError(ErrorMessages.MISSING_TAG); + return super.validate(); + } } diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/GrantPrivilegesToRole.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/GrantPrivilegesToRole.java index 584e073beac..8630282ea60 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/GrantPrivilegesToRole.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/GrantPrivilegesToRole.java @@ -103,4 +103,10 @@ public void handle() { String all = String.join(",", privileges); System.out.println(role + " granted " + all + " on " + entity.getName()); } + + @Override + public Command validate() { + if (privileges == null) exitWithError(ErrorMessages.MISSING_PRIVILEGES); + return super.validate(); + } } diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveAllTags.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveAllTags.java index a7aa3748a15..5221100a8e9 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveAllTags.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RemoveAllTags.java @@ -118,4 +118,10 @@ public void handle() { System.out.println(entity + " has no tags"); } } + + @Override + public Command validate() { + if (name == null || !name.hasName()) exitWithError(ErrorMessages.MISSING_NAME); + return super.validate(); + } } diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RevokePrivilegesFromRole.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RevokePrivilegesFromRole.java index a62e977a2fb..3bfa7cd4526 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RevokePrivilegesFromRole.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/RevokePrivilegesFromRole.java @@ -103,4 +103,10 @@ public void handle() { String all = String.join(",", privileges); System.out.println(role + " revoked " + all + " on " + entity.getName()); } + + @Override + public Command validate() { + if (privileges == null) exitWithError(ErrorMessages.MISSING_PRIVILEGES); + return super.validate(); + } } diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetTagProperty.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetTagProperty.java index b5b46b59a71..da7a267b8d4 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetTagProperty.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/SetTagProperty.java @@ -74,4 +74,10 @@ public void handle() { System.out.println(tag + " 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/TagEntity.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/TagEntity.java index 7bc8ec37649..4a06918850d 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/TagEntity.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/TagEntity.java @@ -105,4 +105,10 @@ public void handle() { System.out.println(entity + " now tagged with " + all); } + + @Override + public Command validate() { + if (name == null || !name.hasName()) exitWithError(ErrorMessages.MISSING_NAME); + return super.validate(); + } } diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/UntagEntity.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/UntagEntity.java index 8f4a4a9cf02..3503d5eb7bf 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/UntagEntity.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/UntagEntity.java @@ -113,4 +113,10 @@ public void handle() { System.out.println(entity + " removed tag " + tags[0].toString() + " now tagged with " + all); } } + + @Override + public Command validate() { + if (name == null || !name.hasName()) exitWithError(ErrorMessages.MISSING_NAME); + return super.validate(); + } } diff --git a/clients/cli/src/test/java/org/apache/gravitino/cli/TestRoleCommands.java b/clients/cli/src/test/java/org/apache/gravitino/cli/TestRoleCommands.java index 0e671067e3f..529979582ff 100644 --- a/clients/cli/src/test/java/org/apache/gravitino/cli/TestRoleCommands.java +++ b/clients/cli/src/test/java/org/apache/gravitino/cli/TestRoleCommands.java @@ -80,6 +80,7 @@ void testListRolesCommand() { doReturn(mockList) .when(commandLine) .newListRoles(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo"); + doReturn(mockList).when(mockList).validate(); commandLine.handleCommandLine(); verify(mockList).handle(); } @@ -98,12 +99,14 @@ void testRoleDetailsCommand() { doReturn(mockDetails) .when(commandLine) .newRoleDetails(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "admin"); + doReturn(mockDetails).when(mockDetails).validate(); commandLine.handleCommandLine(); verify(mockDetails).handle(); } @Test void testRoleDetailsCommandWithMultipleRoles() { + Main.useExit = false; when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true); when(mockCommandLine.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo"); when(mockCommandLine.hasOption(GravitinoOptions.ROLE)).thenReturn(true); @@ -114,7 +117,7 @@ void testRoleDetailsCommandWithMultipleRoles() { new GravitinoCommandLine( mockCommandLine, mockOptions, CommandEntities.ROLE, CommandActions.DETAILS)); - assertThrows(IllegalArgumentException.class, commandLine::handleCommandLine); + assertThrows(RuntimeException.class, commandLine::handleCommandLine); verify(commandLine, never()) .newRoleDetails( eq(GravitinoCommandLine.DEFAULT_URL), eq(false), eq("metalake_demo"), any()); @@ -135,6 +138,7 @@ void testRoleAuditCommand() { doReturn(mockAudit) .when(commandLine) .newRoleAudit(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "group"); + doReturn(mockAudit).when(mockAudit).validate(); commandLine.handleCommandLine(); verify(mockAudit).handle(); } @@ -154,6 +158,7 @@ void testCreateRoleCommand() { .when(commandLine) .newCreateRole( GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", new String[] {"admin"}); + doReturn(mockCreate).when(mockCreate).validate(); commandLine.handleCommandLine(); verify(mockCreate).handle(); } @@ -178,6 +183,7 @@ void testCreateRolesCommand() { eq(false), eq("metalake_demo"), eq(new String[] {"admin", "engineer", "scientist"})); + doReturn(mockCreate).when(mockCreate).validate(); commandLine.handleCommandLine(); verify(mockCreate).handle(); } @@ -201,6 +207,7 @@ void testDeleteRoleCommand() { false, "metalake_demo", new String[] {"admin"}); + doReturn(mockDelete).when(mockDelete).validate(); commandLine.handleCommandLine(); verify(mockDelete).handle(); } @@ -227,6 +234,7 @@ void testDeleteRolesCommand() { eq(false), eq("metalake_demo"), eq(new String[] {"admin", "engineer", "scientist"})); + doReturn(mockDelete).when(mockDelete).validate(); commandLine.handleCommandLine(); verify(mockDelete).handle(); } @@ -247,6 +255,7 @@ void testDeleteRoleForceCommand() { .when(commandLine) .newDeleteRole( GravitinoCommandLine.DEFAULT_URL, false, true, "metalake_demo", new String[] {"admin"}); + doReturn(mockDelete).when(mockDelete).validate(); commandLine.handleCommandLine(); verify(mockDelete).handle(); } @@ -276,10 +285,24 @@ void testGrantPrivilegesToRole() { eq("admin"), any(), eq(privileges)); + doReturn(mockGrant).when(mockGrant).validate(); commandLine.handleCommandLine(); verify(mockGrant).handle(); } + @Test + void testGrantPrivilegesToRoleWithoutPrivileges() { + Main.useExit = false; + GrantPrivilegesToRole spyGrantRole = + spy( + new GrantPrivilegesToRole( + GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "admin", null, null)); + assertThrows(RuntimeException.class, spyGrantRole::validate); + verify(spyGrantRole, never()).handle(); + String errOutput = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim(); + assertEquals(ErrorMessages.MISSING_PRIVILEGES, errOutput); + } + @Test void testRevokePrivilegesFromRole() { RevokePrivilegesFromRole mockRevoke = mock(RevokePrivilegesFromRole.class); @@ -305,10 +328,24 @@ void testRevokePrivilegesFromRole() { eq("admin"), any(), eq(privileges)); + doReturn(mockRevoke).when(mockRevoke).validate(); commandLine.handleCommandLine(); verify(mockRevoke).handle(); } + @Test + void testRevokePrivilegesFromRoleWithoutPrivileges() { + Main.useExit = false; + RevokePrivilegesFromRole spyGrantRole = + spy( + new RevokePrivilegesFromRole( + GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "admin", null, null)); + assertThrows(RuntimeException.class, spyGrantRole::validate); + verify(spyGrantRole, never()).handle(); + String errOutput = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim(); + assertEquals(ErrorMessages.MISSING_PRIVILEGES, errOutput); + } + @Test void testDeleteRoleCommandWithoutRole() { Main.useExit = false; diff --git a/clients/cli/src/test/java/org/apache/gravitino/cli/TestTagCommands.java b/clients/cli/src/test/java/org/apache/gravitino/cli/TestTagCommands.java index d3b0c8bfe18..a94ccee7daa 100644 --- a/clients/cli/src/test/java/org/apache/gravitino/cli/TestTagCommands.java +++ b/clients/cli/src/test/java/org/apache/gravitino/cli/TestTagCommands.java @@ -22,7 +22,6 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThrows; import static org.mockito.ArgumentMatchers.argThat; -import static org.mockito.ArgumentMatchers.isNull; import static org.mockito.Mockito.any; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.eq; @@ -95,6 +94,7 @@ void testListTagsCommand() { doReturn(mockList) .when(commandLine) .newListTags(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo"); + doReturn(mockList).when(mockList).validate(); commandLine.handleCommandLine(); verify(mockList).handle(); } @@ -113,6 +113,7 @@ void testTagDetailsCommand() { doReturn(mockDetails) .when(commandLine) .newTagDetails(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "tagA"); + doReturn(mockDetails).when(mockDetails).validate(); commandLine.handleCommandLine(); verify(mockDetails).handle(); } @@ -157,6 +158,7 @@ void testCreateTagCommand() { "metalake_demo", new String[] {"tagA"}, "comment"); + doReturn(mockCreate).when(mockCreate).validate(); commandLine.handleCommandLine(); verify(mockCreate).handle(); } @@ -164,25 +166,15 @@ void testCreateTagCommand() { @Test void testCreateCommandWithoutTagOption() { Main.useExit = false; - when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true); - when(mockCommandLine.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo"); - when(mockCommandLine.hasOption(GravitinoOptions.TAG)).thenReturn(false); - - GravitinoCommandLine commandLine = + CreateTag spyCreate = spy( - new GravitinoCommandLine( - mockCommandLine, mockOptions, CommandEntities.TAG, CommandActions.CREATE)); + new CreateTag( + GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", null, "comment")); - assertThrows(RuntimeException.class, commandLine::handleCommandLine); - verify(commandLine, never()) - .newCreateTags( - eq(GravitinoCommandLine.DEFAULT_URL), - eq(false), - eq("metalake_demo"), - isNull(), - isNull()); + assertThrows(RuntimeException.class, spyCreate::validate); + verify(spyCreate, never()).handle(); String output = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim(); - assertEquals(output, ErrorMessages.MISSING_TAG); + assertEquals(ErrorMessages.MISSING_TAG, output); } @Test @@ -202,11 +194,16 @@ void testCreateTagsCommand() { doReturn(mockCreate) .when(commandLine) .newCreateTags( - GravitinoCommandLine.DEFAULT_URL, - false, - "metalake_demo", - new String[] {"tagA", "tagB"}, - "comment"); + eq(GravitinoCommandLine.DEFAULT_URL), + eq(false), + eq("metalake_demo"), + argThat( + argument -> + argument.length == 2 + && argument[0].equals("tagA") + && argument[1].equals("tagB")), + eq("comment")); + doReturn(mockCreate).when(mockCreate).validate(); commandLine.handleCommandLine(); verify(mockCreate).handle(); } @@ -226,6 +223,7 @@ void testCreateTagCommandNoComment() { .when(commandLine) .newCreateTags( GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", new String[] {"tagA"}, null); + doReturn(mockCreate).when(mockCreate).validate(); commandLine.handleCommandLine(); verify(mockCreate).handle(); } @@ -245,6 +243,7 @@ void testDeleteTagCommand() { .when(commandLine) .newDeleteTag( GravitinoCommandLine.DEFAULT_URL, false, false, "metalake_demo", new String[] {"tagA"}); + doReturn(mockDelete).when(mockDelete).validate(); commandLine.handleCommandLine(); verify(mockDelete).handle(); } @@ -269,6 +268,7 @@ void testDeleteTagsCommand() { false, "metalake_demo", new String[] {"tagA", "tagB"}); + doReturn(mockDelete).when(mockDelete).validate(); commandLine.handleCommandLine(); verify(mockDelete).handle(); } @@ -289,6 +289,7 @@ void testDeleteTagForceCommand() { .when(commandLine) .newDeleteTag( GravitinoCommandLine.DEFAULT_URL, false, true, "metalake_demo", new String[] {"tagA"}); + doReturn(mockDelete).when(mockDelete).validate(); commandLine.handleCommandLine(); verify(mockDelete).handle(); } @@ -312,64 +313,53 @@ void testSetTagPropertyCommand() { .when(commandLine) .newSetTagProperty( GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "tagA", "property", "value"); + doReturn(mockSetProperty).when(mockSetProperty).validate(); commandLine.handleCommandLine(); verify(mockSetProperty).handle(); } @Test - void testSetTagPropertyCommandWithoutPropertyOption() { + void testSetTagPropertyCommandWithoutPropertyAndValue() { Main.useExit = false; - when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true); - when(mockCommandLine.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo"); - when(mockCommandLine.hasOption(GravitinoOptions.TAG)).thenReturn(true); - when(mockCommandLine.getOptionValues(GravitinoOptions.TAG)).thenReturn(new String[] {"tagA"}); - when(mockCommandLine.hasOption(GravitinoOptions.PROPERTY)).thenReturn(false); - when(mockCommandLine.hasOption(GravitinoOptions.VALUE)).thenReturn(true); - when(mockCommandLine.getOptionValue(GravitinoOptions.VALUE)).thenReturn("value"); - GravitinoCommandLine commandLine = + SetTagProperty spySetProperty = spy( - new GravitinoCommandLine( - mockCommandLine, mockOptions, CommandEntities.TAG, CommandActions.SET)); + new SetTagProperty( + GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "tagA", null, null)); + assertThrows(RuntimeException.class, spySetProperty::validate); + verify(spySetProperty, never()).handle(); + String output = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim(); + assertEquals(output, ErrorMessages.MISSING_PROPERTY_AND_VALUE); + } - assertThrows(RuntimeException.class, commandLine::handleCommandLine); - verify(commandLine, never()) - .newSetTagProperty( - eq(GravitinoCommandLine.DEFAULT_URL), - eq(false), - eq("metalake_demo"), - eq("tagA"), - isNull(), - eq("value")); + @Test + void testSetTagPropertyCommandWithoutPropertyOption() { + Main.useExit = false; + SetTagProperty spySetProperty = + spy( + new SetTagProperty( + GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "tagA", null, "value")); + assertThrows(RuntimeException.class, spySetProperty::validate); + verify(spySetProperty, never()).handle(); String output = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim(); - assertEquals(output, ErrorMessages.INVALID_SET_COMMAND); + assertEquals(output, ErrorMessages.MISSING_PROPERTY); } @Test void testSetTagPropertyCommandWithoutValueOption() { Main.useExit = false; - when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true); - when(mockCommandLine.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo"); - when(mockCommandLine.hasOption(GravitinoOptions.TAG)).thenReturn(true); - when(mockCommandLine.getOptionValues(GravitinoOptions.TAG)).thenReturn(new String[] {"tagA"}); - when(mockCommandLine.hasOption(GravitinoOptions.PROPERTY)).thenReturn(true); - when(mockCommandLine.getOptionValue(GravitinoOptions.PROPERTY)).thenReturn("property"); - when(mockCommandLine.hasOption(GravitinoOptions.VALUE)).thenReturn(false); - GravitinoCommandLine commandLine = + SetTagProperty spySetProperty = spy( - new GravitinoCommandLine( - mockCommandLine, mockOptions, CommandEntities.TAG, CommandActions.SET)); - - assertThrows(RuntimeException.class, commandLine::handleCommandLine); - verify(commandLine, never()) - .newSetTagProperty( - eq(GravitinoCommandLine.DEFAULT_URL), - eq(false), - eq("metalake_demo"), - eq("tagA"), - eq("property"), - isNull()); + new SetTagProperty( + GravitinoCommandLine.DEFAULT_URL, + false, + "metalake_demo", + "tagA", + "property", + null)); + assertThrows(RuntimeException.class, spySetProperty::validate); + verify(spySetProperty, never()).handle(); String output = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim(); - assertEquals(output, ErrorMessages.INVALID_SET_COMMAND); + assertEquals(output, ErrorMessages.MISSING_VALUE); } @Test @@ -418,6 +408,7 @@ void testRemoveTagPropertyCommand() { .when(commandLine) .newRemoveTagProperty( GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "tagA", "property"); + doReturn(mockRemoveProperty).when(mockRemoveProperty).validate(); commandLine.handleCommandLine(); verify(mockRemoveProperty).handle(); } @@ -463,6 +454,7 @@ void testListTagPropertiesCommand() { doReturn(mockListProperties) .when(commandLine) .newListTagProperties(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "tagA"); + doReturn(mockListProperties).when(mockListProperties).validate(); commandLine.handleCommandLine(); verify(mockListProperties).handle(); } @@ -488,6 +480,7 @@ void testDeleteAllTagCommand() { eq("metalake_demo"), any(FullName.class), eq(true)); + doReturn(mockRemoveAllTags).when(mockRemoveAllTags).validate(); commandLine.handleCommandLine(); verify(mockRemoveAllTags).handle(); } @@ -509,6 +502,7 @@ void testUpdateTagCommentCommand() { .when(commandLine) .newUpdateTagComment( GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "tagA", "new comment"); + doReturn(mockUpdateComment).when(mockUpdateComment).validate(); commandLine.handleCommandLine(); verify(mockUpdateComment).handle(); } @@ -556,6 +550,7 @@ void testUpdateTagNameCommand() { doReturn(mockUpdateName) .when(commandLine) .newUpdateTagName(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "tagA", "tagB"); + doReturn(mockUpdateName).when(mockUpdateName).validate(); commandLine.handleCommandLine(); verify(mockUpdateName).handle(); } @@ -602,6 +597,7 @@ void testListEntityTagsCommand() { .when(commandLine) .newListEntityTags( eq(GravitinoCommandLine.DEFAULT_URL), eq(false), eq("metalake_demo"), any()); + doReturn(mockListTags).when(mockListTags).validate(); commandLine.handleCommandLine(); verify(mockListTags).handle(); } @@ -633,6 +629,7 @@ public boolean matches(String[] argument) { return argument != null && argument.length > 0 && "tagA".equals(argument[0]); } })); + doReturn(mockTagEntity).when(mockTagEntity).validate(); commandLine.handleCommandLine(); verify(mockTagEntity).handle(); } @@ -640,27 +637,19 @@ public boolean matches(String[] argument) { @Test void testTagEntityCommandWithoutName() { Main.useExit = false; - when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true); - when(mockCommandLine.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo"); - when(mockCommandLine.hasOption(GravitinoOptions.NAME)).thenReturn(false); - when(mockCommandLine.hasOption(GravitinoOptions.TAG)).thenReturn(true); - when(mockCommandLine.getOptionValues(GravitinoOptions.TAG)).thenReturn(new String[] {"tagA"}); - GravitinoCommandLine commandLine = + TagEntity spyTagEntity = spy( - new GravitinoCommandLine( - mockCommandLine, mockOptions, CommandEntities.TAG, CommandActions.SET)); - - assertThrows(RuntimeException.class, commandLine::handleCommandLine); - verify(commandLine, never()) - .newTagEntity( - eq(GravitinoCommandLine.DEFAULT_URL), - eq(false), - eq("metalake_demo"), - isNull(), - argThat( - argument -> argument != null && argument.length > 0 && "tagA".equals(argument[0]))); + new TagEntity( + GravitinoCommandLine.DEFAULT_URL, + false, + "metalake_demo", + null, + new String[] {"tagA"})); + + assertThrows(RuntimeException.class, spyTagEntity::validate); + verify(spyTagEntity, never()).handle(); String output = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim(); - assertEquals(output, ErrorMessages.MISSING_NAME); + assertEquals(ErrorMessages.MISSING_NAME, output); } @Test @@ -694,6 +683,7 @@ public boolean matches(String[] argument) { && "tagB".equals(argument[1]); } })); + doReturn(mockTagEntity).when(mockTagEntity).validate(); commandLine.handleCommandLine(); verify(mockTagEntity).handle(); } @@ -728,6 +718,7 @@ public boolean matches(String[] argument) { return argument != null && argument.length > 0 && "tagA".equals(argument[0]); } })); + doReturn(mockUntagEntity).when(mockUntagEntity).validate(); commandLine.handleCommandLine(); verify(mockUntagEntity).handle(); } @@ -735,32 +726,19 @@ public boolean matches(String[] argument) { @Test void testUntagEntityCommandWithoutName() { Main.useExit = false; - when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true); - when(mockCommandLine.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo"); - when(mockCommandLine.hasOption(GravitinoOptions.NAME)).thenReturn(false); - when(mockCommandLine.hasOption(GravitinoOptions.TAG)).thenReturn(true); - when(mockCommandLine.getOptionValues(GravitinoOptions.TAG)) - .thenReturn(new String[] {"tagA", "tagB"}); - GravitinoCommandLine commandLine = + UntagEntity spyUntagEntity = spy( - new GravitinoCommandLine( - mockCommandLine, mockOptions, CommandEntities.TAG, CommandActions.REMOVE)); - - assertThrows(RuntimeException.class, commandLine::handleCommandLine); - verify(commandLine, never()) - .newUntagEntity( - eq(GravitinoCommandLine.DEFAULT_URL), - eq(false), - eq("metalake_demo"), - isNull(), - argThat( - argument -> - argument != null - && argument.length > 0 - && "tagA".equals(argument[0]) - && "tagB".equals(argument[1]))); + new UntagEntity( + GravitinoCommandLine.DEFAULT_URL, + false, + "metalake_demo", + null, + new String[] {"tagA"})); + + assertThrows(RuntimeException.class, spyUntagEntity::validate); + verify(spyUntagEntity, never()).handle(); String output = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim(); - assertEquals(output, ErrorMessages.MISSING_NAME); + assertEquals(ErrorMessages.MISSING_NAME, output); } @Test @@ -796,6 +774,7 @@ public boolean matches(String[] argument) { && "tagB".equals(argument[1]); } })); + doReturn(mockUntagEntity).when(mockUntagEntity).validate(); commandLine.handleCommandLine(); verify(mockUntagEntity).handle(); } @@ -803,18 +782,59 @@ public boolean matches(String[] argument) { @Test void testDeleteTagCommandWithoutTagOption() { Main.useExit = false; + DeleteTag spyDeleteTag = + spy(new DeleteTag(GravitinoCommandLine.DEFAULT_URL, false, false, "metalake", null)); + + assertThrows(RuntimeException.class, spyDeleteTag::validate); + verify(spyDeleteTag, never()).handle(); + String output = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim(); + assertEquals(ErrorMessages.MISSING_TAG, output); + } + + @Test + void testRemoveAllTagsCommand() { + Main.useExit = false; + RemoveAllTags mockRemoveAllTags = mock(RemoveAllTags.class); when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true); when(mockCommandLine.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo"); when(mockCommandLine.hasOption(GravitinoOptions.TAG)).thenReturn(false); + when(mockCommandLine.hasOption(GravitinoOptions.NAME)).thenReturn(true); + when(mockCommandLine.getOptionValue(GravitinoOptions.NAME)).thenReturn("catalog.schema.table"); + when(mockCommandLine.hasOption(GravitinoOptions.FORCE)).thenReturn(true); GravitinoCommandLine commandLine = spy( new GravitinoCommandLine( mockCommandLine, mockOptions, CommandEntities.TAG, CommandActions.REMOVE)); - assertThrows(RuntimeException.class, commandLine::handleCommandLine); - verify(commandLine, never()) - .newDeleteTag(GravitinoCommandLine.DEFAULT_URL, false, false, "metalake", null); + doReturn(mockRemoveAllTags) + .when(commandLine) + .newRemoveAllTags( + eq(GravitinoCommandLine.DEFAULT_URL), + eq(false), + eq("metalake_demo"), + argThat( + argument -> + argument != null + && "catalog".equals(argument.getCatalogName()) + && "schema".equals(argument.getSchemaName()) + && "table".equals(argument.getTableName())), + eq(true)); + doReturn(mockRemoveAllTags).when(mockRemoveAllTags).validate(); + commandLine.handleCommandLine(); + verify(mockRemoveAllTags).handle(); + } + + @Test + void testRemoveAllTagsCommandWithoutName() { + Main.useExit = false; + RemoveAllTags spyRemoveAllTags = + spy( + new RemoveAllTags( + GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", null, false)); + + assertThrows(RuntimeException.class, spyRemoveAllTags::validate); + verify(spyRemoveAllTags, never()).handle(); String output = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim(); - assertEquals(output, ErrorMessages.MISSING_TAG); + assertEquals(ErrorMessages.MISSING_NAME, output); } }