From b853112e0b42a4fc2b8c1f4fd9f4cfaff5600fff Mon Sep 17 00:00:00 2001 From: Janne Valkealahti Date: Mon, 16 Jan 2023 17:47:21 +0000 Subject: [PATCH] Replace parser string joining with list - Issue in #622 is that its command type is `String` and internally some incoming arguments(it's List) were converted to String by joining with space. This caused one case with help command to get conversion via spring's ConversionService(CollectionToStringConverter) which joins by commas. That was we saw in failed example. - Remove needed joins in CommandParser and let it just pass List which then works better with ConversionService. - This then needs a `command` option type change from String to String[] which it really is as you should be able to give whole command as an argument. - Backport #622 - Fixes #623 --- .../shell/command/CommandParser.java | 10 +++------- .../shell/command/CommandParserTests.java | 18 +++++++++++++++--- .../shell/standard/commands/Help.java | 11 ++++++++--- .../shell/standard/commands/HelpTests.java | 6 +++--- 4 files changed, 29 insertions(+), 16 deletions(-) diff --git a/spring-shell-core/src/main/java/org/springframework/shell/command/CommandParser.java b/spring-shell-core/src/main/java/org/springframework/shell/command/CommandParser.java index 5c477a04..80962a8b 100644 --- a/spring-shell-core/src/main/java/org/springframework/shell/command/CommandParser.java +++ b/spring-shell-core/src/main/java/org/springframework/shell/command/CommandParser.java @@ -261,7 +261,7 @@ public interface CommandParser { } if (pop != null && pop.option == null) { if (!pop.args.isEmpty()) { - oargs.add(pop.args.stream().collect(Collectors.joining(" "))); + oargs.addAll(pop.args); } } } @@ -370,11 +370,7 @@ public interface CommandParser { .filter(co -> co.getDefaultValue() != null) .forEach(co -> { Object value = co.getDefaultValue(); - if (conversionService != null && co.getType() != null) { - if (conversionService.canConvert(co.getDefaultValue().getClass(), co.getType().getRawClass())) { - value = conversionService.convert(co.getDefaultValue(), co.getType().getRawClass()); - } - } + value = convertOptionType(co, value); results.add(ParserResult.of(co, Collections.emptyList(), value, null)); }); return ParserResults.of(results); @@ -478,7 +474,7 @@ public interface CommandParser { else { if (arityMax > 0) { int limit = Math.min(arguments.size(), arityMax); - value = arguments.stream().limit(limit).collect(Collectors.joining(" ")); + value = arguments.stream().limit(limit).collect(Collectors.toList()); unmapped.addAll(arguments.subList(limit, arguments.size())); } else { diff --git a/spring-shell-core/src/test/java/org/springframework/shell/command/CommandParserTests.java b/spring-shell-core/src/test/java/org/springframework/shell/command/CommandParserTests.java index 8b80b0dd..a4100d77 100644 --- a/spring-shell-core/src/test/java/org/springframework/shell/command/CommandParserTests.java +++ b/spring-shell-core/src/test/java/org/springframework/shell/command/CommandParserTests.java @@ -94,9 +94,9 @@ public class CommandParserTests extends AbstractCommandTests { CommandParserResults results = parser.parse(options, args); assertThat(results.results()).hasSize(2); assertThat(results.results().get(0).option()).isSameAs(option1); - assertThat(results.results().get(0).value()).isEqualTo("foo1 foo2"); + assertThat(results.results().get(0).value()).isEqualTo(Arrays.asList("foo1", "foo2")); assertThat(results.results().get(1).option()).isSameAs(option2); - assertThat(results.results().get(1).value()).isEqualTo("bar1 bar2"); + assertThat(results.results().get(1).value()).isEqualTo(Arrays.asList("bar1", "bar2")); assertThat(results.positional()).isEmpty(); } @@ -215,6 +215,18 @@ public class CommandParserTests extends AbstractCommandTests { assertThat(results.positional()).containsExactly("value", "foo"); } + @Test + public void testMappedFromArgToString() { + CommandOption option1 = longOption("arg1", ResolvableType.forType(String.class), false, 0, 1, 2); + List options = Arrays.asList(option1); + String[] args = new String[]{"--arg1", "value", "foo"}; + CommandParserResults results = parser.parse(options, args); + assertThat(results.results()).hasSize(1); + assertThat(results.results().get(0).option()).isSameAs(option1); + assertThat(results.results().get(0).value()).isEqualTo("value,foo"); + assertThat(results.positional()).isEmpty(); + } + @Test public void testShortOptionsCombined() { CommandOption optionA = shortOption('a'); @@ -381,7 +393,7 @@ public class CommandParserTests extends AbstractCommandTests { assertThat(results.results()).hasSize(2); assertThat(results.results().get(0).option()).isSameAs(option1); assertThat(results.results().get(1).option()).isSameAs(option2); - assertThat(results.results().get(0).value()).isEqualTo("1"); + assertThat(results.results().get(0).value()).isEqualTo(Arrays.asList("1")); // no type so we get raw list assertThat(results.results().get(1).value()).isEqualTo(Arrays.asList("2")); } diff --git a/spring-shell-standard-commands/src/main/java/org/springframework/shell/standard/commands/Help.java b/spring-shell-standard-commands/src/main/java/org/springframework/shell/standard/commands/Help.java index 64023816..b6d61b42 100644 --- a/spring-shell-standard-commands/src/main/java/org/springframework/shell/standard/commands/Help.java +++ b/spring-shell-standard-commands/src/main/java/org/springframework/shell/standard/commands/Help.java @@ -1,5 +1,5 @@ /* - * Copyright 2017-2022 the original author or authors. + * Copyright 2017-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. @@ -22,6 +22,8 @@ import java.io.UncheckedIOException; import java.nio.charset.StandardCharsets; import java.util.HashMap; import java.util.Map; +import java.util.stream.Collectors; +import java.util.stream.Stream; import org.jline.utils.AttributedString; @@ -75,13 +77,16 @@ public class Help extends AbstractShellComponent { @ShellMethod(value = "Display help about available commands") public AttributedString help( @ShellOption(defaultValue = ShellOption.NULL, valueProvider = CommandValueProvider.class, value = { "-C", - "--command" }, help = "The command to obtain help for.", arity = Integer.MAX_VALUE) String command) + "--command" }, help = "The command to obtain help for.", arity = Integer.MAX_VALUE) String[] command) throws IOException { if (command == null) { return renderCommands(); } else { - return renderCommand(command); + String commandStr = Stream.of(command) + .map(c -> c.trim()) + .collect(Collectors.joining(" ")); + return renderCommand(commandStr); } } diff --git a/spring-shell-standard-commands/src/test/java/org/springframework/shell/standard/commands/HelpTests.java b/spring-shell-standard-commands/src/test/java/org/springframework/shell/standard/commands/HelpTests.java index 37018ab5..54899ab3 100644 --- a/spring-shell-standard-commands/src/test/java/org/springframework/shell/standard/commands/HelpTests.java +++ b/spring-shell-standard-commands/src/test/java/org/springframework/shell/standard/commands/HelpTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2022 the original author or authors. + * Copyright 2022-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. @@ -121,7 +121,7 @@ public class HelpTests { .and() .build(); commandCatalog.register(registration); - String help = this.help.help("first-command").toString(); + String help = this.help.help(new String[] { "first-command" }).toString(); help = removeNewLines(help); assertThat(help).isEqualTo(sample()); } @@ -146,7 +146,7 @@ public class HelpTests { @Test public void testUnknownCommand() throws Exception { assertThatThrownBy(() -> { - this.help.help("some unknown command"); + this.help.help(new String[] { "some", "unknown", "command" }); }).isInstanceOf(IllegalArgumentException.class); }