Rework command subsystem

- Focus of these changes are to introduce a new command system based on
  real registrations (new way) instead of continuously (old way) resolve
  methods and its parameters via reflection.
- There's a lot of changes as this resolution via reflection had its
  hooks almost everywhere and thus most changes are just refactorings.
- Order to understand real changes I'd start to look classes under
  `org.springframework.shell.command` package as it defines new registration,
  catalog and parser classes. Also samples contain new classes to demonstrate
  new functionality.
- Fixes #380

spring-shell-standard-commands/src/main/java/org/springframework/shell/standard/commands/Completion.java

@@ -15,8 +15,6 @@
  */
 package org.springframework.shell.standard.commands;
 
-import java.util.stream.Collectors;
-
 import org.springframework.core.io.ResourceLoader;
 import org.springframework.shell.standard.AbstractShellComponent;
 import org.springframework.shell.standard.ShellComponent;
@@ -51,8 +49,7 @@ public class Completion extends AbstractShellComponent {
 
 	@ShellMethod(key = "completion bash", value = "Generate bash completion script")
 	public String bash() {
-		BashCompletions bashCompletions = new BashCompletions(resourceLoader, getCommandRegistry(),
-				getParameterResolver().collect(Collectors.toList()));
+		BashCompletions bashCompletions = new BashCompletions(resourceLoader, getCommandCatalog());
 		return bashCompletions.generate(rootCommand);
 	}
 }

spring-shell-standard-commands/src/main/java/org/springframework/shell/standard/commands/Help.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2017 the original author or authors.
+ * Copyright 2017-2022 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.
@@ -13,15 +13,14 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-
 package org.springframework.shell.standard.commands;
 
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Comparator;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
-import java.util.Set;
 import java.util.SortedMap;
 import java.util.SortedSet;
 import java.util.TreeMap;
@@ -37,24 +36,26 @@ import org.jline.utils.AttributedStyle;
 
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.shell.Availability;
-import org.springframework.shell.MethodTarget;
 import org.springframework.shell.ParameterDescription;
 import org.springframework.shell.Utils;
+import org.springframework.shell.command.CommandOption;
+import org.springframework.shell.command.CommandRegistration;
 import org.springframework.shell.standard.AbstractShellComponent;
 import org.springframework.shell.standard.CommandValueProvider;
 import org.springframework.shell.standard.ShellComponent;
 import org.springframework.shell.standard.ShellMethod;
 import org.springframework.shell.standard.ShellOption;
+import org.springframework.util.StringUtils;
 
 import static java.util.stream.Collectors.groupingBy;
 import static java.util.stream.Collectors.mapping;
 import static java.util.stream.Collectors.toCollection;
-import static java.util.stream.Collectors.toMap;
 
 /**
  * A command to display help about all available commands.
  *
  * @author Eric Bottard
+ * @author Janne Valkealahti
  */
 @ShellComponent
 public class Help extends AbstractShellComponent {
@@ -91,7 +92,7 @@ public class Help extends AbstractShellComponent {
 	@ShellMethod(value = "Display help about available commands.", prefix = "-")
 	public CharSequence help(
 			@ShellOption(defaultValue = ShellOption.NULL, valueProvider = CommandValueProvider.class, value = { "-C",
-					"--command" }, help = "The command to obtain help for.") String command)
+					"--command" }, help = "The command to obtain help for.", arity = Integer.MAX_VALUE) String command)
 			throws IOException {
 		if (command == null) {
 			return listCommands();
@@ -116,16 +117,17 @@ public class Help extends AbstractShellComponent {
 	 * Return a description of a specific command. Uses a layout inspired by *nix man pages.
 	 */
 	private CharSequence documentCommand(String command) {
-		MethodTarget methodTarget = getCommandRegistry().listCommands().get(command);
-		if (methodTarget == null) {
+		Map<String, CommandRegistration> registrations = getCommandCatalog().getRegistrations();
+		CommandRegistration registration = registrations.get(command);
+		if (registration == null) {
 			throw new IllegalArgumentException("Unknown command '" + command + "'");
 		}
 
 		AttributedStringBuilder result = new AttributedStringBuilder().append("\n\n");
-		List<ParameterDescription> parameterDescriptions = getParameterDescriptions(methodTarget);
+		List<ParameterDescription> parameterDescriptions = getParameterDescriptions(registration);
 
 		// NAME
-		documentCommandName(result, command, methodTarget.getHelp());
+		documentCommandName(result, command, registration.getHelp());
 
 		// SYNOPSYS
 		documentSynopsys(result, command, parameterDescriptions);
@@ -134,10 +136,10 @@ public class Help extends AbstractShellComponent {
 		documentOptions(result, parameterDescriptions);
 
 		// ALSO KNOWN AS
-		documentAliases(result, command, methodTarget);
+		documentAliases(result, command, registrations, registration);
 
 		// AVAILABILITY
-		documentAvailability(result, methodTarget);
+		documentAvailability(result, registration);
 
 		result.append("\n");
 		return result;
@@ -242,12 +244,13 @@ public class Help extends AbstractShellComponent {
 		}
 	}
 
-	private void documentAliases(AttributedStringBuilder result, String command, MethodTarget methodTarget) {
-		Set<String> aliases = getCommandRegistry().listCommands().entrySet().stream()
-				.filter(e -> e.getValue().equals(methodTarget))
-				.map(Map.Entry::getKey)
-				.filter(c -> !command.equals(c))
-				.collect(toCollection(TreeSet::new));
+	private void documentAliases(AttributedStringBuilder result, String command,
+			Map<String, CommandRegistration> registrations, CommandRegistration registration) {
+		List<String> aliases = registrations.entrySet().stream()
+			.filter(e -> e.getValue().equals(registration))
+			.map(Map.Entry::getKey)
+			.filter(c -> !command.equals(c))
+			.collect(Collectors.toList());
 
 		if (!aliases.isEmpty()) {
 			result.append("ALSO KNOWN AS", AttributedStyle.BOLD).append("\n");
@@ -257,8 +260,8 @@ public class Help extends AbstractShellComponent {
 		}
 	}
 
-	private void documentAvailability(AttributedStringBuilder result, MethodTarget methodTarget) {
-		Availability availability = methodTarget.getAvailability();
+	private void documentAvailability(AttributedStringBuilder result, CommandRegistration registration) {
+		Availability availability = registration.getAvailability();
 		if (!availability.isAvailable()) {
 			result.append("CURRENTLY UNAVAILABLE", AttributedStyle.BOLD).append("\n");
 			result.append('\t').append("This command is currently not available because ")
@@ -272,20 +275,21 @@ public class Help extends AbstractShellComponent {
 	}
 
 	private CharSequence listCommands() {
-		Map<String, MethodTarget> commandsByName = getCommandRegistry().listCommands();
-
 		AttributedStringBuilder result = new AttributedStringBuilder();
 		result.append("AVAILABLE COMMANDS\n\n", AttributedStyle.BOLD);
 
-		SortedMap<String, Map<String, MethodTarget>> commandsByGroupAndName = commandsByName.entrySet().stream()
-				.collect(groupingBy(e -> e.getValue().getGroup(), TreeMap::new, // group by and sort by command group
-						toMap(Entry::getKey, Entry::getValue)));
-		// display groups, sorted alphabetically, "Default" first
+		SortedMap<String, Map<String, CommandRegistration>> commandsByGroupAndName = getCommandCatalog().getRegistrations().entrySet().stream()
+			.collect(Collectors.groupingBy(
+				e -> StringUtils.hasText(e.getValue().getGroup()) ? e.getValue().getGroup() : "",
+				TreeMap::new,
+				Collectors.toMap(Entry::getKey, Entry::getValue)
+			));
+
 		commandsByGroupAndName.forEach((group, commandsInGroup) -> {
 			if (showGroups) {
 				result.append("".equals(group) ? "Default" : group, AttributedStyle.BOLD).append('\n');
 			}
-			Map<MethodTarget, SortedSet<String>> commandNamesByMethod = commandsInGroup.entrySet().stream()
+			Map<CommandRegistration, SortedSet<String>> commandNamesByMethod = commandsInGroup.entrySet().stream()
 					.collect(groupingBy(Entry::getValue, // group by command method
 							mapping(Entry::getKey, toCollection(TreeSet::new)))); // sort command names
 			// display commands, sorted alphabetically by their first alias
@@ -304,19 +308,15 @@ public class Help extends AbstractShellComponent {
 			}
 		});
 
-		if (commandsByName.values().stream().distinct().anyMatch(m -> !isAvailable(m))) {
-			result.append("Commands marked with (*) are currently unavailable.\nType `help <command>` to learn more.\n\n");
-		}
-
 		return result;
 	}
 
-	private Comparator<Entry<MethodTarget, SortedSet<String>>> sortByFirstCommandName() {
+	private Comparator<Entry<CommandRegistration, SortedSet<String>>> sortByFirstCommandName() {
 		return Comparator.comparing(e -> e.getValue().first());
 	}
 
-	private boolean isAvailable(MethodTarget methodTarget) {
-		return methodTarget.getAvailability().isAvailable();
+	private boolean isAvailable(CommandRegistration methodTarget) {
+		return true;
 	}
 
 	private void appendUnderlinedFormal(AttributedStringBuilder result, ParameterDescription description) {
@@ -330,12 +330,50 @@ public class Help extends AbstractShellComponent {
 		}
 	}
 
-	private List<ParameterDescription> getParameterDescriptions(MethodTarget methodTarget) {
-		return Utils.createMethodParameters(methodTarget.getMethod())
-				.flatMap(mp -> getParameterResolver().filter(pr -> pr.supports(mp)).limit(1L)
-						.flatMap(pr -> pr.describe(mp)))
-				.collect(Collectors.toList());
+	private List<ParameterDescription> getParameterDescriptions(CommandRegistration registration) {
+		List<CommandOption> options = registration.getOptions();
+		List<ParameterDescription> descriptions = new ArrayList<>();
 
+		for (CommandOption option : options) {
+
+			ParameterDescription description = new ParameterDescription();
+			if (option.getType() != null) {
+				description.type(option.getType().toString());
+				description.formal(option.getType().toClass().getSimpleName());
+			}
+			else {
+				description.formal("");
+			}
+			description.help(option.getDescription());
+			description.mandatoryKey(option.isRequired());
+			if (option.getType() != null && option.getType().isAssignableFrom(boolean.class)) {
+				description.defaultValue("false");
+			}
+			else {
+				description.defaultValue(option.getDefaultValue());
+			}
+
+			List<String> keys = new ArrayList<>();
+			if (option.getLongNames() != null) {
+				for (String ln : option.getLongNames()) {
+					keys.add("--" + ln);
+				}
+			}
+			if (option.getShortNames() != null) {
+				for (Character sn : option.getShortNames()) {
+					keys.add("-" + String.valueOf(sn));
+				}
+			}
+			description.keys(keys);
+
+			descriptions.add(description);
+		}
+
+		// return Utils.createMethodParameters(registration.getTarget().getMethod())
+		// 		.flatMap(mp -> getParameterResolver().filter(pr -> pr.supports(mp)).limit(1L)
+		// 				.flatMap(pr -> pr.describe(mp)))
+		// 		.collect(Collectors.toList());
+		return descriptions;
 	}
 
 	private static class DummyContext implements MessageInterpolator.Context {
@@ -361,5 +399,4 @@ public class Help extends AbstractShellComponent {
 			return null;
 		}
 	}
-
 }

spring-shell-standard-commands/src/test/java/org/springframework/shell/standard/commands/HelpTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2017 the original author or authors.
+ * Copyright 2022 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.
@@ -13,14 +13,12 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-
 package org.springframework.shell.standard.commands;
 
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.lang.reflect.Method;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.Locale;
 import java.util.Map;
@@ -28,44 +26,45 @@ import java.util.Optional;
 
 import javax.validation.constraints.Max;
 
-import org.assertj.core.api.Assertions;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.TestInfo;
 import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mockito;
 
 import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.boot.test.mock.mockito.MockBean;
 import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
-import org.springframework.core.convert.support.DefaultConversionService;
 import org.springframework.core.io.ClassPathResource;
-import org.springframework.shell.Command;
-import org.springframework.shell.CommandRegistry;
-import org.springframework.shell.MethodTarget;
-import org.springframework.shell.ParameterResolver;
+import org.springframework.shell.command.CommandCatalog;
+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.shell.standard.StandardParameterResolver;
 import org.springframework.test.context.ContextConfiguration;
 import org.springframework.test.context.junit.jupiter.SpringExtension;
 import org.springframework.util.FileCopyUtils;
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
-/**
- * Tests for the {@link Help} command.
- *
- * @author Eric Bottard
- */
 @ExtendWith(SpringExtension.class)
-@ContextConfiguration(classes = HelpTest.Config.class)
-public class HelpTest {
+@ContextConfiguration(classes = HelpTests.Config.class)
+public class HelpTests {
 
 	private static Locale previousLocale;
 	private String testName;
+	private Map<String, CommandRegistration> registrations = new HashMap<>();
+	private CommandsPojo commandsPojo = new CommandsPojo();
+
+	@MockBean
+	private CommandCatalog commandCatalog;
+
+	@Autowired
+	private Help help;
 
 	@BeforeAll
 	public static void setAssumedLocale() {
@@ -80,25 +79,106 @@ public class HelpTest {
 
 	@BeforeEach
 	public void setup(TestInfo testInfo) {
+		registrations.clear();
 		Optional<Method> testMethod = testInfo.getTestMethod();
 		if (testMethod.isPresent()) {
 			this.testName = testMethod.get().getName();
 		}
+		Mockito.when(commandCatalog.getRegistrations()).thenReturn(registrations);
 	}
 
-	@Autowired
-	private Help help;
-
 	@Test
 	public void testCommandHelp() throws Exception {
+		CommandRegistration registration = CommandRegistration.builder()
+			.command("first-command")
+			.help("A rather extensive description of some command.")
+			.withTarget()
+				.method(commandsPojo, "firstCommand")
+				.and()
+			.withOption()
+				.shortNames('r')
+				.description("Whether to delete recursively")
+				.type(boolean.class)
+				.and()
+			.withOption()
+				.shortNames('f')
+				.description("Do not ask for confirmation. YOLO")
+				.type(boolean.class)
+				.and()
+			.withOption()
+				.shortNames('n')
+				.description("The answer to everything")
+				.defaultValue("42")
+				.type(int.class)
+				.and()
+			.withOption()
+				.shortNames('o')
+				.description("Some other parameters")
+				.type(float[].class)
+				.and()
+			.build();
+		registrations.put("first-command", registration);
+		registrations.put("1st-command", registration);
 		CharSequence help = this.help.help("first-command").toString();
-		Assertions.assertThat(help).isEqualTo(sample());
+		assertThat(help).isEqualTo(sample());
 	}
 
 	@Test
 	public void testCommandList() throws Exception {
+		CommandRegistration registration1 = CommandRegistration.builder()
+			.command("first-command")
+			.help("A rather extensive description of some command.")
+			.withTarget()
+				.method(commandsPojo, "firstCommand")
+				.and()
+			.withOption()
+				.shortNames('r')
+				.and()
+			.build();
+		registrations.put("first-command", registration1);
+		registrations.put("1st-command", registration1);
+
+		CommandRegistration registration2 = CommandRegistration.builder()
+			.command("second-command")
+			.help("The second command. This one is known under several aliases as well.")
+			.withTarget()
+				.method(commandsPojo, "secondCommand")
+				.and()
+			.build();
+		registrations.put("second-command", registration2);
+		registrations.put("yet-another-command", registration2);
+
+		CommandRegistration registration3 = CommandRegistration.builder()
+			.command("second-command")
+			.help("The last command.")
+			.withTarget()
+				.method(commandsPojo, "thirdCommand")
+				.and()
+			.build();
+		registrations.put("third-command", registration3);
+
+		CommandRegistration registration4 = CommandRegistration.builder()
+			.command("first-group-command")
+			.help("The first command in a separate group.")
+			.group("Example Group")
+			.withTarget()
+				.method(commandsPojo, "firstCommandInGroup")
+				.and()
+			.build();
+		registrations.put("first-group-command", registration4);
+
+		CommandRegistration registration5 = CommandRegistration.builder()
+			.command("second-group-command")
+			.help("The second command in a separate group.")
+			.group("Example Group")
+			.withTarget()
+				.method(commandsPojo, "secondCommandInGroup")
+				.and()
+			.build();
+		registrations.put("second-group-command", registration5);
+
 		String list = this.help.help(null).toString();
-		Assertions.assertThat(list).isEqualTo(sample());
+		assertThat(list).isEqualTo(sample());
 	}
 
 	@Test
@@ -109,7 +189,7 @@ public class HelpTest {
 	}
 
 	private String sample() throws IOException {
-		InputStream is = new ClassPathResource(HelpTest.class.getSimpleName() + "-" + testName + ".txt", HelpTest.class).getInputStream();
+		InputStream is = new ClassPathResource(HelpTests.class.getSimpleName() + "-" + testName + ".txt", HelpTests.class).getInputStream();
 		return FileCopyUtils.copyToString(new InputStreamReader(is, "UTF-8")).replace("&", "");
 	}
 
@@ -117,62 +197,18 @@ public class HelpTest {
 	static class Config {
 
 		@Bean
-		public Help help(CommandRegistry commandRegistry) {
+		public Help help() {
 			return new Help();
 		}
 
-		@Bean
-		public CommandRegistry shell() {
-
-			return new CommandRegistry() {
-
-				@Override
-				public Map<String, MethodTarget> listCommands() {
-					Map<String, MethodTarget> result = new HashMap<>();
-					MethodTarget methodTarget = MethodTarget.of("firstCommand", commands(), new Command.Help("A rather extensive description of some command."));
-					result.put("first-command", methodTarget);
-					result.put("1st-command", methodTarget);
-
-					methodTarget = MethodTarget.of("secondCommand", commands(), new Command.Help("The second command. This one is known under several aliases as well."));
-					result.put("second-command", methodTarget);
-					result.put("yet-another-command", methodTarget);
-
-					methodTarget = MethodTarget.of("thirdCommand", commands(), new Command.Help("The last command."));
-					result.put("third-command", methodTarget);
-
-					methodTarget = MethodTarget.of("firstCommandInGroup", commands(), new Command.Help("The first command in a separate group.", "Example Group"));
-					result.put("first-group-command", methodTarget);
-
-					methodTarget = MethodTarget.of("secondCommandInGroup", commands(), new Command.Help("The second command in a separate group.", "Example Group"));
-					result.put("second-group-command", methodTarget);
-
-					return result;
-				}
-
-				@Override
-				public void addCommand(String name, MethodTarget target) {
-				}
-
-				@Override
-				public void removeCommand(String name) {
-				}
-			};
-		}
-
-		@Bean
-		public ParameterResolver parameterResolver() {
-			return new StandardParameterResolver(new DefaultConversionService(), Collections.emptySet());
-		}
-
-		@Bean
-		public Object commands() {
-			return new Commands();
-		}
-
+		// @Bean
+		// public ParameterResolver parameterResolver() {
+		// 	return new StandardParameterResolver(new DefaultConversionService(), Collections.emptySet());
+		// }
 	}
 
 	@ShellComponent
-	static class Commands {
+	static class CommandsPojo {
 
 		@ShellMethod(prefix = "--")
 		public void firstCommand(
@@ -186,28 +222,22 @@ public class HelpTest {
 		        // Single key, arity > 1.
 		        @ShellOption(help = "Some other parameters", arity = 3, value = "-o") float[] o
 		) {
-
 		}
 
 		@ShellMethod
 		public void secondCommand() {
-
 		}
 
 		@ShellMethod
 		public void thirdCommand() {
-
 		}
 
 		@ShellMethod
 		public void firstCommandInGroup() {
-
 		}
 
 		@ShellMethod
 		public void secondCommandInGroup() {
-
 		}
-
 	}
 }

spring-shell-standard-commands/src/test/resources/org/springframework/shell/standard/commands/HelpTests-testCommandHelp.txt

@@ -4,22 +4,22 @@ NAME
 	first-command - A rather extensive description of some command.
 
 SYNOPSYS
-	first-command [-r]  [-f]  [[-n] int]  [-o] float float float  
+	first-command [[-r] boolean]  [[-f] boolean]  [[-n] int]  [-o] float[]  
 
 OPTIONS
-	-r	Whether to delete recursively
+	-r  boolean
+		Whether to delete recursively
 		[Optional, default = false]
 
-	-f or --force
+	-f  boolean
 		Do not ask for confirmation. YOLO
 		[Optional, default = false]
 
 	-n  int
 		The answer to everything
 		[Optional, default = 42]
-		[must be less than or equal to 5]
 
-	-o  float float float
+	-o  float[]
 		Some other parameters
 		[Mandatory]