Skip to content

Commit b107868

Browse files
jvalkealjvalkeal
committed
Handle arity errors
- Introduce new error TooManyArgumentsOptionException and NotEnoughArgumentsOptionException. - Parser not tracks arity min/max and imposes if num of option arguments. - CommandParserExceptionResolver contains better error message handling for these containing more context for a user. - Fixes #614
1 parent 6461a75 commit b107868

File tree

5 files changed

+277
-10
lines changed

5 files changed

+277
-10
lines changed

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

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -332,6 +332,9 @@ ParserResults visit(List<List<String>> lexerResults, List<CommandOption> options
332332
return option.stream().flatMap(o -> {
333333
List<String> subArgs = lr.subList(1, lr.size());
334334
ConvertArgumentsHolder holder = convertArguments(o, subArgs);
335+
if (holder.error != null) {
336+
return Stream.of(ParserResult.of(o, subArgs, null, holder.error));
337+
}
335338
Object value = holder.value;
336339
if (conversionService != null && o.getType() != null && value != null) {
337340
if (conversionService.canConvert(value.getClass(), o.getType().getRawClass())) {
@@ -433,6 +436,22 @@ private ConvertArgumentsHolder convertArguments(CommandOption option, List<Strin
433436
}
434437
}
435438

439+
if (arityMax > 1 && arityMin > -1 && arityMax >= arityMin && (arguments.size() < arityMin || arguments.size() > arityMax)) {
440+
String ln = option.getLongNames() != null
441+
? Stream.of(option.getLongNames()).collect(Collectors.joining(","))
442+
: "";
443+
String sn = option.getShortNames() != null ? Stream.of(option.getShortNames())
444+
.map(n -> Character.toString(n)).collect(Collectors.joining(",")) : "";
445+
if (arguments.size() < arityMin) {
446+
String msg = String.format("Not enough arguments, longnames='%s', shortnames='%s'", ln, sn);
447+
return new ConvertArgumentsHolder(value, unmapped, new NotEnoughArgumentsOptionException(msg, option));
448+
}
449+
if (arguments.size() > arityMax) {
450+
String msg = String.format("Too many arguments, longnames='%s', shortnames='%s'", ln, sn);
451+
return new ConvertArgumentsHolder(value, unmapped, new TooManyArgumentsOptionException(msg, option));
452+
}
453+
}
454+
436455
if (type != null && type.isAssignableFrom(boolean.class)) {
437456
if (arguments.size() == 0) {
438457
value = true;
@@ -469,12 +488,18 @@ else if (type != null && type.isArray()) {
469488
private class ConvertArgumentsHolder {
470489
Object value;
471490
final List<String> unmapped = new ArrayList<>();
491+
CommandParserException error;
472492

473493
ConvertArgumentsHolder(Object value, List<String> unmapped) {
494+
this(value, unmapped, null);
495+
}
496+
497+
ConvertArgumentsHolder(Object value, List<String> unmapped, CommandParserException error) {
474498
this.value = value;
475499
if (unmapped != null) {
476500
this.unmapped.addAll(unmapped);
477501
}
502+
this.error = error;
478503
}
479504
}
480505
}
@@ -509,6 +534,34 @@ public static CommandParserException of(String message) {
509534
}
510535
}
511536

537+
public static class OptionException extends CommandParserException {
538+
539+
private CommandOption option;
540+
541+
public OptionException(String message, CommandOption option) {
542+
super(message);
543+
this.option = option;
544+
}
545+
546+
public CommandOption getOption() {
547+
return option;
548+
}
549+
}
550+
551+
public static class TooManyArgumentsOptionException extends OptionException {
552+
553+
public TooManyArgumentsOptionException(String message, CommandOption option) {
554+
super(message, option);
555+
}
556+
}
557+
558+
public static class NotEnoughArgumentsOptionException extends OptionException {
559+
560+
public NotEnoughArgumentsOptionException(String message, CommandOption option) {
561+
super(message, option);
562+
}
563+
}
564+
512565
public static class MissingOptionException extends CommandParserException {
513566

514567
private CommandOption option;

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

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121

2222
import org.springframework.shell.command.CommandExecution.CommandParserExceptionsException;
2323
import org.springframework.shell.command.CommandParser.MissingOptionException;
24+
import org.springframework.shell.command.CommandParser.NotEnoughArgumentsOptionException;
25+
import org.springframework.shell.command.CommandParser.TooManyArgumentsOptionException;
2426
import org.springframework.util.StringUtils;
2527

2628
/**
@@ -44,6 +46,18 @@ else if (option.getShortNames().length > 0) {
4446
handleShort(builder, option);
4547
}
4648
}
49+
else if (e instanceof NotEnoughArgumentsOptionException neaoe) {
50+
CommandOption option = neaoe.getOption();
51+
if (option.getLongNames().length > 0) {
52+
handleLongNotEnough(builder, option);
53+
}
54+
}
55+
else if (e instanceof TooManyArgumentsOptionException tmaoe) {
56+
CommandOption option = tmaoe.getOption();
57+
if (option.getLongNames().length > 0) {
58+
handleLongTooMany(builder, option);
59+
}
60+
}
4761
else {
4862
builder.append(new AttributedString(e.getMessage(), AttributedStyle.DEFAULT.foreground(AttributedStyle.RED)));
4963
}
@@ -78,4 +92,30 @@ private static void handleShort(AttributedStringBuilder builder, CommandOption o
7892
buf.append(".");
7993
builder.append(new AttributedString(buf.toString(), AttributedStyle.DEFAULT.foreground(AttributedStyle.RED)));
8094
}
95+
96+
private static void handleLongNotEnough(AttributedStringBuilder builder, CommandOption option) {
97+
StringBuilder buf = new StringBuilder();
98+
buf.append("Not enough arguments --");
99+
buf.append(option.getLongNames()[0]);
100+
buf.append(" requires at least " + option.getArityMin());
101+
if (StringUtils.hasText(option.getDescription())) {
102+
buf.append(", ");
103+
buf.append(option.getDescription());
104+
}
105+
buf.append(".");
106+
builder.append(new AttributedString(buf.toString(), AttributedStyle.DEFAULT.foreground(AttributedStyle.RED)));
107+
}
108+
109+
private static void handleLongTooMany(AttributedStringBuilder builder, CommandOption option) {
110+
StringBuilder buf = new StringBuilder();
111+
buf.append("Too many arguments --");
112+
buf.append(option.getLongNames()[0]);
113+
buf.append(" requires at most " + option.getArityMax());
114+
if (StringUtils.hasText(option.getDescription())) {
115+
buf.append(", ");
116+
buf.append(option.getDescription());
117+
}
118+
buf.append(".");
119+
builder.append(new AttributedString(buf.toString(), AttributedStyle.DEFAULT.foreground(AttributedStyle.RED)));
120+
}
81121
}

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

Lines changed: 60 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@
2323
import org.springframework.shell.command.CommandExecution.CommandParserExceptionsException;
2424
import org.springframework.shell.command.CommandParser.CommandParserException;
2525
import org.springframework.shell.command.CommandParser.MissingOptionException;
26+
import org.springframework.shell.command.CommandParser.NotEnoughArgumentsOptionException;
27+
import org.springframework.shell.command.CommandParser.TooManyArgumentsOptionException;
2628

2729
import static org.assertj.core.api.Assertions.assertThat;
2830

@@ -44,7 +46,7 @@ void resolvesMissingLongOption() {
4446
.and()
4547
.build();
4648

47-
CommandHandlingResult resolve = resolver.resolve(of(registration.getOptions().get(0)));
49+
CommandHandlingResult resolve = resolver.resolve(missingOption(registration.getOptions().get(0)));
4850
assertThat(resolve).isNotNull();
4951
assertThat(resolve.message()).contains("--arg1", "Desc arg1");
5052
}
@@ -64,7 +66,7 @@ void resolvesMissingLongOptionWhenAlsoShort() {
6466
.and()
6567
.build();
6668

67-
CommandHandlingResult resolve = resolver.resolve(of(registration.getOptions().get(0)));
69+
CommandHandlingResult resolve = resolver.resolve(missingOption(registration.getOptions().get(0)));
6870
assertThat(resolve).isNotNull();
6971
assertThat(resolve.message()).contains("--arg1", "Desc arg1");
7072
assertThat(resolve.message()).doesNotContain("-x", "Desc x");
@@ -84,14 +86,66 @@ void resolvesMissingShortOption() {
8486
.and()
8587
.build();
8688

87-
CommandHandlingResult resolve = resolver.resolve(of(registration.getOptions().get(0)));
89+
CommandHandlingResult resolve = resolver.resolve(missingOption(registration.getOptions().get(0)));
8890
assertThat(resolve).isNotNull();
8991
assertThat(resolve.message()).contains("-x", "Desc x");
9092
}
9193

92-
static CommandParserExceptionsException of(CommandOption option) {
93-
MissingOptionException moe = new MissingOptionException("msg", option);
94-
List<CommandParserException> parserExceptions = Arrays.asList(moe);
94+
@Test
95+
void resolvesTooManyLongOption() {
96+
CommandRegistration registration = CommandRegistration.builder()
97+
.command("required-value")
98+
.withOption()
99+
.longNames("arg1")
100+
.description("Desc arg1")
101+
.arity(2, 3)
102+
.required()
103+
.and()
104+
.withTarget()
105+
.consumer(ctx -> {})
106+
.and()
107+
.build();
108+
109+
CommandHandlingResult resolve = resolver.resolve(tooManyArguments(registration.getOptions().get(0)));
110+
assertThat(resolve).isNotNull();
111+
assertThat(resolve.message()).contains("--arg1 requires at most", "Desc arg1");
112+
}
113+
114+
@Test
115+
void resolvesNotEnoughLongOption() {
116+
CommandRegistration registration = CommandRegistration.builder()
117+
.command("required-value")
118+
.withOption()
119+
.longNames("arg1")
120+
.description("Desc arg1")
121+
.arity(2, 3)
122+
.required()
123+
.and()
124+
.withTarget()
125+
.consumer(ctx -> {})
126+
.and()
127+
.build();
128+
129+
CommandHandlingResult resolve = resolver.resolve(notEnoughArguments(registration.getOptions().get(0)));
130+
assertThat(resolve).isNotNull();
131+
assertThat(resolve.message()).contains("--arg1 requires at least", "Desc arg1");
132+
}
133+
134+
static CommandParserExceptionsException missingOption(CommandOption option) {
135+
MissingOptionException e = new MissingOptionException("msg", option);
136+
List<CommandParserException> parserExceptions = Arrays.asList(e);
137+
return new CommandParserExceptionsException("msg", parserExceptions);
138+
}
139+
140+
static CommandParserExceptionsException tooManyArguments(CommandOption option) {
141+
TooManyArgumentsOptionException e = new TooManyArgumentsOptionException("msg", option);
142+
List<CommandParserException> parserExceptions = Arrays.asList(e);
143+
return new CommandParserExceptionsException("msg", parserExceptions);
144+
}
145+
146+
static CommandParserExceptionsException notEnoughArguments(CommandOption option) {
147+
NotEnoughArgumentsOptionException e = new NotEnoughArgumentsOptionException("msg", option);
148+
List<CommandParserException> parserExceptions = Arrays.asList(e);
95149
return new CommandParserExceptionsException("msg", parserExceptions);
96150
}
97151
}

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@
2626
import org.springframework.core.convert.ConversionService;
2727
import org.springframework.core.convert.support.DefaultConversionService;
2828
import org.springframework.shell.command.CommandParser.CommandParserResults;
29+
import org.springframework.shell.command.CommandParser.NotEnoughArgumentsOptionException;
30+
import org.springframework.shell.command.CommandParser.TooManyArgumentsOptionException;
2931

3032
import static org.assertj.core.api.Assertions.assertThat;
3133

@@ -295,6 +297,40 @@ public void testLongOptionsWithArray() {
295297
assertThat(results.results().get(0).value()).isEqualTo(new int[] { 1, 2 });
296298
}
297299

300+
@Test
301+
public void testArityErrors() {
302+
CommandOption option1 = CommandOption.of(
303+
new String[] { "arg1" },
304+
null,
305+
null,
306+
ResolvableType.forType(int[].class),
307+
true,
308+
null,
309+
null,
310+
2,
311+
3,
312+
null,
313+
null);
314+
315+
List<CommandOption> options = Arrays.asList(option1);
316+
317+
String[] args1 = new String[]{"--arg1", "1", "2", "3", "4"};
318+
CommandParserResults results1 = parser.parse(options, args1);
319+
assertThat(results1.errors()).hasSize(1);
320+
assertThat(results1.errors().get(0)).isInstanceOf(TooManyArgumentsOptionException.class);
321+
assertThat(results1.results()).hasSize(1);
322+
assertThat(results1.results().get(0).option()).isSameAs(option1);
323+
assertThat(results1.results().get(0).value()).isNull();
324+
325+
String[] args2 = new String[]{"--arg1", "1"};
326+
CommandParserResults results2 = parser.parse(options, args2);
327+
assertThat(results2.errors()).hasSize(1);
328+
assertThat(results2.errors().get(0)).isInstanceOf(NotEnoughArgumentsOptionException.class);
329+
assertThat(results2.results()).hasSize(1);
330+
assertThat(results2.results().get(0).option()).isSameAs(option1);
331+
assertThat(results2.results().get(0).value()).isNull();
332+
}
333+
298334
@Test
299335
public void testMapPositionalArgs1() {
300336
CommandOption option1 = longOption("arg1", 0, 1, 1);

0 commit comments

Comments
 (0)