Fix parser handling of positional args

- Change to have better support for args like:

    "--arg1 a --arg2 b"
    "--arg1 a --arg2 b c"
    "--arg1 a c --arg2 b c"
    "c --arg1 a --arg2 b"

  where option can have default values and position of
  positional args doesn't matter that much.
- Make parser to be aware of if it's handling last option
  so that we can differentiate if error can be given i.e.
  with too many args, etc.
- Backport #795
- Fixes #797

Author: jvalkeal

spring-shell-core/src/main/java/org/springframework/shell/command/parser/Parser.java

@@ -149,6 +149,8 @@ class DefaultNodeVisitor extends AbstractNodeVisitor {
 		private List<OptionNode> invalidOptionNodes = new ArrayList<>();
 		private List<ArgumentResult> 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 @@ protected ParseResult buildResult() {
 				messageResults.addAll(commonMessageResults);
 				messageResults.addAll(validateOptionIsValid(registration));
 
-
-				// add options with default values
-				Set<CommandOption> 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<CommandOption> 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<CommandOption> 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<String> 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 @@ protected ParseResult buildResult() {
 
 					List<String> 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 @@ protected ParseResult buildResult() {
 					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 @@ protected void onExitDirectiveNode(DirectiveNode node) {
 
 		@Override
 		protected void onEnterRootCommandNode(CommandNode node) {
+			expectedOptionCount = optionCountInCommand(node);
 			resolvedCommmand.add(node.getCommand());
 		}
 
@@ -243,6 +258,7 @@ protected void onExitRootCommandNode(CommandNode node) {
 
 		@Override
 		protected void onEnterCommandNode(CommandNode node) {
+			expectedOptionCount = optionCountInCommand(node);
 			resolvedCommmand.add(node.getCommand());
 		}
 
@@ -254,6 +270,7 @@ protected void onExitCommandNode(CommandNode node) {
 
 		@Override
 		protected void onEnterOptionNode(OptionNode node) {
+			optionPos++;
 			commandArgumentPos = 0;
 			currentOptions.clear();
 			currentOptionArgument.clear();
@@ -307,16 +324,40 @@ protected void onExitOptionNode(OptionNode node) {
 					int max = currentOption.getArityMax() > 0 ? currentOption.getArityMax() : Integer.MAX_VALUE;
 					max = Math.min(max, currentOptionArgument.size());
 					List<String> toUse = currentOptionArgument.subList(0, max);
+					List<String> 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 @@ private List<MessageResult> validateOptionIsValid(CommandRegistration registrati
 				})
 				.collect(Collectors.toList());
 		}
+
+		private static long optionCountInCommand(CommandNode node) {
+			if (node == null) {
+				return 0;
+			}
+			return node.getChildren().stream().filter(n -> n instanceof OptionNode).count();
+		}
 	}
 }

spring-shell-core/src/test/java/org/springframework/shell/command/parser/AbstractParsingTests.java

@@ -23,6 +23,7 @@
 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<String, CommandRegistration> registrations = new HashMap<>();
 
 	@BeforeEach