Merge pull request #694 from broadinstitute/pd_check_missing_arg_values

Improved detection of missing argument values

protected/gatk-tools-protected/src/main/java/org/broadinstitute/gatk/tools/walkers/genotyper/UnifiedGenotyper.java

@@ -212,7 +212,7 @@ public class UnifiedGenotyper extends LocusWalker<List<VariantCallContext>, Unif
      * If specified, all available annotations in the group will be applied. See the VariantAnnotator -list argument to view available groups.
      * Keep in mind that RODRequiringAnnotations are not intended to be used as a group, because they require specific ROD inputs.
      */
-    @Argument(fullName="group", shortName="G", doc="One or more classes/groups of annotations to apply to variant calls", required=false)
+    @Argument(fullName="group", shortName="G", doc="One or more classes/groups of annotations to apply to variant calls.  The single value 'none' removes the default group", required=false)
     protected String[] annotationClassesToUse = { "Standard" };
 
     // the calculation arguments

protected/gatk-tools-protected/src/main/java/org/broadinstitute/gatk/tools/walkers/haplotypecaller/HaplotypeCaller.java

@@ -320,7 +320,7 @@ public class HaplotypeCaller extends ActiveRegionWalker<List<VariantContext>, In
      * Which annotations to add to the output VCF file. See the VariantAnnotator -list argument to view available annotations.
      */
     @Advanced
-    @Argument(fullName="annotation", shortName="A", doc="One or more specific annotations to apply to variant calls", required=false)
+    @Argument(fullName="annotation", shortName="A", doc="One or more specific annotations to apply to variant calls.  The single value 'none' removes the default annotations", required=false)
     protected List<String> annotationsToUse = new ArrayList<>(Arrays.asList(new String[]{"ClippingRankSumTest", "DepthPerSampleHC"}));
 
     /**
@@ -334,7 +334,7 @@ public class HaplotypeCaller extends ActiveRegionWalker<List<VariantContext>, In
     /**
      * Which groups of annotations to add to the output VCF file. See the VariantAnnotator -list argument to view available groups.
      */
-    @Argument(fullName="group", shortName="G", doc="One or more classes/groups of annotations to apply to variant calls", required=false)
+    @Argument(fullName="group", shortName="G", doc="One or more classes/groups of annotations to apply to variant calls.  The single value 'none' removes the default group", required=false)
     protected String[] annotationClassesToUse = { "Standard" };
 
     @ArgumentCollection

protected/gatk-tools-protected/src/main/java/org/broadinstitute/gatk/tools/walkers/variantutils/GenotypeGVCFs.java

@@ -133,7 +133,7 @@ public class GenotypeGVCFs extends RodWalker<VariantContext, VariantContextWrite
      * Which annotations to recompute for the combined output VCF file.
      */
     @Advanced
-    @Argument(fullName="annotation", shortName="A", doc="One or more specific annotations to recompute", required=false)
+    @Argument(fullName="annotation", shortName="A", doc="One or more specific annotations to recompute.  The single value 'none' removes the default annotations", required=false)
     protected List<String> annotationsToUse = new ArrayList<>(Arrays.asList(new String[]{"InbreedingCoeff", "FisherStrand", "QualByDepth", "ChromosomeCounts", "GenotypeSummaries"}));
 
     /**

protected/gatk-tools-protected/src/test/java/org/broadinstitute/gatk/tools/walkers/haplotypecaller/HaplotypeCallerParallelIntegrationTest.java

@@ -71,7 +71,7 @@ public class HaplotypeCallerParallelIntegrationTest extends WalkerTest {
         WalkerTestSpec spec = new WalkerTestSpec(
                 "-T HaplotypeCaller --pcr_indel_model NONE -R " + b37KGReference + " --no_cmdline_in_header -I "
                         + privateTestDir + "PCRFree.2x250.Illumina.20_10_11.bam -o %s " +
-                        " -L 20:10,000,000-10,100,000 -G none -A -contamination 0.0 -nct " + nct, 1,
+                        " -L 20:10,000,000-10,100,000 -G none -A none -contamination 0.0 -nct " + nct, 1,
                 Arrays.asList(md5));
         executeTest("HC test parallel HC with NCT with nct " + nct, spec);
     }

protected/gatk-tools-protected/src/test/java/org/broadinstitute/gatk/utils/nanoScheduler/NanoSchedulerIntegrationTest.java

@@ -78,7 +78,7 @@ public class NanoSchedulerIntegrationTest extends WalkerTest {
         WalkerTestSpec spec = new WalkerTestSpec(
                 buildCommandLine(
                         "-T UnifiedGenotyper -R " + b37KGReference,
-                        "--no_cmdline_in_header -G",
+                        "--no_cmdline_in_header -G none",
                         //"--dbsnp " + b37dbSNP132,
                         "-I " + privateTestDir + "NA12878.HiSeq.b37.chr20.10_11mb.bam",
                         "-L 20:10,000,000-10,100,000",

public/gatk-tools-public/src/main/java/org/broadinstitute/gatk/tools/walkers/annotator/interfaces/AnnotationInterfaceManager.java

@@ -32,6 +32,9 @@ import org.broadinstitute.gatk.utils.exceptions.UserException;
 import java.util.*;
 
 public class AnnotationInterfaceManager {
+    private static final String NULL_ANNOTATION_NAME = "none";
+    private static final String NULL_ANNOTATION_GROUP_NAME = "none";
+
     private static PluginManager<InfoFieldAnnotation> infoFieldAnnotationPluginManager = new PluginManager<InfoFieldAnnotation>(InfoFieldAnnotation.class);
     private static PluginManager<GenotypeAnnotation> genotypeAnnotationPluginManager = new PluginManager<GenotypeAnnotation>(GenotypeAnnotation.class);
     private static PluginManager<AnnotationType> annotationTypePluginManager = new PluginManager<AnnotationType>(AnnotationType.class);
@@ -53,7 +56,7 @@ public class AnnotationInterfaceManager {
         for ( Class c : annotationTypePluginManager.getInterfaces() )
             classMap.put(c.getSimpleName(), c);
 
-        if ( annotationGroupsToUse.size() != 1 || !"none".equals(annotationGroupsToUse.get(0)) ) {
+        if ( annotationGroupsToUse.size() != 1 || !NULL_ANNOTATION_GROUP_NAME.equals(annotationGroupsToUse.get(0)) ) {
             for ( String group : annotationGroupsToUse ) {
                 Class interfaceClass = classMap.get(group);
                 if ( interfaceClass == null )
@@ -64,15 +67,17 @@ public class AnnotationInterfaceManager {
         }
 
         // validate the specific classes provided
-        for ( String annotation : annotationsToUse ) {
+        if ( annotationsToUse.size() != 1 || !NULL_ANNOTATION_NAME.equals(annotationsToUse.get(0)) ) {
-            Class annotationClass = classMap.get(annotation);
+            for (String annotation : annotationsToUse) {
-            if ( annotationClass == null )
+                Class annotationClass = classMap.get(annotation);
-                annotationClass = classMap.get(annotation + "Annotation");
+                if (annotationClass == null)
-            if ( annotationClass == null ) {
+                    annotationClass = classMap.get(annotation + "Annotation");
-                if (DeprecatedToolChecks.isDeprecatedAnnotation(annotation) ) {
+                if (annotationClass == null) {
-                    throw new UserException.DeprecatedAnnotation(annotation, DeprecatedToolChecks.getAnnotationDeprecationInfo(annotation));
+                    if (DeprecatedToolChecks.isDeprecatedAnnotation(annotation)) {
-                } else {
+                        throw new UserException.DeprecatedAnnotation(annotation, DeprecatedToolChecks.getAnnotationDeprecationInfo(annotation));
-                    throw new UserException.BadArgumentValue("annotation", "Annotation " + annotation + " was not found; please check that you have specified the annotation name correctly");
+                    } else {
+                        throw new UserException.BadArgumentValue("annotation", "Annotation " + annotation + " was not found; please check that you have specified the annotation name correctly");
+                    }
                 }
             }
         }
@@ -105,7 +110,7 @@ public class AnnotationInterfaceManager {
             }
         });
 
-        if ( annotationGroupsToUse.size() != 1 || !"none".equals(annotationGroupsToUse.get(0)) ) {
+        if ( annotationGroupsToUse.size() != 1 || !NULL_ANNOTATION_GROUP_NAME.equals(annotationGroupsToUse.get(0)) ) {
             for ( String group : annotationGroupsToUse ) {
                 Class interfaceClass = classMap.get(group);
                 if ( interfaceClass == null )
@@ -116,12 +121,14 @@ public class AnnotationInterfaceManager {
         }
 
         // get the specific classes provided
-        for ( String annotation : annotationsToUse ) {
+        if ( annotationsToUse.size() != 1 || !NULL_ANNOTATION_NAME.equals(annotationsToUse.get(0)) ) {
-            Class annotationClass = classMap.get(annotation);
+            for (String annotation : annotationsToUse) {
-            if ( annotationClass == null )
+                Class annotationClass = classMap.get(annotation);
-                annotationClass = classMap.get(annotation + "Annotation");
+                if (annotationClass == null)
-            if ( annotationClass != null )
+                    annotationClass = classMap.get(annotation + "Annotation");
-                classes.add(annotationClass);
+                if (annotationClass != null)
+                    classes.add(annotationClass);
+            }
         }
 
         // note that technically an annotation can work on both the INFO and FORMAT fields

public/gatk-tools-public/src/main/java/org/broadinstitute/gatk/utils/commandline/ArgumentDefinitions.java

@@ -174,8 +174,9 @@ public class ArgumentDefinitions implements Iterable<ArgumentDefinition> {
 
     static DefinitionMatcher VerifiableDefinitionMatcher = new DefinitionMatcher() {
         public boolean matches( ArgumentDefinition definition, Object key ) {
-            // We can perform some sort of validation for anything that isn't a flag.
+            // We can perform some sort of validation for anything that isn't a flag or enum.
-            return !definition.isFlag;
+            // Because enums can have a default value, it might be valid to specify an enum argument with no value
+            return !definition.isFlag  && !definition.argumentType.isEnum();
         }        
     };
 }

public/gatk-tools-public/src/main/java/org/broadinstitute/gatk/utils/commandline/ArgumentMatch.java

@@ -187,7 +187,7 @@ public class ArgumentMatch implements Iterable<ArgumentMatch> {
              * @return True if there's another token waiting in the wings.  False otherwise.
              */
             public boolean hasNext() {
-                return nextToken != null;
+                return nextSite != null;
             }
 
             /**
@@ -195,7 +195,7 @@ public class ArgumentMatch implements Iterable<ArgumentMatch> {
              * @return The next ArgumentMatch in the series.  Should never be null.
              */
             public ArgumentMatch next() {
-                if( nextSite == null || nextToken == null )
+                if( nextSite == null )
                     throw new IllegalStateException( "No more ArgumentMatches are available" );
 
                 ArgumentMatch match = new ArgumentMatch( label, definition, nextSite, nextToken, tags );
@@ -221,10 +221,8 @@ public class ArgumentMatch implements Iterable<ArgumentMatch> {
                         nextSite = siteIterator.next();
                         if( sites.get(nextSite) != null ) {
                             tokenIterator = sites.get(nextSite).iterator();
-                            if( tokenIterator.hasNext() ) {
+                            nextToken = tokenIterator.hasNext() ? tokenIterator.next() : null;
-                                nextToken = tokenIterator.next();
+                            break;
-                                break;
-                            }
                         }
                     }
                 }
@@ -280,6 +278,8 @@ public class ArgumentMatch implements Iterable<ArgumentMatch> {
         for ( final List<ArgumentMatchValue> siteValue : sites.values() ) {
             if ( siteValue != null )
                 values.addAll(siteValue);
+            else
+                values.add(null);
         }
         return values;
     }

public/gatk-tools-public/src/main/java/org/broadinstitute/gatk/utils/commandline/ArgumentTypeDescriptor.java

@@ -736,8 +736,10 @@ class SimpleArgumentTypeDescriptor extends ArgumentTypeDescriptor {
             } else if (type.equals(File.class)) {
                 result = value == null ? null : value.asFile();
             } else {
+                if (value == null)
+                    throw new MissingArgumentValueException(createDefaultArgumentDefinition(source));
                 Constructor ctor = type.getConstructor(String.class);
-                result = ctor.newInstance(value == null ? null : value.asString());
+                result = ctor.newInstance(value.asString());
             }
         } catch (UserException e) {
             throw e;

public/gatk-tools-public/src/main/java/org/broadinstitute/gatk/utils/commandline/ParsingEngine.java

@@ -317,7 +317,7 @@ public class ParsingEngine {
                 // when the argument name is specified and the argument is not a flag type.
                 for(ArgumentMatch verifiableMatch: verifiableMatches) {
                     ArgumentSource argumentSource = argumentSourcesByDefinition.get(verifiableArgument);
-                    if(verifiableMatch.values().size() == 0 && !verifiableArgument.isFlag && argumentSource.createsTypeDefault())
+                    if(verifiableMatch.values().size() == 0 && !verifiableArgument.isFlag && !argumentSource.createsTypeDefault())
                         invalidValues.add(new Pair<ArgumentDefinition,String>(verifiableArgument,null));
                 }
 

public/gatk-tools-public/src/test/java/org/broadinstitute/gatk/utils/commandline/ParsingEngineUnitTest.java

@@ -40,6 +40,8 @@ import java.io.File;
 import java.io.IOException;
 import java.util.List;
 import java.util.EnumSet;
+import java.util.Set;
+
 /**
  * Test suite for the parsing engine.
  */
@@ -135,7 +137,7 @@ public class ParsingEngineUnitTest extends BaseTest {
         Assert.assertEquals(argProvider.foo, 5, "Argument is not correctly initialized");
     }
 
-    @Test(expectedExceptions=MissingArgumentValueException.class)
+    @Test(expectedExceptions=InvalidArgumentValueException.class)
     public void primitiveArgumentNoValueTest() {
         final String[] commandLine = new String[] {"--foo"};
 
@@ -948,6 +950,7 @@ public class ParsingEngineUnitTest extends BaseTest {
         }
     }
 
+    @SuppressWarnings("unused")
     private class NumericRangeArgProvider {
         @Argument(fullName = "intWithHardMinAndMax", minValue = 5, maxValue = 10)
         public int intWithHardMinAndMax;
@@ -1089,4 +1092,50 @@ public class ParsingEngineUnitTest extends BaseTest {
         NumericRangeArgProvider argProvider = new NumericRangeArgProvider();
         parsingEngine.loadArgumentsIntoObject(argProvider);
     }
+
+    @SuppressWarnings("unused")
+    private class VariedTypeArgProvider {
+        @Argument(fullName = "intVal", required=false)
+        private int anInt;
+
+        @Argument(fullName = "stringVal", required=false)
+        private String aString;
+
+        @Argument(fullName = "enumVal", required=false)
+        private TestEnum anEnum;
+
+        @Argument(fullName = "fileVal", required=false)
+        private File aFile;
+
+        @Argument(fullName = "stringSet", required=false)
+        private Set<String> someStrings;
+
+        @Argument(fullName = "intervalVal", required=false)
+        private IntervalBinding<Feature> anInterval;
+    }
+
+    @DataProvider(name = "MissingArgumentValueDataProvider")
+    public Object[][] missingArgumentDataProvider() {
+        return new Object[][]{
+                { new String[]{"--intVal"} },
+                { new String[]{"--stringVal"} },
+                { new String[]{"--enumVal"} },
+                { new String[]{"--fileVal"} },
+                { new String[]{"--stringSet"} },
+                { new String[]{"--stringSet", "aha", "--stringSet"} },
+                { new String[]{"--intervalVal"} }
+        };
+    }
+
+    @Test(dataProvider = "MissingArgumentValueDataProvider",
+          expectedExceptions = {InvalidArgumentValueException.class, MissingArgumentValueException.class})
+    public void testMissingArguments( final String[] commandLine ) {
+        parsingEngine.addArgumentSource( VariedTypeArgProvider.class );
+        parsingEngine.parse( commandLine );
+        parsingEngine.validate();
+
+        VariedTypeArgProvider argProvider = new VariedTypeArgProvider();
+        parsingEngine.loadArgumentsIntoObject(argProvider);
+    }
+
 }