From 3c1dcd1a6c469216130b2dc0e399c69b89bcbf8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rg=20Hohwiller?= Date: Tue, 13 Aug 2024 18:04:06 +0200 Subject: [PATCH] #330: fix CLI to prevent invalid shortoption split (#530) --- .../tools/ide/context/AbstractIdeContext.java | 34 ++++++++---- .../tools/ide/property/KeywordProperty.java | 12 +++++ .../tools/ide/cli/CliAdvancedParsingTest.java | 30 +++++++++++ .../commandlet/EnvironmentCommandletTest.java | 31 ++++++++++- .../tools/ide/log/IdeTestLoggerAssertion.java | 53 +++++++++++-------- .../mvn/repository/java/java/default/bin/java | 2 +- 6 files changed, 128 insertions(+), 34 deletions(-) create mode 100644 cli/src/test/java/com/devonfw/tools/ide/cli/CliAdvancedParsingTest.java diff --git a/cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java b/cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java index d643c5584..3f041c37c 100644 --- a/cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java +++ b/cli/src/main/java/com/devonfw/tools/ide/context/AbstractIdeContext.java @@ -987,21 +987,37 @@ public boolean apply(CliArguments arguments, Commandlet cmd, CompletionCandidate } else { propertyIterator = cmd.getValues().iterator(); } + Property property = null; + if (propertyIterator.hasNext()) { + property = propertyIterator.next(); + } while (!currentArgument.isEnd()) { trace("Trying to match argument '{}'", currentArgument); - Property property = null; + Property currentProperty = property; if (!arguments.isEndOptions()) { - property = cmd.getOption(currentArgument.getKey()); + Property option = cmd.getOption(currentArgument.getKey()); + if (option != null) { + currentProperty = option; + } + } + if (currentProperty == null) { + trace("No option or next value found"); + return false; } - if (property == null) { - if (!propertyIterator.hasNext()) { - trace("No option or next value found"); - return false; + trace("Next property candidate to match argument is {}", currentProperty); + if (currentProperty == property) { + if (!property.isMultiValued()) { + if (propertyIterator.hasNext()) { + property = propertyIterator.next(); + } else { + property = null; + } + } + if ((property != null) && property.isValue() && property.isMultiValued()) { + arguments.endOptions(); } - property = propertyIterator.next(); } - trace("Next property candidate to match argument is {}", property); - boolean matches = property.apply(arguments, this, cmd, collector); + boolean matches = currentProperty.apply(arguments, this, cmd, collector); if (!matches || currentArgument.isCompletion()) { return false; } diff --git a/cli/src/main/java/com/devonfw/tools/ide/property/KeywordProperty.java b/cli/src/main/java/com/devonfw/tools/ide/property/KeywordProperty.java index d3564d586..f3cf0a3a1 100644 --- a/cli/src/main/java/com/devonfw/tools/ide/property/KeywordProperty.java +++ b/cli/src/main/java/com/devonfw/tools/ide/property/KeywordProperty.java @@ -1,5 +1,10 @@ package com.devonfw.tools.ide.property; +import com.devonfw.tools.ide.cli.CliArguments; +import com.devonfw.tools.ide.commandlet.Commandlet; +import com.devonfw.tools.ide.completion.CompletionCandidateCollector; +import com.devonfw.tools.ide.context.IdeContext; + /** * {@link BooleanProperty} for a keyword (e.g. "install" in "ide install mvn 3.9.4"). */ @@ -24,4 +29,11 @@ public boolean isExpectValue() { return false; } + @Override + protected boolean applyValue(String argValue, boolean lookahead, CliArguments args, IdeContext context, Commandlet commandlet, + CompletionCandidateCollector collector) { + + setValue(true); + return true; + } } diff --git a/cli/src/test/java/com/devonfw/tools/ide/cli/CliAdvancedParsingTest.java b/cli/src/test/java/com/devonfw/tools/ide/cli/CliAdvancedParsingTest.java new file mode 100644 index 000000000..6f28699e0 --- /dev/null +++ b/cli/src/test/java/com/devonfw/tools/ide/cli/CliAdvancedParsingTest.java @@ -0,0 +1,30 @@ +package com.devonfw.tools.ide.cli; + +import org.junit.jupiter.api.Test; + +import com.devonfw.tools.ide.context.AbstractIdeContextTest; +import com.devonfw.tools.ide.context.IdeTestContext; + +/** + * Integration test of {@link com.devonfw.tools.ide.context.AbstractIdeContext#run(CliArguments) CLI parsing}. + */ +public class CliAdvancedParsingTest extends AbstractIdeContextTest { + + private static final String PROJECT_MVN = "mvn"; + + /** + * Test that implicit end-options is triggered for multi-valued arguments to prevent splitting odd-formatted short-options like "-version". + */ + @Test + public void testPreventShortOptionsForMultivaluedArguments() { + + // arrange + IdeTestContext context = newContext(PROJECT_MVN); + CliArguments args = new CliArguments("java", "-version"); + args.next(); + // act + context.run(args); + // assert + assertThat(context).logAtInfo().hasNoMessage("java -v -e -r -s -i -o -n").hasMessage("java -version"); + } +} diff --git a/cli/src/test/java/com/devonfw/tools/ide/commandlet/EnvironmentCommandletTest.java b/cli/src/test/java/com/devonfw/tools/ide/commandlet/EnvironmentCommandletTest.java index 06f3a2499..0140a490d 100644 --- a/cli/src/test/java/com/devonfw/tools/ide/commandlet/EnvironmentCommandletTest.java +++ b/cli/src/test/java/com/devonfw/tools/ide/commandlet/EnvironmentCommandletTest.java @@ -93,9 +93,36 @@ public void testRunInfoLogging() { env.run(); // assert if (SystemInfoImpl.INSTANCE.isWindows()) { - assertThat(context).log().hasEntriesWithNothingElseInBetween( // + assertThat(context).log().hasEntries /*WithNothingElseInBetween*/( // + IdeLogEntry.ofInfo("BAR=bar-some-${UNDEFINED}"), // + IdeLogEntry.ofInfo("DOCKER_EDITION=docker"), // + IdeLogEntry.ofInfo("ECLIPSE_VERSION=2023-03"), // + IdeLogEntry.ofInfo("FOO=foo-bar-some-${UNDEFINED}"), // + IdeLogEntry.ofInfo("HOME=" + context.getUserHome() + ""), // + IdeLogEntry.ofInfo("IDE_HOME=" + context.getIdeHome() + ""), // + IdeLogEntry.ofInfo("IDE_TOOLS=mvn,eclipse"), // + IdeLogEntry.ofInfo("INTELLIJ_EDITION=ultimate"), // + IdeLogEntry.ofInfo("JAVA_VERSION=17*"), // IdeLogEntry.ofInfo("M2_REPO=" + context.getUserHome() + "/.m2/repository"), // - new IdeLogEntry(IdeLogLevel.INFO, "PATH=", true) // + IdeLogEntry.ofInfo("MVN_VERSION=3.9.1"), // + new IdeLogEntry(IdeLogLevel.INFO, "PATH=", true), // + IdeLogEntry.ofInfo("SOME=some-${UNDEFINED}"), // + IdeLogEntry.ofInfo("TEST_ARGS1= user1 settings1 workspace1 conf1"), // + IdeLogEntry.ofInfo("TEST_ARGS10=user10 workspace10"), // + IdeLogEntry.ofInfo("TEST_ARGS2= user2 conf2"), // + IdeLogEntry.ofInfo("TEST_ARGS3= user3 workspace3"), // + IdeLogEntry.ofInfo("TEST_ARGS4= settings4"), // + IdeLogEntry.ofInfo("TEST_ARGS5= settings5 conf5"), // + IdeLogEntry.ofInfo("TEST_ARGS6= settings6 workspace6 conf6"), // + IdeLogEntry.ofInfo("TEST_ARGS7=user7 settings7 workspace7 conf7"), // + IdeLogEntry.ofInfo("TEST_ARGS8=settings8 workspace8 conf8"), // + IdeLogEntry.ofInfo("TEST_ARGS9=settings9 workspace9"), // + IdeLogEntry.ofInfo("TEST_ARGSa= user1 settings1 workspace1 conf1 user3 workspace3 confa"), // + IdeLogEntry.ofInfo("TEST_ARGSb=user10 workspace10 settingsb user1 settings1 workspace1 conf1 user3 workspace3 confa userb"), // + IdeLogEntry.ofInfo("TEST_ARGSc= user1 settings1 workspace1 conf1 userc settingsc confc"), // + IdeLogEntry.ofInfo("TEST_ARGSd= user1 settings1 workspace1 conf1 userd workspaced"), // + IdeLogEntry.ofInfo("WORKSPACE=foo-test"), // + IdeLogEntry.ofInfo("WORKSPACE_PATH=" + context.getWorkspacePath()) // ); } else { assertThat(context).log().hasEntriesWithNothingElseInBetween( // diff --git a/cli/src/test/java/com/devonfw/tools/ide/log/IdeTestLoggerAssertion.java b/cli/src/test/java/com/devonfw/tools/ide/log/IdeTestLoggerAssertion.java index a83e4bab7..4efac8e01 100644 --- a/cli/src/test/java/com/devonfw/tools/ide/log/IdeTestLoggerAssertion.java +++ b/cli/src/test/java/com/devonfw/tools/ide/log/IdeTestLoggerAssertion.java @@ -22,42 +22,47 @@ public IdeTestLoggerAssertion(List entries, IdeLogLevel level) { /** * @param message the expected {@link com.devonfw.tools.ide.log.IdeSubLogger#log(String) log message}. + * @return this assertion itself for fluent API calls. */ - public void hasMessage(String message) { + public IdeTestLoggerAssertion hasMessage(String message) { - fulfillsPredicate(e -> e.message().equals(message), PredicateMode.MATCH_ONE, "Could not find log message equal to '" + message + "'"); + return fulfillsPredicate(e -> e.message().equals(message), PredicateMode.MATCH_ONE, "Could not find log message equal to '" + message + "'"); } /** * @param message the {@link String} expected to be {@link String#contains(CharSequence) contained} in a - * {@link com.devonfw.tools.ide.log.IdeSubLogger#log(String) log message}. + * {@link com.devonfw.tools.ide.log.IdeSubLogger#log(String) log message}. + * @return this assertion itself for fluent API calls. */ - public void hasMessageContaining(String message) { + public IdeTestLoggerAssertion hasMessageContaining(String message) { - fulfillsPredicate(e -> e.message().contains(message), PredicateMode.MATCH_ONE, "Could not find log message containing '" + message + "'"); + return fulfillsPredicate(e -> e.message().contains(message), PredicateMode.MATCH_ONE, "Could not find log message containing '" + message + "'"); } /** * @param message the {@link com.devonfw.tools.ide.log.IdeSubLogger#log(String) log message} that is not expected and should not have been logged. + * @return this assertion itself for fluent API calls. */ - public void hasNoMessage(String message) { + public IdeTestLoggerAssertion hasNoMessage(String message) { - fulfillsPredicate(e -> !e.message().equals(message), PredicateMode.MATCH_ALL, "No log message should be equal to '" + message + "'"); + return fulfillsPredicate(e -> !e.message().equals(message), PredicateMode.MATCH_ALL, "No log message should be equal to '" + message + "'"); } /** * @param message the {@link String} expected not be {@link String#contains(CharSequence) contained} in any - * {@link com.devonfw.tools.ide.log.IdeSubLogger#log(String) log message}. + * {@link com.devonfw.tools.ide.log.IdeSubLogger#log(String) log message}. + * @return this assertion itself for fluent API calls. */ - public void hasNoMessageContaining(String message) { + public IdeTestLoggerAssertion hasNoMessageContaining(String message) { - fulfillsPredicate(e -> !e.message().contains(message), PredicateMode.MATCH_ALL, "No log message should contain '" + message + "'"); + return fulfillsPredicate(e -> !e.message().contains(message), PredicateMode.MATCH_ALL, "No log message should contain '" + message + "'"); } /** * @param messages the expected {@link com.devonfw.tools.ide.log.IdeSubLogger#log(String) log message}s in order. + * @return this assertion itself for fluent API calls. */ - public void hasEntries(String... messages) { + public IdeTestLoggerAssertion hasEntries(String... messages) { assert (this.level != null); IdeLogEntry[] entries = new IdeLogEntry[messages.length]; @@ -65,27 +70,29 @@ public void hasEntries(String... messages) { for (String message : messages) { entries[i++] = new IdeLogEntry(this.level, message); } - hasEntries(false, entries); + return hasEntries(false, entries); } /** * @param expectedEntries the expected {@link com.devonfw.tools.ide.log.IdeLogEntry log entries} in order. + * @return this assertion itself for fluent API calls. */ - public void hasEntries(IdeLogEntry... expectedEntries) { + public IdeTestLoggerAssertion hasEntries(IdeLogEntry... expectedEntries) { - hasEntries(false, expectedEntries); + return hasEntries(false, expectedEntries); } /** * @param expectedEntries the expected {@link com.devonfw.tools.ide.log.IdeLogEntry log entries} to be logged in order without any other log statement in - * between them. + * between them. + * @return this assertion itself for fluent API calls. */ - public void hasEntriesWithNothingElseInBetween(IdeLogEntry... expectedEntries) { + public IdeTestLoggerAssertion hasEntriesWithNothingElseInBetween(IdeLogEntry... expectedEntries) { - hasEntries(true, expectedEntries); + return hasEntries(true, expectedEntries); } - private void hasEntries(boolean nothingElseInBetween, IdeLogEntry... expectedEntries) { + private IdeTestLoggerAssertion hasEntries(boolean nothingElseInBetween, IdeLogEntry... expectedEntries) { assert (expectedEntries.length > 0); int i = 0; @@ -101,7 +108,7 @@ private void hasEntries(boolean nothingElseInBetween, IdeLogEntry... expectedEnt } } if (i == expectedEntries.length) { - return; + return this; } if (i > max) { max = i; @@ -127,6 +134,7 @@ private void hasEntries(boolean nothingElseInBetween, IdeLogEntry... expectedEnt appendEntry(error, entry); } Assertions.fail(error.toString()); + return this; } private static void appendEntry(StringBuilder sb, IdeLogEntry entry) { @@ -136,7 +144,7 @@ private static void appendEntry(StringBuilder sb, IdeLogEntry entry) { sb.append('\n'); } - private void fulfillsPredicate(Predicate predicate, PredicateMode mode, String errorMessage) { + private IdeTestLoggerAssertion fulfillsPredicate(Predicate predicate, PredicateMode mode, String errorMessage) { if (this.level != null) { errorMessage = errorMessage + " on level " + this.level; @@ -145,17 +153,18 @@ private void fulfillsPredicate(Predicate predicate, PredicateMode m if ((this.level == null) || (this.level == entry.level())) { if (predicate.test(entry)) { if (mode == PredicateMode.MATCH_ONE) { - return; + return this; } } else if (mode == PredicateMode.MATCH_ALL) { Assertions.fail(errorMessage + "\nFound unexpected log entry: " + entry); - return; + return this; } } } if (mode == PredicateMode.MATCH_ONE) { Assertions.fail(errorMessage); // no log entry matched by predicate } + return this; } } diff --git a/cli/src/test/resources/ide-projects/mvn/repository/java/java/default/bin/java b/cli/src/test/resources/ide-projects/mvn/repository/java/java/default/bin/java index 1982a6d4f..bb96ab46f 100755 --- a/cli/src/test/resources/ide-projects/mvn/repository/java/java/default/bin/java +++ b/cli/src/test/resources/ide-projects/mvn/repository/java/java/default/bin/java @@ -1,2 +1,2 @@ #!/bin/bash -echo "java mvn" \ No newline at end of file +echo java $*