From 2053bf6e4a7a023acce9919308015ba07baa761c Mon Sep 17 00:00:00 2001 From: NewbieOrange Date: Mon, 3 Jan 2022 14:10:03 +0800 Subject: [PATCH] [#1537] AbbreviationMatcher now treats aliases of the same object as one match Fixes #1537. --- src/main/java/picocli/CommandLine.java | 136 +++++++++++----- .../java/picocli/AbbreviationMatcherTest.java | 153 ++++++++++-------- src/test/java/picocli/Issue1537Ambiguous.java | 44 ----- 3 files changed, 183 insertions(+), 150 deletions(-) delete mode 100644 src/test/java/picocli/Issue1537Ambiguous.java diff --git a/src/main/java/picocli/CommandLine.java b/src/main/java/picocli/CommandLine.java index 8d606b7d0..a1be85c14 100644 --- a/src/main/java/picocli/CommandLine.java +++ b/src/main/java/picocli/CommandLine.java @@ -6478,7 +6478,7 @@ private void updatedSubcommandsToInheritFrom(CommandSpec root) { public CommandLine removeSubcommand(String name) { String actualName = name; if (parser().abbreviatedSubcommandsAllowed()) { - actualName = AbbreviationMatcher.match(commands.keySet(), name, subcommandsCaseInsensitive(), commandLine); + actualName = AbbreviationMatcher.match(commands, name, subcommandsCaseInsensitive(), commandLine).getFullName(); } Set removedNames = new TreeSet(); @@ -13311,43 +13311,52 @@ private void processArguments(List parsedCommands, } // if we find another command, we are done with the current command - if (commandSpec.parser().abbreviatedSubcommandsAllowed()) { - arg = AbbreviationMatcher.match(commandSpec.subcommands().keySet(), arg, commandSpec.subcommandsCaseInsensitive(), CommandLine.this); + CommandLine subcommand = commandSpec.subcommands().get(arg); + if (subcommand == null && commandSpec.parser().abbreviatedSubcommandsAllowed()) { + subcommand = AbbreviationMatcher.match(commandSpec.subcommands(), arg, commandSpec.subcommandsCaseInsensitive(), CommandLine.this).getValue(); } - if (commandSpec.subcommands().containsKey(arg)) { - CommandLine subcommand = commandSpec.subcommands().get(arg); + if (subcommand != null) { processSubcommand(subcommand, parseResultBuilder, parsedCommands, args, required, initialized, originalArgs, nowProcessing, separator, arg); return; // remainder done by the command } - if (commandSpec.parent() != null && commandSpec.parent().subcommandsRepeatable() && commandSpec.parent().subcommands().containsKey(arg)) { - tracer.debug("'%s' is a repeatable subcommand of %s%n", arg, commandSpec.parent().qualifiedName());// #454 repeatable subcommands - CommandLine subcommand = commandSpec.parent().subcommands().get(arg); - Set inheritedInitialized = initialized; - if (subcommand.interpreter.parseResultBuilder != null) { - tracer.debug("Subcommand '%s' has been matched before. Making a copy...%n", subcommand.getCommandName()); - subcommand = subcommand.copy(); - subcommand.getCommandSpec().parent(commandSpec.parent()); // hook it up with its parent - inheritedInitialized = new LinkedHashSet(inheritedInitialized); + CommandSpec parent = commandSpec.parent(); + if (parent != null && parent.subcommandsRepeatable()) { + subcommand = parent.subcommands().get(arg); + if (subcommand == null && parent.parser().abbreviatedSubcommandsAllowed()) { + subcommand = AbbreviationMatcher.match(parent.subcommands(), arg, parent.subcommandsCaseInsensitive(), CommandLine.this).getValue(); + } + if (subcommand != null) { + tracer.debug("'%s' is a repeatable subcommand of %s%n", arg, + commandSpec.parent().qualifiedName()); // #454 repeatable subcommands + Set inheritedInitialized = initialized; + if (subcommand.interpreter.parseResultBuilder != null) { + tracer.debug("Subcommand '%s' has been matched before. Making a copy...%n", + subcommand.getCommandName()); + subcommand = subcommand.copy(); + subcommand.getCommandSpec().parent(commandSpec.parent()); // hook it up with its parent + inheritedInitialized = new LinkedHashSet(inheritedInitialized); + } + processSubcommand(subcommand, getParent().interpreter.parseResultBuilder, parsedCommands, args, + required, inheritedInitialized, originalArgs, nowProcessing, separator, arg); + continue; } - processSubcommand(subcommand, getParent().interpreter.parseResultBuilder, parsedCommands, args, required, inheritedInitialized, originalArgs, nowProcessing, separator, arg); - continue; } // First try to interpret the argument as a single option (as opposed to a compact group of options). // 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 aggregatedOptionNames = new LinkedHashSet(); + LinkedHashMap aggregatedOptionNames = new LinkedHashMap(); if (commandSpec.parser().abbreviatedOptionsAllowed()) { - aggregatedOptionNames.addAll(commandSpec.optionsMap().keySet()); - aggregatedOptionNames.addAll(commandSpec.negatedOptionsMap().keySet()); - arg = AbbreviationMatcher.match(aggregatedOptionNames, arg, commandSpec.optionsCaseInsensitive(), CommandLine.this); + aggregatedOptionNames.putAll(commandSpec.optionsMap()); + aggregatedOptionNames.putAll(commandSpec.negatedOptionsMap()); + arg = AbbreviationMatcher.match(aggregatedOptionNames, arg, commandSpec.optionsCaseInsensitive(), CommandLine.this).getFullName(); } 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 + key = AbbreviationMatcher.match(aggregatedOptionNames, key, commandSpec.optionsCaseInsensitive(), CommandLine.this).getFullName(); //#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); @@ -14814,12 +14823,11 @@ public void run() { Help.ColorScheme colors = colorScheme != null ? colorScheme : Help.defaultColorScheme(ansi); if (commands != null) { Map parentSubcommands = parent.getCommandSpec().subcommands(); - String fullName = commands; - if (parent.isAbbreviatedSubcommandsAllowed()) { - fullName = AbbreviationMatcher.match(parentSubcommands.keySet(), fullName, - parent.isSubcommandsCaseInsensitive(), self); + CommandLine subcommand = parentSubcommands.get(commands); + if (subcommand == null && parent.isAbbreviatedSubcommandsAllowed()) { + subcommand = AbbreviationMatcher.match(parentSubcommands, commands, + parent.isSubcommandsCaseInsensitive(), self).getValue(); } - CommandLine subcommand = parentSubcommands.get(fullName); if (subcommand != null) { if (outWriter != null) { subcommand.usage(outWriter, colors); @@ -18373,6 +18381,42 @@ private String getValue(String key, CommandSpec spec) { } static class AbbreviationMatcher { + static class MatchResult { + private final String fullName; + private final V value; + + MatchResult(String fullName, V value) { + this.fullName = fullName; + this.value = value; + } + + String getFullName() { + return fullName; + } + + boolean hasValue() { + return value != null; + } + + V getValue() { + return value; + } + + @Override + public boolean equals(Object o) { + if (!(o instanceof MatchResult)) { + return false; + } + MatchResult p = (MatchResult) o; + return fullName.equals(p.fullName) && Objects.equals(value, p.value); + } + + @Override + public int hashCode() { + return fullName.hashCode() ^ (value == null ? 0 : value.hashCode()); + } + } + public static List splitIntoChunks(String command, boolean caseInsensitive) { List result = new ArrayList(); int start = 0, codepoint; @@ -18421,25 +18465,29 @@ private static String makeCanonical(String str) { return str; } - /** Returns the non-abbreviated name if found, otherwise returns the specified original abbreviation value. */ - public static String match(Set set, String abbreviation, boolean caseInsensitive, CommandLine source) { - if (set.contains(abbreviation) || set.isEmpty()) { // return exact match - return abbreviation; + /** Returns the non-abbreviated name if found, otherwise returns the specified original abbreviation name. */ + public static MatchResult match(Map map, String abbreviation, boolean caseInsensitive, CommandLine source) { + MatchResult result = new MatchResult(abbreviation, map.get(abbreviation)); + if (result.hasValue() || map.isEmpty()) { + return result; } List abbreviatedKeyChunks = splitIntoChunks(abbreviation, caseInsensitive); - List candidates = new ArrayList(); - for (String key : set) { - List keyChunks = splitIntoChunks(key, caseInsensitive); + Map candidates = new LinkedHashMap(); + for (Map.Entry entry : map.entrySet()) { + List keyChunks = splitIntoChunks(entry.getKey(), caseInsensitive); if (matchKeyChunks(abbreviatedKeyChunks, keyChunks, caseInsensitive)) { - candidates.add(key); + if (!result.hasValue()) { + result = new MatchResult(entry.getKey(), entry.getValue()); + } + candidates.put(entry.getKey(), entry.getValue()); } } - if (candidates.size() > 1) { - String str = candidates.toString(); + if (!isAllCandidatesSame(candidates.values())) { + String str = candidates.keySet().toString(); 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 + return result; // return the original with null as value if no match found } private static boolean matchKeyChunks(List abbreviatedKeyChunks, List keyChunks, boolean caseInsensitive) { @@ -18487,5 +18535,19 @@ private static boolean isNonAlphabetic(String str) { } return true; } + + private static boolean isAllCandidatesSame(Collection candidates) { + if (candidates.isEmpty()) { + return true; + } + Iterator iterator = candidates.iterator(); + T first = iterator.next(); + while (iterator.hasNext()) { + if (iterator.next() != first) { // check reference equality as aliases point to the same object + return false; + } + } + return true; + } } } diff --git a/src/test/java/picocli/AbbreviationMatcherTest.java b/src/test/java/picocli/AbbreviationMatcherTest.java index 50d6770c6..207b47a5e 100644 --- a/src/test/java/picocli/AbbreviationMatcherTest.java +++ b/src/test/java/picocli/AbbreviationMatcherTest.java @@ -10,8 +10,8 @@ import java.io.PrintStream; import java.io.PrintWriter; import java.util.Arrays; -import java.util.LinkedHashSet; -import java.util.Set; +import java.util.LinkedHashMap; +import java.util.Map; import static org.junit.Assert.*; import static picocli.CommandLine.AbbreviationMatcher.match; @@ -32,73 +32,73 @@ public class AbbreviationMatcherTest { @Rule public final ProvideSystemProperty ansiOFF = new ProvideSystemProperty("picocli.ansi", "false"); - private Set createSet() { - Set result = new LinkedHashSet(); - result.add("kebab-case-extra"); - result.add("kebab-case-extra-extra"); - result.add("kebab-case"); - result.add("kc"); // alias - result.add("very-long-kebab-case"); - result.add("camelCase"); - result.add("veryLongCamelCase"); - result.add("super-long-option"); - result.add("extremely-long-option-with-many-components"); + private Map createMap() { + Map result = new LinkedHashMap(); + result.put("kebab-case-extra", 1); + result.put("kebab-case-extra-extra", 2); + result.put("kebab-case", 3); + result.put("kc", 4); // alias + result.put("very-long-kebab-case", 5); + result.put("camelCase", 6); + result.put("veryLongCamelCase", 7); + result.put("super-long-option", 8); + result.put("extremely-long-option-with-many-components", 9); return result; } @Test public void testPrefixMatch() { - Set set = createSet(); + Map map = createMap(); 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)); + assertEquals("kebab-case", match(map, "kebab-case", false, cmd).getFullName()); + assertEquals("kebab-case-extra", match(map, "kebab-case-extra", false, cmd).getFullName()); + assertEquals("very-long-kebab-case", match(map, "very-long-kebab-case", false, cmd).getFullName()); + assertEquals("very-long-kebab-case", match(map, "v-l-k-c", false, cmd).getFullName()); + assertEquals("very-long-kebab-case", match(map, "vLKC", false, cmd).getFullName()); + assertEquals("camelCase", match(map, "camelCase", false, cmd).getFullName()); + assertEquals("camelCase", match(map, "cC", false, cmd).getFullName()); + assertEquals("camelCase", match(map, "c-c", false, cmd).getFullName()); + assertEquals("camelCase", match(map, "camC", false, cmd).getFullName()); + assertEquals("camelCase", match(map, "camel", false, cmd).getFullName()); + assertEquals("camelCase", match(map, "ca", false, cmd).getFullName()); + assertEquals("super-long-option", match(map, "s-l-o", false, cmd).getFullName()); + assertEquals("super-long-option", match(map, "s-long-opt", false, cmd).getFullName()); + assertEquals("super-long-option", match(map, "super", false, cmd).getFullName()); + assertEquals("super-long-option", match(map, "sup", false, cmd).getFullName()); + assertEquals("super-long-option", match(map, "sup-long", false, cmd).getFullName()); + assertNotEquals("super-long-option", match(map, "SLO", false, cmd).getFullName()); + assertEquals ("super-long-option", match(map, "sLO", false, cmd).getFullName()); + assertEquals("super-long-option", match(map, "s-opt", false, cmd).getFullName()); + assertEquals("veryLongCamelCase", match(map, "veryLongCamelCase", false, cmd).getFullName()); + assertEquals("veryLongCamelCase", match(map, "vLCC", false, cmd).getFullName()); + assertEquals("veryLongCamelCase", match(map, "v-l-c-c", false, cmd).getFullName()); + assertEquals("extremely-long-option-with-many-components", match(map, "extr-opt-comp", false, cmd).getFullName()); + assertEquals("extremely-long-option-with-many-components", match(map, "extr-long-opt-comp", false, cmd).getFullName()); + assertEquals("extremely-long-option-with-many-components", match(map, "extr-comp", false, cmd).getFullName()); + assertNotEquals("extremely-long-option-with-many-components", match(map, "extr-opt-long-comp", false, cmd).getFullName()); + assertNotEquals("extremely-long-option-with-many-components", match(map, "long", false, cmd).getFullName()); try { - match(set, "vLC", false, cmd); + match(map, "vLC", false, cmd); fail("Expected exception"); } catch (ParameterException ex) { assertEquals("Error: 'vLC' is not unique: it matches 'very-long-kebab-case', 'veryLongCamelCase'", ex.getMessage()); } try { - match(set, "k-c", false, cmd); + match(map, "k-c", false, cmd); fail("Expected exception"); } 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, cmd); + match(map, "kC", false, cmd); fail("Expected exception"); } 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, cmd); + match(map, "keb-ca", false, cmd); fail("Expected exception"); } catch (ParameterException ex) { assertEquals("Error: 'keb-ca' is not unique: it matches 'kebab-case-extra', 'kebab-case-extra-extra', 'kebab-case'", ex.getMessage()); @@ -110,33 +110,33 @@ public void testPrefixMatch() { //|`some-long-command` | `so`, sLC`, `s-l-c`, `soLoCo`, `someCom` @Test public void testUserManualExamples() { - Set original = new LinkedHashSet(); - original.add("--veryLongCamelCase"); - original.add("--super-long-option"); - original.add("some-long-command"); + Map original = new LinkedHashMap(); + original.put("--veryLongCamelCase", 1); + original.put("--super-long-option", 2); + original.put("some-long-command", 3); 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)); + assertNotEquals("--veryLongCamelCase", match(original, "--VLCC", false, cmd).getFullName()); + assertEquals("--veryLongCamelCase", match(original, "--very", false, cmd).getFullName()); + assertEquals("--veryLongCamelCase", match(original, "--vLCC", false, cmd).getFullName()); + assertEquals("--veryLongCamelCase", match(original, "--vCase", false, cmd).getFullName()); + + assertEquals("--super-long-option", match(original, "--sup", false, cmd).getFullName()); + assertNotEquals("--super-long-option", match(original, "--Sup", false, cmd).getFullName()); + assertEquals("--super-long-option", match(original, "--sLO", false, cmd).getFullName()); + assertEquals("--super-long-option", match(original, "--s-l-o", false, cmd).getFullName()); + assertEquals("--super-long-option", match(original, "--s-lon", false, cmd).getFullName()); + assertEquals("--super-long-option", match(original, "--s-opt", false, cmd).getFullName()); + assertEquals("--super-long-option", match(original, "--sOpt", false, cmd).getFullName()); + + assertNotEquals("some-long-command", match(original, "So", false, cmd).getFullName()); + assertEquals("some-long-command", match(original, "so", false, cmd).getFullName()); + assertEquals("some-long-command", match(original, "sLC", false, cmd).getFullName()); + assertEquals("some-long-command", match(original, "s-l-c", false, cmd).getFullName()); + assertEquals("some-long-command", match(original, "soLoCo", false, cmd).getFullName()); + assertNotEquals("some-long-command", match(original, "SoLoCo", false, cmd).getFullName()); + assertEquals("some-long-command", match(original, "someCom", false, cmd).getFullName()); } @Test @@ -563,4 +563,19 @@ class Bean { assertEquals(123, bean.x); } + @Test + public void testAmbiguousAbbreviationSubcommands() { + @Command + class Issue1537Ambiguous implements Runnable { + @Command(name = "chemical-files", aliases = "chem-formats") + void sub() {} + + public void run() {} + } + int exitCode = new CommandLine(new Issue1537Ambiguous()).setAbbreviatedSubcommandsAllowed(true).execute("chem"); + assertEquals(0, exitCode); + CommandLine.ParseResult result = new CommandLine(new Issue1537Ambiguous()).setAbbreviatedSubcommandsAllowed(true).parseArgs("chem"); + assertEquals("chemical-files", result.subcommand().asCommandLineList().get(0).getCommandName()); + } + } diff --git a/src/test/java/picocli/Issue1537Ambiguous.java b/src/test/java/picocli/Issue1537Ambiguous.java deleted file mode 100644 index d7f1aaab5..000000000 --- a/src/test/java/picocli/Issue1537Ambiguous.java +++ /dev/null @@ -1,44 +0,0 @@ -package picocli; - -import org.junit.Rule; -import org.junit.Test; -import org.junit.contrib.java.lang.system.SystemErrRule; -import picocli.CommandLine.Command; - -import static org.junit.Assert.*; - -public class Issue1537Ambiguous { - - @Rule - public SystemErrRule systemErrRule = new SystemErrRule().enableLog().muteForSuccessfulTests(); - - @Command - static class MyCommand implements Runnable { - - @Command(name = "chemical-files", aliases = "chem-formats") - void sub() {} - - public void run() {} - } - - @Test - public void testExecute() { - int exitCode = new CommandLine(new MyCommand()).execute("chem"); - assertEquals(2, exitCode); - - String expected = String.format( - "Unmatched argument at index 0: 'chem'%n" + - "Did you mean: chem-formats or chemical-files?%n"); - assertEquals(expected, systemErrRule.getLog()); - } - - @Test - public void testParseArgs() { - try { - new CommandLine(new MyCommand()).parseArgs("chem"); - fail("expected exception"); - } catch (CommandLine.UnmatchedArgumentException ex) { - assertEquals("Unmatched argument at index 0: 'chem'", ex.getMessage()); - } - } -}