Skip to content

Commit

Permalink
[#1159][#1162] Fix bug where abbreviated options were not matched whe…
Browse files Browse the repository at this point in the history 
…n value attached with '=' separator

Closes #1162
  • Loading branch information
@remkop
remkop committed Aug 31, 2020
1 parent 066ab44 commit fe01f27
Show file tree
Hide file tree
Showing 4 changed files with 110 additions and 79 deletions.
1 change: 1 addition & 0 deletions RELEASE-NOTES.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ Picocli follows [semantic versioning](http://semver.org/).


## <a name="4.5.2-fixes"></a> Fixed issues
* [#1162] Bugfix: Abbreviated options are not matched if value attached with '=' separator (like `-x=3`). Thanks to [Chris Laprun](https://github.com/metacosm) for raising this.
* [#1158] DOC: Fix broken links to GraalVM repo. Thanks to [Andreas Deininger](https://github.com/deining) for the pull request.
* [#1155] DOC: Fix sample code in chapter "Validation". Thanks to [Andreas Deininger](https://github.com/deining) for the pull request.
* [#1157] DOC: Fix typo "a argument group" in user manual. Thanks to sabrina for raising this.
Expand Down
23 changes: 8 additions & 15 deletions src/main/java/picocli/CommandLine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12526,11 +12526,7 @@ private void processArguments(List<CommandLine> parsedCommands,

// if we find another command, we are done with the current command
if (commandSpec.parser().abbreviatedSubcommandsAllowed()) {
try {
arg = AbbreviationMatcher.match(commandSpec.subcommands().keySet(), arg, commandSpec.subcommandsCaseInsensitive());
} catch (IllegalArgumentException ex) {
throw new ParameterException(CommandLine.this, "Error: " + ex.getMessage());
}
arg = AbbreviationMatcher.match(commandSpec.subcommands().keySet(), arg, commandSpec.subcommandsCaseInsensitive(), CommandLine.this);
}
if (commandSpec.subcommands().containsKey(arg)) {
CommandLine subcommand = commandSpec.subcommands().get(arg);
Expand All @@ -12553,21 +12549,17 @@ private void processArguments(List<CommandLine> parsedCommands,
// A single option may be without option parameters, like "-v" or "--verbose" (a boolean value),
// or an option may have one or more option parameters.
// A parameter may be attached to the option.
Set<String> aggregatedOptionNames = new LinkedHashSet<String>();
if (commandSpec.parser().abbreviatedOptionsAllowed()) {
Set<String> aggregatedOptionNames = new LinkedHashSet<String>();
aggregatedOptionNames.addAll(commandSpec.optionsMap().keySet());
aggregatedOptionNames.addAll(commandSpec.negatedOptionsMap().keySet());
Iterator<String> iterator = aggregatedOptionNames.iterator();
try {
arg = AbbreviationMatcher.match(aggregatedOptionNames, arg, commandSpec.optionsCaseInsensitive());
} catch (IllegalArgumentException ex) {
throw new ParameterException(CommandLine.this, "Error: " + ex.getMessage());
}
arg = AbbreviationMatcher.match(aggregatedOptionNames, arg, commandSpec.optionsCaseInsensitive(), CommandLine.this);
}
LookBehind lookBehind = LookBehind.SEPARATE;
int separatorIndex = arg.indexOf(separator);
if (separatorIndex > 0) {
String key = arg.substring(0, separatorIndex);
key = AbbreviationMatcher.match(aggregatedOptionNames, key, commandSpec.optionsCaseInsensitive(), CommandLine.this); //#1159, #1162
// be greedy. Consume the whole arg as an option if possible.
if (isStandaloneOption(key) && isStandaloneOption(arg)) {
tracer.warn("Both '%s' and '%s' are valid option names in %s. Using '%s'...%n", arg, key, getCommandName(), arg);
Expand Down Expand Up @@ -17509,8 +17501,9 @@ private static String makeCanonical(String str) {
return str;
}

public static String match(Set<String> set, String abbreviation, boolean caseInsensitive) {
if (set.contains(abbreviation)) { // return exact match
/** Returns the non-abbreviated name if found, otherwise returns the specified original abbreviation value. */
public static String match(Set<String> set, String abbreviation, boolean caseInsensitive, CommandLine source) {
if (set.contains(abbreviation) || set.isEmpty()) { // return exact match
return abbreviation;
}
List<String> abbreviatedKeyChunks = splitIntoChunks(abbreviation, caseInsensitive);
Expand All @@ -17523,7 +17516,7 @@ public static String match(Set<String> set, String abbreviation, boolean caseIns
}
if (candidates.size() > 1) {
String str = candidates.toString();
throw new IllegalArgumentException("'" + abbreviation + "' is not unique: it matches '" +
throw new ParameterException(source, "Error: '" + abbreviation + "' is not unique: it matches '" +
str.substring(1, str.length() - 1).replace(", ", "', '") + "'");
}
return candidates.isEmpty() ? abbreviation : candidates.get(0); // return the original if no match found
Expand Down
153 changes: 89 additions & 64 deletions src/test/java/picocli/AbbreviationMatcherTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package picocli;

import org.junit.Ignore;
import org.junit.Rule;
import org.junit.Test;
import org.junit.contrib.java.lang.system.ProvideSystemProperty;
Expand All @@ -10,11 +9,19 @@
import java.io.ByteArrayOutputStream;
import java.io.PrintStream;
import java.io.PrintWriter;
import java.util.*;
import java.util.Arrays;
import java.util.LinkedHashSet;
import java.util.Set;

import static org.junit.Assert.*;
import static picocli.CommandLine.*;
import static picocli.CommandLine.AbbreviationMatcher.*;
import static picocli.CommandLine.AbbreviationMatcher.match;
import static picocli.CommandLine.AbbreviationMatcher.splitIntoChunks;
import static picocli.CommandLine.Command;
import static picocli.CommandLine.Model;
import static picocli.CommandLine.Option;
import static picocli.CommandLine.ParameterException;
import static picocli.CommandLine.ParseResult;
import static picocli.CommandLine.UnmatchedArgumentException;

public class AbbreviationMatcherTest {

Expand Down Expand Up @@ -42,58 +49,59 @@ private Set<String> createSet() {
@Test
public void testPrefixMatch() {
Set<String> set = createSet();

assertEquals("kebab-case", match(set, "kebab-case", false));
assertEquals("kebab-case-extra", match(set, "kebab-case-extra", false));
assertEquals("very-long-kebab-case", match(set, "very-long-kebab-case", false));
assertEquals("very-long-kebab-case", match(set, "v-l-k-c", false));
assertEquals("very-long-kebab-case", match(set, "vLKC", false));
assertEquals("camelCase", match(set, "camelCase", false));
assertEquals("camelCase", match(set, "cC", false));
assertEquals("camelCase", match(set, "c-c", false));
assertEquals("camelCase", match(set, "camC", false));
assertEquals("camelCase", match(set, "camel", false));
assertEquals("camelCase", match(set, "ca", false));
assertEquals("super-long-option", match(set, "s-l-o", false));
assertEquals("super-long-option", match(set, "s-long-opt", false));
assertEquals("super-long-option", match(set, "super", false));
assertEquals("super-long-option", match(set, "sup", false));
assertEquals("super-long-option", match(set, "sup-long", false));
assertNotEquals("super-long-option", match(set, "SLO", false));
assertEquals ("super-long-option", match(set, "sLO", false));
assertEquals("super-long-option", match(set, "s-opt", false));
assertEquals("veryLongCamelCase", match(set, "veryLongCamelCase", false));
assertEquals("veryLongCamelCase", match(set, "vLCC", false));
assertEquals("veryLongCamelCase", match(set, "v-l-c-c", false));
assertEquals("extremely-long-option-with-many-components", match(set, "extr-opt-comp", false));
assertEquals("extremely-long-option-with-many-components", match(set, "extr-long-opt-comp", false));
assertEquals("extremely-long-option-with-many-components", match(set, "extr-comp", false));
assertNotEquals("extremely-long-option-with-many-components", match(set, "extr-opt-long-comp", false));
assertNotEquals("extremely-long-option-with-many-components", match(set, "long", false));
CommandLine cmd = new CommandLine(Model.CommandSpec.create());

assertEquals("kebab-case", match(set, "kebab-case", false, cmd));
assertEquals("kebab-case-extra", match(set, "kebab-case-extra", false, cmd));
assertEquals("very-long-kebab-case", match(set, "very-long-kebab-case", false, cmd));
assertEquals("very-long-kebab-case", match(set, "v-l-k-c", false, cmd));
assertEquals("very-long-kebab-case", match(set, "vLKC", false, cmd));
assertEquals("camelCase", match(set, "camelCase", false, cmd));
assertEquals("camelCase", match(set, "cC", false, cmd));
assertEquals("camelCase", match(set, "c-c", false, cmd));
assertEquals("camelCase", match(set, "camC", false, cmd));
assertEquals("camelCase", match(set, "camel", false, cmd));
assertEquals("camelCase", match(set, "ca", false, cmd));
assertEquals("super-long-option", match(set, "s-l-o", false, cmd));
assertEquals("super-long-option", match(set, "s-long-opt", false, cmd));
assertEquals("super-long-option", match(set, "super", false, cmd));
assertEquals("super-long-option", match(set, "sup", false, cmd));
assertEquals("super-long-option", match(set, "sup-long", false, cmd));
assertNotEquals("super-long-option", match(set, "SLO", false, cmd));
assertEquals ("super-long-option", match(set, "sLO", false, cmd));
assertEquals("super-long-option", match(set, "s-opt", false, cmd));
assertEquals("veryLongCamelCase", match(set, "veryLongCamelCase", false, cmd));
assertEquals("veryLongCamelCase", match(set, "vLCC", false, cmd));
assertEquals("veryLongCamelCase", match(set, "v-l-c-c", false, cmd));
assertEquals("extremely-long-option-with-many-components", match(set, "extr-opt-comp", false, cmd));
assertEquals("extremely-long-option-with-many-components", match(set, "extr-long-opt-comp", false, cmd));
assertEquals("extremely-long-option-with-many-components", match(set, "extr-comp", false, cmd));
assertNotEquals("extremely-long-option-with-many-components", match(set, "extr-opt-long-comp", false, cmd));
assertNotEquals("extremely-long-option-with-many-components", match(set, "long", false, cmd));

try {
match(set, "vLC", false);
match(set, "vLC", false, cmd);
fail("Expected exception");
} catch (IllegalArgumentException ex) {
assertEquals("'vLC' is not unique: it matches 'very-long-kebab-case', 'veryLongCamelCase'", ex.getMessage());
} catch (ParameterException ex) {
assertEquals("Error: 'vLC' is not unique: it matches 'very-long-kebab-case', 'veryLongCamelCase'", ex.getMessage());
}
try {
match(set, "k-c", false);
match(set, "k-c", false, cmd);
fail("Expected exception");
} catch (IllegalArgumentException ex) {
assertEquals("'k-c' is not unique: it matches 'kebab-case-extra', 'kebab-case-extra-extra', 'kebab-case'", ex.getMessage());
} catch (ParameterException ex) {
assertEquals("Error: 'k-c' is not unique: it matches 'kebab-case-extra', 'kebab-case-extra-extra', 'kebab-case'", ex.getMessage());
}
try {
match(set, "kC", false);
match(set, "kC", false, cmd);
fail("Expected exception");
} catch (IllegalArgumentException ex) {
assertEquals("'kC' is not unique: it matches 'kebab-case-extra', 'kebab-case-extra-extra', 'kebab-case'", ex.getMessage());
} catch (ParameterException ex) {
assertEquals("Error: 'kC' is not unique: it matches 'kebab-case-extra', 'kebab-case-extra-extra', 'kebab-case'", ex.getMessage());
}
try {
match(set, "keb-ca", false);
match(set, "keb-ca", false, cmd);
fail("Expected exception");
} catch (IllegalArgumentException ex) {
assertEquals("'keb-ca' is not unique: it matches 'kebab-case-extra', 'kebab-case-extra-extra', 'kebab-case'", ex.getMessage());
} catch (ParameterException ex) {
assertEquals("Error: 'keb-ca' is not unique: it matches 'kebab-case-extra', 'kebab-case-extra-extra', 'kebab-case'", ex.getMessage());
}
}

Expand All @@ -107,26 +115,28 @@ public void testUserManualExamples() {
original.add("--super-long-option");
original.add("some-long-command");

assertNotEquals("--veryLongCamelCase", match(original, "--VLCC", false));
assertEquals("--veryLongCamelCase", match(original, "--very", false));
assertEquals("--veryLongCamelCase", match(original, "--vLCC", false));
assertEquals("--veryLongCamelCase", match(original, "--vCase", false));

assertEquals("--super-long-option", match(original, "--sup", false));
assertNotEquals("--super-long-option", match(original, "--Sup", false));
assertEquals("--super-long-option", match(original, "--sLO", false));
assertEquals("--super-long-option", match(original, "--s-l-o", false));
assertEquals("--super-long-option", match(original, "--s-lon", false));
assertEquals("--super-long-option", match(original, "--s-opt", false));
assertEquals("--super-long-option", match(original, "--sOpt", false));

assertNotEquals("some-long-command", match(original, "So", false));
assertEquals("some-long-command", match(original, "so", false));
assertEquals("some-long-command", match(original, "sLC", false));
assertEquals("some-long-command", match(original, "s-l-c", false));
assertEquals("some-long-command", match(original, "soLoCo", false));
assertNotEquals("some-long-command", match(original, "SoLoCo", false));
assertEquals("some-long-command", match(original, "someCom", false));
CommandLine cmd = new CommandLine(Model.CommandSpec.create());

assertNotEquals("--veryLongCamelCase", match(original, "--VLCC", false, cmd));
assertEquals("--veryLongCamelCase", match(original, "--very", false, cmd));
assertEquals("--veryLongCamelCase", match(original, "--vLCC", false, cmd));
assertEquals("--veryLongCamelCase", match(original, "--vCase", false, cmd));

assertEquals("--super-long-option", match(original, "--sup", false, cmd));
assertNotEquals("--super-long-option", match(original, "--Sup", false, cmd));
assertEquals("--super-long-option", match(original, "--sLO", false, cmd));
assertEquals("--super-long-option", match(original, "--s-l-o", false, cmd));
assertEquals("--super-long-option", match(original, "--s-lon", false, cmd));
assertEquals("--super-long-option", match(original, "--s-opt", false, cmd));
assertEquals("--super-long-option", match(original, "--sOpt", false, cmd));

assertNotEquals("some-long-command", match(original, "So", false, cmd));
assertEquals("some-long-command", match(original, "so", false, cmd));
assertEquals("some-long-command", match(original, "sLC", false, cmd));
assertEquals("some-long-command", match(original, "s-l-c", false, cmd));
assertEquals("some-long-command", match(original, "soLoCo", false, cmd));
assertNotEquals("some-long-command", match(original, "SoLoCo", false, cmd));
assertEquals("some-long-command", match(original, "someCom", false, cmd));
}

@Test
Expand Down Expand Up @@ -538,4 +548,19 @@ class App {
assertEquals("Unknown option: '--AB'", ex.getMessage());
}
}

@Test
public void testAbbreviatedOptionWithAttachedValue() {
@Command
class Bean {
@Option(names = "--xxx-yyy")
int x;
}
Bean bean = new Bean();
CommandLine cmd = new CommandLine(bean);
cmd.setAbbreviatedOptionsAllowed(true);
cmd.parseArgs("--x-y=123");
assertEquals(123, bean.x);
}

}
12 changes: 12 additions & 0 deletions src/test/java/picocli/InheritedOptionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -492,4 +492,16 @@ public void testIssue1159() {
assertEquals(345, bean.x);
}

@Test
public void testIssue1159WithEquals() {
Issue1159 bean = new Issue1159();
CommandLine cmd = new CommandLine(bean);
cmd.setAbbreviatedOptionsAllowed(true);
cmd.parseArgs("--x-y=123");
assertEquals(123, bean.x);

cmd.parseArgs("sub", "--x-y=345");
assertEquals(345, bean.x);
}

}

0 comments on commit fe01f27

Please sign in to comment.