From e230a445bb457bbac27bf4c384da449ccbd3f043 Mon Sep 17 00:00:00 2001 From: mark-terry <36909937+mark-terry@users.noreply.github.com> Date: Thu, 25 Mar 2021 14:36:04 +1000 Subject: [PATCH] Updated CLI option validation to handle dependent options with multiple main options. (#2054) Signed-off-by: Mark Terry --- .../org/hyperledger/besu/cli/BesuCommand.java | 21 +++-- .../besu/cli/util/CommandLineUtils.java | 61 ++++++++++++--- .../hyperledger/besu/cli/BesuCommandTest.java | 52 +++++++++++-- .../besu/cli/CommandLineUtilsTest.java | 77 ++++++++++++++++++- 4 files changed, 184 insertions(+), 27 deletions(-) diff --git a/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java b/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java index 1d6f9abd14..61cb2a9369 100644 --- a/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java +++ b/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java @@ -1503,13 +1503,19 @@ public class BesuCommand implements DefaultCommandValues, Runnable { !isMiningEnabled, asList( "--miner-coinbase", - "--min-gas-price", "--min-block-occupancy-ratio", "--miner-extra-data", "--miner-stratum-enabled", "--Xminer-remote-sealers-limit", "--Xminer-remote-sealers-hashrate-ttl")); + CommandLineUtils.checkMultiOptionDependencies( + logger, + commandLine, + List.of("--miner-enabled", "--goquorum-compatibility-enabled"), + List.of(!isMiningEnabled, !isGoQuorumCompatibilityMode), + singletonList("--min-gas-price")); + CommandLineUtils.checkOptionDependencies( logger, commandLine, @@ -2048,11 +2054,14 @@ public class BesuCommand implements DefaultCommandValues, Runnable { commandLine, "--privacy-enabled", !isPrivacyEnabled, - asList( - "--privacy-url", - "--privacy-public-key-file", - "--privacy-multi-tenancy-enabled", - "--privacy-tls-enabled")); + asList("--privacy-multi-tenancy-enabled", "--privacy-tls-enabled")); + + CommandLineUtils.checkMultiOptionDependencies( + logger, + commandLine, + List.of("--privacy-enabled", "--goquorum-compatibility-enabled"), + List.of(!isPrivacyEnabled, !isGoQuorumCompatibilityMode), + List.of("--privacy-url", "--privacy-public-key-file")); checkPrivacyTlsOptionsDependencies(); diff --git a/besu/src/main/java/org/hyperledger/besu/cli/util/CommandLineUtils.java b/besu/src/main/java/org/hyperledger/besu/cli/util/CommandLineUtils.java index 9526842149..b2a63fb62d 100644 --- a/besu/src/main/java/org/hyperledger/besu/cli/util/CommandLineUtils.java +++ b/besu/src/main/java/org/hyperledger/besu/cli/util/CommandLineUtils.java @@ -26,6 +26,8 @@ import picocli.CommandLine; public class CommandLineUtils { public static final String DEPENDENCY_WARNING_MSG = "{} has been ignored because {} was not defined on the command line."; + public static final String MULTI_DEPENDENCY_WARNING_MSG = + "{} ignored because none of {} was defined."; /** * Check if options are passed that require an option to be true to have any effect and log a @@ -51,20 +53,59 @@ public class CommandLineUtils { final boolean isMainOptionCondition, final List dependentOptionsNames) { if (isMainOptionCondition) { - final String affectedOptions = - commandLine.getCommandSpec().options().stream() - .filter( - option -> - Arrays.stream(option.names()).anyMatch(dependentOptionsNames::contains) - && !option.stringValues().isEmpty()) - .map(option -> option.names()[0]) - .collect( - Collectors.collectingAndThen( - Collectors.toList(), StringUtils.joiningWithLastDelimiter(", ", " and "))); + final String affectedOptions = getAffectedOptions(commandLine, dependentOptionsNames); if (!affectedOptions.isEmpty()) { logger.warn(DEPENDENCY_WARNING_MSG, affectedOptions, mainOptionName); } } } + + /** + * Check if options are passed that require an option to be true to have any effect and log a + * warning with the list of affected options. Multiple main options may be passed to check + * dependencies against. + * + *

Note that in future version of PicoCLI some options dependency mechanism may be implemented + * that could replace this. See https://github.com/remkop/picocli/issues/295 + * + * @param logger the logger instance used to log the warning + * @param commandLine the command line containing the options we want to check + * @param mainOptions the names of the main options to test dependency against. Only used for + * display. + * @param isMainOptionCondition the conditions to test dependent options against. If all + * conditions are true, dependent options will be checked. + * @param dependentOptionsNames a list of option names that can't be used if condition is met. + * Example: if --min-gas-price is in the list and condition is that either --miner-enabled or + * --goquorum-compatibility-enabled should not be false, we log a warning. + */ + public static void checkMultiOptionDependencies( + final Logger logger, + final CommandLine commandLine, + final List mainOptions, + final List isMainOptionCondition, + final List dependentOptionsNames) { + if (isMainOptionCondition.stream().allMatch(isTrue -> isTrue)) { + final String affectedOptions = getAffectedOptions(commandLine, dependentOptionsNames); + + if (!affectedOptions.isEmpty()) { + final String joinedMainOptions = + StringUtils.joiningWithLastDelimiter(", ", " or ").apply(mainOptions); + logger.warn(MULTI_DEPENDENCY_WARNING_MSG, affectedOptions, joinedMainOptions); + } + } + } + + private static String getAffectedOptions( + final CommandLine commandLine, final List dependentOptionsNames) { + return commandLine.getCommandSpec().options().stream() + .filter( + option -> + Arrays.stream(option.names()).anyMatch(dependentOptionsNames::contains) + && !option.stringValues().isEmpty()) + .map(option -> option.names()[0]) + .collect( + Collectors.collectingAndThen( + Collectors.toList(), StringUtils.joiningWithLastDelimiter(", ", " and "))); + } } diff --git a/besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java b/besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java index 691ad73d4c..a065c84b28 100644 --- a/besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java +++ b/besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java @@ -26,6 +26,7 @@ import static org.hyperledger.besu.cli.config.NetworkName.MORDOR; import static org.hyperledger.besu.cli.config.NetworkName.RINKEBY; import static org.hyperledger.besu.cli.config.NetworkName.ROPSTEN; import static org.hyperledger.besu.cli.util.CommandLineUtils.DEPENDENCY_WARNING_MSG; +import static org.hyperledger.besu.cli.util.CommandLineUtils.MULTI_DEPENDENCY_WARNING_MSG; import static org.hyperledger.besu.ethereum.api.jsonrpc.RpcApis.ETH; import static org.hyperledger.besu.ethereum.api.jsonrpc.RpcApis.NET; import static org.hyperledger.besu.ethereum.api.jsonrpc.RpcApis.PERM; @@ -73,6 +74,7 @@ import org.hyperledger.besu.ethereum.worldstate.PrunerConfiguration; import org.hyperledger.besu.metrics.StandardMetricCategory; import org.hyperledger.besu.metrics.prometheus.MetricsConfiguration; import org.hyperledger.besu.nat.NatMethod; +import org.hyperledger.besu.util.StringUtils; import org.hyperledger.besu.util.number.Fraction; import org.hyperledger.besu.util.number.Percentage; @@ -2904,11 +2906,7 @@ public class BesuCommandTest extends CommandTestAbstract { "--miner-stratum-enabled"); verifyOptionsConstraintLoggerCall( - "--miner-enabled", - "--miner-coinbase", - "--min-gas-price", - "--miner-extra-data", - "--miner-stratum-enabled"); + "--miner-enabled", "--miner-coinbase", "--miner-extra-data", "--miner-stratum-enabled"); assertThat(commandOutput.toString()).isEmpty(); assertThat(commandErrorOutput.toString()) @@ -2916,6 +2914,17 @@ public class BesuCommandTest extends CommandTestAbstract { "Unable to mine with Stratum if mining is disabled. Either disable Stratum mining (remove --miner-stratum-enabled) or specify mining is enabled (--miner-enabled)"); } + @Test + public void minGasPriceRequiresMainOption() { + parseCommand("--min-gas-price", "0"); + + verifyMultiOptionsConstraintLoggerCall( + List.of("--miner-enabled", "--goquorum-compatibility-enabled"), "--min-gas-price"); + + assertThat(commandOutput.toString()).isEmpty(); + assertThat(commandErrorOutput.toString()).isEmpty(); + } + @Test public void miningParametersAreCaptured() throws Exception { final Address requestedCoinbase = Address.fromHexString("0000011111222223333344444"); @@ -3239,8 +3248,10 @@ public class BesuCommandTest extends CommandTestAbstract { parseCommand("--privacy-url", ENCLAVE_URI, "--privacy-public-key-file", file.toString()); - verifyOptionsConstraintLoggerCall( - "--privacy-enabled", "--privacy-url", "--privacy-public-key-file"); + verifyMultiOptionsConstraintLoggerCall( + List.of("--privacy-enabled", "--goquorum-compatibility-enabled"), + "--privacy-url", + "--privacy-public-key-file"); assertThat(commandOutput.toString()).isEmpty(); assertThat(commandErrorOutput.toString()).isEmpty(); @@ -3448,6 +3459,33 @@ public class BesuCommandTest extends CommandTestAbstract { assertThat(stringArgumentCaptor.getAllValues().get(2)).isEqualTo(mainOption); } + /** + * Check logger calls + * + *

Here we check the calls to logger and not the result of the log line as we don't test the + * logger itself but the fact that we call it. + * + * @param dependentOptions the string representing the list of dependent options names + * @param mainOptions the main option names + */ + private void verifyMultiOptionsConstraintLoggerCall( + final List mainOptions, final String... dependentOptions) { + verify(mockLogger, atLeast(1)) + .warn( + stringArgumentCaptor.capture(), + stringArgumentCaptor.capture(), + stringArgumentCaptor.capture()); + assertThat(stringArgumentCaptor.getAllValues().get(0)).isEqualTo(MULTI_DEPENDENCY_WARNING_MSG); + + for (final String option : dependentOptions) { + assertThat(stringArgumentCaptor.getAllValues().get(1)).contains(option); + } + + final String joinedOptions = + StringUtils.joiningWithLastDelimiter(", ", " or ").apply(mainOptions); + assertThat(stringArgumentCaptor.getAllValues().get(2)).isEqualTo(joinedOptions); + } + @Test public void privacyWithFastSyncMustError() { parseCommand("--sync-mode=FAST", "--privacy-enabled"); diff --git a/besu/src/test/java/org/hyperledger/besu/cli/CommandLineUtilsTest.java b/besu/src/test/java/org/hyperledger/besu/cli/CommandLineUtilsTest.java index eadcbdc884..dd2e5953c2 100644 --- a/besu/src/test/java/org/hyperledger/besu/cli/CommandLineUtilsTest.java +++ b/besu/src/test/java/org/hyperledger/besu/cli/CommandLineUtilsTest.java @@ -16,14 +16,17 @@ package org.hyperledger.besu.cli; import static org.assertj.core.api.Assertions.assertThat; import static org.hyperledger.besu.cli.util.CommandLineUtils.DEPENDENCY_WARNING_MSG; +import static org.hyperledger.besu.cli.util.CommandLineUtils.MULTI_DEPENDENCY_WARNING_MSG; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static picocli.CommandLine.defaultExceptionHandler; import org.hyperledger.besu.cli.util.CommandLineUtils; +import org.hyperledger.besu.util.StringUtils; import java.util.ArrayList; import java.util.Arrays; +import java.util.List; import org.apache.logging.log4j.Logger; import org.junit.Test; @@ -62,6 +65,11 @@ public class CommandLineUtilsTest { arity = "1") final Boolean optionEnabled = true; + @Option( + names = {"--other-option-enabled"}, + arity = "1") + final Boolean otherOptionEnabled = true; + @Option(names = {"--option2"}) final Integer option2 = 2; @@ -104,6 +112,22 @@ public class CommandLineUtilsTest { } } + private static class TestMultiCommandWithDeps extends AbstractTestCommand { + TestMultiCommandWithDeps(final Logger logger) { + super(logger); + } + + @Override + public void run() { + CommandLineUtils.checkMultiOptionDependencies( + logger, + commandLine, + List.of("--option-enabled", "--other-option-enabled"), + List.of(!optionEnabled, !otherOptionEnabled), + Arrays.asList("--option2", "--option3")); + } + } + @Test public void optionsAreNotExpected() { final AbstractTestCommand testCommand = new TestCommandWithDeps(mockLogger); @@ -186,6 +210,26 @@ public class CommandLineUtilsTest { assertThat(testCommand.option4).isEqualTo(40); } + @Test + public void multipleMainOptions() { + final AbstractTestCommand testCommand = new TestMultiCommandWithDeps(mockLogger); + testCommand.commandLine.parseWithHandlers( + new RunLast(), + defaultExceptionHandler(), + "--option-enabled", + "false", + "--other-option-enabled", + "false", + "--option2", + "20"); + verifyMultiOptionsConstraintLoggerCall( + mockLogger, "--option2", "--option-enabled", "--other-option-enabled"); + + assertThat(testCommand.optionEnabled).isFalse(); + assertThat(testCommand.otherOptionEnabled).isFalse(); + assertThat(testCommand.option2).isEqualTo(20); + } + /** * Check logger calls * @@ -193,10 +237,32 @@ public class CommandLineUtilsTest { * logger itself but the fact that we call it. * * @param dependentOptions the string representing the list of dependent options names - * @param mainOption the main option name + * @param mainOptions the main option name */ private void verifyOptionsConstraintLoggerCall( - final Logger logger, final String dependentOptions, final String mainOption) { + final Logger logger, final String dependentOptions, final String... mainOptions) { + verifyCall(logger, dependentOptions, DEPENDENCY_WARNING_MSG, mainOptions); + } + + /** + * Check logger calls, where multiple main options have been specified + * + *

Here we check the calls to logger and not the result of the log line as we don't test the + * logger itself but the fact that we call it. + * + * @param dependentOptions the string representing the list of dependent options names + * @param mainOptions the main option name + */ + private void verifyMultiOptionsConstraintLoggerCall( + final Logger logger, final String dependentOptions, final String... mainOptions) { + verifyCall(logger, dependentOptions, MULTI_DEPENDENCY_WARNING_MSG, mainOptions); + } + + private void verifyCall( + final Logger logger, + final String dependentOptions, + final String dependencyWarningMsg, + final String... mainOptions) { final ArgumentCaptor stringArgumentCaptor = ArgumentCaptor.forClass(String.class); @@ -205,8 +271,11 @@ public class CommandLineUtilsTest { stringArgumentCaptor.capture(), stringArgumentCaptor.capture(), stringArgumentCaptor.capture()); - assertThat(stringArgumentCaptor.getAllValues().get(0)).isEqualTo(DEPENDENCY_WARNING_MSG); + assertThat(stringArgumentCaptor.getAllValues().get(0)).isEqualTo(dependencyWarningMsg); assertThat(stringArgumentCaptor.getAllValues().get(1)).isEqualTo(dependentOptions); - assertThat(stringArgumentCaptor.getAllValues().get(2)).isEqualTo(mainOption); + + final String joinedMainOptions = + StringUtils.joiningWithLastDelimiter(", ", " or ").apply(Arrays.asList(mainOptions)); + assertThat(stringArgumentCaptor.getAllValues().get(2)).isEqualTo(joinedMainOptions); } }