diff --git a/spring-shell-core/src/main/java/org/springframework/shell/command/CommandExecution.java b/spring-shell-core/src/main/java/org/springframework/shell/command/CommandExecution.java index b1b74c5b..9fe7374d 100644 --- a/spring-shell-core/src/main/java/org/springframework/shell/command/CommandExecution.java +++ b/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 javax.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; @@ -191,7 +193,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); } } 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 585a3bad..544c1d5e 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 @@ -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())) { - 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; @@ -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 diff --git a/spring-shell-core/src/main/java/org/springframework/shell/command/CommandRegistration.java b/spring-shell-core/src/main/java/org/springframework/shell/command/CommandRegistration.java index bebccfb3..c3130a89 100644 --- a/spring-shell-core/src/main/java/org/springframework/shell/command/CommandRegistration.java +++ b/spring-shell-core/src/main/java/org/springframework/shell/command/CommandRegistration.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. @@ -137,13 +137,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. * @@ -561,6 +575,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; diff --git a/spring-shell-core/src/test/java/org/springframework/shell/command/CommandExecutionCustomConversionTests.java b/spring-shell-core/src/test/java/org/springframework/shell/command/CommandExecutionCustomConversionTests.java new file mode 100644 index 00000000..fe3dc690 --- /dev/null +++ b/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 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 { + + @Override + public Pojo2 convert(String from) { + return new Pojo2(from); + } + } + + static class Pojo1 { + + public Pojo2 method1Pojo2; + public Pojo2[] method2Pojo2; + public List method3Pojo2; + public Set method4Pojo2; + + public void method1(Pojo2 arg1) { + method1Pojo2 = arg1; + } + + public void method2(Pojo2[] arg1) { + method2Pojo2 = arg1; + } + + public void method3(List arg1) { + method3Pojo2 = arg1; + } + + public void method4(Set arg1) { + method4Pojo2 = arg1; + } + } + + static class Pojo2 { + + public String arg; + + public Pojo2() { + } + + public Pojo2(String arg) { + this.arg = arg; + } + } +} 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 732a439d..a4ceadc7 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 @@ -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 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 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 diff --git a/spring-shell-core/src/test/java/org/springframework/shell/command/CommandRegistrationTests.java b/spring-shell-core/src/test/java/org/springframework/shell/command/CommandRegistrationTests.java index 7aae6e4e..af4a990a 100644 --- a/spring-shell-core/src/test/java/org/springframework/shell/command/CommandRegistrationTests.java +++ b/spring-shell-core/src/test/java/org/springframework/shell/command/CommandRegistrationTests.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. @@ -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() diff --git a/spring-shell-samples/src/main/java/org/springframework/shell/samples/e2e/OptionConversionCommands.java b/spring-shell-samples/src/main/java/org/springframework/shell/samples/e2e/OptionConversionCommands.java index 5d7e567d..79748e60 100644 --- a/spring-shell-samples/src/main/java/org/springframework/shell/samples/e2e/OptionConversionCommands.java +++ b/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 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() { + 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 stringToMyPojoConverter() { return new StringToMyPojoConverter(); } - - @Bean - public Converter> stringToMyPojoSetConverter() { - return new StringToMyPojoSetConverter(); - } } public static class MyPojo { @@ -180,14 +184,4 @@ public class OptionConversionCommands { return new MyPojo(from); } } - - static class StringToMyPojoSetConverter implements Converter> { - - @Override - public Set convert(String from) { - Set set = new HashSet<>(); - set.add(new MyPojo(from)); - return set; - } - } } diff --git a/spring-shell-standard/src/main/java/org/springframework/shell/standard/StandardMethodTargetRegistrar.java b/spring-shell-standard/src/main/java/org/springframework/shell/standard/StandardMethodTargetRegistrar.java index ca96ade2..67ad2c27 100644 --- a/spring-shell-standard/src/main/java/org/springframework/shell/standard/StandardMethodTargetRegistrar.java +++ b/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; @@ -141,8 +142,9 @@ public class StandardMethodTargetRegistrar implements MethodTargetRegistrar, App 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()) diff --git a/spring-shell-standard/src/test/java/org/springframework/shell/standard/StandardMethodTargetRegistrarTests.java b/spring-shell-standard/src/test/java/org/springframework/shell/standard/StandardMethodTargetRegistrarTests.java index d4222e16..5f8b694d 100644 --- a/spring-shell-standard/src/test/java/org/springframework/shell/standard/StandardMethodTargetRegistrarTests.java +++ b/spring-shell-standard/src/test/java/org/springframework/shell/standard/StandardMethodTargetRegistrarTests.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. @@ -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; @@ -500,4 +501,31 @@ public class StandardMethodTargetRegistrarTests { public void foo1(@ShellOption("x") boolean arg1) { } } + + @Test + void OptionWithCustomType() { + applicationContext = new AnnotationConfigApplicationContext(OptionWithCustomType.class); + registrar.setApplicationContext(applicationContext); + 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 arg1) { + } + } + + public static class Pojo { + + } }