Skip to content

Commit

Permalink
fix: Validate rejected-tx cli options without ArgGroup (#95)
Browse files Browse the repository at this point in the history 
Removed PicoCli ArgGroup validation as it didn't work in Besu with config file parsing which uses DefaultValueProvider.

Instead, validate rejected tx cli options in the toDomainObject method. Enforce the rule that node type is required when endpoint is specified. If only node type is specified, we need no custom validation as we are only enabling reporting when endpoint is specified.
  • Loading branch information
@usmansaleem
usmansaleem authored Oct 8, 2024
1 parent 8e9fe21 commit 2902410
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 61 deletions.
63 changes: 23 additions & 40 deletions sequencer/src/main/java/net/consensys/linea/config/LineaRejectedTxReportingCliOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,9 @@
package net.consensys.linea.config;

import java.net.URL;
import java.util.Optional;

import com.google.common.base.MoreObjects;
import net.consensys.linea.plugins.LineaCliOptions;
import picocli.CommandLine.ArgGroup;
import picocli.CommandLine.Option;

/** The Linea Rejected Transaction Reporting CLI options. */
Expand All @@ -35,28 +33,21 @@ public class LineaRejectedTxReportingCliOptions implements LineaCliOptions {
/** The Linea node type. */
public static final String LINEA_NODE_TYPE = "--plugin-linea-node-type";

@ArgGroup(exclusive = false)
DependentOptions dependentOptions; // will be null if no options from this group are specified

static class DependentOptions {
@Option(
names = {REJECTED_TX_ENDPOINT},
hidden = true,
required = true, // required within the group
paramLabel = "<URL>",
description =
"Endpoint URI for reporting rejected transactions. Specify a valid URI to enable reporting.")
URL rejectedTxEndpoint = null;

@Option(
names = {LINEA_NODE_TYPE},
hidden = true,
required = true, // required within the group
paramLabel = "<NODE_TYPE>",
description =
"Linea Node type to use when reporting rejected transactions. (default: ${DEFAULT-VALUE}. Valid values: ${COMPLETION-CANDIDATES})")
LineaNodeType lineaNodeType = null;
}
@Option(
names = {REJECTED_TX_ENDPOINT},
hidden = true,
paramLabel = "<URL>",
description =
"Endpoint URI for reporting rejected transactions. Specify a valid URI to enable reporting.")
URL rejectedTxEndpoint = null;

@Option(
names = {LINEA_NODE_TYPE},
hidden = true,
paramLabel = "<NODE_TYPE>",
description =
"Linea Node type to use when reporting rejected transactions. (Valid values: ${COMPLETION-CANDIDATES})")
LineaNodeType lineaNodeType = null;

/** Default constructor. */
private LineaRejectedTxReportingCliOptions() {}
Expand All @@ -78,23 +69,19 @@ public static LineaRejectedTxReportingCliOptions create() {
public static LineaRejectedTxReportingCliOptions fromConfig(
final LineaRejectedTxReportingConfiguration config) {
final LineaRejectedTxReportingCliOptions options = create();
// both options are required.
if (config.rejectedTxEndpoint() != null && config.lineaNodeType() != null) {
final var depOpts = new DependentOptions();
depOpts.rejectedTxEndpoint = config.rejectedTxEndpoint();
depOpts.lineaNodeType = config.lineaNodeType();
options.dependentOptions = depOpts;
}

options.rejectedTxEndpoint = config.rejectedTxEndpoint();
options.lineaNodeType = config.lineaNodeType();
return options;
}

@Override
public LineaRejectedTxReportingConfiguration toDomainObject() {
final var rejectedTxEndpoint =
Optional.ofNullable(dependentOptions).map(o -> o.rejectedTxEndpoint).orElse(null);
final var lineaNodeType =
Optional.ofNullable(dependentOptions).map(o -> o.lineaNodeType).orElse(null);
// perform validation here, if endpoint is specified then node type is required.
// We can ignore node type if endpoint is not specified.
if (rejectedTxEndpoint != null && lineaNodeType == null) {
throw new IllegalArgumentException(
"Error: Missing required argument(s): " + LINEA_NODE_TYPE + "=<NODE_TYPE>");
}

return LineaRejectedTxReportingConfiguration.builder()
.rejectedTxEndpoint(rejectedTxEndpoint)
Expand All @@ -104,10 +91,6 @@ public LineaRejectedTxReportingConfiguration toDomainObject() {

@Override
public String toString() {
final var rejectedTxEndpoint =
Optional.ofNullable(dependentOptions).map(o -> o.rejectedTxEndpoint).orElse(null);
final var lineaNodeType =
Optional.ofNullable(dependentOptions).map(o -> o.lineaNodeType).orElse(null);

return MoreObjects.toStringHelper(this)
.add(REJECTED_TX_ENDPOINT, rejectedTxEndpoint)
Expand Down
40 changes: 19 additions & 21 deletions ...ncer/src/test/java/net/consensys/linea/config/LineaRejectedTxReportingCliOptionsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
import static org.assertj.core.api.Assertions.assertThatNoException;

import java.net.MalformedURLException;
import java.net.URI;

import org.hyperledger.besu.plugin.services.PicoCLIOptions;
import org.hyperledger.besu.services.PicoCLIOptionsImpl;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand All @@ -40,26 +40,27 @@ static final class MockLineaBesuCommand {
}

private MockLineaBesuCommand command;
private LineaRejectedTxReportingCliOptions lineaRejectedTxReportingCliOptions;
private CommandLine commandLine;
private PicoCLIOptions picoCliService;
private LineaRejectedTxReportingCliOptions txReportingCliOptions;

@BeforeEach
public void setup() {
command = new MockLineaBesuCommand();
commandLine = new CommandLine(command);
picoCliService = new PicoCLIOptionsImpl(commandLine);

lineaRejectedTxReportingCliOptions = LineaRejectedTxReportingCliOptions.create();
picoCliService.addPicoCLIOptions("linea", lineaRejectedTxReportingCliOptions);
// add mixin option before parseArgs is called
final PicoCLIOptionsImpl picoCliService = new PicoCLIOptionsImpl(commandLine);
txReportingCliOptions = LineaRejectedTxReportingCliOptions.create();
picoCliService.addPicoCLIOptions("linea", txReportingCliOptions);
}

@Test
void emptyLineaRejectedTxReportingCliOptions() {
commandLine.parseArgs("--mock-option", "mockValue");

assertThat(command.mockOption).isEqualTo("mockValue");
assertThat(lineaRejectedTxReportingCliOptions.dependentOptions).isNull();
assertThat(txReportingCliOptions.rejectedTxEndpoint).isNull();
assertThat(txReportingCliOptions.lineaNodeType).isNull();
}

@ParameterizedTest
Expand All @@ -72,30 +73,27 @@ void lineaRejectedTxOptionBothOptionsRequired(final LineaNodeType lineaNodeType)
"--plugin-linea-node-type",
lineaNodeType.name());

assertThat(lineaRejectedTxReportingCliOptions.dependentOptions.rejectedTxEndpoint)
// parse args would not throw an exception, toDomainObject will perform the validation
assertThat(txReportingCliOptions.rejectedTxEndpoint)
.isEqualTo(URI.create("http://localhost:8080").toURL());
assertThat(lineaRejectedTxReportingCliOptions.dependentOptions.lineaNodeType)
.isEqualTo(lineaNodeType);
assertThat(txReportingCliOptions.lineaNodeType).isEqualTo(lineaNodeType);
assertThatNoException().isThrownBy(() -> txReportingCliOptions.toDomainObject());
}

@Test
void lineaRejectedTxReportingCliOptionsOnlyEndpointCauseException() {
assertThatExceptionOfType(CommandLine.ParameterException.class)
.isThrownBy(
() ->
commandLine.parseArgs(
"--plugin-linea-rejected-tx-endpoint", "http://localhost:8080"))
commandLine.parseArgs("--plugin-linea-rejected-tx-endpoint", "http://localhost:8080");

assertThatExceptionOfType(IllegalArgumentException.class)
.isThrownBy(() -> txReportingCliOptions.toDomainObject())
.withMessageContaining(
"Error: Missing required argument(s): --plugin-linea-node-type=<NODE_TYPE>");
}

@Test
void lineaRejectedTxReportingCliOptionsOnlyNodeTypeCauseException() {
assertThatExceptionOfType(CommandLine.ParameterException.class)
.isThrownBy(
() -> commandLine.parseArgs("--plugin-linea-node-type", LineaNodeType.SEQUENCER.name()))
.withMessageContaining(
"Error: Missing required argument(s): --plugin-linea-rejected-tx-endpoint=<URL>");
void lineaRejectedTxReportingCliOptionsOnlyNodeTypeParsesWithoutProblem() {
commandLine.parseArgs("--plugin-linea-node-type", LineaNodeType.SEQUENCER.name());
assertThatNoException().isThrownBy(() -> txReportingCliOptions.toDomainObject());
}

@Test
Expand Down

0 comments on commit 2902410

Please sign in to comment.