NC-1737 Enabled warning on CLI dependent options

pantheon/src/main/java/tech/pegasys/pantheon/Pantheon.java

@@ -12,6 +12,7 @@
  */
 package tech.pegasys.pantheon;
 
+import static org.apache.logging.log4j.LogManager.getLogger;
 import static picocli.CommandLine.defaultExceptionHandler;
 
 import tech.pegasys.pantheon.cli.PantheonCommand;
@@ -29,6 +30,7 @@ public static void main(final String... args) {
 
     final PantheonCommand pantheonCommand =
         new PantheonCommand(
+            getLogger(),
             new BlockImporter(),
             new RunnerBuilder(),
             new PantheonControllerBuilder(),

pantheon/src/main/java/tech/pegasys/pantheon/cli/CommandLineUtils.java

@@ -0,0 +1,71 @@
+/*
+ * Copyright 2019 ConsenSys AG.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+package tech.pegasys.pantheon.cli;
+
+import tech.pegasys.pantheon.util.StringUtils;
+
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
+
+import org.apache.logging.log4j.Logger;
+import picocli.CommandLine;
+
+class CommandLineUtils {
+  /**
+   * 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.
+   *
+   * <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 mainOptionName the name of the main option to test dependency against. Only used for
+   *     display.
+   * @param isMainOptionCondition the condition to test the options dependencies, if true will test
+   *     if not won't
+   * @param dependentOptionsNames a list of option names that can't be used if condition is met.
+   *     Example: if --miner-coinbase is in the list and condition is that --miner-enabled should
+   *     not be false, we log a warning.
+   */
+  static void checkOptionDependencies(
+      final Logger logger,
+      final CommandLine commandLine,
+      final String mainOptionName,
+      final boolean isMainOptionCondition,
+      final List<String> dependentOptionsNames) {
+    if (isMainOptionCondition) {
+      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 ")));
+
+      if (!affectedOptions.isEmpty()) {
+        logger.warn(
+            "{} will have no effect unless {} is defined on the command line.",
+            affectedOptions,
+            mainOptionName);
+      }
+    }
+  }
+}

pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java

@@ -62,6 +62,7 @@
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.List;
 import java.util.Optional;
@@ -74,6 +75,7 @@
 import io.vertx.core.Vertx;
 import io.vertx.core.json.DecodeException;
 import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.core.config.Configurator;
 import picocli.CommandLine;
 import picocli.CommandLine.AbstractParseResultHandler;
@@ -100,6 +102,10 @@
 )
 public class PantheonCommand implements DefaultCommandValues, Runnable {
 
+  private final Logger logger;
+
+  private CommandLine commandLine;
+
   public static class RpcApisConverter implements ITypeConverter<RpcApi> {
 
     @Override
@@ -251,7 +257,7 @@ public static class RpcApisConversionException extends Exception {
     names = {"--rpc-http-enabled"},
     description = "Set if the JSON-RPC service should be started (default: ${DEFAULT-VALUE})"
   )
-  private final Boolean isHttpRpcEnabled = false;
+  private final Boolean isRpcHttpEnabled = false;
 
   @Option(
     names = {"--rpc-http-host"},
@@ -471,6 +477,12 @@ private Long configureRefreshDelay(final Long refreshDelay) {
   )
   private final Boolean permissionsAccountsEnabled = false;
 
+  @Option(
+    names = {"--privacy-enabled"},
+    description = "Set if private transaction should be enabled (default: ${DEFAULT-VALUE})"
+  )
+  private final Boolean privacyEnabled = false;
+
   @Option(
     names = {"--privacy-url"},
     description = "The URL on which enclave is running "
@@ -483,12 +495,6 @@ private Long configureRefreshDelay(final Long refreshDelay) {
   )
   private final File privacyPublicKeyFile = null;
 
-  @Option(
-    names = {"--privacy-enabled"},
-    description = "Set if private transaction should be enabled (default: ${DEFAULT-VALUE})"
-  )
-  private final Boolean privacyEnabled = false;
-
   @Option(
     names = {"--privacy-precompiled-address"},
     description =
@@ -497,10 +503,12 @@ private Long configureRefreshDelay(final Long refreshDelay) {
   private final Integer privacyPrecompiledAddress = Address.PRIVACY;
 
   public PantheonCommand(
+      final Logger logger,
       final BlockImporter blockImporter,
       final RunnerBuilder runnerBuilder,
       final PantheonControllerBuilder controllerBuilder,
       final SynchronizerConfiguration.Builder synchronizerConfigurationBuilder) {
+    this.logger = logger;
     this.blockImporter = blockImporter;
     this.runnerBuilder = runnerBuilder;
     this.controllerBuilder = controllerBuilder;
@@ -514,7 +522,7 @@ public void parse(
       final DefaultExceptionHandler<List<Object>> exceptionHandler,
       final String... args) {
 
-    final CommandLine commandLine = new CommandLine(this);
+    commandLine = new CommandLine(this);
 
     commandLine.setCaseInsensitiveEnumValuesAllowed(true);
 
@@ -553,10 +561,26 @@ public void run() {
       Configurator.setAllLevels("", logLevel);
     }
 
-    if (!p2pEnabled && (bootNodes != null && !bootNodes.isEmpty())) {
-      throw new ParameterException(
-          new CommandLine(this), "Unable to specify bootnodes if p2p is disabled.");
-    }
+    // Check that p2p options are able top work or send an error
+    CommandLineUtils.checkOptionDependencies(
+        logger,
+        commandLine,
+        "--p2p-enabled",
+        !p2pEnabled,
+        Arrays.asList(
+            "--bootnodes",
+            "--discovery-enabled",
+            "--max-peers",
+            "--banned-node-id",
+            "--banned-node-ids"));
+
+    // Check that mining options are able top work or send an error
+    CommandLineUtils.checkOptionDependencies(
+        logger,
+        commandLine,
+        "--miner-enabled",
+        !isMiningEnabled,
+        Arrays.asList("--miner-coinbase", "--min-gas-price", "--miner-extra-data"));
 
     //noinspection ConstantConditions
     if (isMiningEnabled && coinbase == null) {
@@ -627,8 +651,21 @@ private File getNodePrivateKeyFile() {
   }
 
   private JsonRpcConfiguration jsonRpcConfiguration() {
+
+    CommandLineUtils.checkOptionDependencies(
+        logger,
+        commandLine,
+        "--rpc-http-enabled",
+        !isRpcHttpEnabled,
+        Arrays.asList(
+            "--rpc-http-api",
+            "--rpc-http-apis",
+            "--rpc-http-cors-origins",
+            "--rpc-http-host",
+            "--rpc-http-port"));
+
     final JsonRpcConfiguration jsonRpcConfiguration = JsonRpcConfiguration.createDefault();
-    jsonRpcConfiguration.setEnabled(isHttpRpcEnabled);
+    jsonRpcConfiguration.setEnabled(isRpcHttpEnabled);
     jsonRpcConfiguration.setHost(rpcHttpHost.toString());
     jsonRpcConfiguration.setPort(rpcHttpPort);
     jsonRpcConfiguration.setCorsAllowedDomains(rpcHttpCorsAllowedOrigins);
@@ -638,6 +675,19 @@ private JsonRpcConfiguration jsonRpcConfiguration() {
   }
 
   private WebSocketConfiguration webSocketConfiguration() {
+
+    CommandLineUtils.checkOptionDependencies(
+        logger,
+        commandLine,
+        "--rpc-ws-enabled",
+        !isRpcWsEnabled,
+        Arrays.asList(
+            "--rpc-ws-api",
+            "--rpc-ws-apis",
+            "--rpc-ws-refresh-delay",
+            "--rpc-ws-host",
+            "--rpc-ws-port"));
+
     final WebSocketConfiguration webSocketConfiguration = WebSocketConfiguration.createDefault();
     webSocketConfiguration.setEnabled(isRpcWsEnabled);
     webSocketConfiguration.setHost(rpcWsHost.toString());
@@ -655,6 +705,24 @@ MetricsConfiguration metricsConfiguration() {
               + "time.  Please refer to CLI reference for more details about this constraint.");
     }
 
+    CommandLineUtils.checkOptionDependencies(
+        logger,
+        commandLine,
+        "--metrics-enabled",
+        !isMetricsEnabled,
+        Arrays.asList("--metrics-host", "--metrics-port"));
+
+    CommandLineUtils.checkOptionDependencies(
+        logger,
+        commandLine,
+        "--metrics-push-enabled",
+        !isMetricsPushEnabled,
+        Arrays.asList(
+            "--metrics-push-host",
+            "--metrics-push-port",
+            "--metrics-push-interval",
+            "--metrics-push-prometheus-job"));
+
     final MetricsConfiguration metricsConfiguration = createDefault();
     metricsConfiguration.setEnabled(isMetricsEnabled);
     metricsConfiguration.setHost(metricsHost.toString());
@@ -683,6 +751,16 @@ private PermissioningConfiguration permissioningConfiguration() {
   }
 
   private PrivacyParameters orionConfiguration() {
+
+    // Check that mining options are able top work or send an error
+    CommandLineUtils.checkOptionDependencies(
+        logger,
+        commandLine,
+        "--privacy-enabled",
+        !privacyEnabled,
+        Arrays.asList(
+            "--privacy-url", "--privacy-public-key-file", "--privacy-precompiled-address"));
+
     final PrivacyParameters privacyParameters = PrivacyParameters.noPrivacy();
     privacyParameters.setEnabled(privacyEnabled);
     privacyParameters.setUrl(privacyUrl.toString());

pantheon/src/main/java/tech/pegasys/pantheon/util/StringUtils.java

@@ -0,0 +1,38 @@
+/*
+ * Copyright 2019 ConsenSys AG.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+package tech.pegasys.pantheon.util;
+
+import java.util.List;
+import java.util.function.Function;
+
+/** some useful tools to display strings in command line help or error messages */
+public class StringUtils {
+
+  /**
+   * Joins a list into string elements with a delimiter but having a last different delimiter
+   * Example: "this thing, that thing and this other thing"
+   *
+   * @param delimiter delimiter for all the items except before the last one
+   * @param lastDelimiter delimiter before the last item
+   * @return a delimited string representation of the list
+   */
+  public static Function<List<String>, String> joiningWithLastDelimiter(
+      final String delimiter, final String lastDelimiter) {
+    return list -> {
+      final int last = list.size() - 1;
+      if (last < 1) return String.join(delimiter, list);
+      return String.join(
+          lastDelimiter, String.join(delimiter, list.subList(0, last)), list.get(last));
+    };
+  }
+}