From eaa363b34c348ec5c320380b1c49fd0f66689f94 Mon Sep 17 00:00:00 2001 From: Janne Valkealahti Date: Mon, 19 Jun 2023 12:30:29 +0100 Subject: [PATCH] Add better support for modified option names - Longnames in a command option if modified via name modifier didn't provide enough backmapping info for command execution experience being accurate. - Add new getLongNamesModified() into CommandOption which is populated if name modifier is used. - Add more hints in CommandExecution for modified option names. - This should bring annotation and programmatic commands up to date. - Backport #777 - Fixes #782 --- .../shell/command/CommandContext.java | 5 ++-- .../shell/command/CommandExecution.java | 5 ++++ .../shell/command/CommandOption.java | 28 +++++++++++++++---- .../shell/command/CommandRegistration.java | 4 ++- .../shell/command/CommandExecutionTests.java | 17 +++++++++++ .../shell/command/CommandParserTests.java | 12 +++++--- .../samples/e2e/OptionNamingCommands.java | 13 +++++++-- 7 files changed, 68 insertions(+), 16 deletions(-) diff --git a/spring-shell-core/src/main/java/org/springframework/shell/command/CommandContext.java b/spring-shell-core/src/main/java/org/springframework/shell/command/CommandContext.java index 325870be..bb3679d2 100644 --- a/spring-shell-core/src/main/java/org/springframework/shell/command/CommandContext.java +++ b/spring-shell-core/src/main/java/org/springframework/shell/command/CommandContext.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. @@ -148,8 +148,9 @@ public interface CommandContext { return results.results().stream() .filter(r -> { Stream l = Arrays.asList(r.option().getLongNames()).stream(); + Stream lm = Arrays.asList(r.option().getLongNamesModified()).stream(); Stream s = Arrays.asList(r.option().getShortNames()).stream().map(n -> Character.toString(n)); - return Stream.concat(l, s) + return Stream.concat(Stream.concat(l, lm), s) .filter(o -> ObjectUtils.nullSafeEquals(o, name)) .findFirst() .isPresent(); 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 009d4023..89d0ca5b 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 @@ -198,6 +198,11 @@ public interface CommandExecution { messageBuilder.setHeader(ArgumentHeaderMethodArgumentResolver.ARGUMENT_PREFIX + n, r.value()); paramValues.put(n, r.value()); } + // need to provide backmapping for orinal names which were modified + for (String n : r.option().getLongNamesModified()) { + messageBuilder.setHeader(ArgumentHeaderMethodArgumentResolver.ARGUMENT_PREFIX + n, r.value()); + paramValues.put(n, r.value()); + } } if (r.option().getShortNames() != null) { for (Character n : r.option().getShortNames()) { diff --git a/spring-shell-core/src/main/java/org/springframework/shell/command/CommandOption.java b/spring-shell-core/src/main/java/org/springframework/shell/command/CommandOption.java index c9773389..396db952 100644 --- a/spring-shell-core/src/main/java/org/springframework/shell/command/CommandOption.java +++ b/spring-shell-core/src/main/java/org/springframework/shell/command/CommandOption.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. @@ -32,6 +32,14 @@ public interface CommandOption { */ String[] getLongNames(); + /** + * Gets a modified long names of an option. Set within a command registration if + * option name modifier were used to have an info about original names. + * + * @return modified long names of an option + */ + String[] getLongNamesModified(); + /** * Gets a short names of an option. * @@ -111,7 +119,7 @@ public interface CommandOption { * @return default command option */ public static CommandOption of(String[] longNames, Character[] shortNames, String description) { - return of(longNames, shortNames, description, null, false, null, null, null, null, null, null); + return of(longNames, null, shortNames, description, null, false, null, null, null, null, null, null); } /** @@ -125,13 +133,14 @@ public interface CommandOption { */ public static CommandOption of(String[] longNames, Character[] shortNames, String description, ResolvableType type) { - return of(longNames, shortNames, description, type, false, null, null, null, null, null, null); + return of(longNames, null, shortNames, description, type, false, null, null, null, null, null, null); } /** * Gets an instance of a default {@link CommandOption}. * * @param longNames the long names + * @param longNamesModified the modified long names * @param shortNames the short names * @param description the description * @param type the type @@ -144,10 +153,10 @@ public interface CommandOption { * @param completion the completion * @return default command option */ - public static CommandOption of(String[] longNames, Character[] shortNames, String description, + public static CommandOption of(String[] longNames, String[] longNamesModified, Character[] shortNames, String description, ResolvableType type, boolean required, String defaultValue, Integer position, Integer arityMin, Integer arityMax, String label, CompletionResolver completion) { - return new DefaultCommandOption(longNames, shortNames, description, type, required, defaultValue, position, + return new DefaultCommandOption(longNames, longNamesModified, shortNames, description, type, required, defaultValue, position, arityMin, arityMax, label, completion); } @@ -157,6 +166,7 @@ public interface CommandOption { public static class DefaultCommandOption implements CommandOption { private String[] longNames; + private String[] longNamesModified; private Character[] shortNames; private String description; private ResolvableType type; @@ -168,11 +178,12 @@ public interface CommandOption { private String label; private CompletionResolver completion; - public DefaultCommandOption(String[] longNames, Character[] shortNames, String description, + public DefaultCommandOption(String[] longNames, String[] longNamesModified, Character[] shortNames, String description, ResolvableType type, boolean required, String defaultValue, Integer position, Integer arityMin, Integer arityMax, String label, CompletionResolver completion) { this.longNames = longNames != null ? longNames : new String[0]; + this.longNamesModified = longNamesModified != null ? longNamesModified : new String[0]; this.shortNames = shortNames != null ? shortNames : new Character[0]; this.description = description; this.type = type; @@ -190,6 +201,11 @@ public interface CommandOption { return longNames; } + @Override + public String[] getLongNamesModified() { + return longNamesModified; + } + @Override public Character[] getShortNames() { return shortNames; 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 efb9f336..cc3b25f4 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 @@ -1245,11 +1245,13 @@ public interface CommandRegistration { options = optionSpecs.stream() .map(o -> { String[] longNames = o.getLongNames(); + String[] longNamesModified = null; Function modifier = o.getOptionNameModifier(); if (modifier != null) { + longNamesModified = Arrays.copyOf(longNames, longNames.length); longNames = Arrays.stream(longNames).map(modifier).toArray(String[]::new); } - return CommandOption.of(longNames, o.getShortNames(), o.getDescription(), o.getType(), + return CommandOption.of(longNames, longNamesModified, o.getShortNames(), o.getDescription(), o.getType(), o.isRequired(), o.getDefaultValue(), o.getPosition(), o.getArityMin(), o.getArityMax(), o.getLabel(), o.getCompletion()); }) diff --git a/spring-shell-core/src/test/java/org/springframework/shell/command/CommandExecutionTests.java b/spring-shell-core/src/test/java/org/springframework/shell/command/CommandExecutionTests.java index 3951ac13..5653839c 100644 --- a/spring-shell-core/src/test/java/org/springframework/shell/command/CommandExecutionTests.java +++ b/spring-shell-core/src/test/java/org/springframework/shell/command/CommandExecutionTests.java @@ -539,4 +539,21 @@ public class CommandExecutionTests extends AbstractCommandTests { Object result = execution.evaluate(r1, new String[]{"--arg1", "myarg1value"}); assertThat(result).isInstanceOf(CommandNotCurrentlyAvailable.class); } + + @Test + public void testExecutionWithModifiedLongOption() { + CommandRegistration r1 = CommandRegistration.builder() + .command("command1") + .withOption() + .longNames("arg1") + .nameModifier(orig -> "x" + orig) + .and() + .withTarget() + .function(function1) + .and() + .build(); + Object result = execution.evaluate(r1, new String[] { "command1", "--xarg1", "myarg1value" }); + assertThat(result).isEqualTo("himyarg1value"); + } + } 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 a4ceadc7..bd8e1733 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 @@ -351,6 +351,7 @@ public class CommandParserTests extends AbstractCommandTests { new String[] { "arg1" }, null, null, + null, ResolvableType.forType(int[].class), true, null, @@ -385,6 +386,7 @@ public class CommandParserTests extends AbstractCommandTests { new String[] { "arg1" }, null, null, + null, ResolvableType.forType(int[].class), false, null, @@ -439,6 +441,7 @@ public class CommandParserTests extends AbstractCommandTests { new String[] { "arg1" }, null, null, + null, ResolvableType.forType(List.class), true, null, @@ -474,7 +477,7 @@ public class CommandParserTests extends AbstractCommandTests { @Test public void testBooleanWithDefault() { ResolvableType type = ResolvableType.forType(boolean.class); - CommandOption option1 = CommandOption.of(new String[] { "arg1" }, new Character[0], "description", type, false, + CommandOption option1 = CommandOption.of(new String[] { "arg1" }, null, new Character[0], "description", type, false, "true", null, null, null, null, null); List options = Arrays.asList(option1); @@ -488,7 +491,7 @@ public class CommandParserTests extends AbstractCommandTests { @Test public void testIntegerWithDefault() { ResolvableType type = ResolvableType.forType(Integer.class); - CommandOption option1 = CommandOption.of(new String[] { "arg1" }, new Character[0], "description", type, false, + CommandOption option1 = CommandOption.of(new String[] { "arg1" }, null, new Character[0], "description", type, false, "1", null, null, null, null, null); List options = Arrays.asList(option1); @@ -502,7 +505,7 @@ public class CommandParserTests extends AbstractCommandTests { @Test public void testIntegerWithGivenValue() { ResolvableType type = ResolvableType.forType(Integer.class); - CommandOption option1 = CommandOption.of(new String[] { "arg1" }, new Character[0], "description", type, false, + CommandOption option1 = CommandOption.of(new String[] { "arg1" }, null, new Character[0], "description", type, false, null, null, null, null, null, null); List options = Arrays.asList(option1); @@ -586,6 +589,7 @@ public class CommandParserTests extends AbstractCommandTests { null, null, null, + null, true, null, 0, @@ -623,7 +627,7 @@ public class CommandParserTests extends AbstractCommandTests { } private static CommandOption longOption(String name, ResolvableType type, boolean required, Integer position, Integer arityMin, Integer arityMax) { - return CommandOption.of(new String[] { name }, new Character[0], "desc", type, required, null, position, + return CommandOption.of(new String[] { name }, null, new Character[0], "desc", type, required, null, position, arityMin, arityMax, null, null); } diff --git a/spring-shell-samples/src/main/java/org/springframework/shell/samples/e2e/OptionNamingCommands.java b/spring-shell-samples/src/main/java/org/springframework/shell/samples/e2e/OptionNamingCommands.java index 7faa35a7..db83b859 100644 --- a/spring-shell-samples/src/main/java/org/springframework/shell/samples/e2e/OptionNamingCommands.java +++ b/spring-shell-samples/src/main/java/org/springframework/shell/samples/e2e/OptionNamingCommands.java @@ -28,12 +28,13 @@ public class OptionNamingCommands { public static class LegacyAnnotation extends BaseE2ECommands { @ShellMethod(key = LEGACY_ANNO + "option-naming-1", group = GROUP) - public void testOptionNaming1Annotation( + public String testOptionNaming1Annotation( @ShellOption("from_snake") String snake, @ShellOption("fromCamel") String camel, @ShellOption("from-kebab") String kebab, @ShellOption("FromPascal") String pascal ) { + return String.format("snake='%s' camel='%s' kebab='%s' pascal='%s' ", snake, camel, kebab, pascal); } } @@ -65,10 +66,16 @@ public class OptionNamingCommands { .withOption() .longNames("arg1") .nameModifier(name -> "x" + name) - .required() + // .required() .and() .withTarget() - .consumer(ctx -> {}) + .function(ctx -> { + String snake = ctx.getOptionValue("from_snake"); + String camel = ctx.getOptionValue("fromCamel"); + String kebab = ctx.getOptionValue("from-kebab"); + String pascal = ctx.getOptionValue("FromPascal"); + return String.format("snake='%s' camel='%s' kebab='%s' pascal='%s' ", snake, camel, kebab, pascal); + }) .and() .build(); }