From f42fa6f865bcb3aec681759c6217af36794fcc18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20G=C3=B3mez-S=C3=A1nchez?= Date: Wed, 11 Oct 2017 11:06:00 +0200 Subject: [PATCH 1/3] Throw CommandLineParserInternalException for immutable collections --- .../argparser/CommandLineArgumentParser.java | 16 +++++++++++----- .../argparser/CollectionArgumentUnitTests.java | 11 +++++++++++ 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java b/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java index 1c32f2db..c83386b9 100644 --- a/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java +++ b/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java @@ -684,11 +684,17 @@ private void setArgument(ArgumentDefinition argumentDefinition, List val if (argumentDefinition.isCollection) { @SuppressWarnings("rawtypes") final Collection c = (Collection) argumentDefinition.getFieldValue(); - if (value == null) { - //user specified this arg=null which is interpreted as empty list - c.clear(); - } else { - c.add(value); + try { + if (value == null) { + //user specified this arg=null which is interpreted as empty list + c.clear(); + } else { + c.add(value); + } + } catch (UnsupportedOperationException e) { + throw new CommandLineException.CommandLineParserInternalException(String.format("Collection arguments seems immutable: \"%s\". Initialized collection should support the clear and add methods, but %s does not.", + argumentDefinition.getLongName(), + c.getClass().getName())); } argumentDefinition.hasBeenSet = true; } else { diff --git a/src/test/java/org/broadinstitute/barclay/argparser/CollectionArgumentUnitTests.java b/src/test/java/org/broadinstitute/barclay/argparser/CollectionArgumentUnitTests.java index 9b21bae5..16252abd 100644 --- a/src/test/java/org/broadinstitute/barclay/argparser/CollectionArgumentUnitTests.java +++ b/src/test/java/org/broadinstitute/barclay/argparser/CollectionArgumentUnitTests.java @@ -277,6 +277,17 @@ public void testCollectionThatCannotBeAutoInitialized() { new CommandLineArgumentParser(o); } + class ImmutableCollectionArguments { + @Argument + public List LIST = Collections.emptyList(); + } + + @Test(expectedExceptions = CommandLineException.CommandLineParserInternalException.class) + public void testCollectionThatIsImmmutable() { + final ImmutableCollectionArguments o = new ImmutableCollectionArguments(); + new CommandLineArgumentParser(o).parseArguments(System.err, new String[]{"--LIST", "A"}); + } + ////////////////////////////////////////////////////////////////// // Helper methods From d0a4dcfe0c6204600912d64efc4ce7a3de5cdd8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20G=C3=B3mez-S=C3=A1nchez?= Date: Mon, 30 Oct 2017 15:00:47 +0100 Subject: [PATCH 2/3] Address comments --- .../argparser/CommandLineArgumentParser.java | 50 +++++++++++-------- .../CollectionArgumentUnitTests.java | 29 +++++++++-- 2 files changed, 53 insertions(+), 26 deletions(-) diff --git a/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java b/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java index c83386b9..f0198aa2 100644 --- a/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java +++ b/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java @@ -15,6 +15,7 @@ import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; import java.util.*; +import java.util.function.Consumer; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -616,9 +617,8 @@ private void setArgument(ArgumentDefinition argumentDefinition, List val // if this is a collection then we only want to clear it once at the beginning, before we process // any of the values, unless we're in APPEND_TO_COLLECTIONS mode, in which case we leave the initial // and append to it - @SuppressWarnings("rawtypes") - final Collection c = (Collection) argumentDefinition.getFieldValue(); - c.clear(); + System.err.println("Clear collection"); + applyToCollection(argumentDefinition, Collection::clear); } values = expandListFile(values); } @@ -680,27 +680,35 @@ private void setArgument(ArgumentDefinition argumentDefinition, List val // check the argument range checkArgumentRange(argumentDefinition, value); + // set the already parsed argument + setParsedArgumentValue(argumentDefinition, value); + } + } - if (argumentDefinition.isCollection) { - @SuppressWarnings("rawtypes") - final Collection c = (Collection) argumentDefinition.getFieldValue(); - try { - if (value == null) { - //user specified this arg=null which is interpreted as empty list - c.clear(); - } else { - c.add(value); - } - } catch (UnsupportedOperationException e) { - throw new CommandLineException.CommandLineParserInternalException(String.format("Collection arguments seems immutable: \"%s\". Initialized collection should support the clear and add methods, but %s does not.", - argumentDefinition.getLongName(), - c.getClass().getName())); - } - argumentDefinition.hasBeenSet = true; + private void setParsedArgumentValue(final ArgumentDefinition argumentDefinition, final Object value) { + if (argumentDefinition.isCollection) { + if (value == null) { + applyToCollection(argumentDefinition, Collection::clear); } else { - argumentDefinition.setFieldValue(value); - argumentDefinition.hasBeenSet = true; + applyToCollection(argumentDefinition, c -> c.add(value)); } + argumentDefinition.hasBeenSet = true; + } else { + argumentDefinition.setFieldValue(value); + argumentDefinition.hasBeenSet = true; + } + } + + // apply a function to a collection, catching UnsuportedOperationException into a more informative + private void applyToCollection(final ArgumentDefinition argumentDefinitionCollection, final Consumer function) { + + final Collection c = (Collection) argumentDefinitionCollection.getFieldValue(); + try { + function.accept(c); + } catch (UnsupportedOperationException e) { + throw new CommandLineException.CommandLineParserInternalException(String.format("Collection arguments seems immutable: \"%s\". Initialized collection should support the clear and add methods, but %s does not.", + argumentDefinitionCollection.getLongName(), + c.getClass().getName())); } } diff --git a/src/test/java/org/broadinstitute/barclay/argparser/CollectionArgumentUnitTests.java b/src/test/java/org/broadinstitute/barclay/argparser/CollectionArgumentUnitTests.java index 16252abd..6613c2a9 100644 --- a/src/test/java/org/broadinstitute/barclay/argparser/CollectionArgumentUnitTests.java +++ b/src/test/java/org/broadinstitute/barclay/argparser/CollectionArgumentUnitTests.java @@ -278,14 +278,33 @@ public void testCollectionThatCannotBeAutoInitialized() { } class ImmutableCollectionArguments { - @Argument - public List LIST = Collections.emptyList(); + @Argument(optional = true) + public List LIST = Arrays.asList("T"); } - @Test(expectedExceptions = CommandLineException.CommandLineParserInternalException.class) - public void testCollectionThatIsImmmutable() { + @DataProvider + public Object[][] argsForImmmutableCollectionTest() { + return new Object[][] { + // replace mode + {Collections.EMPTY_SET, // parser options + new String[]{"--LIST", "A"}}, // arguments (real) + // replace mode + {Collections.EMPTY_SET, // parser options + new String[]{"--LIST", "null"}}, // arguments (null) + + // append mode + {Collections.singleton(CommandLineParserOptions.APPEND_TO_COLLECTIONS), // parser options + new String[]{"--LIST", "A"}}, // arguments (real) + // append mode + {Collections.singleton(CommandLineParserOptions.APPEND_TO_COLLECTIONS), // parser options + new String[]{"--LIST", "null"}}, // arguments (null) + }; + } + + @Test(dataProvider = "argsForImmmutableCollectionTest", expectedExceptions = CommandLineException.CommandLineParserInternalException.class) + public void testCollectionThatIsImmmutable(final Set opts, final String[] args) { final ImmutableCollectionArguments o = new ImmutableCollectionArguments(); - new CommandLineArgumentParser(o).parseArguments(System.err, new String[]{"--LIST", "A"}); + new CommandLineArgumentParser(o, Collections.emptyList(), opts).parseArguments(System.err, args); } ////////////////////////////////////////////////////////////////// From 4f2e0c736d9400b1181d97e746a9234990c80fc4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20G=C3=B3mez-S=C3=A1nchez?= Date: Tue, 31 Oct 2017 17:12:23 +0100 Subject: [PATCH 3/3] Fix build by suppressing warnings --- .../barclay/argparser/CommandLineArgumentParser.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java b/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java index f0198aa2..5ba2c1c5 100644 --- a/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java +++ b/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java @@ -600,7 +600,7 @@ private void setPositionalArgument(final String stringValue) { c.add(value); } - @SuppressWarnings("unchecked") + @SuppressWarnings({"unchecked", "rawtypes"}) private void setArgument(ArgumentDefinition argumentDefinition, List values) { //special treatment for flags if (argumentDefinition.isFlag() && values.isEmpty()){ @@ -685,6 +685,7 @@ private void setArgument(ArgumentDefinition argumentDefinition, List val } } + @SuppressWarnings({"unchecked", "rawtypes"}) private void setParsedArgumentValue(final ArgumentDefinition argumentDefinition, final Object value) { if (argumentDefinition.isCollection) { if (value == null) { @@ -700,6 +701,7 @@ private void setParsedArgumentValue(final ArgumentDefinition argumentDefinition, } // apply a function to a collection, catching UnsuportedOperationException into a more informative + @SuppressWarnings("rawtypes") private void applyToCollection(final ArgumentDefinition argumentDefinitionCollection, final Consumer function) { final Collection c = (Collection) argumentDefinitionCollection.getFieldValue();