Warn on file expansion for files with strings and leading . (#161)

broadinstitute · Mar 9, 2020 · 2441209 · 2441209
1 parent 0abd84d
commit 2441209
Show file tree

Hide file tree

Showing 5 changed files with 40 additions and 13 deletions.
diff --git a/src/main/java/org/broadinstitute/barclay/argparser/ArgumentDefinition.java b/src/main/java/org/broadinstitute/barclay/argparser/ArgumentDefinition.java
@@ -2,6 +2,7 @@
 
 import org.broadinstitute.barclay.utils.Utils;
 
+import java.io.PrintStream;
 import java.lang.reflect.*;
 import java.util.*;
 import java.util.stream.Collectors;
@@ -88,12 +89,14 @@ public Object getArgumentValue() {
      *
      * @param commandLineArgumentParser the {@code CommandLineArgumentParser} managing this argument, used to resolve
      *                                  tag surrogates
+     * @param messageStream output stream where error messages should be written
      * @param preprocessedValues the values to be used to populate the argument. these values may be tag surrogates
      *                           created by the {@link TaggedArgumentParser} that must be resolved using {@link
      *                           TaggedArgumentParser#getTaggedOptionForSurrogate(String)}.
      */
     public abstract void setArgumentValues(
             final CommandLineArgumentParser commandLineArgumentParser,
+            final PrintStream messageStream,
             final List<String> preprocessedValues);
 
     /**

diff --git a/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java b/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java
@@ -163,7 +163,7 @@ public boolean parseArguments(final PrintStream messageStream, String[] args) {
                 return parseArguments(messageStream, newArgs.toArray(new String[newArgs.size()]));
             }
         }
-        return propagateParsedValues(parsedArguments);
+        return propagateParsedValues(parsedArguments, messageStream);
     }
 
     /**
@@ -477,14 +477,15 @@ public String usage(final boolean printCommon, final boolean printHidden) {
         return sb.toString();
     }
 
-    private boolean propagateParsedValues(final OptionSet parsedArguments) {
+    private boolean propagateParsedValues(final OptionSet parsedArguments, final PrintStream messageStream) {
         // named args first
         for (final OptionSpec<?> optSpec : parsedArguments.asMap().keySet()) {
             if (parsedArguments.has(optSpec)) {
                 final NamedArgumentDefinition namedArgumentDefinition = namedArgumentsDefinitionsByAlias.get(optSpec.options().get(0));
                 // Note that these values can be preprocessed tag surrogates
                 namedArgumentDefinition.setArgumentValues(
                         this,
+                        messageStream,
                         optSpec.values(parsedArguments)
                             .stream()
                             .map(Object::toString)
@@ -503,7 +504,7 @@ private boolean propagateParsedValues(final OptionSet parsedArguments) {
                         "Positional arguments were provided '%s' but no positional argument is defined for this tool.",
                         stringValues.stream().collect(Collectors.joining("{", ",","}"))));
             }
-            positionalArgumentDefinition.setArgumentValues(this, stringValues);
+            positionalArgumentDefinition.setArgumentValues(this, messageStream, stringValues);
         }
 
         validateArgumentValues();
@@ -625,6 +626,7 @@ private List<NamedArgumentDefinition> validatePluginArgumentValues() {
      */
     public List<String> expandFromExpansionFile(
             final ArgumentDefinition argumentDefinition,
+            final PrintStream messageStream,
             final String stringValue,
             final List<String> originalValuesForPreservation) {
         List<String> expandedValues = new ArrayList<>();
@@ -633,7 +635,7 @@ public List<String> expandFromExpansionFile(
             // but  preserve the original values for subsequent retrieval during command line
             // display, since expansion files can result in very large post-expansion command lines
             // (its harmless to update this multiple times).
-            expandedValues.addAll(loadCollectionListFile(stringValue));
+            expandedValues.addAll(loadCollectionListFile(stringValue, messageStream));
             argumentDefinition.setOriginalCommandLineValues(originalValuesForPreservation);
         } else {
             expandedValues.add(stringValue);
@@ -648,13 +650,25 @@ public List<String> expandFromExpansionFile(
      * @param collectionListFile a text file containing list values
      * @return false if a fatal error occurred
      */
-    private static List<String> loadCollectionListFile(final String collectionListFile) {
+    private static List<String> loadCollectionListFile(
+            final String collectionListFile,
+            final PrintStream messageStream) {
         try (BufferedReader reader = new BufferedReader(new FileReader(collectionListFile))){
-            return reader.lines()
+            final List<String> filteredStrings = reader.lines()
                     .map(String::trim)
                     .filter(line -> !line.isEmpty())
                     .filter(line -> !line.startsWith(ARGUMENT_FILE_COMMENT))
                     .collect(Collectors.toList());
+            final List<String> suspiciousString = filteredStrings.stream().filter(s -> s.startsWith("@")).limit(1).collect(Collectors.toList());
+            if (!suspiciousString.isEmpty()) {
+                // looks suspiciously like a sequence dictionary...
+                messageStream.println(String.format(
+                        "WARNING: the file %s has a file extension that causes it to be expanded by the argument parser into multiple argument values , " +
+                        "but contains lines with leading '@' characters that may indicate this was unintentional (%s).",
+                        collectionListFile,
+                        suspiciousString));
+            }
+            return filteredStrings;
         } catch (final IOException e) {
             throw new CommandLineException("I/O error loading list file:" + collectionListFile, e);
         }

diff --git a/src/main/java/org/broadinstitute/barclay/argparser/NamedArgumentDefinition.java b/src/main/java/org/broadinstitute/barclay/argparser/NamedArgumentDefinition.java
@@ -5,6 +5,7 @@
 import org.apache.logging.log4j.Logger;
 import org.broadinstitute.barclay.utils.Utils;
 
+import java.io.PrintStream;
 import java.lang.reflect.Field;
 import java.util.*;
 import java.util.stream.Collectors;
@@ -283,12 +284,13 @@ public boolean isControlledByPlugin() {
     @Override
     public void setArgumentValues(
             final CommandLineArgumentParser commandLineArgumentParser,
+            final PrintStream messageStream,
             final List<String> preprocessedValues) // Note these might be tag surrogates
     {
         if (isCollection()) {
-            setCollectionValues(commandLineArgumentParser, preprocessedValues);
+            setCollectionValues(commandLineArgumentParser, messageStream, preprocessedValues);
         } else {
-            setScalarValue(commandLineArgumentParser, preprocessedValues);
+            setScalarValue(commandLineArgumentParser, messageStream, preprocessedValues);
         }
         hasBeenSet = true;
     }
@@ -298,6 +300,7 @@ public void setArgumentValues(
     @SuppressWarnings({"rawtypes", "unchecked"})
     private void setCollectionValues(
             final CommandLineArgumentParser commandLineArgumentParser,
+            final PrintStream messageStream,
             final List<String> preprocessedValues) // Note that some of these might be tag surrogates
     {
         final Collection c = (Collection) getArgumentValue();
@@ -330,6 +333,7 @@ private void setCollectionValues(
                         Collections.singletonList(normalizedSurrogatePair.getRight()) :
                         commandLineArgumentParser.expandFromExpansionFile(
                                 this,
+                                messageStream,
                                 normalizedSurrogatePair.getRight(),
                                 preprocessedValues);
 
@@ -350,6 +354,7 @@ private void setCollectionValues(
     // one value, in which case we throw.
     private void setScalarValue(
             final CommandLineArgumentParser commandLineArgumentParser,
+            final PrintStream messageStream,
             final List<String> originalValues) // Note that these might be tag surrogates
     {
         if (getHasBeenSet() || originalValues.size() > 1) {

diff --git a/src/main/java/org/broadinstitute/barclay/argparser/PositionalArgumentDefinition.java b/src/main/java/org/broadinstitute/barclay/argparser/PositionalArgumentDefinition.java
@@ -2,6 +2,7 @@
 
 import org.broadinstitute.barclay.utils.Utils;
 
+import java.io.PrintStream;
 import java.lang.reflect.Field;
 import java.util.Collection;
 import java.util.List;
@@ -64,11 +65,16 @@ public String getCommandLineDisplayString() {
     @SuppressWarnings("unchecked")
     public void setArgumentValues(
             final CommandLineArgumentParser commandLineArgumentParser,
+            final PrintStream messageStream,
             final List<String> stringValues)
     {
         final List<String> expandedValues = stringValues
                 .stream()
-                .flatMap(s -> commandLineArgumentParser.expandFromExpansionFile(this, s, stringValues).stream())
+                .flatMap(s -> commandLineArgumentParser.expandFromExpansionFile(
+                        this,
+                        messageStream,
+                        s,
+                        stringValues).stream())
                 .collect(Collectors.toList());
         for (final String stringValue : expandedValues) {
             final Object value = constructFromString(stringValue, POSITIONAL_ARGUMENTS_NAME);

diff --git a/...test/java/org/broadinstitute/barclay/argparser/PositionalArgumentsDefinitionUnitTest.java b/...test/java/org/broadinstitute/barclay/argparser/PositionalArgumentsDefinitionUnitTest.java
@@ -6,7 +6,6 @@
 
 import java.lang.reflect.Field;
 import java.util.Arrays;
-import java.util.Collections;
 import java.util.List;
 
 public class PositionalArgumentsDefinitionUnitTest {
@@ -22,7 +21,7 @@ class MinElements {
         final PositionalArgumentDefinition def =
                 new PositionalArgumentDefinition(field.getAnnotation(PositionalArguments.class), o, field);
         final CommandLineArgumentParser clp = new CommandLineArgumentParser(o);
-        def.setArgumentValues(clp, Arrays.asList(new String[] { "arg1"}));
+        def.setArgumentValues(clp, System.out, Arrays.asList(new String[] { "arg1"}));
 
         // we only validate minimum number of arguments on validation, after we've seen all of the values
         def.validateValues(clp);
@@ -41,7 +40,7 @@ class MaxElements {
         final CommandLineArgumentParser clp = new CommandLineArgumentParser(o);
 
         // exceeding the maximum number of arguments is validated when args are added
-        def.setArgumentValues(clp, Arrays.asList(new String[] { "arg1", "arg2", "arg3"}));
+        def.setArgumentValues(clp, System.out, Arrays.asList(new String[] { "arg1", "arg2", "arg3"}));
     }
 
     @Test(expectedExceptions = CommandLineException.CommandLineParserInternalException.class)
@@ -85,7 +84,7 @@ class MinAndMaxElements {
         final Field field = getFieldForFieldName(o, "stringArg");
         final PositionalArgumentDefinition def =
                 new PositionalArgumentDefinition(field.getAnnotation(PositionalArguments.class), o, field);
-        def.setArgumentValues(new CommandLineArgumentParser(o), Arrays.asList(args));
+        def.setArgumentValues(new CommandLineArgumentParser(o), System.out, Arrays.asList(args));
         Assert.assertEquals(def.getCommandLineDisplayString(), expecteDisplayString);
     }