When using UseCommandHandler with subcomand, Help overrides RootCommand

[leonardochaia]

Hi all,

I love this project. We are using it to build dotnet-affected!

I'm not sure if this is a known issue, but I'm migrating to use the Hosting setup and I'm seeing different behavior between declaring the RootCommand with a Handler, and binding a handler through the HostBuilder.

It only happens when using a subcommand. If I remove the subcommad, it works.

For example:

public class PublicApiTests
{
    private class SubCommand : Command
    {
        public SubCommand() 
            : base("thing")
        {
        }
    }
    private class HelloRootCommand : RootCommand
    {
        public HelloRootCommand()
        {
            this.AddCommand(new SubCommand());
        }
    }
    
    private class HelloHandler : ICommandHandler
    {
        public Task<int> InvokeAsync(InvocationContext context)
        {
            context.Console.Out.WriteLine("Hello World");
            return Task.FromResult(0);
        }
    }

    [Fact]
    public void Should_Print_HelloWorld()
    {
        var console = new TestConsole();
        new CommandLineBuilder(new HelloRootCommand() {Handler = new HelloHandler()})
            .UseDefaults()
            .UseHost(host =>
            {
                // .. configure DI
            })
            .Build()
            .Invoke("", console);
        
        Assert.Contains("Hello World", console.Out.ToString());
    }
    
   // this test fails! It prints the help
    [Fact]
    public void With_DI_Handler_Should_Print_HelloWorld()
    {
        var console = new TestConsole();
        new CommandLineBuilder(new HelloRootCommand())
            .UseHost(host =>
            {
                // .. configure DI
                host.UseCommandHandler<HelloRootCommand, HelloHandler>();
            })
            .UseDefaults()
            .Build()
            .Invoke("", console);
        
        Assert.Contains("Hello World", console.Out.ToString());
    }
}

EDIT: Changing the order of UseDefaults does not seem to make any difference.

[jonsequitur]

Because you haven't assigned the Handler on your root command in the second example, "" isn't a valid command line input. If you assign it a handler, it will work.

[leonardochaia]

Hey @jonsequitur ! appreciate the answer!

The thing is, I wanna instantiate my handler using DI, that's why I use the host.UseCommandHandler for it..
If I could just create instantiate the handler I would do it there ofc.

As I understand it, using host.UseCommandHandler is something like "binding the handler to the command".
It works fine for other sub commands so it should also work for the root command

EDIT: BTW, since I have your attention if you happen to click the links and see something being used incorrectly please do let me know.. Thank you for your time!

[jonsequitur]

Oh, sorry I missed that. Because the RootCommand is instantiated up-front, DI doesn't get involved.

@fredrikhr, thoughts on this?

[fredrikhr]

Hmm, looking at the source for UseCommandHandler, I can't understand how that extension method could ever work in the first place. But I haven't written that part, it appears that @Gronex did. Maybe he has some more input?

command-line-api/src/System.CommandLine.Hosting/HostingExtensions.cs

Lines 85 to 118 in a2ea4b1

	public static IHostBuilder UseCommandHandler<TCommand, THandler>(this IHostBuilder builder)
	where TCommand : Command
	where THandler : ICommandHandler
	{
	return builder.UseCommandHandler(typeof(TCommand), typeof(THandler));
	}
	public static IHostBuilder UseCommandHandler(this IHostBuilder builder, Type commandType, Type handlerType)
	{
	if (!typeof(Command).IsAssignableFrom(commandType))
	{
	throw new ArgumentException($"{nameof(commandType)} must be a type of {nameof(Command)}", nameof(handlerType));
	}
	if (!typeof(ICommandHandler).IsAssignableFrom(handlerType))
	{
	throw new ArgumentException($"{nameof(handlerType)} must implement {nameof(ICommandHandler)}", nameof(handlerType));
	}
	if (builder.Properties[typeof(InvocationContext)] is InvocationContext invocation
	&& invocation.ParseResult.CommandResult.Command is Command command
	&& command.GetType() == commandType)
	{
	invocation.BindingContext.AddService(handlerType, c => c.GetService<IHost>().Services.GetService(handlerType));
	builder.ConfigureServices(services =>
	{
	services.AddTransient(handlerType);
	});
	command.Handler = CommandHandler.Create(handlerType.GetMethod(nameof(ICommandHandler.InvokeAsync)));
	}
	return builder;
	}

Instead, I'd create an ICommandHandler like this:

var myCommand = new MyCommand()
{
  Handler = CommandHandler.Create(
  (InvocationContext context, IHost host) =>
  {
    var commandHandler = host.Services.GetRequiredService<MyCommandHandler>();
    return commandHandler.Invoke(context);
  }
};

that should enable you to use your Handler from DI, while still maintaining the Command for Parsing outside of Hosting.

The problem is that configuring the Command line argument parser happens outside the Generic Host, it's the parser that ultimately creates the host, and that happens after the command-line has already been parsed.

[leonardochaia]

I think it works by registering it against the internal's service provider here:

invocation.BindingContext.AddService(handlerType, c => c.GetService<IHost>().Services.GetService(handlerType)); 

I would assume that handler's are discovered from that service provider, and instantiated if they are not there?

command.Handler = CommandHandler.Create(handlerType.GetMethod(nameof(ICommandHandler.InvokeAsync))); 

Anyways, I think this is a very interesting topic. My goal was to find a way to inject services into command, and be able to override them for testing purposes.
I ended up not using the .Hosting package and wrote my own setup instead... I didn't wanna start/stop a complete generic host just for DI..

So I have this "builder" that allows me to ConfigureServices like one would do one the IHostBuilder:

Then I can do something among these lines:

 public static AffectedCommandLineBuilder CreateAffectedCommandLineBuilder()
        {
            return new AffectedCommandLineBuilder(new AffectedRootCommand())
                .ConfigureServices(services =>
                {
                    services.AddTransient<ICommandExecutionContext, CommandExecutionContext>();
                    services.AddTransient<IChangesProvider, GitChangesProvider>();
                    services.AddFromBindingContext<CommandExecutionData>();
                })
                .ConfigureCommandLine(builder =>
                {
                    builder.UseDefaults()
                        .UseCommandHandler<AffectedRootCommand, AffectedRootCommand.AffectedCommandHandler>()
                        .UseCommandHandler<ChangesCommand, ChangesCommand.CommandHandler>()
                        .UseCommandHandler<GenerateCommand, GenerateCommand.CommandHandler>();
                    builder.UseRenderingErrorHandler(new Dictionary<Type, RenderingErrorConfig>()
                    {
                        [typeof(NoChangesException)] = new(AffectedExitCodes.NothingChanged, new NoChangesView()),
                    });
                });
        }

So my commands can look like this:

        {
            private readonly ICommandExecutionContext _context;
            private readonly IConsole _console;

            public CommandHandler(ICommandExecutionContext context, IConsole console)
            {
                _context = context;
                _console = console;
            }

            public Task<int> InvokeAsync(InvocationContext ic)
            {
                if (!_context.ChangedProjects.Any())
                {
                    throw new NoChangesException();
                }

                var view = new NodesWithChangesView(_context.ChangedProjects);
                _console.Append(view);

                return Task.FromResult(0);
            }
        }

I think I don't need the UseCommandHandler thing If I just override the handlers with something like you have shown. i.e get my custom IServiceProvider and resolve the handler from there.

In order to make it work with the RootCommand, I had to hard-code the handler on the command's definition

I feel like this should be a common requirement. Ideally, and thinking out loud, I think it would be nice if the BindingContext would use a normal IServiceProvider that we can customize so that we can add and override services for the entire application.
Perhaps have a root service provider, and then create scopes per each command invocation.

[Gronex]

It seems like the UseCommandHandler code is only really able to handle leaf commands, as the parser has a check on Handler on the command it resolved to, followed by a check on weatcher the command is the last one.

Unfortunately for this senario the UseHost middleware is set after the parsing of the command so the handlers has not been set yet. If it is an error that it allows commands without assigned handlers during parsing if it is a leaf command i don't know.

To work around the limitation what @fredrikhr suggested should work.

[leonardochaia]

Hi @Gronex ! I understand. It's basically not gonna work for the RootCommand as it is right now.

Do you guys think we need to figure a way to improve how the lib deals with DI? I'm not really talking about generic host integration.

I think that adding/overriding services should be an easy task to do.

[fredrikhr]

Hmm, I think we should redesign UseCommandHandler and basically encapsulate it like my example did. That would require UseCommandHandler to extend the command line builder instead. So then it probably should be called sth. like UseHostedCommandHandler...

[Gronex]

Does it even have to be connected to the Hosted environment? The idea is to easily register the handler so a DI framework could inject the appropriate services into it, that does not strictly need to rely on the host.

If it does not it would definately feel better to streamline the DI so there is only one in play even when using the GenericHost

[leonardochaia]

@Gronex I definitively agree with everything you just said.

I believe we should focus on improving DI support internally, independently of the Hosting package. This should in turn simplify stuff for generic host integration. We should aim at using Microsoft.DependencyInjection implementations and provide a way for users to customize service registration, ideally including overriding internal stuff via replacing interface implementations.

Regarding the Hosting integration, TBH I would have expected to see the opposite: CommandLine integrating into the GenericHost, not the other way around.

What I'm saying is, I would have expected this:

HostBulder.Create()
.ConfigureAppConfiguration...
.ConfigureServices...
.ConfigureCommandLineDefaults // Instead of calling ConfigureWebHostDefaults
.Build()

I would have expected the CLI to be a feature of the Host, not the other way around.
We could perhaps run inside a IHostedService like ASP.NET Core does?

I would be happy to work on a proof of concept if you guys think this could be something we need to explore.

[fredrikhr]

@Gronex

Does it even have to be connected to the Hosted environment? The idea is to easily register the handler so a DI framework could inject the appropriate services into it, that does not strictly need to rely on the host.

I agree, however the problem is that we have two separate DI-systems here (the one from the InvocationContext and the one from Ms.Ext.DI when using the Generic Host. The these two DI-systems create instances is very different (Ms.Ext.DI does ctor injection and uses IServiceCollection to register services, InvocationContext uses ModelBinder and factory registration).

Therefore, the late-bound DI-from-Generic-host injected CommandHandler needs to be wrapped as I showed #1344 (comment)

@leonardochaia

Regarding the Hosting integration, TBH I would have expected to see the opposite: CommandLine integrating into the GenericHost, not the other way around. [...] I would have expected the CLI to be a feature of the Host, not the other way around.
We could perhaps run inside a IHostedService like ASP.NET Core does?

I do understand that sentiment. Personally, I prefer it the way it is now, but in my opinion we should have it both ways. I see this as two perpendicular lines that cross each other at an intersection. Currently we only have a small bypass ramp that allows going from one line to the other in one direction. Ideally we'd like a cloverleaf-intersection of these two features.

So when I originally designed the Sys.CommandLine.Hosting glue, I admit it was easier to make System.CommandLine wrap around the Generic Host than the other way round. However consider the following snippet from ASP.NET (Program.cs)

public static class Program
{
    public static void Main(string[] args) => CreateHostBuilder(args).Build().Run();

    public static IHostBuilder CreateHostBuilder(string[] args) =>
        Host.CreateDefaultBuilder(args)
        .ConfigureWebHostDefaults(webBuilder =>
        {
            webBuilder.ConfigureServices(services => { /* add services here */ });
            webBuilder.Configure((context, app) => { /* configure HTTP pipeline here */ });
        });
}

You see here that the code in Main is responsible of accepting and handling the args (here simply forwarding to the Generic Host, but there could be more code here).

Now using System.CommandLine.Hosting

public static class Program
{
    public static void Main(string[] args)
    {
        var rootCommand = new RootCommand { Handler = new MyRootCommandHandler() };
        var parser = new CommandLineBuilder(rootCommand)
            .UseDefaults()
            .UseHost(CreateHostBuilder)
            .Build();
        
        parser.Invoke(args);
    }

    public static IHostBuilder CreateHostBuilder(string[] args) =>
        Host.CreateDefaultBuilder(args)
        .ConfigureWebHostDefaults(webBuilder =>
        {
            webBuilder.ConfigureServices(services => { /* add services here */ });
            webBuilder.Configure((context, app) => { /* configure HTTP pipeline here */ });
        });
}

public class MyRootCommandHandler : ICommandHandler
{
    public Task<int> InvokeAsync(InvocationContext context)
    {
        var host = context.GetHost();
        /* Do something ... */
        return Task.FromResult(0);
    }
}

So generally these two code-snippets are really similar. Now your suggestion of having System.CommandLine be an IHostedService does has merits. However that would necessitate of somehow passing the command-line arguments from Main into the Generic Host and then injecting that into a Hosted service and then do the command-line argument parsing. There are some problems with that: Configuring the Host from command-line arguments would not be possible, e.g. myapp [config:ConfigKey=ConfigValue] uses the config directive and passes that to IConfiguration in the generic host, however that happens when the Host is being set-up. If parsing was done as a hosted service, the Host would already be fully materialized and running making it difficult to post-setup this.

[leonardochaia]

Hey! Appreciate you guys taking the time to analyze this!

The these two DI-systems create instances is very different (Ms.Ext.DI does ctor injection and uses IServiceCollection to register services, InvocationContext uses ModelBinder and factory registration).

So..Is there a way we can MSDI for everything?
I'd like to see people reuse their knowledge they have from ASP.NET when building a CLI application.
I mean, can't we instantiate handlers using MSDI and resolve everything they need at constructor time?

I know that we are now binding CommandLine Options to the handler's properties. Perhaps we can use inject IOptions for that?
If we could inject everything we need when creating a handler, there's no need for another way to resolve dependencies (like binding them to a property of the handler).

Does this make sense? I'm hoping the learning curve for building a CLI can be lowered if we integrate more with the .NET ecosystem. (MSDI, IOptions)

I think doing that will leave us closer for having a deeper integration with generic host.

I do understand that sentiment. Personally, I prefer it the way it is now, but in my opinion we should have it both ways.

Sure thing, both ways can't hurt. That being said, I think we should have a stronger integration with GenericHost. Being able to share services between the two by default seems like what most people would expect.
I think we can have that conversation once we agree what to do with the DI situation? I mean, if we can't have a single DI container the GenericHost integration is gonna be quite limited, or hacky (like registering everything on container A into container B).

That being said, I was thinking on something like this:

public static class Program
{
    public static void Main(string[] args) => CreateHostBuilder(args).Build().Run();

    public static IHostBuilder CreateHostBuilder(string[] args) =>
        Host.CreateDefaultBuilder(args)
        .ConfigureServices(...)
        .ConfigureAppConfiguration(...)
        .ConfigureCommandLineDefaults<MyRootCommand>(args)
        // or
        .ConfigureCommandLineDefaults<MyRootCommand>(args, builder=> ... )
}

However that would necessitate of somehow passing the command-line arguments from Main into the Generic Host and then injecting that into a Hosted service and then do the command-line argument parsing.

I think we can require them as an argument on the HostBuilder extension method and register a singleton or factory service which contains them.

public IHostBuilder ConfigureCommandLineDefaults<TRootCommand>(
   this IHostBuilder hostBuilder,
   args: string[], Action<CommandLineBuilder>? fn = null) {
   hostBuilder.ConfigureServices(services=> {
       services.AddSingleton<CommandLineArguments>(()=> new CommandLineArguments(args));
   });
   // configure the builder to use MSDI
   var builder = new CommandLineBuilder<TRootCommand>();
   fn?.Invoke(builder)
   return hostBuilder;
}

There are some problems with that: Configuring the Host from command-line arguments would not be possible, e.g. myapp [config:ConfigKey=ConfigValue] uses the config directive and passes that to IConfiguration in the generic host, however that happens when the Host is being set-up. If parsing was done as a hosted service, the Host would already be fully materialized and running making it difficult to post-setup this.

I didn't know about the [config] directive, that being said GenericHost already supports binding to IConfiguration values through arguments, it is not very pretty but it is supported already. So perhaps there is no need for the directive at all, unless I am missing something?

That being said,

If parsing was done as a hosted service, the Host would already be fully materialized and running making it difficult to post-setup this.

This is completely true. You can configure some of the Host stuff on the HostBuilder extension method, but once the host is running and you are in the context of a HostedService, yes you are done, no more changing the host.

I know this is not easy and getting closer to this would require huge changes on how instantiation is being done.
But I feel like we need to have this conversation.. I think that we should try to keep it similar to ASP.NET as much as we can.

Thank you all!
Have a great Sunday.

[fredrikhr]

I feel my two code snippets clearly show how similar the code is compared between ASP.NET and System.CommandLine. Could you elaborate on what you think is so dissimilar there that it confuses people?

I also see ModelBinding in commandline as completely different from ctor injection in Ms.Ext.DI. I personally prefer these to be separate.

And given the alomost identical code as shown in my sample, I feel Sys.CommandLine.Hosting does what you are asking for: A tight integration between CommandLine and Generic Host.

Yes, the Host.CreateDefaultBuilder does accept an array of strings (taken from the command-line arguments), but the only supported arguments are --config-key=value. If you ever supply sub-commands or option values it throws. You also only get the value as a string since it's put into IConfiguration which is purely string key-value-based.

That is also the reason why the extension methods to the HostBuilder are problematic: Host.CreateDefaultBuilder fully consumes the passed arguments, there is no way to somehow pass them along the way during the subsequent configure-extensions.

The config-directive is a feature implemented by Sys.CommandLine.Hosting.

[leonardochaia]

I feel my two code snippets clearly show how similar the code is compared between ASP.NET and System.CommandLine.

I agree with you. They are very similar. However, if CommandLine would follow the same approach as ASP.NET and other frameworks, which configure the HostBuilder, I think it would be more in line with what the rest of the ecosystem is doing. That's all I'm saying.

Could you elaborate on what you think is so dissimilar there that it confuses people?

I can only speak for my self, but personally, my confusion was not is in the HostBuilder configuration, I mean that's just reading the docs and configuring it properly. That being said, since I know the HostBuilder API, I was expecting to add the CommandLine to the HostBuilder, not the other way around. But reading the docs quickly clarifies this. It was just my first instinct, having built stuff that integrates with HostBuilder before.

Personally, it took me a while to figure out the DI situation, specially when working with the GenericHost.
All I wanted to do was register services and resolve them in the Handlers. (injecting interfaces and replacing implementations for testing purposes). I couldn't achieve what I wanted, so I ended up writing my own

So, as a conclusion for the DI perspective:

1. I think there should be an easier way to register services and inject those into a Handler.
2. IServiceCollection support would be ideal, so that we can use extension methods like AddTransient with or without factories.

I think that's useful for all CLIs, like the dotnet-affected tool I've written, where we wanna mock some of the stuff we inject in handlers.

Regarding GenericHost:

1. I would have expected to be able to inject services that I have registered in the GenericHost into my Handlers. Including DbContexts/Repositories. Perhaps when thinking about ASP.NET this does not make much sense. But if you have everything build up against the generic host, it would be super easier to add like an local admin CLI or migration tool for your app injecting the infra that you already have. In the enterprise applications I work on, we try to keep things tied to the GenericHost independently of ASP.NET whenever possible. For example, we do all our DI registration of Repositories and Domain layer implementations against the generic host (i.e HostBuilder.ConfigureServices)
2. People that has work on ASP.NET probably have heard about the IOptions pattern, which is the recommended way to inject IConfiguration values from MS (AFAIK). Hence, being able to declare and resolve CommandLine options as IOptions is something that, at least to me, would make sense.

I think this is more useful for enterprises which have built infrastructure on-top of GenericHost which is (partially) independent from ASP.NET

All this being said, I think that these would be some cool features!

1. Providing ways to full-featured DI registration without GenericHost
2. Resolve handlers using DI
3. When using GenericHost, be able to inject its services into Handlers.

Personally, I'm more interested in 1 and 2, cause I can probably write something that works (for my particular use case) to resolve 3, but I would need to have 1 and 2 supported by CommandLine if I'm even gonna think about it.

I just wanted to start the conversation.. I think you have very valid points, and I think I have expressed mines as clearly as possible, that being said, I'm happy to help and to provide more information about my use case if required.

[fredrikhr]

@leonardochaia Now I am really confused. You describe it as if the Host in ASP.NET and the Generic Host were two separate things, whereas they are (they used to be, but now they are 100% identical). ASP.NET simply registers some services (e.g. Kestrel) and that's it, there is nothing being done differently in ASP.NET compared to any other app using the Generic Host. Your statement about something built on Generic Host that is independant from ASP.NET does not make sense. There is no way to be dependant on ASP.NET itself, unless you mean implementing an MVC controller (or something similar that somehow uses HttpContext).

As I showed in my example snippets both the ASP.NET and System.CommandLine.Hosting code has a prelude in which you handle command-line arguments. In ASP.NET that's a one-liner in Main, in System.CommandLine that's building the parser and then invoking it. I still cannot see the dissimilarity here. Neither can I understand how your first instinct is that parsing command-line arguments is done anywhere else than in the prelude. (Prelude here means: The code that configures and builds the Host and then starts it.)

ASP.NET uses the IOptions to bind IConfiguration to concrete objects. Yes, we'd do exactly the same using System.CommandLine. The method that binds against an options instance from CommandLine arguments/options is called BindCommandLine (an extension method to the OptionsBuilder<T> implemented in System.CommandLine.Hosting).

Granted, your use case where you materialise the command-handler from DI is a scenario I never envisioned, but as I showed in my earlier comment that is easily doable without breaking things. But I am working on an extension method that allows to do that as a one-liner, I'll tag you once I make the PR. (edit: Opened PR #1356)