Broken command discovery

[piotrooo]

@fmbenhassine said here:

@piotrooo Thank you for the PR! But I think there is deeper issue: command discovery is still broken. In the hello world example, spring-shell-starter brings spring-shell-standard which is supposed to add basic commands, but that is not the case. I mean with this PR, the hello world sample now works again and the command hello is recognized, but it's impossible to exit the shell.. exit and quit are not registered.

Moreover, I do not understand why ShellRunner#run returns a boolean and not void as it was before. The javadoc mentions Return value indicates if run operation happened and no further runners should be used. but in my opinion it is not the ShellRunner's repsonsbility to decide the flow of execution (ie if another runner should run or not). More explicitly, If I want to implement a custom runner, I don't know if I should return true or false in my custom implementation. In others words, having to do the following seems awkward to me:

boolean run = runner.run(args.getSourceArgs());
if (run) {
   return;
}

I think we need to fix that before proceeding as well.. Thoughts?

[piotrooo]

I mean with this PR, the hello world sample now works again and the command hello is recognized, but it's impossible to exit the shell.. exit and quit are not registered.

@fmbenhassine yes, after your clarification, I totally agree. You're right.

Moreover, I do not understand why ShellRunner#run returns a boolean and not void as it was before.

Indeed, that's awkward...

I was confused the first time I started using Spring Shell — why can I set the following set of parameters:

spring.shell.interactive.enabled=true
spring.shell.noninteractive.enabled=true
spring.shell.script.enabled=true

There should by only one runner at a time. The logic behind this works fine, but it's also reflected in the properties.

For Spring Boot, we should introduce something like this:

spring.shell.mode.type=interactive|noninteractive|script|custom(?)
spring.shell.mode.custom-runner=SomeClass.class # <- I'm not sure it's a good approach?

That configuration would allow us to select only one runner, without any precedence logic as it is right now.

And that should be the very first step. After that, we can start considering how to discover commands (or maybe even redesign the approach).

[piotrooo]

@fmbenhassine the second thing is how we should define and look up commands.

New stereotype

I’m considering creating a @Component-like stereotype annotation to mark classes as shell “controllers” (e.g. @Cli).
Classes annotated with this are considered as holders for commands

@Component
public @interface Cli {}

Analogy:

@RestController
class SomeRestController {}

Spring Shell Equivalent:

@Cli
class SomeCli {}

Class customizations

If you want to customize CLI classes — for example, set them as parent commands, add aliases, or assign them to groups — you can use the @Command annotation.
This annotation is meant only for configuring commands, not for discovering or registering beans.

Analogy:

@RestController
@RequestMapping("/api")
class SomeRestController {}

Spring Shell Equivalent:

@Cli
@Command("parent")
class SomeCli {}

@Cli
@Command("root", "parent")
class SomeCli {}

@Cli
@Command("root parent")
class SomeCli {}

Registering commands

To register commands, we use the @Command annotation — similar to how controller methods are mapped with @RequestMapping.
Methods inside CLI classes represent commands.

Analogy:

@RestController
@RequestMapping("/api")
class SomeRestController {
    @Get("/users")
    List<User> getUsers() {}
}

Spring Shell Equivalent:

@Cli
@Command("parent")
class SomeCli {
    @Command("child")
    String child() {
        return "this is child";
    }
}

Corner cases

1. Only a single command in a @Cli

What if we have only one command in a CLI class?
Should we call it parent or parent child (where child is the method name)?

@Cli
@Command("parent")
class SomeCli {
    @Command
    String child() {
        return "this is child";
    }
}

2. Aliases

How should we handle aliases?

Conclusion

This is how I imagine using a CLI — just like a regular web application, but for the terminal. WDYT @fmbenhassine?

Maybe it’s a completely wrong direction, but I’d like to hear your thoughts on this proposal.

[piotrooo]

One additional corner case to consider.

Should we allow the following command definition?

import org.springframework.boot.SpringApplication;
import org.springframework.boot.autoconfigure.SpringBootApplication;
import org.springframework.shell.core.command.annotation.Command;
import org.springframework.shell.core.command.annotation.Option;

@SpringBootApplication
public class SpringShellApplication {

	public static void main(String[] args) {
		SpringApplication.run(SpringShellApplication.class, args);
	}

	@Command
	public String hello(@Option(defaultValue = "World") String name) {
		return "Hello " + name + "!";
	}

}

[fmbenhassine]

Thank you for all these ideas! There is a lot going on here, so I will try to cover all your input.

Regarding command registration:

@Cli
@Command("parent")
class SomeCli {
    @Command("child")
    String child() {
        return "this is child";
    }
}

This was the case before with @ShellComponent (class level, like @Cli) and @ShellMethod (method level, like @Command), see https://docs.spring.io/spring-shell/reference/3.2/getting-started.html#using-spring-shell-your-first-command. But that approach and those annotations were deprecated, so I don't think we need to introduce something similar now.

I don't think we need a new class level annotation to mark a class as a source of command definitions, we already have the @Configuration from Spring Framework or @SpringBootApplication from Boot. I really see a Shell app as simple as defining a Spring context with a "command" bean:

@Configuration
public class MyShellApplication {

	@Bean
	public Command hello() {
		return new MyHelloCommand();
	}

}

That's similar to a batch app like:

@Configuration
public class MyBatchApplication {

	@Bean
	public Job job() {
		return new MyBatchJob();
	}

}

But unfortunately, that would require a redesign of the current Command interface to define options, aliases, business logic, etc and make it consistent with its annotation counterpart, @Command.

So I guess the middle ground is to have the same programming model with a simpler approach to command registration/discovery by the framework:

Without Spring Boot

Non boot users can enable command scanning with @EnableCommand (I would remove @CommandScan, there is really no need for two annotations to do the same thing, this is more than confusing), something like:

@EnableCommand(SpringShellApplication.class)
public class SpringShellApplication {

	public static void main(String[] args) throws Exception {
		ApplicationContext context = new AnnotationConfigApplicationContext(SpringShellApplication.class);
		InteractiveShellRunner shellRunner = context.getBean(InteractiveShellRunner.class); // or another runner
		shellRunner.run(args);
	}

	@Command
	public String hello(@Option(defaultValue = "World") String name) {
		return "Hello " + name + "!";
	}

}

More details in #1207

With Spring Boot

The added value of Boot here would be to load the context and discover/register commands (same as @EnableCommand in a non Boot world), load the runner from properties/classpath, etc. So as a Boot Shell user, I can write something like:

@SpringBootApplication
public class SpringShellApplication {

	public static void main(String[] args) {
		SpringApplication.run(SpringShellApplication.class, args);
	}

	@Command
	public String hello(@Option(defaultValue = "World") String name) {
		return "Hello " + name + "!";
	}

}

More details in #1206

[piotrooo]

But that approach and those annotations were deprecated, so I don't think we need to introduce something similar now.

Clear 👍

But unfortunately, that would require a redesign of the current Command interface to define options, aliases, business logic, etc and make it consistent with its annotation counterpart, @Command.

Command or maybe CommandDefinition will be better?

I would remove @CommandScan, there is really no need for two annotations to do the same thing, this is more than confusing

Totally! Completely agree.

[fmbenhassine]

Thank you for your feedback.

Command or maybe CommandDefinition will be better?

I understand that CommandDefinition is named after BeanDefinition, which is great! However, I just discovered that Command was not used in the framework, and removing it has no effect.. This is very surprising to me as I believe Command is the very central API for the entire framework. I Let me think about this a bit further and I will get back to you.

[piotrooo]

If you mean the Command interface, then yes — it only contains the Help inner class.

So Command also looks extremely appealing. One thing is certain: the current CommandRegistration name looks bizarre.

[fmbenhassine]

Yes, I mean the Command interface, not the annotation.

One thing is certain: the current CommandRegistration name looks bizarre.

Definitely!

So Command also looks extremely appealing.

Indeed. I remember when I worked on removing Spring Shell dependency from Spring StateMachine, I had to come with a mini framework to register commands just for the demos (see spring-projects/spring-statemachine@1a017dc). And I used something as simple as Command and CommandRunner that gets beans of type Command from the application context. There is no custom discovery mechanism.. It's all Spring, as simple as that. Obviously that is the most simple design that does the job (ie a couple demos for SSM), but Spring Shell can and should obviously do a more elaborate work with command options, aliases, etc. But the fundamental idea is the same: providing a programming model that is based on a central concept, Command, that could be implemented by the user (See BasicCommand) and registered as Spring beans. This is the approach I described earlier in #1209 (comment), when I said: "I really see a Shell app as simple as defining a Spring context with a "command" bean":

@Configuration
public class MyShellApplication {

	@Bean
	public Command hello() {
		return new MyHelloCommand();
	}

}

As of today, this programming model is not possible in Spring Shell v3. The Command interface is empty (and not even used!) and the user has to use CommandRegistration: https://docs.spring.io/spring-shell/reference/3.2/commands/registration/programmatic.html. There is no interface to implement. This DSL is nothing more than a syntactic sugar and should be a nice to have thing, not the single entry point to define/contribute commands. For me, this is really a big issue, and a blocker for v4 M2 (which was planned today, but I postponed it to next week). Thoughts?

[fmbenhassine]

Regarding the shell runner:

I was confused the first time I started using Spring Shell — why can I set the following set of parameters:

spring.shell.interactive.enabled=true
spring.shell.noninteractive.enabled=true
spring.shell.script.enabled=true
There should by only one runner at a time. The logic behind this works fine, but it's also reflected in the properties.

For Spring Boot, we should introduce something like this:

spring.shell.mode.type=interactive|noninteractive|script|custom(?)
spring.shell.mode.custom-runner=SomeClass.class # <- I'm not sure it's a good approach?
That configuration would allow us to select only one runner, without any precedence logic as it is right now.

First, I was also confused with all these properties and their precedence and I agree there should be only one runner active at a time. Probably a simpler way to configure this is to specify which runner to use:

spring.shell.runner=RunnerType.class

which would default to the InteractiveShellRunner.class and could be any ShellRunner implementation (one of the those built-in Spring Shell or a custom one). This approach requires a single property instead of two (spring.shell.mode.type + spring.shell.mode.custom-runner). Do you agree?

All in all, I am just trying to keep things as simple as possible. What do you think?

[piotrooo]

Do you think using a class type is more convenient than enum (strategy)?

But the argument for having only one property is really good and it's sufficient and easy 👍

[fmbenhassine]

I think using class type is better, because with an enum, every time we add a new implementation we would need to update the enum. Do you see the point?

However, the caveat with using a class type is its instantiation (we would require a default constructor to be able to instantiate the class, which is a strong constraint). But that is not of a big issue since we can use a bean name that we can get from the context to run the app after it was prepared by Spring (after DI &co).

[piotrooo]

I think using class type is better, because with an enum, every time we add a new implementation we would need to update the enum. Do you see the point?

Yes! Let's do it as you proposed.

[fmbenhassine]

Ok, let's try that and see if things get in the way.