diff --git a/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java b/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java index bc072ae4..6a0dff5f 100644 --- a/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java +++ b/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java @@ -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) { @@ -668,7 +633,7 @@ private void setArgument(ArgumentDefinition argumentDefinition, List val } // check the argument range - checkArgumentRange(argumentDefinition, value); + argumentDefinition.validateValueForRange(value); if (argumentDefinition.isCollection) { @SuppressWarnings("rawtypes") @@ -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 @@ -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(){ @@ -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). diff --git a/src/test/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParserTest.java b/src/test/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParserTest.java index 4a1812fa..266a4c53 100644 --- a/src/test/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParserTest.java +++ b/src/test/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParserTest.java @@ -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 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 initializedValid = new ArrayList() + {{ + 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", @@ -941,7 +1002,7 @@ public static List 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 { @@ -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]); } }