Skip to content

Commit

Permalink
Remove TOML table headings before checking for valid config parameters (
Browse files Browse the repository at this point in the history 
hyperledger#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 <matthew1001@gmail.com>

---------

Signed-off-by: Matthew Whitehead <matthew1001@gmail.com>
Co-authored-by: Matthew Whitehead <matthew1001@gmail.com>
Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com>
  • Loading branch information
@matthew1001 @macfarla @eum602
3 people authored and eum602 committed Nov 3, 2023
1 parent 7d16b0c commit 4eab1f1
Show file tree
Hide file tree
Showing 2 changed files with 137 additions and 7 deletions.
70 changes: 63 additions & 7 deletions besu/src/main/java/org/hyperledger/besu/cli/util/TomlConfigFileDefaultProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@
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;
Expand Down Expand Up @@ -102,11 +104,49 @@ private String getEntryAsString(final OptionSpec spec) {
private Optional<String> 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<String> 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<String> getDottedKeyName(final OptionSpec spec) {
List<String> 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) {
Expand Down Expand Up @@ -142,7 +182,8 @@ private String getNumericEntryAsString(final OptionSpec spec) {
// 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() {
Expand Down Expand Up @@ -184,8 +225,23 @@ public void loadConfigurationFromFile() {
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<String> optionsWithoutTables = new HashSet<String>();
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<String> unknownOptionsList =
result.keySet().stream()
optionsWithoutTables.stream()
.filter(option -> !commandSpec.optionsMap().containsKey("--" + option))
.collect(Collectors.toSet());

Expand Down
74 changes: 74 additions & 0 deletions besu/src/test/java/org/hyperledger/besu/cli/TomlConfigFileDefaultProviderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -298,4 +298,78 @@ public void unknownOptionMustThrow() throws IOException {
.isInstanceOf(ParameterException.class)
.hasMessage("Unknown option in TOML configuration file: invalid_option");
}

@Test
public void tomlTableHeadingsMustBeIgnored() throws IOException {

when(mockCommandLine.getCommandSpec()).thenReturn(mockCommandSpec);

Map<String, OptionSpec> 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<String, OptionSpec> 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");
}
}

0 comments on commit 4eab1f1

Please sign in to comment.