Updated CLI option validation to handle dependent options with multiple main options. (#2054)

Signed-off-by: Mark Terry <mark.terry@consensys.net>

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(
+        asList("--privacy-multi-tenancy-enabled", "--privacy-tls-enabled"));
-            "--privacy-url",
+
-            "--privacy-public-key-file",
+    CommandLineUtils.checkMultiOptionDependencies(
-            "--privacy-multi-tenancy-enabled",
+        logger,
-            "--privacy-tls-enabled"));
+        commandLine,
+        List.of("--privacy-enabled", "--goquorum-compatibility-enabled"),
+        List.of(!isPrivacyEnabled, !isGoQuorumCompatibilityMode),
+        List.of("--privacy-url", "--privacy-public-key-file"));
 
     checkPrivacyTlsOptionsDependencies();
 

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,8 +53,52 @@ public class CommandLineUtils {
       final boolean isMainOptionCondition,
       final List<String> dependentOptionsNames) {
     if (isMainOptionCondition) {
-      final String affectedOptions =
+      final String affectedOptions = getAffectedOptions(commandLine, dependentOptionsNames);
-          commandLine.getCommandSpec().options().stream()
+
+      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.
+   *
+   * <p>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<String> mainOptions,
+      final List<Boolean> isMainOptionCondition,
+      final List<String> 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<String> dependentOptionsNames) {
+    return commandLine.getCommandSpec().options().stream()
         .filter(
             option ->
                 Arrays.stream(option.names()).anyMatch(dependentOptionsNames::contains)
@@ -61,10 +107,5 @@ public class CommandLineUtils {
         .collect(
             Collectors.collectingAndThen(
                 Collectors.toList(), StringUtils.joiningWithLastDelimiter(", ", " and ")));
-
-      if (!affectedOptions.isEmpty()) {
-        logger.warn(DEPENDENCY_WARNING_MSG, affectedOptions, mainOptionName);
-      }
-    }
   }
 }

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-enabled", "--miner-coinbase", "--miner-extra-data", "--miner-stratum-enabled");
-        "--miner-coinbase",
-        "--min-gas-price",
-        "--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(
+    verifyMultiOptionsConstraintLoggerCall(
-        "--privacy-enabled", "--privacy-url", "--privacy-public-key-file");
+        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
+   *
+   * <p>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<String> 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");

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
+   *
+   * <p>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<String> 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);
   }
 }