From e469c148dfb8d952fa14cb2f8eb9cdc286f403a1 Mon Sep 17 00:00:00 2001 From: Janne Valkealahti Date: Sat, 18 Feb 2023 14:20:41 +0000 Subject: [PATCH] Use correct type with set - When target is set and only one option argument is given, we should not convert to list as user expects string to xxx Converter to work. - This is how it used to work and previous changes caused regression. - Bug is actually in an old parser and new parser works fine. - Backport #667 - Fixes #670 --- .../shell/command/CommandParser.java | 11 +- .../samples/e2e/OptionConversionCommands.java | 193 ++++++++++++++++++ .../e2e/OptionConversionCommandsTests.java | 57 ++++++ 3 files changed, 259 insertions(+), 2 deletions(-) create mode 100644 spring-shell-samples/src/main/java/org/springframework/shell/samples/e2e/OptionConversionCommands.java create mode 100644 spring-shell-samples/src/test/java/org/springframework/shell/samples/e2e/OptionConversionCommandsTests.java 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 099559ce..3f0e11bf 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 @@ -478,9 +478,16 @@ public interface CommandParser { // if it looks like type is a collection just get as list // as conversion will happen later. we just need to know // if user has Set, List, Collection, etc without worrying - // about generics. + // about generics. handle explicit size 1 so that we're + // able to handle string to xxx converter as that what + // user expects. else if (type != null && type.asCollection() != ResolvableType.NONE) { - value = arguments.stream().collect(Collectors.toList()); + if (arguments.size() == 1) { + value = arguments.get(0); + } + else { + value = arguments.stream().collect(Collectors.toList()); + } } else { if (!arguments.isEmpty()) { 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 new file mode 100644 index 00000000..eb1d4077 --- /dev/null +++ b/spring-shell-samples/src/main/java/org/springframework/shell/samples/e2e/OptionConversionCommands.java @@ -0,0 +1,193 @@ +/* + * 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.samples.e2e; + +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; + +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.core.convert.converter.Converter; +import org.springframework.shell.command.CommandRegistration; +import org.springframework.shell.standard.ShellComponent; +import org.springframework.shell.standard.ShellMethod; +import org.springframework.shell.standard.ShellOption; +import org.springframework.stereotype.Component; + +public class OptionConversionCommands { + + @ShellComponent + public static class LegacyAnnotation extends BaseE2ECommands { + + @ShellMethod(key = LEGACY_ANNO + "option-conversion-integer", group = GROUP) + public String optionConversionIntegerAnnotation( + @ShellOption Integer arg1 + ) { + return "Hello " + arg1; + } + + @ShellMethod(key = LEGACY_ANNO + "option-conversion-custom", group = GROUP) + public String optionConversionCustomAnnotation( + @ShellOption MyPojo arg1 + ) { + return "Hello " + arg1; + } + + @ShellMethod(key = LEGACY_ANNO + "option-conversion-customset", group = GROUP) + public String optionConversionCustomSetAnnotation( + @ShellOption Set arg1 + ) { + return "Hello " + arg1; + } + + @ShellMethod(key = LEGACY_ANNO + "option-conversion-customarray", group = GROUP) + public String optionConversionCustomArrayAnnotation( + @ShellOption MyPojo[] arg1 + ) { + return "Hello " + Arrays.asList(arg1); + } + } + + @Component + public static class Registration extends BaseE2ECommands { + + @Bean + public CommandRegistration optionConversionIntegerRegistration() { + return getBuilder() + .command(REG, "option-conversion-integer") + .group(GROUP) + .withOption() + .longNames("arg1") + .type(Integer.class) + .and() + .withTarget() + .function(ctx -> { + Integer arg1 = ctx.getOptionValue("arg1"); + return "Hello " + arg1; + }) + .and() + .build(); + } + + @Bean + public CommandRegistration optionConversionCustomRegistration() { + return getBuilder() + .command(REG, "option-conversion-custom") + .group(GROUP) + .withOption() + .longNames("arg1") + .type(MyPojo.class) + .and() + .withTarget() + .function(ctx -> { + MyPojo arg1 = ctx.getOptionValue("arg1"); + return "Hello " + arg1; + }) + .and() + .build(); + } + + @Bean + public CommandRegistration optionConversionCustomSetRegistration() { + return getBuilder() + .command(REG, "option-conversion-customset") + .group(GROUP) + .withOption() + .longNames("arg1") + .type(Set.class) + .and() + .withTarget() + .function(ctx -> { + Set arg1 = ctx.getOptionValue("arg1"); + return "Hello " + arg1; + }) + .and() + .build(); + } + + @Bean + public CommandRegistration optionConversionCustomArrayRegistration() { + return getBuilder() + .command(REG, "option-conversion-customarray") + .group(GROUP) + .withOption() + .longNames("arg1") + .type(MyPojo[].class) + .and() + .withTarget() + .function(ctx -> { + MyPojo[] arg1 = ctx.getOptionValue("arg1"); + return "Hello " + Arrays.asList(arg1); + }) + .and() + .build(); + } + } + + @Configuration(proxyBeanMethods = false) + public static class CommonConfiguration { + + @Bean + public Converter stringToMyPojoConverter() { + return new StringToMyPojoConverter(); + } + + @Bean + public Converter> stringToMyPojoSetConverter() { + return new StringToMyPojoSetConverter(); + } + } + + public static class MyPojo { + private String arg; + + public MyPojo(String arg) { + this.arg = arg; + } + + public String getArg() { + return arg; + } + + public void setArg(String arg) { + this.arg = arg; + } + + @Override + public String toString() { + return "MyPojo [arg=" + arg + "]"; + } + } + + static class StringToMyPojoConverter implements Converter { + + @Override + public MyPojo convert(String from) { + 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-samples/src/test/java/org/springframework/shell/samples/e2e/OptionConversionCommandsTests.java b/spring-shell-samples/src/test/java/org/springframework/shell/samples/e2e/OptionConversionCommandsTests.java new file mode 100644 index 00000000..d7bce991 --- /dev/null +++ b/spring-shell-samples/src/test/java/org/springframework/shell/samples/e2e/OptionConversionCommandsTests.java @@ -0,0 +1,57 @@ +/* + * 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.samples.e2e; + +import org.junit.jupiter.params.ParameterizedTest; + +import org.springframework.shell.samples.AbstractSampleTests; +import org.springframework.shell.samples.e2e.OptionConversionCommands.CommonConfiguration; +import org.springframework.shell.samples.e2e.OptionConversionCommands.LegacyAnnotation; +import org.springframework.shell.samples.e2e.OptionConversionCommands.Registration; +import org.springframework.shell.test.ShellTestClient.BaseShellSession; +import org.springframework.test.context.ContextConfiguration; + +@ContextConfiguration(classes = { LegacyAnnotation.class, Registration.class, CommonConfiguration.class }) +class OptionConversionCommandsTests extends AbstractSampleTests { + + @ParameterizedTest + @E2ESource(command = "option-conversion-integer --arg1 1") + void optionConversionInteger(String command, boolean interactive) { + BaseShellSession session = createSession(command, interactive); + assertScreenContainsText(session, "Hello 1"); + } + + @ParameterizedTest + @E2ESource(command = "option-conversion-custom --arg1 hi") + void optionConversionCustom(String command, boolean interactive) { + BaseShellSession session = createSession(command, interactive); + assertScreenContainsText(session, "Hello MyPojo [arg=hi]"); + } + + @ParameterizedTest + @E2ESource(command = "option-conversion-customset --arg1 hi") + void optionConversionCustomSet(String command, boolean interactive) { + BaseShellSession session = createSession(command, interactive); + assertScreenContainsText(session, "Hello [MyPojo [arg=hi]]"); + } + + @ParameterizedTest + @E2ESource(command = "option-conversion-customarray --arg1 hi") + void optionConversionCustomArray(String command, boolean interactive) { + BaseShellSession session = createSession(command, interactive); + assertScreenContainsText(session, "Hello [MyPojo [arg=hi]]"); + } +}