From 39577fd799f3374b4176b0e032b1a69d4bfa4af3 Mon Sep 17 00:00:00 2001 From: Matt Whitehead Date: Fri, 9 Jun 2023 03:04:09 +0100 Subject: [PATCH] Remove TOML table headings before checking for valid config parameters (#5483) * Remove TOML table headings before checking for valid config parameters * Use dotted paths to find parameters under table headings Signed-off-by: Matthew Whitehead --------- Signed-off-by: Matthew Whitehead Co-authored-by: Matthew Whitehead Co-authored-by: Sally MacFarlane --- .../util/TomlConfigFileDefaultProvider.java | 70 ++++++++++++++++-- .../TomlConfigFileDefaultProviderTest.java | 74 +++++++++++++++++++ 2 files changed, 137 insertions(+), 7 deletions(-) diff --git a/besu/src/main/java/org/hyperledger/besu/cli/util/TomlConfigFileDefaultProvider.java b/besu/src/main/java/org/hyperledger/besu/cli/util/TomlConfigFileDefaultProvider.java index aaeb80fc72..ec9ee7d593 100644 --- a/besu/src/main/java/org/hyperledger/besu/cli/util/TomlConfigFileDefaultProvider.java +++ b/besu/src/main/java/org/hyperledger/besu/cli/util/TomlConfigFileDefaultProvider.java @@ -19,7 +19,9 @@ import org.hyperledger.besu.datatypes.Wei; import java.io.File; import java.io.IOException; import java.math.BigInteger; +import java.util.ArrayList; import java.util.Arrays; +import java.util.HashSet; import java.util.List; import java.util.Optional; import java.util.Set; @@ -102,11 +104,49 @@ public class TomlConfigFileDefaultProvider implements IDefaultValueProvider { private Optional getKeyName(final OptionSpec spec) { // If any of the names of the option are used as key in the toml results // then returns the value of first one. - return Arrays.stream(spec.names()) - // remove leading dashes on option name as we can have "--" or "-" options - .map(name -> name.replaceFirst("^-+", "")) - .filter(result::contains) - .findFirst(); + Optional keyName = + Arrays.stream(spec.names()) + // remove leading dashes on option name as we can have "--" or "-" options + .map(name -> name.replaceFirst("^-+", "")) + .filter(result::contains) + .findFirst(); + + if (keyName.isEmpty()) { + // If the base key name doesn't exist in the file it may be under a TOML table heading + // e.g. TxPool.tx-pool-max-size + keyName = getDottedKeyName(spec); + } + + return keyName; + } + + /* + For all spec names, look to see if any of the TOML keyPathSet entries contain + the name. A key path set might look like ["TxPool", "tx-max-pool-size"] where + "TxPool" is the TOML table heading (which we ignore) and "tx-max-pool-size" is + the name of the option being requested. For a request for "tx-max-pool-size" this + function will return "TxPool.tx-max-pool-size" which can then be used directly + as a query on the TOML result structure. + */ + private Optional getDottedKeyName(final OptionSpec spec) { + List foundNames = new ArrayList<>(); + + Arrays.stream(spec.names()) + .forEach( + nextSpecName -> { + String specName = + result.keyPathSet().stream() + .filter(option -> option.contains(nextSpecName.replaceFirst("^-+", ""))) + .findFirst() + .orElse(new ArrayList<>()) + .stream() + .collect(Collectors.joining(".")); + if (specName.length() > 0) { + foundNames.add(specName); + } + }); + + return foundNames.stream().findFirst(); } private String getListEntryAsString(final OptionSpec spec) { @@ -142,7 +182,8 @@ public class TomlConfigFileDefaultProvider implements IDefaultValueProvider { // return the string representation of the numeric value corresponding to the option in toml // file - this works for integer, double, and float // or null if not present in the config - return getKeyName(spec).map(result::get).map(String::valueOf).orElse(null); + + return getKeyName(spec).map(result::get).map(Object::toString).orElse(null); } private void checkConfigurationValidity() { @@ -184,8 +225,23 @@ public class TomlConfigFileDefaultProvider implements IDefaultValueProvider { private void checkUnknownOptions(final TomlParseResult result) { final CommandSpec commandSpec = commandLine.getCommandSpec(); + // Besu ignores TOML table headings (e.g. [TxPool]) so we use keyPathSet() and take the + // last element in each one. For a TOML parameter that's not defined inside a table, the lists + // returned in keyPathSet() will contain a single entry - the config parameter itself. For a + // TOML + // entry that is in a table the list will contain N entries, the last one being the config + // parameter itself. + final Set optionsWithoutTables = new HashSet(); + result.keyPathSet().stream() + .forEach( + strings -> { + optionsWithoutTables.add(strings.get(strings.size() - 1)); + }); + + // Once we've stripped TOML table headings from the lists, we can check that the remaining + // options are valid final Set unknownOptionsList = - result.keySet().stream() + optionsWithoutTables.stream() .filter(option -> !commandSpec.optionsMap().containsKey("--" + option)) .collect(Collectors.toSet()); diff --git a/besu/src/test/java/org/hyperledger/besu/cli/TomlConfigFileDefaultProviderTest.java b/besu/src/test/java/org/hyperledger/besu/cli/TomlConfigFileDefaultProviderTest.java index d764aab06d..1481db9526 100644 --- a/besu/src/test/java/org/hyperledger/besu/cli/TomlConfigFileDefaultProviderTest.java +++ b/besu/src/test/java/org/hyperledger/besu/cli/TomlConfigFileDefaultProviderTest.java @@ -298,4 +298,78 @@ public class TomlConfigFileDefaultProviderTest { .isInstanceOf(ParameterException.class) .hasMessage("Unknown option in TOML configuration file: invalid_option"); } + + @Test + public void tomlTableHeadingsMustBeIgnored() throws IOException { + + when(mockCommandLine.getCommandSpec()).thenReturn(mockCommandSpec); + + Map validOptionsMap = new HashMap<>(); + validOptionsMap.put("--a-valid-option", null); + validOptionsMap.put("--another-valid-option", null); + validOptionsMap.put("--onemore-valid-option", null); + when(mockCommandSpec.optionsMap()).thenReturn(validOptionsMap); + + final File tempConfigFile = temp.newFile("config.toml"); + final BufferedWriter fileWriter = Files.newBufferedWriter(tempConfigFile.toPath(), UTF_8); + + fileWriter.write("a-valid-option=123"); + fileWriter.newLine(); + fileWriter.write("[ignoreme]"); + fileWriter.newLine(); + fileWriter.write("another-valid-option=456"); + fileWriter.newLine(); + fileWriter.write("onemore-valid-option=789"); + fileWriter.newLine(); + fileWriter.flush(); + + final TomlConfigFileDefaultProvider providerUnderTest = + new TomlConfigFileDefaultProvider(mockCommandLine, tempConfigFile); + + assertThat( + providerUnderTest.defaultValue( + OptionSpec.builder("a-valid-option").type(Integer.class).build())) + .isEqualTo("123"); + + assertThat( + providerUnderTest.defaultValue( + OptionSpec.builder("another-valid-option").type(Integer.class).build())) + .isEqualTo("456"); + + assertThat( + providerUnderTest.defaultValue( + OptionSpec.builder("onemore-valid-option").type(Integer.class).build())) + .isEqualTo("789"); + } + + @Test + public void tomlTableHeadingsMustNotSkipValidationOfUnknownOptions() throws IOException { + + when(mockCommandLine.getCommandSpec()).thenReturn(mockCommandSpec); + + Map validOptionsMap = new HashMap<>(); + validOptionsMap.put("--a-valid-option", null); + when(mockCommandSpec.optionsMap()).thenReturn(validOptionsMap); + + final File tempConfigFile = temp.newFile("config.toml"); + final BufferedWriter fileWriter = Files.newBufferedWriter(tempConfigFile.toPath(), UTF_8); + + fileWriter.write("[ignoreme]"); + fileWriter.newLine(); + fileWriter.write("a-valid-option=123"); + fileWriter.newLine(); + fileWriter.write("invalid-option=789"); + fileWriter.newLine(); + fileWriter.flush(); + + final TomlConfigFileDefaultProvider providerUnderTest = + new TomlConfigFileDefaultProvider(mockCommandLine, tempConfigFile); + + assertThatThrownBy( + () -> + providerUnderTest.defaultValue( + OptionSpec.builder("an-option").type(String.class).build())) + .isInstanceOf(ParameterException.class) + .hasMessage("Unknown option in TOML configuration file: invalid-option"); + } }