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 #699
- Fixes #700

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

@@ -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/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/main/java/org/springframework/shell/command/parser/Parser.java

@@ -27,6 +27,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.core.convert.support.DefaultConversionService;
 import org.springframework.shell.command.CommandOption;
 import org.springframework.shell.command.CommandRegistration;
@@ -204,7 +205,8 @@ public interface Parser {
 						optionResults.add(OptionResult.of(o, null));
 					}
 					else {
-						Object value = convertOptionType(o, asdf);
+						Object toConvertValue = asdf.size() == 1 ? asdf.get(0) : asdf;
+						Object value = convertOptionType(o, toConvertValue);
 						optionResults.add(OptionResult.of(o, value));
 					}
 
@@ -362,8 +364,11 @@ public interface Parser {
 				return true;
 			}
 			if (conversionService != null && option.getType() != null && value != null) {
-				if (conversionService.canConvert(value.getClass(), option.getType().getRawClass())) {
-					value = conversionService.convert(value, option.getType().getRawClass());
+				Object source = value;
+				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;

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

@@ -0,0 +1,257 @@
+/*
+ * 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.BeforeEach;
+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 {
+
+	private CommandExecution execution;
+	private CommandCatalog commandCatalog;
+
+	@BeforeEach
+	public void setupCommandExecutionTests() {
+		commandCatalog = CommandCatalog.of();
+		DefaultConversionService conversionService = new DefaultConversionService();
+		conversionService.addConverter(new StringToMyPojo2Converter());
+		List<HandlerMethodArgumentResolver> resolvers = new ArrayList<>();
+		resolvers.add(new ArgumentHeaderMethodArgumentResolver(conversionService, null));
+		resolvers.add(new CommandContextMethodArgumentResolver());
+		execution = CommandExecution.of(resolvers, null, null, conversionService, commandCatalog);
+	}
+
+	@Test
+	public void testCustomPojo() {
+		Pojo1 pojo1 = new Pojo1();
+
+		CommandRegistration r1 = CommandRegistration.builder()
+			.command("command1")
+			.description("help")
+			.withOption()
+				.longNames("arg1")
+				.and()
+			.withTarget()
+				.method(pojo1, "method1")
+				.and()
+			.build();
+		commandCatalog.register(r1);
+		execution.evaluate(new String[] { "command1", "--arg1", "myarg1value" });
+		assertThat(pojo1.method1Pojo2).isNotNull();
+	}
+
+	@Test
+	public void testCustomPojoArray() {
+		Pojo1 pojo1 = new Pojo1();
+
+		CommandRegistration r1 = CommandRegistration.builder()
+			.command("command1")
+			.description("help")
+			.withOption()
+				.longNames("arg1")
+				.and()
+			.withTarget()
+				.method(pojo1, "method2")
+				.and()
+			.build();
+		commandCatalog.register(r1);
+		execution.evaluate(new String[] { "command1", "--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() {
+		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();
+		commandCatalog.register(r1);
+		execution.evaluate(new String[] { "command1", "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() {
+		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();
+		commandCatalog.register(r1);
+		execution.evaluate(new String[] { "command1", "--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() {
+		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();
+		commandCatalog.register(r1);
+		execution.evaluate(new String[] { "command1", "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() {
+		Pojo1 pojo1 = new Pojo1();
+
+		CommandRegistration r1 = CommandRegistration.builder()
+			.command("command1")
+			.description("help")
+			.withOption()
+				.longNames("arg1")
+				.and()
+			.withTarget()
+				.method(pojo1, "method4")
+				.and()
+			.build();
+		commandCatalog.register(r1);
+		execution.evaluate(new String[] { "command1", "--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() {
+		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();
+		commandCatalog.register(r1);
+		execution.evaluate(new String[] { "command1", "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");
+	}
+
+	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/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.command.annotation.Command;
@@ -56,6 +57,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
@@ -141,12 +149,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 -> {
@@ -183,11 +192,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 {
@@ -218,14 +222,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 {
+
+	}
 }