mirror of https://github.com/hyperledger/besu
NC-1737 Enabled warning on CLI dependent options (#679)
fixes NC-1737 by adding an option dependency mechanism for "enabled" options This mechanism is aimed at preventing the use of options if the associated service is disabled. This is not a generic system as our need is simple here. But in future version of PicoCLI some options dependency mechanism may be implemented that could replace this. See https://github.com/remkop/picocli/issues/295 Our current mechanism apply to p2p, rpc (http+ws), mining, privacy and metrics. It logs a warning wherever some options are used that require another one to be enabled. Also renamed inconsistent isRpcHttpEnabled option variable and moved --privacy-enabled option to be on top of other privacy options. injects a Logger in PantheonCommand constructor to log and also provides injection capabilities for a mock in tests. adds unit tests updates tests logger name to TEST_LOGGER to prevent confusion with the mock remove --max-trailing-peers after merge with master where it was deleted updated rules for metrics Signed-off-by: Adrian Sutton <adrian.sutton@consensys.net>pull/2/head
Nicolas MASSART
6 years ago
committed by
GitHub
parent
82a771fa60
commit
a7ad81da35
@ -0,0 +1,71 @@ |
||||
/* |
||||
* Copyright 2019 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; |
||||
|
||||
import tech.pegasys.pantheon.util.StringUtils; |
||||
|
||||
import java.util.Arrays; |
||||
import java.util.List; |
||||
import java.util.stream.Collectors; |
||||
|
||||
import org.apache.logging.log4j.Logger; |
||||
import picocli.CommandLine; |
||||
|
||||
class CommandLineUtils { |
||||
/** |
||||
* Check if options are passed that require an option to be true to have any effect and log a |
||||
* warning with the list of affected options. |
||||
* |
||||
* <p>Note that in future version of PicoCLI some options dependency mechanism may be implemented |
||||
* that could replace this. See https://github.com/remkop/picocli/issues/295
|
||||
* |
||||
* @param logger the logger instance used to log the warning |
||||
* @param commandLine the command line containing the options we want to check |
||||
* @param mainOptionName the name of the main option to test dependency against. Only used for |
||||
* display. |
||||
* @param isMainOptionCondition the condition to test the options dependencies, if true will test |
||||
* if not won't |
||||
* @param dependentOptionsNames a list of option names that can't be used if condition is met. |
||||
* Example: if --miner-coinbase is in the list and condition is that --miner-enabled should |
||||
* not be false, we log a warning. |
||||
*/ |
||||
static void checkOptionDependencies( |
||||
final Logger logger, |
||||
final CommandLine commandLine, |
||||
final String mainOptionName, |
||||
final boolean isMainOptionCondition, |
||||
final List<String> dependentOptionsNames) { |
||||
if (isMainOptionCondition) { |
||||
String affectedOptions = |
||||
commandLine |
||||
.getCommandSpec() |
||||
.options() |
||||
.stream() |
||||
.filter( |
||||
option -> |
||||
Arrays.stream(option.names()).anyMatch(dependentOptionsNames::contains) |
||||
&& !option.stringValues().isEmpty()) |
||||
.map(option -> option.names()[0]) |
||||
.collect( |
||||
Collectors.collectingAndThen( |
||||
Collectors.toList(), StringUtils.joiningWithLastDelimiter(", ", " and "))); |
||||
|
||||
if (!affectedOptions.isEmpty()) { |
||||
logger.warn( |
||||
"{} will have no effect unless {} is defined on the command line.", |
||||
affectedOptions, |
||||
mainOptionName); |
||||
} |
||||
} |
||||
} |
||||
} |
||||
@ -0,0 +1,38 @@ |
||||
/* |
||||
* Copyright 2019 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.util; |
||||
|
||||
import java.util.List; |
||||
import java.util.function.Function; |
||||
|
||||
/** some useful tools to display strings in command line help or error messages */ |
||||
public class StringUtils { |
||||
|
||||
/** |
||||
* Joins a list into string elements with a delimiter but having a last different delimiter |
||||
* Example: "this thing, that thing and this other thing" |
||||
* |
||||
* @param delimiter delimiter for all the items except before the last one |
||||
* @param lastDelimiter delimiter before the last item |
||||
* @return a delimited string representation of the list |
||||
*/ |
||||
public static Function<List<String>, String> joiningWithLastDelimiter( |
||||
final String delimiter, final String lastDelimiter) { |
||||
return list -> { |
||||
final int last = list.size() - 1; |
||||
if (last < 1) return String.join(delimiter, list); |
||||
return String.join( |
||||
lastDelimiter, String.join(delimiter, list.subList(0, last)), list.get(last)); |
||||
}; |
||||
} |
||||
} |
||||
@ -0,0 +1,186 @@ |
||||
/* |
||||
* Copyright 2019 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; |
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat; |
||||
import static org.mockito.Mockito.verify; |
||||
import static org.mockito.Mockito.verifyNoMoreInteractions; |
||||
import static picocli.CommandLine.defaultExceptionHandler; |
||||
|
||||
import java.util.ArrayList; |
||||
import java.util.Arrays; |
||||
|
||||
import org.apache.logging.log4j.Logger; |
||||
import org.junit.Test; |
||||
import org.junit.runner.RunWith; |
||||
import org.mockito.ArgumentCaptor; |
||||
import org.mockito.Mock; |
||||
import org.mockito.junit.MockitoJUnitRunner; |
||||
import picocli.CommandLine; |
||||
import picocli.CommandLine.Command; |
||||
import picocli.CommandLine.Option; |
||||
import picocli.CommandLine.RunLast; |
||||
|
||||
@RunWith(MockitoJUnitRunner.class) |
||||
public class CommandLineUtilsTest { |
||||
@Mock Logger mockLogger; |
||||
|
||||
@Command(description = "This command is for testing.", name = "testcommand") |
||||
private abstract static class AbstractTestCommand implements Runnable { |
||||
|
||||
final Logger logger; |
||||
final CommandLine commandLine; |
||||
|
||||
AbstractTestCommand(final Logger logger) { |
||||
this.logger = logger; |
||||
commandLine = new CommandLine(this); |
||||
} |
||||
|
||||
// Completely disables p2p within Pantheon.
|
||||
@Option( |
||||
names = {"--option-enabled"}, |
||||
arity = "1" |
||||
) |
||||
final Boolean optionEnabled = true; |
||||
|
||||
@Option(names = {"--option2"}) |
||||
private final Integer option2 = 2; |
||||
|
||||
@Option(names = {"--option3"}) |
||||
private final Integer option3 = 3; |
||||
|
||||
@Option(names = {"--option4"}) |
||||
private final Integer option4 = 4; |
||||
} |
||||
|
||||
private static class TestCommandWithDeps extends AbstractTestCommand { |
||||
|
||||
TestCommandWithDeps(final Logger logger) { |
||||
super(logger); |
||||
} |
||||
|
||||
@Override |
||||
public void run() { |
||||
// Check that mining options are able top work or send an error
|
||||
CommandLineUtils.checkOptionDependencies( |
||||
logger, |
||||
commandLine, |
||||
"--option-enabled", |
||||
!optionEnabled, |
||||
Arrays.asList("--option2", "--option3")); |
||||
} |
||||
} |
||||
|
||||
private static class TestCommandWithoutDeps extends AbstractTestCommand { |
||||
|
||||
TestCommandWithoutDeps(final Logger logger) { |
||||
super(logger); |
||||
} |
||||
|
||||
@Override |
||||
public void run() { |
||||
// Check that mining options are able top work or send an error
|
||||
CommandLineUtils.checkOptionDependencies( |
||||
logger, commandLine, "--option-enabled", !optionEnabled, new ArrayList<>()); |
||||
} |
||||
} |
||||
|
||||
@Test |
||||
public void optionsAreNotExpected() { |
||||
AbstractTestCommand testCommand = new TestCommandWithDeps(mockLogger); |
||||
testCommand.commandLine.parseWithHandlers( |
||||
new RunLast(), |
||||
defaultExceptionHandler(), |
||||
"--option-enabled", |
||||
"false", |
||||
"--option2", |
||||
"20", |
||||
"--option3", |
||||
"30", |
||||
"--option4", |
||||
"40"); |
||||
verifyOptionsConstraintLoggerCall(mockLogger, "--option2 and --option3", "--option-enabled"); |
||||
} |
||||
|
||||
@Test |
||||
public void optionIsNotExpected() { |
||||
AbstractTestCommand testCommand = new TestCommandWithDeps(mockLogger); |
||||
testCommand.commandLine.parseWithHandlers( |
||||
new RunLast(), |
||||
defaultExceptionHandler(), |
||||
"--option-enabled", |
||||
"false", |
||||
"--option2", |
||||
"20", |
||||
"--option4", |
||||
"40"); |
||||
verifyOptionsConstraintLoggerCall(mockLogger, "--option2", "--option-enabled"); |
||||
} |
||||
|
||||
@Test |
||||
public void optionsAreExpected() { |
||||
AbstractTestCommand testCommand = new TestCommandWithDeps(mockLogger); |
||||
testCommand.commandLine.parseWithHandlers( |
||||
new RunLast(), |
||||
defaultExceptionHandler(), |
||||
"--option2", |
||||
"20", |
||||
"--option3", |
||||
"30", |
||||
"--option4", |
||||
"40"); |
||||
verifyNoMoreInteractions(mockLogger); |
||||
} |
||||
|
||||
@Test |
||||
public void noDependencies() { |
||||
AbstractTestCommand testCommand = new TestCommandWithoutDeps(mockLogger); |
||||
testCommand.commandLine.parseWithHandlers( |
||||
new RunLast(), |
||||
defaultExceptionHandler(), |
||||
"--option-enabled", |
||||
"false", |
||||
"--option2", |
||||
"20", |
||||
"--option3", |
||||
"30", |
||||
"--option4", |
||||
"40"); |
||||
verifyNoMoreInteractions(mockLogger); |
||||
} |
||||
|
||||
/** |
||||
* Check logger calls |
||||
* |
||||
* <p>Here we check the calls to logger and not the result of the log line as we don't test the |
||||
* logger itself but the fact that we call it. |
||||
* |
||||
* @param dependentOptions the string representing the list of dependent options names |
||||
* @param mainOption the main option name |
||||
*/ |
||||
private void verifyOptionsConstraintLoggerCall( |
||||
final Logger logger, final String dependentOptions, final String mainOption) { |
||||
|
||||
ArgumentCaptor<String> stringArgumentCaptor = ArgumentCaptor.forClass(String.class); |
||||
|
||||
verify(logger) |
||||
.warn( |
||||
stringArgumentCaptor.capture(), |
||||
stringArgumentCaptor.capture(), |
||||
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); |
||||
assertThat(stringArgumentCaptor.getAllValues().get(2)).isEqualTo(mainOption); |
||||
} |
||||
} |
||||
@ -0,0 +1,47 @@ |
||||
/* |
||||
* Copyright 2019 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.util; |
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat; |
||||
|
||||
import java.util.Arrays; |
||||
import java.util.HashMap; |
||||
import java.util.List; |
||||
import java.util.Map; |
||||
import java.util.Map.Entry; |
||||
import java.util.stream.Collectors; |
||||
|
||||
import org.junit.Test; |
||||
|
||||
public class StringUtilsTest { |
||||
|
||||
@Test |
||||
public void joiningWithLastDelimiter() { |
||||
Map<List<String>, String> testCases = new HashMap<>(); |
||||
//noinspection ArraysAsListWithZeroOrOneArgument
|
||||
testCases.put(Arrays.asList("item1"), "item1"); |
||||
testCases.put(Arrays.asList("item1", "item2"), "item1 and item2"); |
||||
testCases.put(Arrays.asList("item1", "item2", "item3"), "item1, item2 and item3"); |
||||
|
||||
for (Entry<List<String>, String> entry : testCases.entrySet()) { |
||||
String joinedResult = |
||||
entry |
||||
.getKey() |
||||
.stream() |
||||
.collect( |
||||
Collectors.collectingAndThen( |
||||
Collectors.toList(), StringUtils.joiningWithLastDelimiter(", ", " and "))); |
||||
assertThat(joinedResult).isEqualTo(entry.getValue()); |
||||
} |
||||
} |
||||
} |
||||
Loading…
Reference in new issue