From 7b547e53a8075279672ef7323ebedb9e3aa14b66 Mon Sep 17 00:00:00 2001 From: Janne Valkealahti Date: Sat, 26 Feb 2022 08:47:13 +0000 Subject: [PATCH] Add dynamic command registration - Extend CommandRegistry for add/remove methods. - For rest of shell classes move to use registry directly instead of caching commands as registry is not immutable anymore. - Add new sample - Fixes #379 --- .../shell/CommandRegistry.java | 21 ++++-- .../shell/ConfigurableCommandRegistry.java | 10 +++ .../springframework/shell/MethodTarget.java | 8 +++ .../java/org/springframework/shell/Shell.java | 36 ++-------- .../org/springframework/shell/ShellTest.java | 55 +++++++--------- .../samples/standard/RegisterCommands.java | 65 +++++++++++++++++++ .../shell/standard/commands/HelpTest.java | 43 +++++++----- 7 files changed, 155 insertions(+), 83 deletions(-) create mode 100644 spring-shell-samples/src/main/java/org/springframework/shell/samples/standard/RegisterCommands.java diff --git a/spring-shell-core/src/main/java/org/springframework/shell/CommandRegistry.java b/spring-shell-core/src/main/java/org/springframework/shell/CommandRegistry.java index 68349308..0051ea4d 100644 --- a/spring-shell-core/src/main/java/org/springframework/shell/CommandRegistry.java +++ b/spring-shell-core/src/main/java/org/springframework/shell/CommandRegistry.java @@ -1,5 +1,5 @@ /* - * Copyright 2015 the original author or authors. + * Copyright 2015-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,7 +13,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - package org.springframework.shell; import java.util.Map; @@ -23,13 +22,27 @@ import java.util.Map; * discover available commands. * * @author Eric Bottard + * @author Janne Valkealahti */ public interface CommandRegistry { - /** * Return the mapping from command trigger keywords to implementation. */ - public Map listCommands(); + Map listCommands(); + /** + * Register a new command. + * + * @param name the command name + * @param target the method target + */ + void addCommand(String name, MethodTarget target); + + /** + * Deregister a command. + * + * @param name the command name + */ + void removeCommand(String name); } diff --git a/spring-shell-core/src/main/java/org/springframework/shell/ConfigurableCommandRegistry.java b/spring-shell-core/src/main/java/org/springframework/shell/ConfigurableCommandRegistry.java index 46797477..2159c803 100644 --- a/spring-shell-core/src/main/java/org/springframework/shell/ConfigurableCommandRegistry.java +++ b/spring-shell-core/src/main/java/org/springframework/shell/ConfigurableCommandRegistry.java @@ -58,6 +58,16 @@ public class ConfigurableCommandRegistry implements CommandRegistry { .collect(Collectors.toMap(e -> e.getKey(), e -> e.getValue())); } + @Override + public void addCommand(String name, MethodTarget target) { + commands.put(name, target); + } + + @Override + public void removeCommand(String name) { + commands.remove(name); + } + public void register(String name, MethodTarget target) { MethodTarget previous = commands.get(name); if (previous != null) { diff --git a/spring-shell-core/src/main/java/org/springframework/shell/MethodTarget.java b/spring-shell-core/src/main/java/org/springframework/shell/MethodTarget.java index 0eced9cd..23d01527 100644 --- a/spring-shell-core/src/main/java/org/springframework/shell/MethodTarget.java +++ b/spring-shell-core/src/main/java/org/springframework/shell/MethodTarget.java @@ -68,6 +68,14 @@ public class MethodTarget implements Command { this.interactionMode = interactionMode; } + /** + * Construct a MethodTarget for the unique method named {@literal name} on the given object. Fails with an exception + * in case of overloaded method. + */ + public static MethodTarget of(String name, Object bean, String description, String group) { + return of(name, bean, new Help(description, group)); + } + /** * Construct a MethodTarget for the unique method named {@literal name} on the given object. Fails with an exception * in case of overloaded method. diff --git a/spring-shell-core/src/main/java/org/springframework/shell/Shell.java b/spring-shell-core/src/main/java/org/springframework/shell/Shell.java index 1a2b7c61..1d15705a 100644 --- a/spring-shell-core/src/main/java/org/springframework/shell/Shell.java +++ b/spring-shell-core/src/main/java/org/springframework/shell/Shell.java @@ -22,13 +22,11 @@ import java.lang.reflect.UndeclaredThrowableException; import java.nio.channels.ClosedByInterruptException; import java.util.ArrayList; import java.util.Arrays; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; import java.util.stream.Collectors; -import javax.annotation.PostConstruct; import javax.validation.ConstraintViolation; import javax.validation.Validator; import javax.validation.ValidatorFactory; @@ -36,7 +34,6 @@ import javax.validation.ValidatorFactory; import org.jline.utils.Signals; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.context.ApplicationContext; import org.springframework.core.MethodParameter; import org.springframework.core.annotation.AnnotationAwareOrderComparator; import org.springframework.util.ReflectionUtils; @@ -66,15 +63,10 @@ public class Shell { */ public static final Object NO_INPUT = new Object(); - @Autowired - protected ApplicationContext applicationContext; - - private CommandRegistry commandRegistry; + private final CommandRegistry commandRegistry; private Validator validator = Utils.defaultValidator(); - protected Map methodTargets = new HashMap<>(); - protected List parameterResolvers; /** @@ -93,13 +85,6 @@ public class Shell { this.validator = validatorFactory.getValidator(); } - @PostConstruct - public void gatherMethodTargets() throws Exception { - methodTargets = commandRegistry.listCommands(); - methodTargets.values() - .forEach(this::validateParameterResolvers); - } - @Autowired public void setParameterResolvers(List resolvers) { this.parameterResolvers = new ArrayList<>(resolvers); @@ -158,6 +143,7 @@ public class Shell { List words = input.words(); if (command != null) { + Map methodTargets = commandRegistry.listCommands(); MethodTarget methodTarget = methodTargets.get(command); Availability availability = methodTarget.getAvailability(); if (availability.isAvailable()) { @@ -240,6 +226,7 @@ public class Shell { if (best != null) { CompletionContext argsContext = context.drop(best.split(" ").length); // Try to complete arguments + Map methodTargets = commandRegistry.listCommands(); MethodTarget methodTarget = methodTargets.get(best); Method method = methodTarget.getMethod(); @@ -260,6 +247,7 @@ public class Shell { // Workaround for https://github.com/spring-projects/spring-shell/issues/150 // (sadly, this ties this class to JLine somehow) int lastWordStart = prefix.lastIndexOf(' ') + 1; + Map methodTargets = commandRegistry.listCommands(); return methodTargets.entrySet().stream() .filter(e -> e.getKey().startsWith(prefix)) .map(e -> toCommandProposal(e.getKey().substring(lastWordStart), e.getValue())) @@ -313,30 +301,16 @@ public class Shell { return args; } - /** - * Verifies that we have at least one {@link ParameterResolver} that supports each of the - * {@link MethodParameter}s in the method. - */ - private void validateParameterResolvers(MethodTarget methodTarget) { - Utils.createMethodParameters(methodTarget.getMethod()) - .forEach(parameter -> { - parameterResolvers.stream() - .filter(resolver -> resolver.supports(parameter)) - .findFirst() - .orElseThrow(() -> new ParameterResolverMissingException(parameter)); - }); - } - /** * Returns the longest command that can be matched as first word(s) in the given buffer. * * @return a valid command name, or {@literal null} if none matched */ private String findLongestCommand(String prefix) { + Map methodTargets = commandRegistry.listCommands(); String result = methodTargets.keySet().stream() .filter(command -> prefix.equals(command) || prefix.startsWith(command + " ")) .reduce("", (c1, c2) -> c1.length() > c2.length() ? c1 : c2); return "".equals(result) ? null : result; } - } diff --git a/spring-shell-core/src/test/java/org/springframework/shell/ShellTest.java b/spring-shell-core/src/test/java/org/springframework/shell/ShellTest.java index c146a1f2..66d5e993 100644 --- a/spring-shell-core/src/test/java/org/springframework/shell/ShellTest.java +++ b/spring-shell-core/src/test/java/org/springframework/shell/ShellTest.java @@ -19,7 +19,9 @@ package org.springframework.shell; import java.io.IOException; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.stream.Collectors; import org.junit.jupiter.api.BeforeEach; @@ -29,15 +31,11 @@ import org.mockito.InjectMocks; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; -import org.springframework.context.ApplicationContext; - import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.Assertions.fail; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.isA; import static org.mockito.Mockito.doThrow; -import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; /** @@ -54,6 +52,9 @@ public class ShellTest { @Mock ResultHandlerService resultHandlerService; + @Mock + CommandRegistry commandRegistry; + @Mock private ParameterResolver parameterResolver; @@ -77,7 +78,8 @@ public class ShellTest { when(parameterResolver.resolve(any(), any())).thenReturn(valueResult); doThrow(new Exit()).when(resultHandlerService).handle(any()); - shell.methodTargets = Collections.singletonMap("hello world", MethodTarget.of("helloWorld", this, new Command.Help("Say hello"))); + when(commandRegistry.listCommands()).thenReturn(Collections.singletonMap("hello world", + MethodTarget.of("helloWorld", this, new Command.Help("Say hello")))); try { shell.run(inputProvider); @@ -95,7 +97,8 @@ public class ShellTest { when(inputProvider.readInput()).thenReturn(() -> "hello world how are you doing ?"); doThrow(new Exit()).when(resultHandlerService).handle(isA(CommandNotFound.class)); - shell.methodTargets = Collections.singletonMap("bonjour", MethodTarget.of("helloWorld", this, new Command.Help("Say hello"))); + when(commandRegistry.listCommands()).thenReturn(Collections.singletonMap("bonjour", + MethodTarget.of("helloWorld", this, new Command.Help("Say hello")))); try { shell.run(inputProvider); @@ -112,7 +115,8 @@ public class ShellTest { when(inputProvider.readInput()).thenReturn(() -> "helloworld how are you doing ?"); doThrow(new Exit()).when(resultHandlerService).handle(isA(CommandNotFound.class)); - shell.methodTargets = Collections.singletonMap("hello", MethodTarget.of("helloWorld", this, new Command.Help("Say hello"))); + when(commandRegistry.listCommands()).thenReturn( + Collections.singletonMap("hello", MethodTarget.of("helloWorld", this, new Command.Help("Say hello")))); try { shell.run(inputProvider); @@ -131,7 +135,8 @@ public class ShellTest { when(parameterResolver.resolve(any(), any())).thenReturn(valueResult); doThrow(new Exit()).when(resultHandlerService).handle(any()); - shell.methodTargets = Collections.singletonMap("hello world", MethodTarget.of("helloWorld", this, new Command.Help("Say hello"))); + when(commandRegistry.listCommands()).thenReturn(Collections.singletonMap("hello world", + MethodTarget.of("helloWorld", this, new Command.Help("Say hello")))); try { shell.run(inputProvider); @@ -149,7 +154,8 @@ public class ShellTest { when(inputProvider.readInput()).thenReturn(() -> "fail"); doThrow(new Exit()).when(resultHandlerService).handle(isA(SomeException.class)); - shell.methodTargets = Collections.singletonMap("fail", MethodTarget.of("failing", this, new Command.Help("Will throw an exception"))); + when(commandRegistry.listCommands()).thenReturn(Collections.singletonMap("fail", + MethodTarget.of("failing", this, new Command.Help("Will throw an exception")))); try { shell.run(inputProvider); @@ -169,36 +175,19 @@ public class ShellTest { shell.run(inputProvider); } - // no need to test as we're moving away from postconstruct - // @Test - public void parametersSupported() throws Exception { - when(parameterResolver.supports(any())).thenReturn(false); - shell.applicationContext = mock(ApplicationContext.class); - when(shell.applicationContext.getBeansOfType(MethodTargetRegistrar.class)) - .thenReturn(Collections.singletonMap("foo", r -> { - r.register("hw", MethodTarget.of("helloWorld", this, new Command.Help("hellow world"))); - })); - - assertThatThrownBy(() -> { - shell.gatherMethodTargets(); - }).isInstanceOf(ParameterResolverMissingException.class); - } - - // @Test + @Test public void commandNameCompletion() throws Exception { - shell.applicationContext = mock(ApplicationContext.class); + Map methodTargets = new HashMap<>(); + methodTargets.put("hello world", MethodTarget.of("helloWorld", this, new Command.Help("hellow world"))); + methodTargets.put("another command", MethodTarget.of("helloWorld", this, new Command.Help("another command"))); when(parameterResolver.supports(any())).thenReturn(true); - when(shell.applicationContext.getBeansOfType(MethodTargetRegistrar.class)) - .thenReturn(Collections.singletonMap("foo", r -> { - r.register("hello world", MethodTarget.of("helloWorld", this, new Command.Help("hellow world"))); - r.register("another command", MethodTarget.of("helloWorld", this, new Command.Help("another command"))); - })); - shell.gatherMethodTargets(); + when(commandRegistry.listCommands()).thenReturn(methodTargets); // Invoke at very start List proposals = shell.complete(new CompletionContext(Arrays.asList(""), 0, "".length())) .stream().map(CompletionProposal::value).collect(Collectors.toList()); - assertThat(proposals).containsExactly("another command", "hello world"); + assertThat(proposals).containsExactlyInAnyOrder("another command", "hello world"); + // assertThat(proposals).containsExactly("another command", "hello world"); // Invoke in middle of first word proposals = shell.complete(new CompletionContext(Arrays.asList("hel"), 0, "hel".length())) diff --git a/spring-shell-samples/src/main/java/org/springframework/shell/samples/standard/RegisterCommands.java b/spring-shell-samples/src/main/java/org/springframework/shell/samples/standard/RegisterCommands.java new file mode 100644 index 00000000..8bdb09e1 --- /dev/null +++ b/spring-shell-samples/src/main/java/org/springframework/shell/samples/standard/RegisterCommands.java @@ -0,0 +1,65 @@ +/* + * 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. + * 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.standard; + +import org.springframework.shell.MethodTarget; +import org.springframework.shell.standard.AbstractShellComponent; +import org.springframework.shell.standard.ShellComponent; +import org.springframework.shell.standard.ShellMethod; +import org.springframework.shell.standard.ShellOption; + +@ShellComponent +public class RegisterCommands extends AbstractShellComponent { + + private final PojoMethods pojoMethods = new PojoMethods(); + + @ShellMethod(key = "register add", value = "Register commands", group = "Register Commands") + public String register() { + MethodTarget target1 = MethodTarget.of("dynamic1", pojoMethods, "Dynamic1 command", "Register Commands"); + MethodTarget target2 = MethodTarget.of("dynamic2", pojoMethods, "Dynamic2 command", "Register Commands"); + MethodTarget target3 = MethodTarget.of("dynamic3", pojoMethods, "Dynamic3 command", "Register Commands"); + getCommandRegistry().addCommand("register dynamic1", target1); + getCommandRegistry().addCommand("register dynamic2", target2); + getCommandRegistry().addCommand("register dynamic3", target3); + return "Registered commands dynamic1, dynamic2, dynamic3"; + } + + @ShellMethod(key = "register remove", value = "Deregister commands", group = "Register Commands") + public String deregister() { + getCommandRegistry().removeCommand("register dynamic1"); + getCommandRegistry().removeCommand("register dynamic2"); + getCommandRegistry().removeCommand("register dynamic3"); + return "Deregistered commands dynamic1, dynamic2, dynamic3"; + } + + public static class PojoMethods { + + @ShellMethod + public String dynamic1() { + return "dynamic1"; + } + + @ShellMethod + public String dynamic2(String arg1) { + return "dynamic2" + arg1; + } + + @ShellMethod + public String dynamic3(@ShellOption(defaultValue = ShellOption.NULL) String arg1) { + return "dynamic3" + arg1; + } + } +} diff --git a/spring-shell-standard-commands/src/test/java/org/springframework/shell/standard/commands/HelpTest.java b/spring-shell-standard-commands/src/test/java/org/springframework/shell/standard/commands/HelpTest.java index cc5d6224..3baade57 100644 --- a/spring-shell-standard-commands/src/test/java/org/springframework/shell/standard/commands/HelpTest.java +++ b/spring-shell-standard-commands/src/test/java/org/springframework/shell/standard/commands/HelpTest.java @@ -123,26 +123,39 @@ public class HelpTest { @Bean public CommandRegistry shell() { - return () -> { - Map 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); + return new CommandRegistry() { - methodTarget = MethodTarget.of("thirdCommand", commands(), new Command.Help("The last command.")); - result.put("third-command", methodTarget); + @Override + public Map listCommands() { + Map 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("firstCommandInGroup", commands(), new Command.Help("The first command in a separate group.", "Example Group")); - result.put("first-group-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("secondCommandInGroup", commands(), new Command.Help("The second command in a separate group.", "Example Group")); - result.put("second-group-command", methodTarget); + methodTarget = MethodTarget.of("thirdCommand", commands(), new Command.Help("The last command.")); + result.put("third-command", methodTarget); - return result; + 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) { + } }; }