From e693a7da9cfcc8fd7526c8f25dad4d8f4c8a916d Mon Sep 17 00:00:00 2001 From: mbrunton Date: Tue, 6 Oct 2015 15:07:26 -0700 Subject: [PATCH] SHL-183: Fix command prefix autocomplete bug --- .../shell/converters/FileConverter.java | 2 +- .../shell/core/SimpleParser.java | 116 ++++++------------ .../shell/core/SimpleParserTests.java | 15 ++- 3 files changed, 50 insertions(+), 83 deletions(-) diff --git a/src/main/java/org/springframework/shell/converters/FileConverter.java b/src/main/java/org/springframework/shell/converters/FileConverter.java index b2bcd578..5e3fac76 100644 --- a/src/main/java/org/springframework/shell/converters/FileConverter.java +++ b/src/main/java/org/springframework/shell/converters/FileConverter.java @@ -70,7 +70,7 @@ public abstract class FileConverter implements Converter { for (File file : directory.listFiles()) { if (adjustedUserInput == null || adjustedUserInput.length() == 0 || - file.getName().toLowerCase().startsWith(adjustedUserInput.toLowerCase())) { + file.getName().startsWith(adjustedUserInput)) { String completion = ""; if (directoryData.length() > 0) diff --git a/src/main/java/org/springframework/shell/core/SimpleParser.java b/src/main/java/org/springframework/shell/core/SimpleParser.java index 133f6e16..a28a5120 100644 --- a/src/main/java/org/springframework/shell/core/SimpleParser.java +++ b/src/main/java/org/springframework/shell/core/SimpleParser.java @@ -45,7 +45,6 @@ import org.springframework.util.StringUtils; /** * Default implementation of {@link Parser}. - * * @author Ben Alex * @since 1.0 */ @@ -81,7 +80,6 @@ public class SimpleParser implements Parser { /** * get all mandatory options keys. For the options with multiple keys, the keys will be in one row. - * * @param cliOptions options * @return mandatory options keys */ @@ -91,7 +89,6 @@ public class SimpleParser implements Parser { /** * get all options key. - * * @param cliOptions * @param includeOptionalOptions * @return options keys @@ -235,8 +232,9 @@ public class SimpleParser implements Parser { // Accept a default if the user specified the option, but didn't provide a value if (specifiedKeyWithoutValue) { value = cliOption.specifiedDefaultValue(); - } else if (!specifiedKey) { - // Accept a default if the user didn't specify the option at all + } + else if (!specifiedKey) { + // Accept a default if the user didn't specify the option at all value = cliOption.unspecifiedDefaultValue(); } @@ -419,7 +417,6 @@ public class SimpleParser implements Parser { /** * Normalises the given raw user input string ready for parsing - * * @param rawInput the string to normalise; can't be null * @return a non-null string */ @@ -429,7 +426,7 @@ public class SimpleParser implements Parser { } private Set getSpecifiedUnavailableOptions(final Set cliOptions, - final Map options) { + final Map options) { Set cliOptionKeySet = new LinkedHashSet(); for (CliOption cliOption : cliOptions) { for (String key : cliOption.key()) { @@ -463,7 +460,7 @@ public class SimpleParser implements Parser { } private Collection locateTargets(final String buffer, final boolean strictMatching, - final boolean checkAvailabilityIndicators) { + final boolean checkAvailabilityIndicators) { Assert.notNull(buffer, "Buffer required"); final Collection result = new HashSet(); @@ -515,80 +512,44 @@ public class SimpleParser implements Parser { * @param strictMatching true if ALL words of 'command' need to be matched */ static String isMatch(final String buffer, final String command, final boolean strictMatching) { + Assert.isTrue(command.charAt(command.length() - 1) != ' ', "Command must not end with a space"); if ("".equals(buffer.trim())) { return ""; } - String[] commandWords = StringUtils.delimitedListToStringArray(command, " "); - int lastCommandWordUsed = 0; - Assert.notEmpty(commandWords, "Command required"); - String bufferToReturn = null; - String lastWord = null; - - next_buffer_loop: - for (int bufferIndex = 0; bufferIndex < buffer.length(); bufferIndex++) { - String bufferSoFarIncludingThis = buffer.substring(0, bufferIndex + 1); - String bufferRemaining = buffer.substring(bufferIndex + 1); - - int bufferLastIndexOfWord = bufferSoFarIncludingThis.lastIndexOf(" "); - String wordSoFarIncludingThis = bufferSoFarIncludingThis; - if (bufferLastIndexOfWord != -1) { - wordSoFarIncludingThis = bufferSoFarIncludingThis.substring(bufferLastIndexOfWord); - } - - if (wordSoFarIncludingThis.equals(" ") || bufferIndex == buffer.length() - 1) { - if (bufferIndex == buffer.length() - 1 && !"".equals(wordSoFarIncludingThis.trim())) { - lastWord = wordSoFarIncludingThis.trim(); + if (buffer.length() <= command.length()) { + // Buffer is shorter or equal in length to command + int lastSpaceIndex = command.lastIndexOf(' '); + if (strictMatching && lastSpaceIndex >= 0) { + // Check buffer touches last command word + if (buffer.length() < lastSpaceIndex + 2) { + return null; } - - // At end of word or buffer. Let's see if a word matched or not - for (int candidate = lastCommandWordUsed; candidate < commandWords.length; candidate++) { - if (lastWord != null && lastWord.length() > 0 && commandWords[candidate].startsWith(lastWord)) { - if (bufferToReturn == null) { - // This is the first match, so ensure the intended match really represents the start of a - // command and not a later word within it - if (lastCommandWordUsed == 0 && candidate > 0) { - // This is not a valid match - break next_buffer_loop; - } - } - - if (bufferToReturn != null) { - // We already matched something earlier, so ensure we didn't skip any word - if (candidate != lastCommandWordUsed + 1) { - // User has skipped a word - bufferToReturn = null; - break next_buffer_loop; - } - } - - bufferToReturn = bufferRemaining; - lastCommandWordUsed = candidate; - if (candidate + 1 == commandWords.length) { - // This was a match for the final word in the command, so abort - break next_buffer_loop; - } - // There are more words left to potentially match, so continue - continue next_buffer_loop; - } - } - - // This word is unrecognised as part of a command, so abort - bufferToReturn = null; - break next_buffer_loop; } - - lastWord = wordSoFarIncludingThis.trim(); - } - - // We only consider it a match if ALL words were actually used - if (bufferToReturn != null) { - if (!strictMatching || lastCommandWordUsed + 1 == commandWords.length) { - return bufferToReturn; + // Just need to check buffer is a prefix of command + if (command.startsWith(buffer)) { + return ""; + } + else { + return null; } } + else { + // Buffer is longer than command. Check command is a prefix of buffer. + if (!buffer.startsWith(command)) { + return null; + } - return null; // Not a match + String bufferRemaining = buffer.substring(command.length()); + if (bufferRemaining.length() > 0) { + // Check first char after command is a space + if (bufferRemaining.charAt(0) != ' ') { + return null; + } + return bufferRemaining.substring(1); + } + return bufferRemaining; + } } @Override @@ -1010,14 +971,13 @@ public class SimpleParser implements Parser { /** * populate completion for mandatory options - * - * @param translated user's input + * @param translated user's input * @param unspecified unspecified options - * @param value the option key - * @param results completion list + * @param value the option key + * @param results completion list */ private void handleMandatoryCompletion(String translated, List unspecified, String value, - SortedSet results) { + SortedSet results) { StringBuilder strBuilder = new StringBuilder(translated); if (!translated.endsWith(" ")) { strBuilder.append(" "); diff --git a/src/test/java/org/springframework/shell/core/SimpleParserTests.java b/src/test/java/org/springframework/shell/core/SimpleParserTests.java index 6e999273..53366650 100644 --- a/src/test/java/org/springframework/shell/core/SimpleParserTests.java +++ b/src/test/java/org/springframework/shell/core/SimpleParserTests.java @@ -319,10 +319,8 @@ public class SimpleParserTests { candidates.clear(); offset = parser.completeAdvanced(buffer, buffer.length(), candidates); - // TODO - // assertThat(candidates, hasItem(completionThat(is(equalTo("file --option"))))); - // assertThat(candidates, not(hasItem(completionThat(startsWith("fileMore"))))); - + assertThat(candidates, hasItem(completionThat(is(equalTo("file --option "))))); + assertThat(candidates, not(hasItem(completionThat(startsWith("fileMore"))))); } @Test @@ -444,6 +442,15 @@ public class SimpleParserTests { assertThat(result, nullValue(ParseResult.class)); } + @Test + public void testCommandPrefixCollisionFalseAmbiguity() { + parser.add(new SamePrefixCommands()); + buffer = "foo "; + offset = parser.completeAdvanced(buffer, buffer.length(), candidates); + + assertThat(candidates, not(hasItem(completionThat(startsWith("fooBar "))))); + } + @Test public void testMixedCaseOptions_SHL_98() { parser.add(new MixedCaseOptions());