From b5b4f4e85efea7b7d4c341ea99139db3268b37f8 Mon Sep 17 00:00:00 2001 From: Nicolas MASSART Date: Thu, 14 Mar 2019 11:09:12 +0100 Subject: [PATCH] Better bootnodes option error message (#1092) change --bootnodes option to a method to be able to handle the check and error fixes PAN-2387 and PAN-2015 Handling this as a method enables to keep the parameter as a string and not a strongly typed value. It then is always accepted by the command line so it's always considered as a param and doesn't raise an unknown param error but also immediately checked against the enode pattern and intelligible matching error is displayed. Converter is removed and only a simple lambda is used in a try block leaving the handling of the error message close to the option thus making code easier to understand. Anyway, as we use a Collection of Strings as input no converter is needed anymore. Update and add tests to check non regression and handle the new behaviour. Also this change can change the order in which options are handled in the spec. So tests are updated to check for the presence of options but not specifying a static order. verifyOptionsConstraintLoggerCall now loops through the options to check they are contained in the error string but not in a specific order. Signed-off-by: Adrian Sutton --- .../pegasys/pantheon/cli/PantheonCommand.java | 16 +++-- .../pantheon/cli/PantheonCommandTest.java | 71 +++++++++++-------- .../EnodeToURIPropertyConverterTest.java | 35 --------- 3 files changed, 54 insertions(+), 68 deletions(-) delete mode 100644 pantheon/src/test/java/tech/pegasys/pantheon/cli/custom/EnodeToURIPropertyConverterTest.java diff --git a/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java b/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java index 4b5ab58fe1..1ea10af439 100644 --- a/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java +++ b/pantheon/src/main/java/tech/pegasys/pantheon/cli/PantheonCommand.java @@ -29,7 +29,6 @@ import tech.pegasys.pantheon.Runner; import tech.pegasys.pantheon.RunnerBuilder; import tech.pegasys.pantheon.cli.PublicKeySubCommand.KeyLoader; import tech.pegasys.pantheon.cli.custom.CorsAllowedOriginsProperty; -import tech.pegasys.pantheon.cli.custom.EnodeToURIPropertyConverter; import tech.pegasys.pantheon.cli.custom.JsonRPCWhitelistHostsProperty; import tech.pegasys.pantheon.cli.custom.RpcAuthFileValidator; import tech.pegasys.pantheon.cli.rlp.RLPSubCommand; @@ -59,6 +58,7 @@ import tech.pegasys.pantheon.util.BlockImporter; import tech.pegasys.pantheon.util.InvalidConfigurationException; import tech.pegasys.pantheon.util.PermissioningConfigurationValidator; import tech.pegasys.pantheon.util.bytes.BytesValue; +import tech.pegasys.pantheon.util.enode.EnodeURL; import java.io.File; import java.io.IOException; @@ -75,6 +75,7 @@ import java.util.Optional; import java.util.Set; import java.util.function.Function; import java.util.function.Supplier; +import java.util.stream.Collectors; import java.util.stream.Stream; import com.google.common.base.Suppliers; @@ -186,9 +187,16 @@ public class PantheonCommand implements DefaultCommandValues, Runnable { "Comma separated enode URLs for P2P discovery bootstrap. " + "Default is a predefined list.", split = ",", - arity = "0..*", - converter = EnodeToURIPropertyConverter.class) - private final Collection bootNodes = null; + arity = "0..*") + void setBootnodes(final List values) { + try { + bootNodes = values.stream().map((s) -> new EnodeURL(s).toURI()).collect(Collectors.toList()); + } catch (IllegalArgumentException e) { + throw new ParameterException(commandLine, e.getMessage()); + } + } + + private Collection bootNodes = null; @Option( names = {"--max-peers"}, diff --git a/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java b/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java index 6d7e196369..8c57011dcf 100644 --- a/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java +++ b/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java @@ -707,7 +707,7 @@ public class PantheonCommandTest extends CommandTestAbstract { String.join(",", nodes)); verifyOptionsConstraintLoggerCall( - "--discovery-enabled, --bootnodes, --max-peers and --banned-node-ids", "--p2p-enabled"); + "--p2p-enabled", "--discovery-enabled", "--bootnodes", "--max-peers", "--banned-node-ids"); assertThat(commandOutput.toString()).isEmpty(); assertThat(commandErrorOutput.toString()).isEmpty(); @@ -733,16 +733,6 @@ public class PantheonCommandTest extends CommandTestAbstract { assertThat(commandErrorOutput.toString()).isEmpty(); } - @Ignore("NC-2015 - Temporarily enabling zero-arg --bootnodes to permit 'bootnode' configuration") - @Test - public void callingWithBootnodesOptionButNoValueMustDisplayErrorAndUsage() { - parseCommand("--bootnodes"); - assertThat(commandOutput.toString()).isEmpty(); - final String expectedErrorOutputStart = - "Missing required parameter for option '--bootnodes' at index 0 ()"; - assertThat(commandErrorOutput.toString()).startsWith(expectedErrorOutputStart); - } - @Test public void callingWithBootnodesOptionButNoValueMustPassEmptyBootnodeList() { parseCommand("--bootnodes"); @@ -756,22 +746,33 @@ public class PantheonCommandTest extends CommandTestAbstract { assertThat(commandErrorOutput.toString()).isEmpty(); } - @Ignore( - "NC-2015 - Temporarily enabling zero-arg --bootnodes to permit 'bootnode' configuration, which changes the error.") @Test - public void callingWithInvalidBootnodesMustDisplayErrorAndUsage() { + public void callingWithValidBootnodeMustSucceed() { + parseCommand( + "--bootnodes", + "enode://d2567893371ea5a6fa6371d483891ed0d129e79a8fc74d6df95a00a6545444cd4a6960bbffe0b4e2edcf35135271de57ee559c0909236bbc2074346ef2b5b47c@127.0.0.1:30304"); + assertThat(commandOutput.toString()).isEmpty(); + assertThat(commandErrorOutput.toString()).isEmpty(); + } + + @Test + public void callingWithInvalidBootnodeMustDisplayErrorAndUsage() { parseCommand("--bootnodes", "invalid_enode_url"); assertThat(commandOutput.toString()).isEmpty(); final String expectedErrorOutputStart = - "Invalid value for option '--bootnodes' at index 0 ()"; + "Invalid enode URL syntax. Enode URL should have the following format " + + "'enode://@:[?discport=]'."; assertThat(commandErrorOutput.toString()).startsWith(expectedErrorOutputStart); } + // This test ensures non regression on https://pegasys1.atlassian.net/browse/PAN-2387 @Test - public void callingWithInvalidBootnodesAndZeroArityMustDisplayAlternateErrorAndUsage() { - parseCommand("--bootnodes", "invalid_enode_url"); + public void callingWithInvalidBootnodeAndEqualSignMustDisplayErrorAndUsage() { + parseCommand("--bootnodes=invalid_enode_url"); assertThat(commandOutput.toString()).isEmpty(); - final String expectedErrorOutputStart = "Unmatched argument: invalid_enode_url"; + final String expectedErrorOutputStart = + "Invalid enode URL syntax. Enode URL should have the following format " + + "'enode://@:[?discport=]'."; assertThat(commandErrorOutput.toString()).startsWith(expectedErrorOutputStart); } @@ -944,8 +945,11 @@ public class PantheonCommandTest extends CommandTestAbstract { "all"); verifyOptionsConstraintLoggerCall( - "--rpc-http-host, --rpc-http-port, --rpc-http-cors-origins and --rpc-http-api", - "--rpc-http-enabled"); + "--rpc-http-enabled", + "--rpc-http-host", + "--rpc-http-port", + "--rpc-http-cors-origins", + "--rpc-http-api"); assertThat(commandOutput.toString()).isEmpty(); assertThat(commandErrorOutput.toString()).isEmpty(); @@ -1354,7 +1358,7 @@ public class PantheonCommandTest extends CommandTestAbstract { parseCommand("--rpc-ws-api", "ETH,NET", "--rpc-ws-host", "0.0.0.0", "--rpc-ws-port", "1234"); verifyOptionsConstraintLoggerCall( - "--rpc-ws-host, --rpc-ws-port and --rpc-ws-api", "--rpc-ws-enabled"); + "--rpc-ws-enabled", "--rpc-ws-host", "--rpc-ws-port", "--rpc-ws-api"); assertThat(commandOutput.toString()).isEmpty(); assertThat(commandErrorOutput.toString()).isEmpty(); @@ -1457,8 +1461,11 @@ public class PantheonCommandTest extends CommandTestAbstract { "job-name"); verifyOptionsConstraintLoggerCall( - "--metrics-push-host, --metrics-push-port, --metrics-push-interval and --metrics-push-prometheus-job", - "--metrics-push-enabled"); + "--metrics-push-enabled", + "--metrics-push-host", + "--metrics-push-port", + "--metrics-push-interval", + "--metrics-push-prometheus-job"); assertThat(commandOutput.toString()).isEmpty(); assertThat(commandErrorOutput.toString()).isEmpty(); @@ -1468,7 +1475,7 @@ public class PantheonCommandTest extends CommandTestAbstract { public void metricsOptionsRequiresPullMetricsToBeEnabled() { parseCommand("--metrics-host", "0.0.0.0", "--metrics-port", "1234"); - verifyOptionsConstraintLoggerCall("--metrics-host and --metrics-port", "--metrics-enabled"); + verifyOptionsConstraintLoggerCall("--metrics-enabled", "--metrics-host", "--metrics-port"); assertThat(commandOutput.toString()).isEmpty(); assertThat(commandErrorOutput.toString()).isEmpty(); @@ -1660,7 +1667,7 @@ public class PantheonCommandTest extends CommandTestAbstract { "0x1122334455667788990011223344556677889900112233445566778899001122"); verifyOptionsConstraintLoggerCall( - "--miner-coinbase, --min-gas-price and --miner-extra-data", "--miner-enabled"); + "--miner-enabled", "--miner-coinbase", "--min-gas-price", "--miner-extra-data"); assertThat(commandOutput.toString()).isEmpty(); assertThat(commandErrorOutput.toString()).isEmpty(); @@ -1865,8 +1872,10 @@ public class PantheonCommandTest extends CommandTestAbstract { String.valueOf(Byte.MAX_VALUE - 1)); verifyOptionsConstraintLoggerCall( - "--privacy-url, --privacy-precompiled-address and --privacy-public-key-file", - "--privacy-enabled"); + "--privacy-enabled", + "--privacy-url", + "--privacy-precompiled-address", + "--privacy-public-key-file"); assertThat(commandOutput.toString()).isEmpty(); assertThat(commandErrorOutput.toString()).isEmpty(); @@ -1930,7 +1939,7 @@ public class PantheonCommandTest extends CommandTestAbstract { * @param mainOption the main option name */ private void verifyOptionsConstraintLoggerCall( - final String dependentOptions, final String mainOption) { + final String mainOption, final String... dependentOptions) { verify(mockLogger, atLeast(1)) .warn( stringArgumentCaptor.capture(), @@ -1938,7 +1947,11 @@ public class PantheonCommandTest extends CommandTestAbstract { stringArgumentCaptor.capture()); assertThat(stringArgumentCaptor.getAllValues().get(0)) .isEqualTo("{} will have no effect unless {} is defined on the command line."); - assertThat(stringArgumentCaptor.getAllValues().get(1)).isEqualTo(dependentOptions); + + for (String option : dependentOptions) { + assertThat(stringArgumentCaptor.getAllValues().get(1)).contains(option); + } + assertThat(stringArgumentCaptor.getAllValues().get(2)).isEqualTo(mainOption); } diff --git a/pantheon/src/test/java/tech/pegasys/pantheon/cli/custom/EnodeToURIPropertyConverterTest.java b/pantheon/src/test/java/tech/pegasys/pantheon/cli/custom/EnodeToURIPropertyConverterTest.java deleted file mode 100644 index 7b93b8460c..0000000000 --- a/pantheon/src/test/java/tech/pegasys/pantheon/cli/custom/EnodeToURIPropertyConverterTest.java +++ /dev/null @@ -1,35 +0,0 @@ -/* - * Copyright 2018 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.custom; - -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; - -import java.net.URI; -import java.util.function.Function; - -import org.junit.Test; - -public class EnodeToURIPropertyConverterTest { - - @Test - @SuppressWarnings("unchecked") - public void converterDelegatesToFunction() { - Function function = mock(Function.class); - - new EnodeToURIPropertyConverter(function).convert("foo"); - - verify(function).apply(eq("foo")); - } -}