Restore support for command grouping at class level in 4.0

[ThomasVitale]

In Spring Shell 3.4, it was possible to define all commands belonging to the same group in the same class, and specify group information only once.

Example in 3.4:

@Command(command = "image build", group = "Image")
public class ImageBuildCommands {

    @Command(command = "buildpacks", description = "Build a container image using Buildpacks.")
    public void buildpacks() {
        ...
    }

    @Command(command = "dockerfile", description = "Build a container image using Dockerfile.")
    public void dockerfile() {
        ...
    }
    
}

Now, in Spring Shell 4.0, the @Command annotation cannot be assigned to a class anymore. It was indeed confusing to have such an annotation available at class level, but it was useful for configuring groups information.

Example in 4.0:

@Component
public class ImageBuildCommands {

    @Command(name = { "image", "build", "buildpacks" }, description = "Build a container image using Buildpacks.", group = "Image")
    public void buildpacks() {
        ...
    }

    @Command(name = { "image", "build", "dockerfile" }, description = "Build a container image using Dockerfile.", group = "Image")
    public void dockerfile() {
        ...
    }
    
}

Notice how I had to duplicate the group = "Image" attribute for each command in the group. Furthermore, I have to duplicate the parent commands for each command in the group: name = { "image", "build", "..." }. And things become even more verbose when attributes like alias are used, requiring duplication of the parent alias commands for each command in the group.

How about introducing a @CommandGroup annotation to restore some of the functionality in 3.4? Otherwise, currently in 4.0, things become very verbose and error-prone.

I'm available to help with a potential PR.

[brettryan]

What's a little dangerous about the new behaviour is that if you happen to have two classes that previously qualified commands by a "command" prefix, this would prevent command collision, unfortunately, now; there's no collision and the last bean registered will override the previous.

@Component
public class Commands1 {
    @Command(name = "my-command")
    void myCommand() {
        // ...
    }
}

@Component
public class Commands2 {
    @Command(name = "my-command")
    void myCommand() {
        // ...
    }
}

Here, you don't know which command is going to be registered.

[fmbenhassine]

Thank you for opening this issue. I talked about this here, but I will repeat the most relevant details in this issue for more visibility.

First thing, I want to give more context about the reasoning behind this change. I really don't see the need for using the same annotation at two different levels to inherit common attributes, especially an @Command annotation (named in singular) at a class level in which multiple commands are defined (how confusing is that?).

How about introducing a @CommandGroup annotation to restore some of the functionality in 3.4?

Spring Shell v2 had a ShellCommandGroup which was way less confusing than using the same @Command annotation at two different levels as in v3. I am open to improve things with a different annotation if really needed.

Now that said, you can still use constants to avoid duplication and define commons attributes:

import org.springframework.shell.core.command.annotation.Command;
import org.springframework.stereotype.Component;

@Component
public class ImageBuildCommands {

	private static final String GROUP = "Image";
	private static final String ROOT_COMMAND = "image build";

    @Command(name = { ROOT_COMMAND, "buildpacks" }, description = "Build a container image using Buildpacks.", group = GROUP)
    public void buildpacks() {
        
    }

    @Command(name = { ROOT_COMMAND, "dockerfile" }, description = "Build a container image using Dockerfile.", group = GROUP)
    public void dockerfile() {
        
    }
    
}

I am aware that the one "downside" of this is having to define the group attribute on both commands, which is really only an issue from a puristic point of view (but still a valid issue, which is probably related to the fact that the Java annotation model does not support inheritance ), but this is only an "issue" for the annotation approach, as you could use the programmatic approach and make all related annotations inherit from a base class (AbstractCommand for instance) and define common attributes at the base class level.

Now I still want to share some puristic thoughts.. How is that different from the case where one wants to make several Spring bean definitions inherit the same attributes from the parent's class annotation? Like this for instance:

import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;

@Configuration
//@Configuration(autowireCandidate = false) // Why isn't this a problem even if not possible? 
public class RelatedBeans {

	public static final boolean AUTOWIRE_CANDIDATE = false;

	@Bean(autowireCandidate = AUTOWIRE_CANDIDATE)
	public String bean1() {
		return "bean1";
	}

	@Bean(autowireCandidate = AUTOWIRE_CANDIDATE)
	public String bean2() {
		return "bean2";
	}

}

It's amazing how no one is complaining against Spring Framework about not being able to do that, because to me, this is technically the exact same problem.

[ThomasVitale]

Thank you, @fmbenhassine! I'll also share here my comment from the other thread.

To me the use case of having a class-level annotation and a method-level annotation is very similar to how @RequestMapping and similar annotations work in Spring WebMVC regarding the path.

In Spring WebMVC I can do this and the path defined in the class-level annotation is combined with the path defined on the method-level annotation. Very convenient, clear, and readable. At a glance, I can tell all the mappings defined in this class have a /demo base path.

@RestController
@RequestMapping(path = "/demo")
class DemoController {

	@GetMapping(path = "/hello")
	String hello() {
		return "Hello World!";
	}

	@GetMapping(path = "/greetings")
	String greetings() {
		return "Greetings, World!";
	}
	
}

I would be very inconvenient having to do something like the following, without the class-level annotation. This is more error-prone and not much readable. I cannot safely infer that all the mappings defined in this class have a /demo base path unless I inspect the code for all of them, and possibly write auto-tests to ensure I don't miss one or misspell anything. I imagine many people would complain if this was how Spring WebMVC was implemented.

@RestController
class DemoController {
	
	private static final String BASE_PATH = "/demo";

	@GetMapping(path = BASE_PATH + "/hello")
	String hello() {
		return "Hello World!";
	}

	@GetMapping(path = BASE_PATH + "/greetings")
	String greetings() {
		return "Greetings, World!";
	}

}

The same model applies to other Spring projects, such as for GraphQL or for WebSockets, where the mappings paths are combined in a similar way. So I would say that there are various Spring projects where this feature of combining parameters between class-level annotations and method-level annotations is supported. And it feels natural as a Spring user since it's used in other places of the Spring ecosystem.

I still believe it would be nice to introduce a new, dedicated class-level annotation (I agree that re-using @Command was confusing).

[piotrooo]

@fmbenhassine for the reference we had this discussion months ago #1209

@ThomasVitale FYI.

[fmbenhassine]

Thank you for the feedback, @ThomasVitale . In the Spring MVC example you shared, note how the base path and the actual path are defined with two different annotations (@RequestMapping(path = "/demo") and @GetMapping(path = "/hello")) and not the same @GetMapping annotation, which is exactly the point I was trying to make when I said: "I really don't see the need for using the same annotation at two different levels to inherit common attributes, especially an @Command annotation (named in singular) at a class level in which multiple commands are defined".

I imagine many people would complain if this was how Spring WebMVC was implemented.

To confirm the previous point, I also imagine how many people would be confused if Spring MVC was implemented like this:

@GetMapping(path = "/demo")
class DemoController {

	@GetMapping(path = "/hello")
	String hello() {
		return "Hello World!";
	}

	@GetMapping(path = "/greetings")
	String greetings() {
		return "Greetings, World!";
	}
	
}

which is the direct analogy with Spring Shell if we replace @GetMapping with @Command.

Unfortunately I don't have much context about these design decisions back then, but it feels to me that many things were designed in a similar fashion to Picocli, which supports both approaches, and which to me is even more confusing.. I am referring to this Spring Boot example where the @Command annotation is placed on a class, and options/arguments annotations are placed on the fields of the class (now where/what is the method that represents the command's logic?). This approach is a completely different way of designing / defining a command in a declarative way compared to placing annotations on a method and its parameters. I might be wrong, but I am always quickly confused when facing multiple ways of doing the same thing.. This is probably due to my way of thinking following the unix philosophy of making one thing do one thing and do it well and the Zen of Python where there should be one-- and preferably only one --obvious way to do it.

Anyway, I don't want to deviate from the main issue, but as mentioned previously, I am open to improve things with a different annotation as in v2 if needed.