Skip to content

Rework command subsystem #396

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ target/
*.iws
.DS_Store
spring-shell.log
shell.log
shell.log.*.gz

# Visual Studio Code
.vscode/*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* Copyright 2021-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.boot;

import java.util.List;
import java.util.stream.Collectors;

import org.springframework.beans.factory.ObjectProvider;
import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.shell.MethodTargetRegistrar;
import org.springframework.shell.command.CommandCatalog;
import org.springframework.shell.command.CommandCatalogCustomizer;
import org.springframework.shell.command.CommandRegistration;
import org.springframework.shell.command.CommandResolver;

@Configuration(proxyBeanMethods = false)
public class CommandCatalogAutoConfiguration {

@Bean
@ConditionalOnMissingBean(CommandCatalog.class)
public CommandCatalog commandCatalog(ObjectProvider<MethodTargetRegistrar> methodTargetRegistrars,
ObjectProvider<CommandResolver> commandResolvers,
ObjectProvider<CommandCatalogCustomizer> commandCatalogCustomizers) {
List<CommandResolver> resolvers = commandResolvers.orderedStream().collect(Collectors.toList());
CommandCatalog catalog = CommandCatalog.of(resolvers, null);
methodTargetRegistrars.orderedStream().forEach(resolver -> {
resolver.register(catalog);
});
commandCatalogCustomizers.orderedStream().forEach(customizer -> {
customizer.customize(catalog);
});
return catalog;
}

@Bean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I would consider ordering this default customizer as line44 spins through them ordered.
  2. Would we ever want to disable or override this default behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Now that you mentioned it, we probably should have a flag.

public CommandCatalogCustomizer defaultCommandCatalogCustomizer(ObjectProvider<CommandRegistration> commandRegistrations) {
return catalog -> {
commandRegistrations.orderedStream().forEach(registration -> {
catalog.register(registration);
});
};
}
}

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2021 the original author or authors.
* Copyright 2021-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.
Expand Down Expand Up @@ -34,7 +34,7 @@
import org.springframework.context.annotation.Configuration;
import org.springframework.context.event.ContextClosedEvent;
import org.springframework.context.event.EventListener;
import org.springframework.shell.CommandRegistry;
import org.springframework.shell.command.CommandCatalog;

@Configuration(proxyBeanMethods = false)
public class LineReaderAutoConfiguration {
Expand All @@ -45,15 +45,15 @@ public class LineReaderAutoConfiguration {

private Parser parser;

private CommandRegistry commandRegistry;
private CommandCatalog commandRegistry;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll mention this once here as its a [NIT] but worth pointing out... the var name needs to change as well. Its still reads "Registry" below in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack


private org.jline.reader.History jLineHistory;

@Value("${spring.application.name:spring-shell}.log")
private String historyPath;

public LineReaderAutoConfiguration(Terminal terminal, Completer completer, Parser parser,
CommandRegistry commandRegistry, org.jline.reader.History jLineHistory) {
CommandCatalog commandRegistry, org.jline.reader.History jLineHistory) {
this.terminal = terminal;
this.completer = completer;
this.parser = parser;
Expand All @@ -79,7 +79,7 @@ public LineReader lineReader() {
public AttributedString highlight(LineReader reader, String buffer) {
int l = 0;
String best = null;
for (String command : commandRegistry.listCommands().keySet()) {
for (String command : commandRegistry.getRegistrations().keySet()) {
if (buffer.startsWith(command) && command.length() > l) {
l = command.length();
best = command;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,24 +1,35 @@
package org.springframework.shell.boot;

import java.util.Set;
import java.util.stream.Collectors;
import java.util.ArrayList;
import java.util.List;

import org.springframework.beans.factory.ObjectProvider;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.core.convert.ConversionService;
import org.springframework.shell.ParameterResolver;
import org.springframework.shell.standard.StandardParameterResolver;
import org.springframework.shell.standard.ValueProvider;
import org.springframework.core.convert.support.DefaultConversionService;
import org.springframework.messaging.handler.annotation.support.HeadersMethodArgumentResolver;
import org.springframework.messaging.handler.invocation.HandlerMethodArgumentResolver;
import org.springframework.shell.command.ArgumentHeaderMethodArgumentResolver;
import org.springframework.shell.command.CommandContextMethodArgumentResolver;
import org.springframework.shell.command.CommandExecution.CommandExecutionHandlerMethodArgumentResolvers;
import org.springframework.shell.completion.CompletionResolver;
import org.springframework.shell.completion.DefaultCompletionResolver;
import org.springframework.shell.standard.ShellOptionMethodArgumentResolver;

@Configuration(proxyBeanMethods = false)
public class ParameterResolverAutoConfiguration {

@Bean
public ParameterResolver standardParameterResolver(ConversionService conversionService,
ObjectProvider<ValueProvider> valueProviders) {
Set<ValueProvider> collect = valueProviders.orderedStream().collect(Collectors.toSet());
return new StandardParameterResolver(conversionService, collect);
public CompletionResolver defaultCompletionResolver() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any need to disable or override this default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently I don't think so.

return new DefaultCompletionResolver();
}

@Bean
public CommandExecutionHandlerMethodArgumentResolvers commandExecutionHandlerMethodArgumentResolvers() {
List<HandlerMethodArgumentResolver> resolvers = new ArrayList<>();
resolvers.add(new ArgumentHeaderMethodArgumentResolver(new DefaultConversionService(), null));
resolvers.add(new HeadersMethodArgumentResolver());
resolvers.add(new CommandContextMethodArgumentResolver());
resolvers.add(new ShellOptionMethodArgumentResolver(new DefaultConversionService(), null));
return new CommandExecutionHandlerMethodArgumentResolvers(resolvers);
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2017-2021 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.
Expand All @@ -18,6 +18,8 @@

import java.util.Set;

import org.jline.terminal.Terminal;

import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean;
import org.springframework.boot.convert.ApplicationConversionService;
import org.springframework.context.ApplicationContext;
Expand All @@ -27,10 +29,10 @@
import org.springframework.core.convert.ConversionService;
import org.springframework.core.convert.support.DefaultConversionService;
import org.springframework.format.support.FormattingConversionService;
import org.springframework.shell.CommandRegistry;
import org.springframework.shell.ResultHandler;
import org.springframework.shell.ResultHandlerService;
import org.springframework.shell.Shell;
import org.springframework.shell.command.CommandCatalog;
import org.springframework.shell.result.GenericResultHandlerService;
import org.springframework.shell.result.ResultHandlerConfig;

Expand Down Expand Up @@ -61,7 +63,7 @@ public ResultHandlerService resultHandlerService(Set<ResultHandler<?>> resultHan
}

@Bean
public Shell shell(ResultHandlerService resultHandlerService, CommandRegistry commandRegistry) {
return new Shell(resultHandlerService, commandRegistry);
public Shell shell(ResultHandlerService resultHandlerService, CommandCatalog commandRegistry, Terminal terminal) {
return new Shell(resultHandlerService, commandRegistry, terminal);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@

import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.shell.CommandRegistry;
import org.springframework.shell.MethodTargetRegistrar;
import org.springframework.shell.command.CommandCatalog;
import org.springframework.shell.standard.CommandValueProvider;
import org.springframework.shell.standard.EnumValueProvider;
import org.springframework.shell.standard.FileValueProvider;
Expand All @@ -35,7 +35,7 @@
public class StandardAPIAutoConfiguration {

@Bean
public ValueProvider commandValueProvider(CommandRegistry commandRegistry) {
public ValueProvider commandValueProvider(CommandCatalog commandRegistry) {
return new CommandValueProvider(commandRegistry);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@ org.springframework.shell.boot.ShellContextAutoConfiguration,\
org.springframework.shell.boot.SpringShellAutoConfiguration,\
org.springframework.shell.boot.ShellRunnerAutoConfiguration,\
org.springframework.shell.boot.ApplicationRunnerAutoConfiguration,\
org.springframework.shell.boot.CommandRegistryAutoConfiguration,\
org.springframework.shell.boot.CommandCatalogAutoConfiguration,\
org.springframework.shell.boot.LineReaderAutoConfiguration,\
org.springframework.shell.boot.CompleterAutoConfiguration,\
org.springframework.shell.boot.JLineAutoConfiguration,\
org.springframework.shell.boot.JLineShellAutoConfiguration,\
org.springframework.shell.boot.JCommanderParameterResolverAutoConfiguration,\
org.springframework.shell.boot.ParameterResolverAutoConfiguration,\
org.springframework.shell.boot.StandardAPIAutoConfiguration,\
org.springframework.shell.boot.ThemingAutoConfiguration,\
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
/*
* 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.boot;

import java.util.Collections;

import org.assertj.core.api.InstanceOfAssertFactories;
import org.junit.jupiter.api.Test;

import org.springframework.boot.autoconfigure.AutoConfigurations;
import org.springframework.boot.test.context.runner.ApplicationContextRunner;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.shell.command.CommandCatalog;
import org.springframework.shell.command.CommandRegistration;
import org.springframework.shell.command.CommandResolver;

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

public class CommandCatalogAutoConfigurationTests {

private final ApplicationContextRunner contextRunner = new ApplicationContextRunner()
.withConfiguration(AutoConfigurations.of(CommandCatalogAutoConfiguration.class));

@Test
void defaultCommandCatalog() {
this.contextRunner.run((context) -> assertThat(context).hasSingleBean(CommandCatalog.class));
}

@Test
void testCommandResolvers() {
this.contextRunner.withUserConfiguration(CustomCommandResolverConfiguration.class)
.run((context) -> {
CommandCatalog commandCatalog = context.getBean(CommandCatalog.class);
assertThat(commandCatalog).extracting("resolvers").asInstanceOf(InstanceOfAssertFactories.LIST)
.hasSize(1);
});
}

@Test
void customCommandCatalog() {
this.contextRunner.withUserConfiguration(CustomCommandCatalogConfiguration.class)
.run((context) -> {
CommandCatalog commandCatalog = context.getBean(CommandCatalog.class);
assertThat(commandCatalog).isSameAs(CustomCommandCatalogConfiguration.testCommandCatalog);
});
}

@Test
void registerCommandRegistration() {
this.contextRunner.withUserConfiguration(CustomCommandRegistrationConfiguration.class)
.run((context) -> {
CommandCatalog commandCatalog = context.getBean(CommandCatalog.class);
assertThat(commandCatalog.getRegistrations().get("customcommand")).isNotNull();
});
}

@Configuration
static class CustomCommandResolverConfiguration {

@Bean
CommandResolver customCommandResolver() {
return () -> Collections.emptyList();
}
}

@Configuration
static class CustomCommandCatalogConfiguration {

static final CommandCatalog testCommandCatalog = CommandCatalog.of();

@Bean
CommandCatalog customCommandCatalog() {
return testCommandCatalog;
}
}

@Configuration
static class CustomCommandRegistrationConfiguration {

@Bean
CommandRegistration commandRegistration() {
return CommandRegistration.builder()
.command("customcommand")
.withTarget()
.function(ctx -> {
return null;
})
.and()
.build();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@

import org.springframework.boot.autoconfigure.AutoConfigurations;
import org.springframework.boot.test.context.runner.ApplicationContextRunner;
import org.springframework.shell.ParameterResolver;
import org.springframework.shell.Shell;
import org.springframework.shell.command.CommandExecution.CommandExecutionHandlerMethodArgumentResolvers;
import org.springframework.shell.completion.CompletionResolver;
import org.springframework.shell.context.ShellContext;
import org.springframework.shell.jline.InteractiveShellRunner;
import org.springframework.shell.jline.NonInteractiveShellRunner;
Expand All @@ -46,7 +47,8 @@ class ShellRunnerAutoConfigurationTests {
.withBean(LineReader.class, () -> mock(LineReader.class))
.withBean(Parser.class, () -> mock(Parser.class))
.withBean(ShellContext.class, () -> mock(ShellContext.class))
.withBean(ParameterResolver.class, () -> mock(ParameterResolver.class));
.withBean(CompletionResolver.class, () -> mock(CompletionResolver.class))
.withBean(CommandExecutionHandlerMethodArgumentResolvers.class, () -> mock(CommandExecutionHandlerMethodArgumentResolvers.class));

@Nested
class Interactive {
Expand Down
Loading