diff --git a/spring-shell-core/src/main/java/org/springframework/shell/command/parser/Parser.java b/spring-shell-core/src/main/java/org/springframework/shell/command/parser/Parser.java index 8902fd18..a24d22bd 100644 --- a/spring-shell-core/src/main/java/org/springframework/shell/command/parser/Parser.java +++ b/spring-shell-core/src/main/java/org/springframework/shell/command/parser/Parser.java @@ -149,6 +149,8 @@ public interface Parser { private List invalidOptionNodes = new ArrayList<>(); private List argumentResults = new ArrayList<>(); private int commandArgumentPos = 0; + private int optionPos = -1; + private long expectedOptionCount; DefaultNodeVisitor(CommandModel commandModel, ConversionService conversionService, ParserConfig config) { this.commandModel = commandModel; @@ -166,31 +168,27 @@ public interface Parser { messageResults.addAll(commonMessageResults); messageResults.addAll(validateOptionIsValid(registration)); - - // add options with default values - Set resolvedOptions1 = optionResults.stream() + // we should already have options defined with arguments. + // go through positional arguments and fill using those and + // then fill from option default values. + Set resolvedOptions = optionResults.stream() .map(or -> or.option()) .collect(Collectors.toSet()); - registration.getOptions().stream() - .filter(o -> o.getDefaultValue() != null) - .filter(o -> !resolvedOptions1.contains(o)) - .forEach(o -> { - resolvedOptions1.add(o); - Object value = convertOptionType(o, o.getDefaultValue()); - optionResults.add(OptionResult.of(o, value)); - }); + // get sorted list by position as we later match by order List optionsForArguments = registration.getOptions().stream() - .filter(o -> !resolvedOptions1.contains(o)) + .filter(o -> !resolvedOptions.contains(o)) .filter(o -> o.getPosition() > -1) .sorted(Comparator.comparingInt(o -> o.getPosition())) .collect(Collectors.toList()); + // leftover arguments to match into needed options List argumentValues = argumentResults.stream() .sorted(Comparator.comparingInt(ar -> ar.position())) .map(ar -> ar.value()) .collect(Collectors.toList()); + // try to find matching arguments int i = 0; for (CommandOption o : optionsForArguments) { int aMax = o.getArityMax(); @@ -202,11 +200,17 @@ public interface Parser { List asdf = argumentValues.subList(i, j); if (asdf.isEmpty()) { - optionResults.add(OptionResult.of(o, null)); + // don't arguments so only add if we know + // it's going to get added later via default value + if (o.getDefaultValue() == null) { + resolvedOptions.add(o); + optionResults.add(OptionResult.of(o, null)); + } } else { Object toConvertValue = asdf.size() == 1 ? asdf.get(0) : asdf; Object value = convertOptionType(o, toConvertValue); + resolvedOptions.add(o); optionResults.add(OptionResult.of(o, value)); } @@ -216,6 +220,16 @@ public interface Parser { i = j; } + // possibly fill in from default values + registration.getOptions().stream() + .filter(o -> o.getDefaultValue() != null) + .filter(o -> !resolvedOptions.contains(o)) + .forEach(o -> { + resolvedOptions.add(o); + Object value = convertOptionType(o, o.getDefaultValue()); + optionResults.add(OptionResult.of(o, value)); + }); + // can only validate after optionResults has been populated messageResults.addAll(validateOptionNotMissing(registration)); } @@ -234,6 +248,7 @@ public interface Parser { @Override protected void onEnterRootCommandNode(CommandNode node) { + expectedOptionCount = optionCountInCommand(node); resolvedCommmand.add(node.getCommand()); } @@ -243,6 +258,7 @@ public interface Parser { @Override protected void onEnterCommandNode(CommandNode node) { + expectedOptionCount = optionCountInCommand(node); resolvedCommmand.add(node.getCommand()); } @@ -254,6 +270,7 @@ public interface Parser { @Override protected void onEnterOptionNode(OptionNode node) { + optionPos++; commandArgumentPos = 0; currentOptions.clear(); currentOptionArgument.clear(); @@ -307,16 +324,40 @@ public interface Parser { int max = currentOption.getArityMax() > 0 ? currentOption.getArityMax() : Integer.MAX_VALUE; max = Math.min(max, currentOptionArgument.size()); List toUse = currentOptionArgument.subList(0, max); + List toUnused = currentOptionArgument.subList(max, currentOptionArgument.size()); + toUnused.forEach(a -> { + argumentResults.add(ArgumentResult.of(a, commandArgumentPos++)); + }); - if (currentOption.getArityMin() > -1 && currentOptionArgument.size() < currentOption.getArityMin()) { - String arg = currentOption.getLongNames()[0]; - commonMessageResults.add(MessageResult.of(ParserMessage.NOT_ENOUGH_OPTION_ARGUMENTS, 0, arg, - currentOptionArgument.size())); + // if we're not in a last option + // "--arg1 a b --arg2 c" vs "--arg1 a --arg2 b c" + // we know to impose restriction to argument count, + // last option is different as we can't really fail + // because number of argument to eat dependes on arity + // and rest would go back to positional args. + if (optionPos + 1 < expectedOptionCount) { + if (currentOption.getArityMin() > -1 && currentOptionArgument.size() < currentOption.getArityMin()) { + String arg = currentOption.getLongNames()[0]; + commonMessageResults.add(MessageResult.of(ParserMessage.NOT_ENOUGH_OPTION_ARGUMENTS, 0, arg, + currentOptionArgument.size())); + } + else if (currentOption.getArityMax() > -1 && currentOptionArgument.size() > currentOption.getArityMax()) { + String arg = currentOption.getLongNames()[0]; + commonMessageResults.add(MessageResult.of(ParserMessage.TOO_MANY_OPTION_ARGUMENTS, 0, arg, + currentOption.getArityMax())); + } } - else if (currentOption.getArityMax() > -1 && currentOptionArgument.size() > currentOption.getArityMax()) { - String arg = currentOption.getLongNames()[0]; - commonMessageResults.add(MessageResult.of(ParserMessage.TOO_MANY_OPTION_ARGUMENTS, 0, arg, - currentOption.getArityMax())); + else { + if (currentOption.getArityMin() > -1 && toUse.size() < currentOption.getArityMin()) { + String arg = currentOption.getLongNames()[0]; + commonMessageResults.add(MessageResult.of(ParserMessage.NOT_ENOUGH_OPTION_ARGUMENTS, 0, arg, + toUse.size())); + } + else if (currentOption.getArityMax() > -1 && toUse.size() > currentOption.getArityMax()) { + String arg = currentOption.getLongNames()[0]; + commonMessageResults.add(MessageResult.of(ParserMessage.TOO_MANY_OPTION_ARGUMENTS, 0, arg, + currentOption.getArityMax())); + } } Object value = null; @@ -415,5 +456,12 @@ public interface Parser { }) .collect(Collectors.toList()); } + + private static long optionCountInCommand(CommandNode node) { + if (node == null) { + return 0; + } + return node.getChildren().stream().filter(n -> n instanceof OptionNode).count(); + } } } diff --git a/spring-shell-core/src/test/java/org/springframework/shell/command/parser/AbstractParsingTests.java b/spring-shell-core/src/test/java/org/springframework/shell/command/parser/AbstractParsingTests.java index 4c2f9ca2..21ef76a3 100644 --- a/spring-shell-core/src/test/java/org/springframework/shell/command/parser/AbstractParsingTests.java +++ b/spring-shell-core/src/test/java/org/springframework/shell/command/parser/AbstractParsingTests.java @@ -23,6 +23,7 @@ import java.util.Map; import org.junit.jupiter.api.BeforeEach; import org.springframework.shell.command.CommandRegistration; +import org.springframework.shell.command.CommandRegistration.OptionArity; import org.springframework.shell.command.parser.Ast.AstResult; import org.springframework.shell.command.parser.Lexer.LexerResult; import org.springframework.shell.command.parser.Parser.ParseResult; @@ -251,6 +252,67 @@ abstract class AbstractParsingTests { .and() .build(); + static final CommandRegistration ROOT7_POSITIONAL_ONE_ARG_STRING_DEFAULT = CommandRegistration.builder() + .command("root7") + .withOption() + .longNames("arg1") + .defaultValue("arg1default") + .type(String.class) + .position(0) + .and() + .withTarget() + .consumer(ctx -> {}) + .and() + .build(); + + static final CommandRegistration ROOT7_POSITIONAL_TWO_ARG_STRING_DEFAULT = CommandRegistration.builder() + .command("root7") + .withOption() + .longNames("arg1") + .defaultValue("arg1default") + .type(String.class) + .arity(OptionArity.EXACTLY_ONE) + .position(0) + .and() + .withOption() + .longNames("arg2") + .defaultValue("arg2default") + .type(String.class) + .arity(OptionArity.EXACTLY_ONE) + .position(1) + .and() + .withTarget() + .consumer(ctx -> {}) + .and() + .build(); + + static final CommandRegistration ROOT7_POSITIONAL_TWO_ARG_STRING_DEFAULT_ONE_NODEFAULT = CommandRegistration.builder() + .command("root7") + .withOption() + .longNames("arg1") + .defaultValue("arg1default") + .type(String.class) + .arity(OptionArity.EXACTLY_ONE) + .position(0) + .and() + .withOption() + .longNames("arg2") + .defaultValue("arg2default") + .type(String.class) + .arity(OptionArity.EXACTLY_ONE) + .position(1) + .and() + .withOption() + .longNames("arg3") + .type(String.class) + .arity(OptionArity.EXACTLY_ONE) + .position(2) + .and() + .withTarget() + .consumer(ctx -> {}) + .and() + .build(); + Map registrations = new HashMap<>(); @BeforeEach diff --git a/spring-shell-core/src/test/java/org/springframework/shell/command/parser/ParserTests.java b/spring-shell-core/src/test/java/org/springframework/shell/command/parser/ParserTests.java index d31934ac..45550dfc 100644 --- a/spring-shell-core/src/test/java/org/springframework/shell/command/parser/ParserTests.java +++ b/spring-shell-core/src/test/java/org/springframework/shell/command/parser/ParserTests.java @@ -445,6 +445,195 @@ class ParserTests extends AbstractParsingTests { } ); } - } + @Test + void noArgsGetsOnlyDefault() { + register(ROOT7_POSITIONAL_ONE_ARG_STRING_DEFAULT); + ParseResult result = parse("root7"); + assertThat(result.messageResults()).isEmpty(); + assertThat(result.argumentResults()).isEmpty(); + assertThat(result.optionResults()).isNotNull().satisfiesExactly( + r -> { + assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg1" }); + assertThat(r.value()).isEqualTo("arg1default"); + } + ); + } + + @Test + void positionOverridesDefault() { + register(ROOT7_POSITIONAL_ONE_ARG_STRING_DEFAULT); + ParseResult result = parse("root7", "a"); + assertThat(result.messageResults()).isEmpty(); + assertThat(result.argumentResults()).satisfiesExactly( + r -> { + assertThat(r.value()).isEqualTo("a"); + assertThat(r.position()).isEqualTo(0); + } + ); + assertThat(result.optionResults()).isNotNull().satisfiesExactly( + r -> { + assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg1" }); + assertThat(r.value()).isEqualTo("a"); + } + ); + } + + @Test + void noArgsGetsOnlyDefaults() { + register(ROOT7_POSITIONAL_TWO_ARG_STRING_DEFAULT); + ParseResult result = parse("root7"); + assertThat(result.messageResults()).isEmpty(); + assertThat(result.argumentResults()).isEmpty(); + assertThat(result.optionResults()).isNotNull().satisfiesExactly( + r -> { + assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg1" }); + assertThat(r.value()).isEqualTo("arg1default"); + }, + r -> { + assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg2" }); + assertThat(r.value()).isEqualTo("arg2default"); + } + ); + } + + @Test + void positionOverridesDefaultKeepsDefault() { + register(ROOT7_POSITIONAL_TWO_ARG_STRING_DEFAULT); + ParseResult result = parse("root7", "a"); + assertThat(result.messageResults()).isEmpty(); + assertThat(result.argumentResults()).satisfiesExactly( + r -> { + assertThat(r.value()).isEqualTo("a"); + assertThat(r.position()).isEqualTo(0); + } + ); + assertThat(result.optionResults()).isNotNull().satisfiesExactly( + r -> { + assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg1" }); + assertThat(r.value()).isEqualTo("a"); + }, + r -> { + assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg2" }); + assertThat(r.value()).isEqualTo("arg2default"); + } + ); + } + + @Test + void positionOverridesDefaultsKeepsDefault() { + register(ROOT7_POSITIONAL_TWO_ARG_STRING_DEFAULT); + ParseResult result = parse("root7", "a", "b"); + assertThat(result.messageResults()).isEmpty(); + assertThat(result.argumentResults()).satisfiesExactly( + r -> { + assertThat(r.value()).isEqualTo("a"); + assertThat(r.position()).isEqualTo(0); + }, + r -> { + assertThat(r.value()).isEqualTo("b"); + assertThat(r.position()).isEqualTo(1); + } + ); + assertThat(result.optionResults()).isNotNull().satisfiesExactly( + r -> { + assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg1" }); + assertThat(r.value()).isEqualTo("a"); + }, + r -> { + assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg2" }); + assertThat(r.value()).isEqualTo("b"); + } + ); + } + + @Test + void positionOverridesDefaultsKeepsDefaultWhenOption() { + register(ROOT7_POSITIONAL_TWO_ARG_STRING_DEFAULT); + ParseResult result = parse("root7", "--arg1", "a", "b"); + assertThat(result.messageResults()).isEmpty(); + assertThat(result.argumentResults()).satisfiesExactly( + r -> { + assertThat(r.value()).isEqualTo("b"); + assertThat(r.position()).isEqualTo(0); + } + ); + assertThat(result.optionResults()).isNotNull().satisfiesExactly( + r -> { + assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg1" }); + assertThat(r.value()).isEqualTo("a"); + }, + r -> { + assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg2" }); + assertThat(r.value()).isEqualTo("b"); + } + ); + } + + @Test + void positionWithLastHavingNoDefault() { + register(ROOT7_POSITIONAL_TWO_ARG_STRING_DEFAULT_ONE_NODEFAULT); + ParseResult result; + + result = parse("root7", "--arg1", "a", "--arg2", "b", "c"); + assertThat(result.messageResults()).isEmpty(); + assertThat(result.argumentResults()).satisfiesExactly( + r -> { + assertThat(r.value()).isEqualTo("c"); + assertThat(r.position()).isEqualTo(0); + } + ); + assertThat(result.optionResults()).isNotNull().satisfiesExactly( + r -> { + assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg1" }); + assertThat(r.value()).isEqualTo("a"); + }, + r -> { + assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg2" }); + assertThat(r.value()).isEqualTo("b"); + }, + r -> { + assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg3" }); + assertThat(r.value()).isEqualTo("c"); + } + ); + } + + @Test + void tooManyArgsInMiddleShouldCreateError() { + register(ROOT7_POSITIONAL_TWO_ARG_STRING_DEFAULT_ONE_NODEFAULT); + ParseResult result; + + result = parse("root7", "--arg1", "a", "b", "--arg2", "c", "d"); + assertThat(result.messageResults()).satisfiesExactlyInAnyOrder( + message -> { + ParserAssertions.assertThat(message.parserMessage()).hasCode(2004).hasType(ParserMessage.Type.ERROR); + } + ); + assertThat(result.argumentResults()).satisfiesExactly( + r -> { + assertThat(r.value()).isEqualTo("b"); + assertThat(r.position()).isEqualTo(0); + }, + r -> { + assertThat(r.value()).isEqualTo("d"); + assertThat(r.position()).isEqualTo(0); + } + ); + assertThat(result.optionResults()).isNotNull().satisfiesExactly( + r -> { + assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg1" }); + assertThat(r.value()).isEqualTo("a"); + }, + r -> { + assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg2" }); + assertThat(r.value()).isEqualTo("c"); + }, + r -> { + assertThat(r.option().getLongNames()).isEqualTo(new String[] { "arg3" }); + assertThat(r.value()).isEqualTo("b"); + } + ); + } + } } diff --git a/spring-shell-samples/spring-shell-sample-e2e/src/main/java/org/springframework/shell/samples/e2e/PositionalArgumentsCommands.java b/spring-shell-samples/spring-shell-sample-e2e/src/main/java/org/springframework/shell/samples/e2e/PositionalArgumentsCommands.java new file mode 100644 index 00000000..b94fe8c9 --- /dev/null +++ b/spring-shell-samples/spring-shell-sample-e2e/src/main/java/org/springframework/shell/samples/e2e/PositionalArgumentsCommands.java @@ -0,0 +1,71 @@ +/* + * Copyright 2023 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.shell.samples.e2e; + +import org.springframework.shell.command.CommandRegistration.OptionArity; +import org.springframework.shell.command.annotation.Command; +import org.springframework.shell.command.annotation.Option; +import org.springframework.shell.standard.ShellComponent; +import org.springframework.shell.standard.ShellMethod; +import org.springframework.shell.standard.ShellOption; + +public class PositionalArgumentsCommands { + + @ShellComponent + public static class LegacyAnnotation extends BaseE2ECommands { + + @ShellMethod(key = LEGACY_ANNO + "positional-args-1", group = GROUP) + public String testPositionalArgs1( + @ShellOption(arity = 1, defaultValue = ShellOption.NULL) String arg1, + @ShellOption(arity = 1, defaultValue = ShellOption.NULL) String arg2, + @ShellOption(arity = 1, defaultValue = ShellOption.NULL) String arg3 + ) { + return String.format("Hi arg1='%s' arg2='%s' arg3='%s'", arg1, arg2, arg3); + } + + @ShellMethod(key = LEGACY_ANNO + "positional-args-2", group = GROUP) + public String testPositionalArgs2( + @ShellOption(arity = 1, defaultValue = "defaultArg1") String arg1, + @ShellOption(arity = 1, defaultValue = "defaultArg2") String arg2, + @ShellOption(arity = 1, defaultValue = ShellOption.NULL) String arg3 + ) { + return String.format("Hi arg1='%s' arg2='%s' arg3='%s'", arg1, arg2, arg3); + } + } + + @Command(command = BaseE2ECommands.ANNO, group = BaseE2ECommands.GROUP) + public static class Annotation extends BaseE2ECommands { + + @Command(command = "positional-args-1") + public String testPositionalArgs1( + @Option(arity = OptionArity.EXACTLY_ONE) String arg1, + @Option(arity = OptionArity.EXACTLY_ONE) String arg2, + @Option(arity = OptionArity.EXACTLY_ONE) String arg3 + ) { + return String.format("Hi arg1='%s' arg2='%s' arg3='%s'", arg1, arg2, arg3); + } + + @Command(command = "positional-args-2") + public String testPositionalArgs2( + @Option(arity = OptionArity.EXACTLY_ONE, defaultValue = "defaultArg1") String arg1, + @Option(arity = OptionArity.EXACTLY_ONE, defaultValue = "defaultArg2") String arg2, + @Option(arity = OptionArity.EXACTLY_ONE) String arg3 + ) { + return String.format("Hi arg1='%s' arg2='%s' arg3='%s'", arg1, arg2, arg3); + } + } + +}