From ba071f084b71c20d837b0b2972b412d62cfddad7 Mon Sep 17 00:00:00 2001 From: VigneshSK17 Date: Wed, 8 Jan 2025 15:45:47 -0500 Subject: [PATCH 1/3] Updated Enable methods to check for enable and disable, no tests --- .../apache/gravitino/cli/TestableCommandLine.java | 8 ++++---- .../gravitino/cli/commands/CatalogEnable.java | 13 ++++++++++++- .../gravitino/cli/commands/MetalakeEnable.java | 14 +++++++++++++- 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/TestableCommandLine.java b/clients/cli/src/main/java/org/apache/gravitino/cli/TestableCommandLine.java index d9e3b9288ee..f56c955b59b 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/TestableCommandLine.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/TestableCommandLine.java @@ -896,8 +896,8 @@ protected RevokePrivilegesFromRole newRevokePrivilegesFromRole( } protected MetalakeEnable newMetalakeEnable( - String url, boolean ignore, String metalake, boolean enableAllCatalogs) { - return new MetalakeEnable(url, ignore, metalake, enableAllCatalogs); + String url, boolean ignore, String metalake, boolean disable, boolean enableAllCatalogs) { + return new MetalakeEnable(url, ignore, metalake, disable, enableAllCatalogs); } protected MetalakeDisable newMetalakeDisable(String url, boolean ignore, String metalake) { @@ -905,8 +905,8 @@ protected MetalakeDisable newMetalakeDisable(String url, boolean ignore, String } protected CatalogEnable newCatalogEnable( - String url, boolean ignore, String metalake, String catalog, boolean enableMetalake) { - return new CatalogEnable(url, ignore, metalake, catalog, enableMetalake); + String url, boolean ignore, String metalake, String catalog, boolean disable, boolean enableMetalake) { + return new CatalogEnable(url, ignore, metalake, catalog, disable, enableMetalake); } protected CatalogDisable newCatalogDisable( diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/CatalogEnable.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/CatalogEnable.java index 8646baee292..f148b5d138e 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/CatalogEnable.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/CatalogEnable.java @@ -29,6 +29,7 @@ public class CatalogEnable extends Command { private final String metalake; private final String catalog; + private final boolean disable; private final boolean enableMetalake; /** @@ -38,13 +39,15 @@ public class CatalogEnable extends Command { * @param ignoreVersions If true don't check the client/server versions match. * @param metalake The name of the metalake. * @param catalog The name of the catalog. + * @param disable Whether the disable flag is also turned on * @param enableMetalake Whether to enable it's metalake */ public CatalogEnable( - String url, boolean ignoreVersions, String metalake, String catalog, boolean enableMetalake) { + String url, boolean ignoreVersions, String metalake, String catalog, boolean disable, boolean enableMetalake) { super(url, ignoreVersions); this.metalake = metalake; this.catalog = catalog; + this.disable = disable; this.enableMetalake = enableMetalake; } @@ -71,4 +74,12 @@ public void handle() { System.out.println(metalake + "." + catalog + " has been enabled."); } + + @Override + public Command validate() { + if (disable) { + exitWithError(ErrorMessages.INVALID_ENABLE_DISABLE); + } + return super.validate(); + } } diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/MetalakeEnable.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/MetalakeEnable.java index 34ba23a61bb..2cd32c2b65e 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/MetalakeEnable.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/MetalakeEnable.java @@ -29,6 +29,7 @@ public class MetalakeEnable extends Command { private final String metalake; + protected final Boolean disable; private Boolean enableAllCatalogs; /** @@ -36,13 +37,16 @@ public class MetalakeEnable extends Command { * * @param url The URL of the Gravitino server. * @param ignoreVersions If true don't check the client/server versions match. + * @param disable Whether the disable flag is also turned on * @param metalake The name of the metalake. + * * @param enableAllCatalogs Whether to enable all catalogs. */ public MetalakeEnable( - String url, boolean ignoreVersions, String metalake, boolean enableAllCatalogs) { + String url, boolean ignoreVersions, String metalake, boolean disable, boolean enableAllCatalogs) { super(url, ignoreVersions); this.metalake = metalake; + this.disable = disable; this.enableAllCatalogs = enableAllCatalogs; } @@ -69,4 +73,12 @@ public void handle() { System.out.println(msgBuilder); } + + @Override + public Command validate() { + if (disable) { + exitWithError(ErrorMessages.INVALID_ENABLE_DISABLE); + } + return super.validate(); + } } From c4ded5f1e3c4d6f958c92927e40874b72a6fed1a Mon Sep 17 00:00:00 2001 From: VigneshSK17 Date: Thu, 9 Jan 2025 00:33:34 -0500 Subject: [PATCH 2/3] Added working tests --- .../gravitino/cli/TestableCommandLine.java | 7 ++++- .../gravitino/cli/commands/CatalogEnable.java | 7 ++++- .../cli/commands/MetalakeEnable.java | 7 +++-- .../gravitino/cli/TestCatalogCommands.java | 29 ++++++------------- .../gravitino/cli/TestMetalakeCommands.java | 28 ++++++------------ 5 files changed, 35 insertions(+), 43 deletions(-) diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/TestableCommandLine.java b/clients/cli/src/main/java/org/apache/gravitino/cli/TestableCommandLine.java index f56c955b59b..12f929586d1 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/TestableCommandLine.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/TestableCommandLine.java @@ -905,7 +905,12 @@ protected MetalakeDisable newMetalakeDisable(String url, boolean ignore, String } protected CatalogEnable newCatalogEnable( - String url, boolean ignore, String metalake, String catalog, boolean disable, boolean enableMetalake) { + String url, + boolean ignore, + String metalake, + String catalog, + boolean disable, + boolean enableMetalake) { return new CatalogEnable(url, ignore, metalake, catalog, disable, enableMetalake); } diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/CatalogEnable.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/CatalogEnable.java index f148b5d138e..ebf4dbe40ed 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/CatalogEnable.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/CatalogEnable.java @@ -43,7 +43,12 @@ public class CatalogEnable extends Command { * @param enableMetalake Whether to enable it's metalake */ public CatalogEnable( - String url, boolean ignoreVersions, String metalake, String catalog, boolean disable, boolean enableMetalake) { + String url, + boolean ignoreVersions, + String metalake, + String catalog, + boolean disable, + boolean enableMetalake) { super(url, ignoreVersions); this.metalake = metalake; this.catalog = catalog; diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/MetalakeEnable.java b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/MetalakeEnable.java index 2cd32c2b65e..4877322f9cd 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/commands/MetalakeEnable.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/commands/MetalakeEnable.java @@ -39,11 +39,14 @@ public class MetalakeEnable extends Command { * @param ignoreVersions If true don't check the client/server versions match. * @param disable Whether the disable flag is also turned on * @param metalake The name of the metalake. - * * @param enableAllCatalogs Whether to enable all catalogs. */ public MetalakeEnable( - String url, boolean ignoreVersions, String metalake, boolean disable, boolean enableAllCatalogs) { + String url, + boolean ignoreVersions, + String metalake, + boolean disable, + boolean enableAllCatalogs) { super(url, ignoreVersions); this.metalake = metalake; this.disable = disable; 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 afa19b94c5a..040f5835eac 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 @@ -21,7 +21,6 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThrows; -import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -49,6 +48,7 @@ import org.apache.gravitino.cli.commands.UpdateCatalogComment; import org.apache.gravitino.cli.commands.UpdateCatalogName; import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -459,7 +459,7 @@ void testEnableCatalogCommand() { doReturn(mockEnable) .when(commandLine) .newCatalogEnable( - GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "catalog", false); + GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "catalog", false, false); doReturn(mockEnable).when(mockEnable).validate(); commandLine.handleCommandLine(); verify(mockEnable).handle(); @@ -482,7 +482,7 @@ void testEnableCatalogCommandWithRecursive() { doReturn(mockEnable) .when(commandLine) .newCatalogEnable( - GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "catalog", true); + GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "catalog", false, true); doReturn(mockEnable).when(mockEnable).validate(); commandLine.handleCommandLine(); verify(mockEnable).handle(); @@ -513,24 +513,13 @@ void testDisableCatalogCommand() { @SuppressWarnings("DefaultCharset") void testCatalogWithDisableAndEnableOptions() { 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"); - when(mockCommandLine.hasOption(GravitinoOptions.DISABLE)).thenReturn(true); - when(mockCommandLine.hasOption(GravitinoOptions.ENABLE)).thenReturn(true); - - GravitinoCommandLine commandLine = + CatalogEnable mockCatalogEnable = spy( - new GravitinoCommandLine( - mockCommandLine, mockOptions, CommandEntities.CATALOG, CommandActions.UPDATE)); + new CatalogEnable( + GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "catalog", true, false)); - assertThrows(RuntimeException.class, commandLine::handleCommandLine); - verify(commandLine, never()) - .newCatalogEnable( - GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "catalog", false); - verify(commandLine, never()) - .newCatalogDisable(GravitinoCommandLine.DEFAULT_URL, false, "melake_demo", "catalog"); - assertTrue(errContent.toString().contains(ErrorMessages.INVALID_ENABLE_DISABLE)); + assertThrows(RuntimeException.class, mockCatalogEnable::validate); + String errOutput = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim(); + Assertions.assertEquals(ErrorMessages.INVALID_ENABLE_DISABLE, errOutput); } } diff --git a/clients/cli/src/test/java/org/apache/gravitino/cli/TestMetalakeCommands.java b/clients/cli/src/test/java/org/apache/gravitino/cli/TestMetalakeCommands.java index dae2fe63400..88baa0c43a2 100644 --- a/clients/cli/src/test/java/org/apache/gravitino/cli/TestMetalakeCommands.java +++ b/clients/cli/src/test/java/org/apache/gravitino/cli/TestMetalakeCommands.java @@ -21,10 +21,8 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertThrows; -import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -46,8 +44,8 @@ import org.apache.gravitino.cli.commands.SetMetalakeProperty; import org.apache.gravitino.cli.commands.UpdateMetalakeComment; import org.apache.gravitino.cli.commands.UpdateMetalakeName; -import org.junit.Assert; import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -386,7 +384,7 @@ void testEnableMetalakeCommand() { mockCommandLine, mockOptions, CommandEntities.METALAKE, CommandActions.UPDATE)); doReturn(mockEnable) .when(commandLine) - .newMetalakeEnable(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", false); + .newMetalakeEnable(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", false, false); doReturn(mockEnable).when(mockEnable).validate(); commandLine.handleCommandLine(); verify(mockEnable).handle(); @@ -405,7 +403,7 @@ void testEnableMetalakeCommandWithRecursive() { mockCommandLine, mockOptions, CommandEntities.METALAKE, CommandActions.UPDATE)); doReturn(mockEnable) .when(commandLine) - .newMetalakeEnable(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", true); + .newMetalakeEnable(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", false, true); doReturn(mockEnable).when(mockEnable).validate(); commandLine.handleCommandLine(); verify(mockEnable).handle(); @@ -434,21 +432,13 @@ void testDisableMetalakeCommand() { @SuppressWarnings("DefaultCharset") void testMetalakeWithDisableAndEnableOptions() { Main.useExit = false; - when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true); - when(mockCommandLine.getOptionValue(CommandEntities.METALAKE)).thenReturn("metalake_demo"); - when(mockCommandLine.hasOption(GravitinoOptions.ENABLE)).thenReturn(true); - when(mockCommandLine.hasOption(GravitinoOptions.DISABLE)).thenReturn(true); - - GravitinoCommandLine commandLine = + MetalakeEnable mockMetalakeEnable = spy( - new GravitinoCommandLine( - mockCommandLine, mockOptions, CommandEntities.METALAKE, CommandActions.UPDATE)); + new MetalakeEnable( + GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", true, false)); - Assert.assertThrows(RuntimeException.class, commandLine::handleCommandLine); - verify(commandLine, never()) - .newMetalakeEnable(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", false); - verify(commandLine, never()) - .newMetalakeEnable(GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", false); - assertTrue(errContent.toString().contains(ErrorMessages.INVALID_ENABLE_DISABLE)); + assertThrows(RuntimeException.class, mockMetalakeEnable::validate); + String errOutput = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim(); + Assertions.assertEquals(ErrorMessages.INVALID_ENABLE_DISABLE, errOutput); } } From 0c55e2052ac1118455dcd93fda2ce36683c0e5b6 Mon Sep 17 00:00:00 2001 From: VigneshSK17 Date: Mon, 13 Jan 2025 22:19:47 -0500 Subject: [PATCH 3/3] Fully updated to refactored Command handler --- .../gravitino/cli/CatalogCommandHandler.java | 7 ++--- .../gravitino/cli/MetalakeCommandHandler.java | 7 ++--- .../gravitino/cli/TestCatalogCommands.java | 26 ++++++++++++++++--- .../gravitino/cli/TestMetalakeCommands.java | 19 +++++++++++--- 4 files changed, 41 insertions(+), 18 deletions(-) diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/CatalogCommandHandler.java b/clients/cli/src/main/java/org/apache/gravitino/cli/CatalogCommandHandler.java index 8e238406854..75c0f03ccb3 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/CatalogCommandHandler.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/CatalogCommandHandler.java @@ -186,14 +186,11 @@ private void handlePropertiesCommand() { /** Handles the "UPDATE" command. */ private void handleUpdateCommand() { - if (line.hasOption(GravitinoOptions.ENABLE) && line.hasOption(GravitinoOptions.DISABLE)) { - System.err.println(ErrorMessages.INVALID_ENABLE_DISABLE); - Main.exit(-1); - } if (line.hasOption(GravitinoOptions.ENABLE)) { boolean enableMetalake = line.hasOption(GravitinoOptions.ALL); + boolean disable = line.hasOption(GravitinoOptions.DISABLE); gravitinoCommandLine - .newCatalogEnable(url, ignore, metalake, catalog, enableMetalake) + .newCatalogEnable(url, ignore, metalake, catalog, disable, enableMetalake) .validate() .handle(); } diff --git a/clients/cli/src/main/java/org/apache/gravitino/cli/MetalakeCommandHandler.java b/clients/cli/src/main/java/org/apache/gravitino/cli/MetalakeCommandHandler.java index 993116f19f5..c7af1e6ab95 100644 --- a/clients/cli/src/main/java/org/apache/gravitino/cli/MetalakeCommandHandler.java +++ b/clients/cli/src/main/java/org/apache/gravitino/cli/MetalakeCommandHandler.java @@ -167,14 +167,11 @@ private void handlePropertiesCommand() { /** Handles the "UPDATE" command. */ private void handleUpdateCommand() { - if (line.hasOption(GravitinoOptions.ENABLE) && line.hasOption(GravitinoOptions.DISABLE)) { - System.err.println(ErrorMessages.INVALID_ENABLE_DISABLE); - Main.exit(-1); - } if (line.hasOption(GravitinoOptions.ENABLE)) { boolean enableAllCatalogs = line.hasOption(GravitinoOptions.ALL); + boolean disable = line.hasOption(GravitinoOptions.DISABLE); gravitinoCommandLine - .newMetalakeEnable(url, ignore, metalake, enableAllCatalogs) + .newMetalakeEnable(url, ignore, metalake, disable, enableAllCatalogs) .validate() .handle(); } 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 040f5835eac..b2e745a7967 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 @@ -513,12 +513,30 @@ void testDisableCatalogCommand() { @SuppressWarnings("DefaultCharset") void testCatalogWithDisableAndEnableOptions() { Main.useExit = false; - CatalogEnable mockCatalogEnable = + 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"); + when(mockCommandLine.hasOption(GravitinoOptions.DISABLE)).thenReturn(true); + when(mockCommandLine.hasOption(GravitinoOptions.ENABLE)).thenReturn(true); + + GravitinoCommandLine commandLine = spy( - new CatalogEnable( - GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", "catalog", true, false)); + new GravitinoCommandLine( + mockCommandLine, mockOptions, CommandEntities.CATALOG, CommandActions.UPDATE)); - assertThrows(RuntimeException.class, mockCatalogEnable::validate); + assertThrows( + RuntimeException.class, + () -> + commandLine + .newCatalogEnable( + GravitinoCommandLine.DEFAULT_URL, + false, + "metalake_demo", + "catalog", + true, + false) + .validate()); String errOutput = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim(); Assertions.assertEquals(ErrorMessages.INVALID_ENABLE_DISABLE, errOutput); } diff --git a/clients/cli/src/test/java/org/apache/gravitino/cli/TestMetalakeCommands.java b/clients/cli/src/test/java/org/apache/gravitino/cli/TestMetalakeCommands.java index 88baa0c43a2..881ffb82db6 100644 --- a/clients/cli/src/test/java/org/apache/gravitino/cli/TestMetalakeCommands.java +++ b/clients/cli/src/test/java/org/apache/gravitino/cli/TestMetalakeCommands.java @@ -432,12 +432,23 @@ void testDisableMetalakeCommand() { @SuppressWarnings("DefaultCharset") void testMetalakeWithDisableAndEnableOptions() { Main.useExit = false; - MetalakeEnable mockMetalakeEnable = + when(mockCommandLine.hasOption(GravitinoOptions.METALAKE)).thenReturn(true); + when(mockCommandLine.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo"); + when(mockCommandLine.hasOption(GravitinoOptions.DISABLE)).thenReturn(true); + when(mockCommandLine.hasOption(GravitinoOptions.ENABLE)).thenReturn(true); + + GravitinoCommandLine commandLine = spy( - new MetalakeEnable( - GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", true, false)); + new GravitinoCommandLine( + mockCommandLine, mockOptions, CommandEntities.METALAKE, CommandActions.UPDATE)); - assertThrows(RuntimeException.class, mockMetalakeEnable::validate); + assertThrows( + RuntimeException.class, + () -> + commandLine + .newMetalakeEnable( + GravitinoCommandLine.DEFAULT_URL, false, "metalake_demo", true, false) + .validate()); String errOutput = new String(errContent.toByteArray(), StandardCharsets.UTF_8).trim(); Assertions.assertEquals(ErrorMessages.INVALID_ENABLE_DISABLE, errOutput); }