diff --git a/src/main/java/org/broadinstitute/barclay/argparser/Argument.java b/src/main/java/org/broadinstitute/barclay/argparser/Argument.java index 5ef053d9..b08aaffb 100644 --- a/src/main/java/org/broadinstitute/barclay/argparser/Argument.java +++ b/src/main/java/org/broadinstitute/barclay/argparser/Argument.java @@ -64,6 +64,14 @@ */ boolean common() default false; + /** + * Suppress automatic expansion of files with names ending in recognized extensions + * ({@link CommandLineArgumentParser#COLLECTION_LIST_FILE_EXTENSIONS}) when such files are provided + * on the commandline for collection arguments. + * @return true if file expansion should be suppressed for this argument + */ + boolean suppressFileExpansion() default false; + /** * Does this option have special treatment in the argument parsing system. * Some examples are arguments_file and help, which have special behavior in the parser. diff --git a/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java b/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java index 88d1e33d..fd9433e6 100644 --- a/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java +++ b/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java @@ -57,8 +57,13 @@ public final class CommandLineArgumentParser implements CommandLineParser { public static final String COMMENT = "#"; public static final String POSITIONAL_ARGUMENTS_NAME = "Positional Argument"; - // Extension for collection argument list files - static final String COLLECTION_LIST_FILE_EXTENSION = ".args"; + /** + * Extensions recognized for collection argument file expansion. + */ + static final String COLLECTION_LIST_FILE_EXTENSIONS_LIST = ".list"; + static final String COLLECTION_LIST_FILE_EXTENSIONS_ARGS = ".args"; + static final Set COLLECTION_LIST_FILE_EXTENSIONS = new HashSet<>( + Arrays.asList(COLLECTION_LIST_FILE_EXTENSIONS_LIST, COLLECTION_LIST_FILE_EXTENSIONS_ARGS)); private static final Logger logger = LogManager.getLogger(); @@ -630,7 +635,7 @@ private void setArgument(ArgumentDefinition argumentDefinition, List val final Collection c = (Collection) argumentDefinition.getFieldValue(); c.clear(); } - values = expandListFile(values); + values = expandListFile(argumentDefinition, values); } for (int i = 0; i < values.size(); i++) { @@ -709,24 +714,34 @@ private void setArgument(ArgumentDefinition argumentDefinition, List val } /** - * Expand any collection values that are ".list" argument files, and add them - * to the list of values for that argument. - * @param originalValues + * Expand any collection value that references a filename that ends in one of the accepted expansion + * extensions, and add the contents of the file to the list of values for that argument. + * @param argDef the ArgumentDefinition being populated + * @param originalValues list of original values provided on the command line * @return a list containing the original entries in {@code originalValues}, with any * values from list files expanded in place, preserving both the original list order and * the file order */ - private List expandListFile(final List originalValues) { - List expandedValues = new ArrayList<>(originalValues.size()); - for (String stringValue: originalValues) { - if (stringValue.endsWith(COLLECTION_LIST_FILE_EXTENSION)) { - expandedValues.addAll(loadCollectionListFile(stringValue)); - } - else { - expandedValues.add(stringValue); + private List expandListFile(final ArgumentDefinition argDef, final List originalValues) { + if (!argDef.suppressFileExpansion) { + List expandedValues = new ArrayList<>(originalValues.size()); + for (final String stringValue: originalValues) { + if (COLLECTION_LIST_FILE_EXTENSIONS.stream().anyMatch(ext -> stringValue.endsWith(ext))) { + // If any value provided for this argument is an expansion file, expand it in place, + // but also 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)); + argDef.preserveOriginalValueList(originalValues); + } + else { + expandedValues.add(stringValue); + } } + return expandedValues; + } else { + return originalValues; } - return expandedValues; } /** @@ -1134,6 +1149,13 @@ public static class ArgumentDefinition { final Double minRecommendedValue; final boolean isHidden; final boolean isAdvanced; + final boolean suppressFileExpansion; + + // Override values to be used when displaying this argument as part of the command line. When argument + // file expansion is triggered, this is used to preserve the original values provided by the user so that + // these are displayed, rather than the expanded values, which may be large, and can consist of intermixed + // literal values and expansion file names. + private List originalCommandLineValues; public ArgumentDefinition( final Field field, @@ -1154,6 +1176,11 @@ public ArgumentDefinition( this.mutuallyExclusive = new LinkedHashSet<>(Arrays.asList(annotation.mutex())); this.controllingDescriptor = controllingDescriptor; + this.suppressFileExpansion = annotation.suppressFileExpansion(); + + if (suppressFileExpansion && !isCollection) { + throw new CommandLineException.CommandLineParserInternalException("Can only suppress file expansion for collection arguments"); + } Object tmpDefault = getFieldValue(); if (tmpDefault != null) { @@ -1261,7 +1288,7 @@ public String getLongName(){ } /** - * Comparator for sorting ArgumentDefinitions in alphabetical order b y longName + * Comparator for sorting ArgumentDefinitions in alphabetical order by longName */ public static Comparator sortByLongName = (argDef1, argDef2) -> String.CASE_INSENSITIVE_ORDER.compare(argDef1.getLongName(), argDef2.getLongName()); @@ -1270,7 +1297,6 @@ public String getLongName(){ * Helper for pretty printing this option. * @param value A value this argument was given * @return a string - * */ private String prettyNameValue(Object value) { if (value != null) { @@ -1288,6 +1314,15 @@ private String prettyNameValue(Object value) { return ""; } + /** + * Provide an override value to be used when when displaying this field as part of a command line. This is + * used to record the original value provided by the user when file expansion was used. + * @param commandLineDisplayValue + */ + void preserveOriginalValueList(final List commandLineDisplayValue) { + this.originalCommandLineValues = commandLineDisplayValue; + } + /** * @return A string representation of this argument and it's value(s) which would be valid if copied and pasted * back as a command line argument @@ -1295,16 +1330,16 @@ private String prettyNameValue(Object value) { public String toCommandLineString(){ final Object value = getFieldValue(); if (this.isCollection){ - Collection collect = (Collection)value; + Collection collect = originalCommandLineValues == null ? + (Collection)value : + originalCommandLineValues; return collect.stream() .map(this::prettyNameValue) .collect(Collectors.joining(" ")); - } else { return prettyNameValue(value); } } - } /** diff --git a/src/test/java/org/broadinstitute/barclay/argparser/CollectionArgumentUnitTests.java b/src/test/java/org/broadinstitute/barclay/argparser/CollectionArgumentUnitTests.java index 9b21bae5..7abda541 100644 --- a/src/test/java/org/broadinstitute/barclay/argparser/CollectionArgumentUnitTests.java +++ b/src/test/java/org/broadinstitute/barclay/argparser/CollectionArgumentUnitTests.java @@ -144,48 +144,145 @@ public void testInitializedCollections( } ////////////////////////////////////////////////////////////////// - // tests for .list files + // tests for .list/.args file expansion + + abstract class CollectionForListFileArguments { + abstract List getList1(); + abstract List getList2(); + } + + class EnabledListFileArguments extends CollectionForListFileArguments { - class CollectionForListFileArguments { @Argument public List LIST = makeList("foo", "bar"); @Argument public List LIST2 = makeList("baz"); + + + public List getList1() { return LIST; }; + public List getList2() { return LIST2; }; } - @DataProvider(name="listFileArguments") - public Object[][] listFileArguments() { - final String[] inputArgs = new String[] { "shmiggle0", "shmiggle1", "shmiggle2" }; + class SuppressedListFileArguments extends CollectionForListFileArguments { + + @Argument(suppressFileExpansion = true) + public List LIST = makeList("foo", "bar"); + + @Argument(suppressFileExpansion = true) + public List LIST2 = makeList("baz"); + + public List getList1() { return LIST; }; + public List getList2() { return LIST2; }; + } + + @DataProvider(name="expansionFileArguments") + public Object[][] expansionFileArguments() throws IOException { + final String[] expansionFileContents = new String[] { "shmiggle0", "shmiggle1", "shmiggle2" }; return new Object[][] { + + // enabled file expansion tests { // replace mode + new EnabledListFileArguments(), Collections.EMPTY_SET, // parser options - inputArgs, // args + createTemporaryExpansionFile("enabledExpansion", + CommandLineArgumentParser.COLLECTION_LIST_FILE_EXTENSIONS_ARGS),// expansion file + expansionFileContents, // args for expansion file new String[] {"shmiggle0", "shmiggle1", "shmiggle2"}, // expected result + false // expect temp file name }, { // append mode + new EnabledListFileArguments(), Collections.singleton(CommandLineParserOptions.APPEND_TO_COLLECTIONS), // parser options - inputArgs, // args + createTemporaryExpansionFile("enabledExpansion", + CommandLineArgumentParser.COLLECTION_LIST_FILE_EXTENSIONS_ARGS),// expansion file + expansionFileContents, // args for expansion file new String[] {"foo", "bar", "shmiggle0", "shmiggle1", "shmiggle2"}, // expected result + false // expect temp file name + }, + { + // replace mode + new EnabledListFileArguments(), + Collections.EMPTY_SET, // parser options + createTemporaryExpansionFile("enabledExpansion", + CommandLineArgumentParser.COLLECTION_LIST_FILE_EXTENSIONS_LIST),// expansion file + expansionFileContents, // args for expansion file + new String[] {"shmiggle0", "shmiggle1", "shmiggle2"}, // expected result + false // expect temp file name + }, + { + // append mode + new EnabledListFileArguments(), + Collections.singleton(CommandLineParserOptions.APPEND_TO_COLLECTIONS), // parser options + createTemporaryExpansionFile("enabledExpansion", + CommandLineArgumentParser.COLLECTION_LIST_FILE_EXTENSIONS_LIST),// expansion file + expansionFileContents, // args for expansion file + new String[] {"foo", "bar", "shmiggle0", "shmiggle1", "shmiggle2"}, // expected result + false // expect temp file name + }, + + // suppressed file expansion file tests + { + // replace mode + new SuppressedListFileArguments(), + Collections.EMPTY_SET, // parser options + createTemporaryExpansionFile("enabledExpansion", + CommandLineArgumentParser.COLLECTION_LIST_FILE_EXTENSIONS_ARGS),// expansion file + expansionFileContents, // args for expansion file + new String[] {}, // expected result + true, + }, + { + // append mode + new SuppressedListFileArguments(), + Collections.singleton(CommandLineParserOptions.APPEND_TO_COLLECTIONS), // parser options + createTemporaryExpansionFile("enabledExpansion", + CommandLineArgumentParser.COLLECTION_LIST_FILE_EXTENSIONS_ARGS),// expansion file + expansionFileContents, // args for expansion file + new String[] {"foo", "bar"}, // expected result + true, + }, + { + // replace mode + new SuppressedListFileArguments(), + Collections.EMPTY_SET, // parser options + createTemporaryExpansionFile("enabledExpansion", + CommandLineArgumentParser.COLLECTION_LIST_FILE_EXTENSIONS_LIST),// expansion file + expansionFileContents, // args for expansion file + new String[] {}, // expected result + true, + }, + { + // append mode + new SuppressedListFileArguments(), + Collections.singleton(CommandLineParserOptions.APPEND_TO_COLLECTIONS), // parser options + createTemporaryExpansionFile("enabledExpansion", + CommandLineArgumentParser.COLLECTION_LIST_FILE_EXTENSIONS_LIST),// expansion file + expansionFileContents, // args for expansion file + new String[] {"foo", "bar"}, // expected result + true, }, }; } - // Test that .list files populate collections with file contents, both mpdes - @Test(dataProvider="listFileArguments") - public void testCollectionFromListFile( + // Test that .list and .args files populate collections with file contents, both parser modes + @Test(dataProvider="expansionFileArguments") + public void testCollectionFromExpansionFile( + final CollectionForListFileArguments argumentContainer, final Set parserOptions, - final String [] argList, - final String[] expectedList) throws IOException + final File argListFile, + final String [] expansionFileContents, + final String[] expectedList, + final boolean expectFileNameInOutput // true when expansion is suppressed + ) throws IOException { - final File argListFile = createListArgumentFile("argListFile", argList); + populateExpansionFile(argListFile, expansionFileContents); // use a single file argument - final CollectionForListFileArguments o = new CollectionForListFileArguments(); final CommandLineParser clp = new CommandLineArgumentParser( - o, + argumentContainer, Collections.emptyList(), parserOptions ); @@ -193,7 +290,17 @@ public void testCollectionFromListFile( final String[] args = {"--LIST", argListFile.getAbsolutePath()}; Assert.assertTrue(clp.parseArguments(System.err, args)); - Assert.assertEquals(o.LIST, makeList(expectedList)); + // if the results are expected to include the file name because expansion was suppressed, + // add the filename to the list of expected outputs + final List actualResult = argumentContainer.getList1(); + final List expectedResult; + if (expectFileNameInOutput) { + expectedResult = makeList(expectedList); + expectedResult.add(argListFile.getAbsolutePath()); + } else { + expectedResult = makeList(expectedList); + } + Assert.assertEquals(argumentContainer.getList1(), expectedResult); } @DataProvider(name="mixedListFileArguments") @@ -203,6 +310,7 @@ public Object[][] mixedListFileArguments() { return new Object[][] { { // replace mode + new EnabledListFileArguments(), Collections.EMPTY_SET, // parser options inputArgList1, // args list 1 inputArgList2, // args list 2 @@ -217,6 +325,7 @@ public Object[][] mixedListFileArguments() { }, { // append mode + new EnabledListFileArguments(), Collections.singleton(CommandLineParserOptions.APPEND_TO_COLLECTIONS), // parser options inputArgList1, // args list 1 inputArgList2, // args list 2 @@ -233,9 +342,10 @@ public Object[][] mixedListFileArguments() { }; } - // Test that .list files intermixed with explicit command line values populate collections correctly, both mpdes + // Test that .list files intermixed with explicit command line values populate collections correctly, both modes @Test(dataProvider="mixedListFileArguments") public void testCollectionFromListFileMixed( + final CollectionForListFileArguments argumentContainer, final Set parserOptions, final String [] argList1, final String [] argList2, @@ -244,11 +354,12 @@ public void testCollectionFromListFileMixed( ) throws IOException { // use two file arguments - File listFile = createListArgumentFile("testFile1", argList1); - File listFile2 = createListArgumentFile("testFile2", argList2); - final CollectionForListFileArguments o = new CollectionForListFileArguments(); + final File listFile = createTemporaryExpansionFile("testFile1", CommandLineArgumentParser.COLLECTION_LIST_FILE_EXTENSIONS_ARGS); + populateExpansionFile(listFile, argList1); + final File listFile2 = createTemporaryExpansionFile("testFile2", CommandLineArgumentParser.COLLECTION_LIST_FILE_EXTENSIONS_LIST); + populateExpansionFile(listFile2, argList2); final CommandLineParser clp = new CommandLineArgumentParser( - o, + argumentContainer, Collections.emptyList(), parserOptions ); @@ -261,8 +372,36 @@ public void testCollectionFromListFileMixed( "--LIST2", "anotherCommandLineValue"}; Assert.assertTrue(clp.parseArguments(System.err, args)); - Assert.assertEquals(o.LIST, makeList(expectedList1)); - Assert.assertEquals(o.LIST2, makeList(expectedList2)); + Assert.assertEquals(argumentContainer.getList1(), makeList(expectedList1)); + Assert.assertEquals(argumentContainer.getList2(), makeList(expectedList2)); + } + + @Test + public void testGetCommandLineWithExpansionFile() throws IOException{ + final File expansionFile = createTemporaryExpansionFile("expansionCommandLine", ".txt"); + // mix command line values and file values + final String[] args = new String[] { + "--LIST", "commandLineValue", + "--LIST", expansionFile.getAbsolutePath(), + "--LIST2", "anotherCommandLineValue"}; + + final EnabledListFileArguments fo = new EnabledListFileArguments(); + final CommandLineArgumentParser clp = new CommandLineArgumentParser(fo); + Assert.assertTrue(clp.parseArguments(System.err, args)); + final String expectedCommandLine = + String.format("EnabledListFileArguments --LIST commandLineValue --LIST %s --LIST2 anotherCommandLineValue ", expansionFile.getAbsolutePath()); + Assert.assertEquals(clp.getCommandLine(), expectedCommandLine); + } + + class NonCollectionFileExpansionSuppression { + @Argument(suppressFileExpansion = true) + public int bogusFileExpansionArg; + } + + @Test(expectedExceptions = CommandLineException.CommandLineParserInternalException.class) + public void testRejectNonCollectionFileExpansionSuppression() { + final NonCollectionFileExpansionSuppression fo = new NonCollectionFileExpansionSuppression(); + final CommandLineArgumentParser clp = new CommandLineArgumentParser(fo); } class UninitializedCollectionThatCannotBeAutoInitializedArguments { @@ -280,13 +419,17 @@ public void testCollectionThatCannotBeAutoInitialized() { ////////////////////////////////////////////////////////////////// // Helper methods - private File createListArgumentFile(final String fileName, final String[] argList) throws IOException { - final File listFile = File.createTempFile(fileName, CommandLineArgumentParser.COLLECTION_LIST_FILE_EXTENSION); + private File createTemporaryExpansionFile(final String fileNameRoot, final String expansionExtension) throws IOException { + final File listFile = File.createTempFile(fileNameRoot, expansionExtension); listFile.deleteOnExit(); - try (final PrintWriter writer = new PrintWriter(listFile)) { + return listFile; + } + + private File populateExpansionFile(final File expansionFile, final String[] argList) throws IOException { + try (final PrintWriter writer = new PrintWriter(expansionFile)) { Arrays.stream(argList).forEach(arg -> writer.println(arg)); } - return listFile; + return expansionFile; } public static List makeList(final String... list) {