[PAN-2844] Report unknown options from config file (#1703)

Signed-off-by: Adrian Sutton <adrian.sutton@consensys.net>

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

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");
+  }
 }

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<String, OptionSpec> 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<String, OptionSpec> 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<String, OptionSpec> 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());
+    }
+  }
 }

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"