Fix option type parsing

- In `CommandRegistration` add `ResolvableType` for `OptionSpec` giving
  more spesific handling of a type.
- In `CommandParser` handle source and target types so that we
  have generics with `List`, `Set` and arrays working better.
- In `HandlerMethodArgumentResolver` add better handling for
  `ConversionService` for generic types.
- In `StandardMethodTargetRegistrar` add better types via `ResolvableType`
  now that `CommandRegistration` support it.
- In `OptionConversionCommands` remove converter from `String` to `Set` as
  now things should work as is if generic in a `Set` has a converter.
- Backport #694
- Fixes #699

spring-shell-core/src/main/java/org/springframework/shell/command/CommandExecution.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.
@@ -24,8 +24,10 @@ import jakarta.validation.Validator;
 import org.jline.terminal.Terminal;
 
 import org.springframework.core.MethodParameter;
+import org.springframework.core.ResolvableType;
 import org.springframework.core.annotation.Order;
 import org.springframework.core.convert.ConversionService;
+import org.springframework.core.convert.TypeDescriptor;
 import org.springframework.messaging.Message;
 import org.springframework.messaging.handler.invocation.HandlerMethodArgumentResolver;
 import org.springframework.messaging.support.MessageBuilder;
@@ -253,7 +255,13 @@ public interface CommandExecution {
 
 		@Override
 		public Object resolveArgument(MethodParameter parameter, Message<?> message) throws Exception {
-			return conversionService.convert(paramValues.get(parameter.getParameterName()), parameter.getParameterType());
+			Object source = paramValues.get(parameter.getParameterName());
+			if (source == null) {
+				return null;
+			}
+			TypeDescriptor sourceType = new TypeDescriptor(ResolvableType.forClass(source.getClass()), null, null);
+			TypeDescriptor targetType = new TypeDescriptor(parameter);
+			return conversionService.convert(source, sourceType, targetType);
 		}
 
 	}

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

@@ -28,6 +28,7 @@ import java.util.stream.Stream;
 
 import org.springframework.core.ResolvableType;
 import org.springframework.core.convert.ConversionService;
+import org.springframework.core.convert.TypeDescriptor;
 import org.springframework.shell.Utils;
 import org.springframework.util.StringUtils;
 
@@ -280,7 +281,11 @@ public interface CommandParser {
 					if (!oargs.isEmpty() && !oargs.get(0).startsWith("-")) {
 						// as we now have a candicate option, try to see if there is a
 						// conversion we can do and the use it.
-						Object value = convertOptionType(o, oargs);
+						Object convertSource = oargs;
+						if (oargs.size() == 1) {
+							convertSource = oargs.get(0);
+						}
+						Object value = convertOptionType(o, convertSource);
 						results.add(new DefaultCommandParserResult(o, value));
 						requiredOptions.remove(o);
 					}
@@ -299,8 +304,11 @@ public interface CommandParser {
 
 		private Object convertOptionType(CommandOption option, Object value) {
 			if (conversionService != null && option.getType() != null && value != null) {
-				if (conversionService.canConvert(value.getClass(), option.getType().getRawClass())) {
+				Object source = value;
-					value = conversionService.convert(value, option.getType().getRawClass());
+				TypeDescriptor sourceType = new TypeDescriptor(ResolvableType.forClass(source.getClass()), null, null);
+				TypeDescriptor targetType = new TypeDescriptor(option.getType(), null, null);
+				if (conversionService.canConvert(sourceType, targetType)) {
+					value = conversionService.convert(source, sourceType, targetType);
 				}
 			}
 			return value;
@@ -472,8 +480,14 @@ public interface CommandParser {
 						value = Boolean.parseBoolean(arguments.get(0));
 					}
 				}
+				// see for collection section below
 				else if (type != null && type.isArray()) {
-					value = arguments.stream().collect(Collectors.toList()).toArray();
+					if (arguments.size() == 1) {
+						value = arguments.get(0);
+					}
+					else {
+						value = arguments.stream().collect(Collectors.toList());
+					}
 				}
 				// if it looks like type is a collection just get as list
 				// as conversion will happen later. we just need to know

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

@@ -175,13 +175,27 @@ public interface CommandRegistration {
 		OptionSpec shortNames(Character... names);
 
 		/**
-		 * Define a type for an option.
+		 * Define a type for an option. This method is a shortcut for
+		 * {@link #type(ResolvableType)} which is a preferred way to
+		 * define type with generics. Will override one from
+		 * {@link #type(ResolvableType)}.
 		 *
 		 * @param type the type
 		 * @return option spec for chaining
+		 * @see #type(ResolvableType)
 		 */
 		OptionSpec type(Type type);
 
+		/**
+		 * Define a {@link ResolvableType} for an option. This method is
+		 * a preferred way to define type with generics. Will override one
+		 * from {@link #type(Type)}.
+		 *
+		 * @param type the resolvable type
+		 * @return option spec for chaining
+		 */
+		OptionSpec type(ResolvableType type);
+
 		/**
 		 * Define a {@code description} for an option.
 		 *
@@ -817,6 +831,12 @@ public interface CommandRegistration {
 			return this;
 		}
 
+		@Override
+		public OptionSpec type(ResolvableType type) {
+			this.type = type;
+			return this;
+		}
+
 		@Override
 		public OptionSpec description(String description) {
 			this.description = description;

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

@@ -0,0 +1,251 @@
+/*
+ * 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.command;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Set;
+
+import org.junit.jupiter.api.Test;
+
+import org.springframework.core.ResolvableType;
+import org.springframework.core.convert.converter.Converter;
+import org.springframework.core.convert.support.DefaultConversionService;
+import org.springframework.messaging.handler.invocation.HandlerMethodArgumentResolver;
+import org.springframework.shell.command.CommandRegistration.OptionArity;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class CommandExecutionCustomConversionTests {
+
+	@Test
+	public void testCustomPojo() {
+		CommandExecution execution = build();
+		Pojo1 pojo1 = new Pojo1();
+
+		CommandRegistration r1 = CommandRegistration.builder()
+			.command("command1")
+			.description("help")
+			.withOption()
+				.longNames("arg1")
+				.and()
+			.withTarget()
+				.method(pojo1, "method1")
+				.and()
+			.build();
+		execution.evaluate(r1, new String[]{"--arg1", "myarg1value"});
+		assertThat(pojo1.method1Pojo2).isNotNull();
+	}
+
+	@Test
+	public void testCustomPojoArray() {
+		CommandExecution execution = build();
+		Pojo1 pojo1 = new Pojo1();
+
+		CommandRegistration r1 = CommandRegistration.builder()
+			.command("command1")
+			.description("help")
+			.withOption()
+				.longNames("arg1")
+				.and()
+			.withTarget()
+				.method(pojo1, "method2")
+				.and()
+			.build();
+		execution.evaluate(r1, new String[]{"--arg1", "a,b"});
+		assertThat(pojo1.method2Pojo2).isNotNull();
+		assertThat(pojo1.method2Pojo2.length).isEqualTo(2);
+		assertThat(pojo1.method2Pojo2[0].arg).isEqualTo("a");
+		assertThat(pojo1.method2Pojo2[1].arg).isEqualTo("b");
+	}
+
+	@Test
+	public void testCustomPojoArrayPositional() {
+		CommandExecution execution = build();
+		Pojo1 pojo1 = new Pojo1();
+
+		CommandRegistration r1 = CommandRegistration.builder()
+			.command("command1")
+			.description("help")
+			.withOption()
+				.longNames("arg1")
+				.arity(OptionArity.ONE_OR_MORE)
+				.position(0)
+				.and()
+			.withTarget()
+				.method(pojo1, "method2")
+				.and()
+			.build();
+		execution.evaluate(r1, new String[]{"a,b"});
+		assertThat(pojo1.method2Pojo2).isNotNull();
+		assertThat(pojo1.method2Pojo2.length).isEqualTo(2);
+		assertThat(pojo1.method2Pojo2[0].arg).isEqualTo("a");
+		assertThat(pojo1.method2Pojo2[1].arg).isEqualTo("b");
+	}
+
+	@Test
+	public void testCustomPojoList() {
+		CommandExecution execution = build();
+		Pojo1 pojo1 = new Pojo1();
+
+		ResolvableType type = ResolvableType.forClassWithGenerics(List.class, Pojo1.class);
+
+		CommandRegistration r1 = CommandRegistration.builder()
+			.command("command1")
+			.description("help")
+			.withOption()
+				.longNames("arg1")
+				.type(type)
+				.and()
+			.withTarget()
+				.method(pojo1, "method3")
+				.and()
+			.build();
+		execution.evaluate(r1, new String[]{"--arg1", "a,b"});
+		assertThat(pojo1.method3Pojo2).isNotNull();
+		assertThat(pojo1.method3Pojo2.size()).isEqualTo(2);
+		assertThat(pojo1.method3Pojo2.get(0)).isInstanceOf(Pojo2.class);
+		assertThat(pojo1.method3Pojo2.get(0).arg).isEqualTo("a");
+		assertThat(pojo1.method3Pojo2.get(1).arg).isEqualTo("b");
+	}
+
+	@Test
+	public void testCustomPojoListPositional() {
+		CommandExecution execution = build();
+		Pojo1 pojo1 = new Pojo1();
+
+		ResolvableType type = ResolvableType.forClassWithGenerics(List.class, Pojo1.class);
+
+		CommandRegistration r1 = CommandRegistration.builder()
+			.command("command1")
+			.description("help")
+			.withOption()
+				.longNames("arg1")
+				.arity(OptionArity.ONE_OR_MORE)
+				.position(0)
+				.type(type)
+				.and()
+			.withTarget()
+				.method(pojo1, "method3")
+				.and()
+			.build();
+		execution.evaluate(r1, new String[]{"a,b"});
+		assertThat(pojo1.method3Pojo2).isNotNull();
+		assertThat(pojo1.method3Pojo2.size()).isEqualTo(2);
+		assertThat(pojo1.method3Pojo2.get(0)).isInstanceOf(Pojo2.class);
+		assertThat(pojo1.method3Pojo2.get(0).arg).isEqualTo("a");
+		assertThat(pojo1.method3Pojo2.get(1).arg).isEqualTo("b");
+	}
+
+	@Test
+	public void testCustomPojoSet() {
+		CommandExecution execution = build();
+		Pojo1 pojo1 = new Pojo1();
+
+		CommandRegistration r1 = CommandRegistration.builder()
+			.command("command1")
+			.description("help")
+			.withOption()
+				.longNames("arg1")
+				.and()
+			.withTarget()
+				.method(pojo1, "method4")
+				.and()
+			.build();
+		execution.evaluate(r1, new String[]{"--arg1", "a,b"});
+		assertThat(pojo1.method4Pojo2).isNotNull();
+		assertThat(pojo1.method4Pojo2.size()).isEqualTo(2);
+		assertThat(pojo1.method4Pojo2.iterator().next()).isInstanceOf(Pojo2.class);
+		assertThat(pojo1.method4Pojo2.stream().map(pojo -> pojo.arg).toList()).containsExactly("a", "b");
+	}
+
+	@Test
+	public void testCustomPojoSetPositional() {
+		CommandExecution execution = build();
+		Pojo1 pojo1 = new Pojo1();
+
+		CommandRegistration r1 = CommandRegistration.builder()
+			.command("command1")
+			.description("help")
+			.withOption()
+				.longNames("arg1")
+				.arity(OptionArity.ONE_OR_MORE)
+				.position(0)
+				.and()
+			.withTarget()
+				.method(pojo1, "method4")
+				.and()
+			.build();
+		execution.evaluate(r1, new String[]{"a,b"});
+		assertThat(pojo1.method4Pojo2).isNotNull();
+		assertThat(pojo1.method4Pojo2.size()).isEqualTo(2);
+		assertThat(pojo1.method4Pojo2.iterator().next()).isInstanceOf(Pojo2.class);
+		assertThat(pojo1.method4Pojo2.stream().map(pojo -> pojo.arg).toList()).containsExactly("a", "b");
+	}
+
+	private CommandExecution build() {
+		DefaultConversionService conversionService = new DefaultConversionService();
+		conversionService.addConverter(new StringToMyPojo2Converter());
+		List<HandlerMethodArgumentResolver> resolvers = new ArrayList<>();
+		resolvers.add(new ArgumentHeaderMethodArgumentResolver(conversionService, null));
+		resolvers.add(new CommandContextMethodArgumentResolver());
+		return CommandExecution.of(resolvers, null, null, conversionService);
+	}
+
+	static class StringToMyPojo2Converter implements Converter<String, Pojo2> {
+
+		@Override
+		public Pojo2 convert(String from) {
+			return new Pojo2(from);
+		}
+	}
+
+	static class Pojo1 {
+
+		public Pojo2 method1Pojo2;
+		public Pojo2[] method2Pojo2;
+		public List<Pojo2> method3Pojo2;
+		public Set<Pojo2> method4Pojo2;
+
+		public void method1(Pojo2 arg1) {
+			method1Pojo2 = arg1;
+		}
+
+		public void method2(Pojo2[] arg1) {
+			method2Pojo2 = arg1;
+		}
+
+		public void method3(List<Pojo2> arg1) {
+			method3Pojo2 = arg1;
+		}
+
+		public void method4(Set<Pojo2> arg1) {
+			method4Pojo2 = arg1;
+		}
+	}
+
+	static class Pojo2 {
+
+		public String arg;
+
+		public Pojo2() {
+		}
+
+		public Pojo2(String arg) {
+			this.arg = arg;
+		}
+	}
+}

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

@@ -419,7 +419,7 @@ public class CommandParserTests extends AbstractCommandTests {
 	}
 
 	@Test
-	public void testMapPositionalArgs11() {
+	public void testMapPositionalArgsNoTypeDefined() {
 		CommandOption option1 = longOption("arg1", 0, 1, 1);
 		CommandOption option2 = longOption("arg2", 1, 1, 2);
 		List<CommandOption> options = Arrays.asList(option1, option2);
@@ -430,7 +430,30 @@ public class CommandParserTests extends AbstractCommandTests {
 		assertThat(results.results().get(1).option()).isSameAs(option2);
 		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"));
+		assertThat(results.results().get(1).value()).isEqualTo("2");
+	}
+
+	@Test
+	public void testMapPositionalArgsWhenTypeList() {
+		CommandOption option1 = CommandOption.of(
+				new String[] { "arg1" },
+				null,
+				null,
+				ResolvableType.forType(List.class),
+				true,
+				null,
+				0,
+				1,
+				1,
+				null,
+				null);
+
+		List<CommandOption> options = Arrays.asList(option1);
+		String[] args = new String[]{"1"};
+		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(Arrays.asList("1"));
 	}
 
 	@Test

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

@@ -213,6 +213,26 @@ public class CommandRegistrationTests extends AbstractCommandTests {
 		assertThat(registration.getOptions().get(0).getLabel()).isEqualTo("mylabel");
 	}
 
+	@Test
+	public void testOptionWithResolvableType() {
+		ResolvableType rtype = ResolvableType.forClassWithGenerics(List.class, String.class);
+		CommandRegistration registration = CommandRegistration.builder()
+			.command("command1")
+			.withOption()
+				.longNames("arg")
+				.type(rtype)
+				.and()
+			.withTarget()
+				.consumer(ctx -> {})
+				.and()
+			.build();
+		assertThat(registration.getCommand()).isEqualTo("command1");
+		assertThat(registration.getOptions()).hasSize(1);
+		assertThat(registration.getOptions().get(0).getType()).satisfies(type -> {
+			assertThat(type).isEqualTo(rtype);
+		});
+	}
+
 	@Test
 	public void testOptionWithRequired() {
 		CommandRegistration registration = CommandRegistration.builder()

spring-shell-samples/src/main/java/org/springframework/shell/samples/e2e/OptionConversionCommands.java

@@ -16,11 +16,12 @@
 package org.springframework.shell.samples.e2e;
 
 import java.util.Arrays;
-import java.util.HashSet;
+import java.util.List;
 import java.util.Set;
 
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
+import org.springframework.core.ResolvableType;
 import org.springframework.core.convert.converter.Converter;
 import org.springframework.shell.command.CommandRegistration;
 import org.springframework.shell.standard.ShellComponent;
@@ -54,6 +55,13 @@ public class OptionConversionCommands {
 			return "Hello " + arg1;
 		}
 
+		@ShellMethod(key = LEGACY_ANNO + "option-conversion-customlist", group = GROUP)
+		public String optionConversionCustomListAnnotation(
+			@ShellOption List<MyPojo> arg1
+		) {
+			return "Hello " + arg1;
+		}
+
 		@ShellMethod(key = LEGACY_ANNO + "option-conversion-customarray", group = GROUP)
 		public String optionConversionCustomArrayAnnotation(
 			@ShellOption MyPojo[] arg1
@@ -103,12 +111,13 @@ public class OptionConversionCommands {
 
 		@Bean
 		public CommandRegistration optionConversionCustomSetRegistration() {
-			return getBuilder()
+			ResolvableType rtype = ResolvableType.forClassWithGenerics(Set.class, MyPojo.class);
+			return CommandRegistration.builder()
 				.command(REG, "option-conversion-customset")
 				.group(GROUP)
 				.withOption()
 					.longNames("arg1")
-					.type(Set.class)
+					.type(rtype)
 					.and()
 				.withTarget()
 					.function(ctx -> {
@@ -145,11 +154,6 @@ public class OptionConversionCommands {
 		public Converter<String, MyPojo> stringToMyPojoConverter() {
 			return new StringToMyPojoConverter();
 		}
-
-		@Bean
-		public Converter<String, Set<MyPojo>> stringToMyPojoSetConverter() {
-			return new StringToMyPojoSetConverter();
-		}
 	}
 
 	public static class MyPojo {
@@ -180,14 +184,4 @@ public class OptionConversionCommands {
 			return new MyPojo(from);
 		}
 	}
-
-	static class StringToMyPojoSetConverter implements Converter<String, Set<MyPojo>> {
-
-		@Override
-		public Set<MyPojo> convert(String from) {
-			Set<MyPojo> set = new HashSet<>();
-			set.add(new MyPojo(from));
-			return set;
-		}
-	}
 }

spring-shell-standard/src/main/java/org/springframework/shell/standard/StandardMethodTargetRegistrar.java

@@ -16,6 +16,7 @@
 package org.springframework.shell.standard;
 
 import java.lang.reflect.Method;
+import java.lang.reflect.Type;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -146,8 +147,9 @@ public class StandardMethodTargetRegistrar implements MethodTargetRegistrar {
 						if (!longNames.isEmpty() || !shortNames.isEmpty()) {
 							log.debug("Registering longNames='{}' shortNames='{}'", longNames, shortNames);
 							Class<?> parameterType = mp.getParameterType();
+							Type genericParameterType = mp.getGenericParameterType();
 							OptionSpec optionSpec = builder.withOption()
-								.type(parameterType)
+								.type(genericParameterType)
 								.longNames(longNames.toArray(new String[0]))
 								.shortNames(shortNames.toArray(new Character[0]))
 								.position(mp.getParameterIndex())

spring-shell-standard/src/test/java/org/springframework/shell/standard/StandardMethodTargetRegistrarTests.java

@@ -17,6 +17,7 @@
 package org.springframework.shell.standard;
 
 import java.util.Map;
+import java.util.Set;
 
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
@@ -501,4 +502,31 @@ public class StandardMethodTargetRegistrarTests {
 		public void foo1(@ShellOption("x") boolean arg1) {
 		}
 	}
+
+	@Test
+	void OptionWithCustomType() {
+		applicationContext = new AnnotationConfigApplicationContext(OptionWithCustomType.class);
+		registrar = new StandardMethodTargetRegistrar(applicationContext, builder);
+		registrar.register(catalog);
+
+		assertThat(catalog.getRegistrations().get("foo1")).isNotNull();
+		assertThat(catalog.getRegistrations().get("foo1")).satisfies(reg -> {
+			assertThat(reg.getOptions().get(0)).satisfies(option -> {
+				assertThat(option.getType().getGeneric(0).getType()).isEqualTo(Pojo.class);
+			});
+		});
+
+	}
+
+	@ShellComponent
+	public static class OptionWithCustomType {
+
+		@ShellMethod(value = "foo1", prefix = "-")
+		public void foo1(@ShellOption Set<Pojo> arg1) {
+		}
+	}
+
+	public static class Pojo {
+
+	}
 }