Skip to content

Commit

Permalink
Add support and tests for initial value out of range.
Browse files Browse the repository at this point in the history
  • Loading branch information
@cmnbroad
cmnbroad committed Mar 1, 2017
1 parent 5f46a61 commit fb7a0e4
Show file tree
Hide file tree
Showing 2 changed files with 139 additions and 51 deletions.
125 changes: 76 additions & 49 deletions src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -527,41 +527,6 @@ private void validatePluginArguments() {
// finally, give each plugin a chance to trim down any unseen instances from it's own list
pluginDescriptors.entrySet().forEach(e -> e.getValue().validateArguments());
}
/**
* Check the provided value against any range constraints specified in the Argument annotation
* for the corresponding field. Throw an exception if limits are violated.
*
* - Only checks numeric types (int, double, etc.)
*/
private void checkArgumentRange(final ArgumentDefinition argumentDefinition, final Object argumentValue) {
// Only validate numeric types because we have already ensured at constructor time that only numeric types have bounds
if (!Number.class.isAssignableFrom(argumentDefinition.type)) {
return;
}

final Double argumentDoubleValue = (argumentValue == null) ? null : ((Number)argumentValue).doubleValue();

// Check hard limits first, if specified
if (argumentDefinition.hasBoundedRange() && isOutOfRange(argumentDefinition.minValue, argumentDefinition.maxValue, argumentDoubleValue)) {
throw new CommandLineException.OutOfRangeArgumentValue(argumentDefinition.getLongName(), argumentDefinition.minValue, argumentDefinition.maxValue, argumentValue);
}
// Check recommended values
if (argumentDefinition.hasRecommendedRange() && isOutOfRange(argumentDefinition.minRecommendedValue, argumentDefinition.maxRecommendedValue, argumentDoubleValue)) {
final boolean outMinValue = argumentDefinition.minRecommendedValue != Double.NEGATIVE_INFINITY;
final boolean outMaxValue = argumentDefinition.maxRecommendedValue != Double.POSITIVE_INFINITY;
if (outMinValue && outMaxValue) {
logger.warn("Argument --{} has value {}, but recommended within range ({},{})",
argumentDefinition.getLongName(), argumentDoubleValue, argumentDefinition.minRecommendedValue, argumentDefinition.maxRecommendedValue);
} else if (outMinValue) {
logger.warn("Argument --{} has value {}, but minimum recommended is {}",
argumentDefinition.getLongName(), argumentDoubleValue, argumentDefinition.minRecommendedValue);
} else if (outMaxValue) {
logger.warn("Argument --{} has value {}, but maximum recommended is {}",
argumentDefinition.getLongName(), argumentDoubleValue, argumentDefinition.maxRecommendedValue);
}
// if there is no recommended value, do not log anything
}
}

// null values are always out of range
private static boolean isOutOfRange(final double minValue, final double maxValue, final Double value) {
Expand Down Expand Up @@ -668,7 +633,7 @@ private void setArgument(ArgumentDefinition argumentDefinition, List<String> val
}

// check the argument range
checkArgumentRange(argumentDefinition, value);
argumentDefinition.validateValueForRange(value);

if (argumentDefinition.isCollection) {
@SuppressWarnings("rawtypes")
Expand Down Expand Up @@ -1154,19 +1119,6 @@ public ArgumentDefinition(
// bounds should be only set for numeric arguments and if the type is integer it should
// be set to an integer
this.type = CommandLineParser.getUnderlyingType(this.field);
if (! Number.class.isAssignableFrom(this.type)) {
if (hasBoundedRange() || hasRecommendedRange()) {
throw new CommandLineException.CommandLineParserInternalException(String.format("Min/max value ranges can only be set for numeric arguments. Argument --%s has a minimum or maximum value but has a non-numeric type.", this.getLongName()));
}
}
if (Integer.class.isAssignableFrom(this.type)) {
if (!isInfinityOrMathematicalInteger(this.maxValue)
|| !isInfinityOrMathematicalInteger(this.minValue)
|| !isInfinityOrMathematicalInteger(this.maxRecommendedValue)
|| !isInfinityOrMathematicalInteger(this.minRecommendedValue)) {
throw new CommandLineException.CommandLineParserInternalException(String.format("Integer argument --%s has a minimum or maximum attribute with a non-integral value.", this.getLongName()));
}
}
this.isHidden = field.getAnnotation(Hidden.class) != null;
if (this.isHidden && !this.optional) {
// required arguments cannot be hidden, because they should be provided in the command line
Expand All @@ -1177,6 +1129,7 @@ public ArgumentDefinition(
// required arguments cannot be advanced, because they represent options that should be changed carefully
throw new CommandLineException.CommandLineParserInternalException(String.format("A required argument cannot be annotated with @Advanced: %s", this.getLongName()));
}
validateRangeDefinitions();
}

public Object getFieldValue(){
Expand Down Expand Up @@ -1212,6 +1165,80 @@ private boolean hasRecommendedRange() {
return this.maxRecommendedValue != Double.POSITIVE_INFINITY || this.minRecommendedValue != Double.NEGATIVE_INFINITY;
}

// Validate the min/max range attribute values set in the code, and make sure
// any initial values are within range.
private void validateRangeDefinitions() {
if (hasBoundedRange() || hasRecommendedRange()) {
if (! Number.class.isAssignableFrom(this.type)) {
throw new CommandLineException.CommandLineParserInternalException(
String.format("Min/max value ranges can only be set for numeric arguments. "+
"Argument --%s has a minimum or maximum value but has a non-numeric type.",
this.getLongName()));
}
if (Integer.class.isAssignableFrom(this.type)) {
if (!isInfinityOrMathematicalInteger(this.maxValue)
|| !isInfinityOrMathematicalInteger(this.minValue)
|| !isInfinityOrMathematicalInteger(this.maxRecommendedValue)
|| !isInfinityOrMathematicalInteger(this.minRecommendedValue)) {
throw new CommandLineException.CommandLineParserInternalException(
String.format("Integer argument --%s has a minimum or maximum attribute with a non-integral value.",
this.getLongName()));
}
}

// check to see if the initial value(s) are out of range
try {
if (isCollection) {
//@SuppressWarnings("rawtypes")
final Collection c = (Collection) getFieldValue();
c.forEach(el -> validateValueForRange(el));
} else {
validateValueForRange(getFieldValue());
}
} catch (final CommandLineException.OutOfRangeArgumentValue e) {
// translate this to an internal exception since its the coder's fault, not the user's
throw new CommandLineException.CommandLineParserInternalException(
"Initial field value is outside the argument range bounds ", e);
}
}
}

/**
* Check the provided value against any range constraints specified in the Argument annotation
* for the corresponding field. Throw an exception if limits are violated.
*
* - Only checks numeric types (int, double, etc.)
*/
private void validateValueForRange(final Object argumentValue) {
// Only validate numeric types because we have already ensured at constructor time that only numeric types have bounds
if (!Number.class.isAssignableFrom(type)) {
return;
}

final Double argumentDoubleValue = (argumentValue == null) ? null : ((Number)argumentValue).doubleValue();

// Check hard limits first, if specified
if (hasBoundedRange() && isOutOfRange(minValue, maxValue, argumentDoubleValue)) {
throw new CommandLineException.OutOfRangeArgumentValue(getLongName(), minValue, maxValue, argumentValue);
}
// Check recommended values
if (hasRecommendedRange() && isOutOfRange(minRecommendedValue, maxRecommendedValue, argumentDoubleValue)) {
final boolean outMinValue = minRecommendedValue != Double.NEGATIVE_INFINITY;
final boolean outMaxValue = maxRecommendedValue != Double.POSITIVE_INFINITY;
if (outMinValue && outMaxValue) {
logger.warn("Argument --{} has value {}, but recommended within range ({},{})",
getLongName(), argumentDoubleValue, minRecommendedValue, maxRecommendedValue);
} else if (outMinValue) {
logger.warn("Argument --{} has value {}, but minimum recommended is {}",
getLongName(), argumentDoubleValue, minRecommendedValue);
} else if (outMaxValue) {
logger.warn("Argument --{} has value {}, but maximum recommended is {}",
getLongName(), argumentDoubleValue, maxRecommendedValue);
}
// if there is no recommended value, do not log anything
}
}

/**
* Determine if this argument definition is controlled by a plugin (and thus subject to
* descriptor dependency validation).
Expand Down
65 changes: 63 additions & 2 deletions src/test/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParserTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -875,6 +875,67 @@ class WithDoubleBoundariesArgumentsForInteger {
final CommandLineArgumentParser clp = new CommandLineArgumentParser(new WithDoubleBoundariesArgumentsForInteger());
}

@Test(expectedExceptions = CommandLineException.CommandLineParserInternalException.class)
public void testBoundedIntegerIntializedOutsideRange() throws Exception {
@CommandLineProgramProperties(
summary = "tool with bounded integer with bad initial value",
oneLineSummary = "tool with bounded integer with bad initial value",
programGroup = TestProgramGroup.class
)
class BoundedIntegerIntializedOutsideRange {
@Argument(doc = "Integer in the range [0, 30]",
optional = false,
minValue = 0, maxValue = 30,
minRecommendedValue = 10, maxRecommendedValue = 15)
public Integer startLifeOutOfRange = new Integer(97);
}
final BoundedIntegerIntializedOutsideRange o = new BoundedIntegerIntializedOutsideRange();
final CommandLineArgumentParser clp = new CommandLineArgumentParser(o);
}

@Test(expectedExceptions = CommandLineException.CommandLineParserInternalException.class)
public void testBoundedCollectionIntializedOutOfRange() throws Exception {
@CommandLineProgramProperties(
summary = "tool with bounded collection argument with bad initial value",
oneLineSummary = "ool with bounded collection argument with bad initial value",
programGroup = TestProgramGroup.class
)
class BoundedCollectionIntializedOutsideRange {
@Argument(doc = "Integer collection in the range [0, 30]",
optional = false,
minValue = 0, maxValue = 30,
minRecommendedValue = 10, maxRecommendedValue = 15)
public List<Integer> startLifeOutOfRange = Arrays.asList(new Integer[]{ 25, 97 });
}

final BoundedCollectionIntializedOutsideRange o = new BoundedCollectionIntializedOutsideRange();
final CommandLineArgumentParser clp = new CommandLineArgumentParser(o);
}

@Test(expectedExceptions = CommandLineException.OutOfRangeArgumentValue.class)
public void testBoundedCollectionReset() throws Exception {
@CommandLineProgramProperties(
summary = "tool with bounded argument with bad initial value",
oneLineSummary = "bounded collection argument",
programGroup = TestProgramGroup.class
)
class BoundedCollectionIntializedValid {
@Argument(doc = "Integer collection in the range [0, 30]",
optional = false,
minValue = 0, maxValue = 30,
minRecommendedValue = 10, maxRecommendedValue = 15)
public List<Integer> initializedValid = new ArrayList<Integer>()
{{
add(7);
add(4);
}};
}

final BoundedCollectionIntializedValid o = new BoundedCollectionIntializedValid();
final CommandLineArgumentParser clp = new CommandLineArgumentParser(o);
Assert.assertTrue(clp.parseArguments(System.err, new String[]{"--initializedValid", "null"}));
}

@CommandLineProgramProperties(
summary = "tool with boundaries",
oneLineSummary = "tools with boundaries",
Expand Down Expand Up @@ -941,7 +1002,7 @@ public static List<String> makeList(final String... list) {

@CommandLineProgramProperties(
summary = "tool with bounded collection argument",
oneLineSummary = "bounded collection argumen",
oneLineSummary = "bounded collection argument",
programGroup = TestProgramGroup.class
)
public class BoundedCollection {
Expand All @@ -968,7 +1029,7 @@ public void testGoodBoundedCollections(final String[] argv, final Integer[] expe
Assert.assertTrue(clp.parseArguments(System.err, argv));
Assert.assertEquals(expectedIntegers.length, o.intListArg.size());
for (int i = 0; i < expectedIntegers.length; i++) {
Assert.assertEquals(o.intListArg.get(i).intValue(), expectedIntegers[i].intValue());
Assert.assertEquals(o.intListArg.get(i), expectedIntegers[i]);
}
}

Expand Down

0 comments on commit fb7a0e4

Please sign in to comment.