Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#6136] refactor: move check for enable and disable within validate (CLI) #6163

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[#6136] refactor: move check for enable and disable within validate (CLI) #6163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ba071f0
Updated Enable methods to check for enable and disable, no tests
VigneshSK17 Jan 8, 2025
c4ded5f
Added working tests
VigneshSK17 Jan 9, 2025
0c55e20
Fully updated to refactored Command handler
VigneshSK17 Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions clients/cli/src/main/java/org/apache/gravitino/cli/CatalogCommandHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
7 changes: 2 additions & 5 deletions clients/cli/src/main/java/org/apache/gravitino/cli/MetalakeCommandHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
13 changes: 9 additions & 4 deletions clients/cli/src/main/java/org/apache/gravitino/cli/TestableCommandLine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -896,17 +896,22 @@ 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) {
return new MetalakeDisable(url, ignore, metalake);
}

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(
Expand Down
18 changes: 17 additions & 1 deletion clients/cli/src/main/java/org/apache/gravitino/cli/commands/CatalogEnable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand All @@ -38,13 +39,20 @@ 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;
}

Expand All @@ -71,4 +79,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();
}
}
17 changes: 16 additions & 1 deletion clients/cli/src/main/java/org/apache/gravitino/cli/commands/MetalakeEnable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,20 +29,27 @@
public class MetalakeEnable extends Command {

private final String metalake;
protected final Boolean disable;
private Boolean enableAllCatalogs;

/**
* Enable a metalake
*
* @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;
}

Expand All @@ -69,4 +76,12 @@ public void handle() {

System.out.println(msgBuilder);
}

@Override
public Command validate() {
if (disable) {
exitWithError(ErrorMessages.INVALID_ENABLE_DISABLE);
}
return super.validate();
}
}
27 changes: 17 additions & 10 deletions clients/cli/src/test/java/org/apache/gravitino/cli/TestCatalogCommands.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -525,12 +525,19 @@ void testCatalogWithDisableAndEnableOptions() {
new GravitinoCommandLine(
mockCommandLine, mockOptions, CommandEntities.CATALOG, CommandActions.UPDATE));

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,
() ->
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why you have changed this test in this way. Can you explain the changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that I should detect if there's a RuntimeException when trying to enable and disable a catalog at the same time and then check if the error message is INVALID_ENABLE_DISABLE.

Is there an alternative to simplify this test or is am I misunderstanding the intent of these tests?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So why change existing working tests that are needed? Did you mean to add a new test instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see what you mean.

The reason why is because to check if both flags are true, I pass the disable flag's result into EnableCatalogCommand as a required parameter.

Is this the proper standard or not? Apologies for dragging this on for so long.

}
}
27 changes: 14 additions & 13 deletions clients/cli/src/test/java/org/apache/gravitino/cli/TestMetalakeCommands.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -435,20 +433,23 @@ void testDisableMetalakeCommand() {
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.getOptionValue(GravitinoOptions.METALAKE)).thenReturn("metalake_demo");
when(mockCommandLine.hasOption(GravitinoOptions.DISABLE)).thenReturn(true);
when(mockCommandLine.hasOption(GravitinoOptions.ENABLE)).thenReturn(true);

GravitinoCommandLine commandLine =
spy(
new GravitinoCommandLine(
mockCommandLine, mockOptions, CommandEntities.METALAKE, CommandActions.UPDATE));

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,
() ->
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, I'm not sure these changes are required.

}
}
Loading