Skip to content

Commit cd9651e

Browse files
jvalkealjvalkeal
committed
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 spring-projects#694 - Fixes spring-projects#699
1 parent 8ce7a11 commit cd9651e

File tree

9 files changed

+388
-28
lines changed

9 files changed

+388
-28
lines changed

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

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2022 the original author or authors.
2+
* Copyright 2022-2023 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -24,8 +24,10 @@
2424
import org.jline.terminal.Terminal;
2525

2626
import org.springframework.core.MethodParameter;
27+
import org.springframework.core.ResolvableType;
2728
import org.springframework.core.annotation.Order;
2829
import org.springframework.core.convert.ConversionService;
30+
import org.springframework.core.convert.TypeDescriptor;
2931
import org.springframework.messaging.Message;
3032
import org.springframework.messaging.handler.invocation.HandlerMethodArgumentResolver;
3133
import org.springframework.messaging.support.MessageBuilder;
@@ -253,7 +255,13 @@ public boolean supportsParameter(MethodParameter parameter) {
253255

254256
@Override
255257
public Object resolveArgument(MethodParameter parameter, Message<?> message) throws Exception {
256-
return conversionService.convert(paramValues.get(parameter.getParameterName()), parameter.getParameterType());
258+
Object source = paramValues.get(parameter.getParameterName());
259+
if (source == null) {
260+
return null;
261+
}
262+
TypeDescriptor sourceType = new TypeDescriptor(ResolvableType.forClass(source.getClass()), null, null);
263+
TypeDescriptor targetType = new TypeDescriptor(parameter);
264+
return conversionService.convert(source, sourceType, targetType);
257265
}
258266

259267
}

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

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828

2929
import org.springframework.core.ResolvableType;
3030
import org.springframework.core.convert.ConversionService;
31+
import org.springframework.core.convert.TypeDescriptor;
3132
import org.springframework.shell.Utils;
3233
import org.springframework.util.StringUtils;
3334

@@ -280,7 +281,11 @@ public CommandParserResults parse(List<CommandOption> options, String[] args) {
280281
if (!oargs.isEmpty() && !oargs.get(0).startsWith("-")) {
281282
// as we now have a candicate option, try to see if there is a
282283
// conversion we can do and the use it.
283-
Object value = convertOptionType(o, oargs);
284+
Object convertSource = oargs;
285+
if (oargs.size() == 1) {
286+
convertSource = oargs.get(0);
287+
}
288+
Object value = convertOptionType(o, convertSource);
284289
results.add(new DefaultCommandParserResult(o, value));
285290
requiredOptions.remove(o);
286291
}
@@ -299,8 +304,11 @@ public CommandParserResults parse(List<CommandOption> options, String[] args) {
299304

300305
private Object convertOptionType(CommandOption option, Object value) {
301306
if (conversionService != null && option.getType() != null && value != null) {
302-
if (conversionService.canConvert(value.getClass(), option.getType().getRawClass())) {
303-
value = conversionService.convert(value, option.getType().getRawClass());
307+
Object source = value;
308+
TypeDescriptor sourceType = new TypeDescriptor(ResolvableType.forClass(source.getClass()), null, null);
309+
TypeDescriptor targetType = new TypeDescriptor(option.getType(), null, null);
310+
if (conversionService.canConvert(sourceType, targetType)) {
311+
value = conversionService.convert(source, sourceType, targetType);
304312
}
305313
}
306314
return value;
@@ -472,8 +480,14 @@ private ConvertArgumentsHolder convertArguments(CommandOption option, List<Strin
472480
value = Boolean.parseBoolean(arguments.get(0));
473481
}
474482
}
483+
// see for collection section below
475484
else if (type != null && type.isArray()) {
476-
value = arguments.stream().collect(Collectors.toList()).toArray();
485+
if (arguments.size() == 1) {
486+
value = arguments.get(0);
487+
}
488+
else {
489+
value = arguments.stream().collect(Collectors.toList());
490+
}
477491
}
478492
// if it looks like type is a collection just get as list
479493
// as conversion will happen later. we just need to know

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

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -175,13 +175,27 @@ public interface OptionSpec {
175175
OptionSpec shortNames(Character... names);
176176

177177
/**
178-
* Define a type for an option.
178+
* Define a type for an option. This method is a shortcut for
179+
* {@link #type(ResolvableType)} which is a preferred way to
180+
* define type with generics. Will override one from
181+
* {@link #type(ResolvableType)}.
179182
*
180183
* @param type the type
181184
* @return option spec for chaining
185+
* @see #type(ResolvableType)
182186
*/
183187
OptionSpec type(Type type);
184188

189+
/**
190+
* Define a {@link ResolvableType} for an option. This method is
191+
* a preferred way to define type with generics. Will override one
192+
* from {@link #type(Type)}.
193+
*
194+
* @param type the resolvable type
195+
* @return option spec for chaining
196+
*/
197+
OptionSpec type(ResolvableType type);
198+
185199
/**
186200
* Define a {@code description} for an option.
187201
*
@@ -817,6 +831,12 @@ public OptionSpec type(Type type) {
817831
return this;
818832
}
819833

834+
@Override
835+
public OptionSpec type(ResolvableType type) {
836+
this.type = type;
837+
return this;
838+
}
839+
820840
@Override
821841
public OptionSpec description(String description) {
822842
this.description = description;

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

Lines changed: 251 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,251 @@
1+
/*
2+
* Copyright 2023 the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package org.springframework.shell.command;
17+
18+
import java.util.ArrayList;
19+
import java.util.List;
20+
import java.util.Set;
21+
22+
import org.junit.jupiter.api.Test;
23+
24+
import org.springframework.core.ResolvableType;
25+
import org.springframework.core.convert.converter.Converter;
26+
import org.springframework.core.convert.support.DefaultConversionService;
27+
import org.springframework.messaging.handler.invocation.HandlerMethodArgumentResolver;
28+
import org.springframework.shell.command.CommandRegistration.OptionArity;
29+
30+
import static org.assertj.core.api.Assertions.assertThat;
31+
32+
public class CommandExecutionCustomConversionTests {
33+
34+
@Test
35+
public void testCustomPojo() {
36+
CommandExecution execution = build();
37+
Pojo1 pojo1 = new Pojo1();
38+
39+
CommandRegistration r1 = CommandRegistration.builder()
40+
.command("command1")
41+
.description("help")
42+
.withOption()
43+
.longNames("arg1")
44+
.and()
45+
.withTarget()
46+
.method(pojo1, "method1")
47+
.and()
48+
.build();
49+
execution.evaluate(r1, new String[]{"--arg1", "myarg1value"});
50+
assertThat(pojo1.method1Pojo2).isNotNull();
51+
}
52+
53+
@Test
54+
public void testCustomPojoArray() {
55+
CommandExecution execution = build();
56+
Pojo1 pojo1 = new Pojo1();
57+
58+
CommandRegistration r1 = CommandRegistration.builder()
59+
.command("command1")
60+
.description("help")
61+
.withOption()
62+
.longNames("arg1")
63+
.and()
64+
.withTarget()
65+
.method(pojo1, "method2")
66+
.and()
67+
.build();
68+
execution.evaluate(r1, new String[]{"--arg1", "a,b"});
69+
assertThat(pojo1.method2Pojo2).isNotNull();
70+
assertThat(pojo1.method2Pojo2.length).isEqualTo(2);
71+
assertThat(pojo1.method2Pojo2[0].arg).isEqualTo("a");
72+
assertThat(pojo1.method2Pojo2[1].arg).isEqualTo("b");
73+
}
74+
75+
@Test
76+
public void testCustomPojoArrayPositional() {
77+
CommandExecution execution = build();
78+
Pojo1 pojo1 = new Pojo1();
79+
80+
CommandRegistration r1 = CommandRegistration.builder()
81+
.command("command1")
82+
.description("help")
83+
.withOption()
84+
.longNames("arg1")
85+
.arity(OptionArity.ONE_OR_MORE)
86+
.position(0)
87+
.and()
88+
.withTarget()
89+
.method(pojo1, "method2")
90+
.and()
91+
.build();
92+
execution.evaluate(r1, new String[]{"a,b"});
93+
assertThat(pojo1.method2Pojo2).isNotNull();
94+
assertThat(pojo1.method2Pojo2.length).isEqualTo(2);
95+
assertThat(pojo1.method2Pojo2[0].arg).isEqualTo("a");
96+
assertThat(pojo1.method2Pojo2[1].arg).isEqualTo("b");
97+
}
98+
99+
@Test
100+
public void testCustomPojoList() {
101+
CommandExecution execution = build();
102+
Pojo1 pojo1 = new Pojo1();
103+
104+
ResolvableType type = ResolvableType.forClassWithGenerics(List.class, Pojo1.class);
105+
106+
CommandRegistration r1 = CommandRegistration.builder()
107+
.command("command1")
108+
.description("help")
109+
.withOption()
110+
.longNames("arg1")
111+
.type(type)
112+
.and()
113+
.withTarget()
114+
.method(pojo1, "method3")
115+
.and()
116+
.build();
117+
execution.evaluate(r1, new String[]{"--arg1", "a,b"});
118+
assertThat(pojo1.method3Pojo2).isNotNull();
119+
assertThat(pojo1.method3Pojo2.size()).isEqualTo(2);
120+
assertThat(pojo1.method3Pojo2.get(0)).isInstanceOf(Pojo2.class);
121+
assertThat(pojo1.method3Pojo2.get(0).arg).isEqualTo("a");
122+
assertThat(pojo1.method3Pojo2.get(1).arg).isEqualTo("b");
123+
}
124+
125+
@Test
126+
public void testCustomPojoListPositional() {
127+
CommandExecution execution = build();
128+
Pojo1 pojo1 = new Pojo1();
129+
130+
ResolvableType type = ResolvableType.forClassWithGenerics(List.class, Pojo1.class);
131+
132+
CommandRegistration r1 = CommandRegistration.builder()
133+
.command("command1")
134+
.description("help")
135+
.withOption()
136+
.longNames("arg1")
137+
.arity(OptionArity.ONE_OR_MORE)
138+
.position(0)
139+
.type(type)
140+
.and()
141+
.withTarget()
142+
.method(pojo1, "method3")
143+
.and()
144+
.build();
145+
execution.evaluate(r1, new String[]{"a,b"});
146+
assertThat(pojo1.method3Pojo2).isNotNull();
147+
assertThat(pojo1.method3Pojo2.size()).isEqualTo(2);
148+
assertThat(pojo1.method3Pojo2.get(0)).isInstanceOf(Pojo2.class);
149+
assertThat(pojo1.method3Pojo2.get(0).arg).isEqualTo("a");
150+
assertThat(pojo1.method3Pojo2.get(1).arg).isEqualTo("b");
151+
}
152+
153+
@Test
154+
public void testCustomPojoSet() {
155+
CommandExecution execution = build();
156+
Pojo1 pojo1 = new Pojo1();
157+
158+
CommandRegistration r1 = CommandRegistration.builder()
159+
.command("command1")
160+
.description("help")
161+
.withOption()
162+
.longNames("arg1")
163+
.and()
164+
.withTarget()
165+
.method(pojo1, "method4")
166+
.and()
167+
.build();
168+
execution.evaluate(r1, new String[]{"--arg1", "a,b"});
169+
assertThat(pojo1.method4Pojo2).isNotNull();
170+
assertThat(pojo1.method4Pojo2.size()).isEqualTo(2);
171+
assertThat(pojo1.method4Pojo2.iterator().next()).isInstanceOf(Pojo2.class);
172+
assertThat(pojo1.method4Pojo2.stream().map(pojo -> pojo.arg).toList()).containsExactly("a", "b");
173+
}
174+
175+
@Test
176+
public void testCustomPojoSetPositional() {
177+
CommandExecution execution = build();
178+
Pojo1 pojo1 = new Pojo1();
179+
180+
CommandRegistration r1 = CommandRegistration.builder()
181+
.command("command1")
182+
.description("help")
183+
.withOption()
184+
.longNames("arg1")
185+
.arity(OptionArity.ONE_OR_MORE)
186+
.position(0)
187+
.and()
188+
.withTarget()
189+
.method(pojo1, "method4")
190+
.and()
191+
.build();
192+
execution.evaluate(r1, new String[]{"a,b"});
193+
assertThat(pojo1.method4Pojo2).isNotNull();
194+
assertThat(pojo1.method4Pojo2.size()).isEqualTo(2);
195+
assertThat(pojo1.method4Pojo2.iterator().next()).isInstanceOf(Pojo2.class);
196+
assertThat(pojo1.method4Pojo2.stream().map(pojo -> pojo.arg).toList()).containsExactly("a", "b");
197+
}
198+
199+
private CommandExecution build() {
200+
DefaultConversionService conversionService = new DefaultConversionService();
201+
conversionService.addConverter(new StringToMyPojo2Converter());
202+
List<HandlerMethodArgumentResolver> resolvers = new ArrayList<>();
203+
resolvers.add(new ArgumentHeaderMethodArgumentResolver(conversionService, null));
204+
resolvers.add(new CommandContextMethodArgumentResolver());
205+
return CommandExecution.of(resolvers, null, null, conversionService);
206+
}
207+
208+
static class StringToMyPojo2Converter implements Converter<String, Pojo2> {
209+
210+
@Override
211+
public Pojo2 convert(String from) {
212+
return new Pojo2(from);
213+
}
214+
}
215+
216+
static class Pojo1 {
217+
218+
public Pojo2 method1Pojo2;
219+
public Pojo2[] method2Pojo2;
220+
public List<Pojo2> method3Pojo2;
221+
public Set<Pojo2> method4Pojo2;
222+
223+
public void method1(Pojo2 arg1) {
224+
method1Pojo2 = arg1;
225+
}
226+
227+
public void method2(Pojo2[] arg1) {
228+
method2Pojo2 = arg1;
229+
}
230+
231+
public void method3(List<Pojo2> arg1) {
232+
method3Pojo2 = arg1;
233+
}
234+
235+
public void method4(Set<Pojo2> arg1) {
236+
method4Pojo2 = arg1;
237+
}
238+
}
239+
240+
static class Pojo2 {
241+
242+
public String arg;
243+
244+
public Pojo2() {
245+
}
246+
247+
public Pojo2(String arg) {
248+
this.arg = arg;
249+
}
250+
}
251+
}

0 commit comments

Comments
 (0)