From 405405e4e64ba08e1f0ba92160202298dd9f88e9 Mon Sep 17 00:00:00 2001 From: Usman Saleem Date: Tue, 16 Jul 2019 08:53:18 +1000 Subject: [PATCH] [PAN-2844] Report unknown options from config file (#1703) Signed-off-by: Adrian Sutton --- .../util/TomlConfigFileDefaultProvider.java | 26 ++++++++- .../pantheon/cli/PantheonCommandTest.java | 29 +++++++++- .../TomlConfigFileDefaultProviderTest.java | 54 +++++++++++++++++-- .../src/test/resources/complete_config.toml | 1 - 4 files changed, 100 insertions(+), 10 deletions(-) diff --git a/pantheon/src/main/java/tech/pegasys/pantheon/cli/util/TomlConfigFileDefaultProvider.java b/pantheon/src/main/java/tech/pegasys/pantheon/cli/util/TomlConfigFileDefaultProvider.java index ad9eccdce8..d83636a664 100644 --- a/pantheon/src/main/java/tech/pegasys/pantheon/cli/util/TomlConfigFileDefaultProvider.java +++ b/pantheon/src/main/java/tech/pegasys/pantheon/cli/util/TomlConfigFileDefaultProvider.java @@ -18,6 +18,7 @@ import java.io.File; import java.io.IOException; import java.util.Arrays; import java.util.Optional; +import java.util.Set; import java.util.stream.Collectors; import net.consensys.cava.toml.Toml; @@ -26,6 +27,7 @@ import net.consensys.cava.toml.TomlParseResult; import picocli.CommandLine; import picocli.CommandLine.IDefaultValueProvider; import picocli.CommandLine.Model.ArgSpec; +import picocli.CommandLine.Model.CommandSpec; import picocli.CommandLine.Model.OptionSpec; import picocli.CommandLine.ParameterException; @@ -125,11 +127,13 @@ public class TomlConfigFileDefaultProvider implements IDefaultValueProvider { result.errors().stream() .map(TomlParseError::toString) .collect(Collectors.joining("%n")); - ; + throw new ParameterException( - commandLine, String.format("Invalid TOML configuration : %s", errors)); + commandLine, String.format("Invalid TOML configuration: %s", errors)); } + checkUnknownOptions(result); + this.result = result; } catch (final IOException e) { @@ -140,4 +144,22 @@ public class TomlConfigFileDefaultProvider implements IDefaultValueProvider { checkConfigurationValidity(); } + + private void checkUnknownOptions(final TomlParseResult result) { + final CommandSpec commandSpec = commandLine.getCommandSpec(); + + final Set unknownOptionsList = + result.keySet().stream() + .filter(option -> !commandSpec.optionsMap().containsKey("--" + option)) + .collect(Collectors.toSet()); + + if (!unknownOptionsList.isEmpty()) { + final String options = unknownOptionsList.size() > 1 ? "options" : "option"; + final String csvUnknownOptions = + unknownOptionsList.stream().collect(Collectors.joining(", ")); + throw new ParameterException( + commandLine, + String.format("Unknown %s in TOML configuration file: %s", options, csvUnknownOptions)); + } + } } 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 77f5130f77..32db6b439d 100644 --- a/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java +++ b/pantheon/src/test/java/tech/pegasys/pantheon/cli/PantheonCommandTest.java @@ -232,7 +232,7 @@ public class PantheonCommandTest extends CommandTestAbstract { parseCommand("--config-file", tempConfigFile.toString()); final String expectedOutputStart = - "Invalid TOML configuration : Unexpected '.', expected a-z, A-Z, 0-9, ', \", a table key, " + "Invalid TOML configuration: Unexpected '.', expected a-z, A-Z, 0-9, ', \", a table key, " + "a newline, or end-of-input (line 1, column 1)"; assertThat(commandErrorOutput.toString()).startsWith(expectedOutputStart); assertThat(commandOutput.toString()).isEmpty(); @@ -264,7 +264,7 @@ public class PantheonCommandTest extends CommandTestAbstract { parseCommand("--config-file", tempConfigFile.toString()); final String expectedOutputStart = - "Invalid TOML configuration : Unexpected '=', expected ', \", ''', \"\"\", a number, " + "Invalid TOML configuration: Unexpected '=', expected ', \", ''', \"\"\", a number, " + "a boolean, a date/time, an array, or a table (line 1, column 8)"; assertThat(commandErrorOutput.toString()).startsWith(expectedOutputStart); assertThat(commandOutput.toString()).isEmpty(); @@ -2670,4 +2670,29 @@ public class PantheonCommandTest extends CommandTestAbstract { .contains( "Invalid value for option '--Xincoming-tx-messages-keep-alive-seconds': 'acbd' is not an int"); } + + @Test + public void tomlThatHasInvalidOptions() throws IOException { + assumeTrue(isFullInstantiation()); + + final URL configFile = this.getClass().getResource("/complete_config.toml"); + // update genesis file path, "similar" valid option and add invalid options + final Path genesisFile = createFakeGenesisFile(GENESIS_VALID_JSON); + final String updatedConfig = + Resources.toString(configFile, UTF_8) + .replace("/opt/pantheon/genesis.json", escapeTomlString(genesisFile.toString())) + .replace("rpc-http-api", "rpc-http-apis") + + System.lineSeparator() + + "invalid_option=true" + + System.lineSeparator() + + "invalid_option2=true"; + + final Path toml = createTempFile("toml", updatedConfig.getBytes(UTF_8)); + + // Parse it. + parseCommand("--config-file", toml.toString()); + + assertThat(commandErrorOutput.toString()) + .contains("Unknown options in TOML configuration file: invalid_option, invalid_option2"); + } } diff --git a/pantheon/src/test/java/tech/pegasys/pantheon/cli/TomlConfigFileDefaultProviderTest.java b/pantheon/src/test/java/tech/pegasys/pantheon/cli/TomlConfigFileDefaultProviderTest.java index d34568ef51..aa08370788 100644 --- a/pantheon/src/test/java/tech/pegasys/pantheon/cli/TomlConfigFileDefaultProviderTest.java +++ b/pantheon/src/test/java/tech/pegasys/pantheon/cli/TomlConfigFileDefaultProviderTest.java @@ -14,6 +14,7 @@ package tech.pegasys.pantheon.cli; import static java.nio.charset.StandardCharsets.UTF_8; import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.when; import tech.pegasys.pantheon.cli.util.TomlConfigFileDefaultProvider; import tech.pegasys.pantheon.ethereum.core.Wei; @@ -23,6 +24,8 @@ import java.io.File; import java.io.IOException; import java.nio.file.Files; import java.util.Collection; +import java.util.HashMap; +import java.util.Map; import org.junit.Rule; import org.junit.Test; @@ -32,6 +35,7 @@ import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; import picocli.CommandLine; +import picocli.CommandLine.Model.CommandSpec; import picocli.CommandLine.Model.OptionSpec; import picocli.CommandLine.ParameterException; @@ -39,12 +43,20 @@ import picocli.CommandLine.ParameterException; public class TomlConfigFileDefaultProviderTest { @Mock CommandLine mockCommandLine; + @Mock CommandSpec mockCommandSpec; + @Rule public final TemporaryFolder temp = new TemporaryFolder(); @Rule public ExpectedException exceptionRule = ExpectedException.none(); @Test - public void defaultValueIsNullIfNoMatchingKeyFoundOtherwiseTheValue() throws IOException { + public void defaultValueForMatchingKey() throws IOException { + when(mockCommandLine.getCommandSpec()).thenReturn(mockCommandSpec); + Map validOptionsMap = new HashMap<>(); + validOptionsMap.put("--a-short-option", null); + validOptionsMap.put("--a-longer-option", null); + when(mockCommandSpec.optionsMap()).thenReturn(validOptionsMap); + final File tempConfigFile = temp.newFile("config.toml"); try (final BufferedWriter fileWriter = Files.newBufferedWriter(tempConfigFile.toPath(), UTF_8)) { @@ -57,9 +69,6 @@ public class TomlConfigFileDefaultProviderTest { final TomlConfigFileDefaultProvider providerUnderTest = new TomlConfigFileDefaultProvider(mockCommandLine, tempConfigFile); - // this option must not be found in config - assertThat(providerUnderTest.defaultValue(OptionSpec.builder("myoption").build())).isNull(); - // this option must be found in config assertThat(providerUnderTest.defaultValue(OptionSpec.builder("a-short-option").build())) .isEqualTo("123"); @@ -82,6 +91,17 @@ public class TomlConfigFileDefaultProviderTest { @Test public void defaultValueForOptionMustMatchType() throws IOException { + when(mockCommandLine.getCommandSpec()).thenReturn(mockCommandSpec); + Map validOptionsMap = new HashMap<>(); + validOptionsMap.put("--a-boolean-option", null); + validOptionsMap.put("--another-boolean-option", null); + validOptionsMap.put("--a-multi-value-option", null); + validOptionsMap.put("--an-int-value-option", null); + validOptionsMap.put("--a-wei-value-option", null); + validOptionsMap.put("--a-string-value-option", null); + + when(mockCommandSpec.optionsMap()).thenReturn(validOptionsMap); + final File tempConfigFile = temp.newFile("config.toml"); try (final BufferedWriter fileWriter = Files.newBufferedWriter(tempConfigFile.toPath(), UTF_8)) { @@ -167,7 +187,7 @@ public class TomlConfigFileDefaultProviderTest { exceptionRule.expect(ParameterException.class); exceptionRule.expectMessage( - "Invalid TOML configuration : Unexpected '=', expected ', \", ''', " + "Invalid TOML configuration: Unexpected '=', expected ', \", ''', " + "\"\"\", a number, a boolean, a date/time, an array, or a table (line 1, column 19)"); final File tempConfigFile = temp.newFile("config.toml"); @@ -183,4 +203,28 @@ public class TomlConfigFileDefaultProviderTest { providerUnderTest.defaultValue(OptionSpec.builder("an-option").type(String.class).build()); } } + + @Test + public void unknownOptionMustThrow() throws IOException { + + exceptionRule.expect(ParameterException.class); + exceptionRule.expectMessage("Unknown option in TOML configuration file: invalid_option"); + + when(mockCommandLine.getCommandSpec()).thenReturn(mockCommandSpec); + Map validOptionsMap = new HashMap<>(); + when(mockCommandSpec.optionsMap()).thenReturn(validOptionsMap); + + final File tempConfigFile = temp.newFile("config.toml"); + try (final BufferedWriter fileWriter = + Files.newBufferedWriter(tempConfigFile.toPath(), UTF_8)) { + + fileWriter.write("invalid_option=true"); + fileWriter.flush(); + + final TomlConfigFileDefaultProvider providerUnderTest = + new TomlConfigFileDefaultProvider(mockCommandLine, tempConfigFile); + + providerUnderTest.defaultValue(OptionSpec.builder("an-option").type(String.class).build()); + } + } } diff --git a/pantheon/src/test/resources/complete_config.toml b/pantheon/src/test/resources/complete_config.toml index b6323e9ab0..56ea14a8a3 100644 --- a/pantheon/src/test/resources/complete_config.toml +++ b/pantheon/src/test/resources/complete_config.toml @@ -30,7 +30,6 @@ genesis-file="/opt/pantheon/genesis.json" # Path network-id=42 sync-mode="fast"# should be FAST or FULL (or fast or full) fast-sync-min-peers=13 -ottoman=false # true means using ottoman testnet if genesis file uses iBFT #mining miner-coinbase="0x0000000000000000000000000000000000000002" \ No newline at end of file