Easier addition of injection friendly handler

[Gronex]

This pull request adds an easier way to make use of the full Generic Host dependency setup instead of manually having to call the service provider for a service where the bound parameters has to be parsed down after wards.

[dnfclas]

All CLA requirements met.

[jonsequitur]

Can you expand a bit on the use case? Is there an approach where we can make this composable, or just the default binding behavior for all command handlers when using a host, rather than adding an either/or choice for how to create and assign a command handler?

[Gronex]

The goal is really to not have to write the wrapping that instantiates the full DI setup for every command.
However in some scenarios the use case is probably simple enough that creating a class is not worth the hassle, but maybe you would not use the generic host setup then?

If it was to be the default for host based handlers, you would have to somehow still make a connection between the class to use as a "Controller" and the command to do the execution.
I suppose you could just provide a single class that will then be automatically registered in the service collection, and use that one, but then you risk having some incredibly big main entry point, in case of a lot of commands.

Maybe it could be done with an attribute, to signal that a class functions as a command handler, for a set of command names? That approach would cost a bit of performance though in the form of a typescan.

[pjmagee]

@Gronex I feel like I had a little trouble wiring everything up due to the Handler. What do you think about my approach here? Thoughts? Observations?

One observation I have is that I had to change how services were accessing IConfiguration values from the constructor, to get properties which were then accessed during execution. I dont know if this is good or bad really. I had originally used Microsoft.Extensions.Configuration.CommandLine which set IConfiguration keys to the values passed in the string[] args. Since swapping from that to System.CommandLine i found it a little difficult to grasp how to configure and compose everything with ServiceCollection DI framework.

https://gist.github.com/pjmagee/994833753deba47c3b95c073b8377232

[jonsequitur]

Another approach that might be worth exploring is to take @pjmagee's inheritance-based model further and allow the custom command classes to implement ICommandHandler. The model binder should have no problem instantiating those command objects directly.

[Gronex]

I do like the inheritance approach, and making use of the ICommandHandler seems like it would make the handler a bit more closely connected with the arguments that are passed to it, which seems to make sense. And you could avoid having to tell the framework about the handler as it would be the command itself.

@pjmagee I do see your point about not recieving the IConfiguration along with the args any more, but i kind of like that, as it allows you to separate the origin of the class, from normal injection and user provided arguments.

[jonsequitur]

I just put together this example of one way that a handler can be added by convention.

    public class HandlerByConventionExample
    {
        [Fact]
        public async Task It_finds_handler_by_convention_on_derived_command_types()
        {
            var root = new RootCommand
            {
                new One(),
                new Command("two")
            };

            var parser = new CommandLineBuilder(root)
                         .UseDefaults()
                         .UseMethodAsCommandHandler(nameof(One.InvokeAsync))
                         .Build();

            var console = new TestConsole();

            await parser.InvokeAsync("one --int-option 123 --string-option hello!", console);

            console.Out.ToString()
                   .Should()
                   .Contain("--int-option=123")
                   .And
                   .Contain("--string-option=hello!");
        }

        public class One : Command
        {
            public One() : base("one")
            {
                AddOption(new Option<int>("--int-option"));
                AddOption(new Option<string>("--string-option"));
            }

            public async Task InvokeAsync(int intOption, string stringOption, IConsole console)
            {
                console.Out.WriteLine($"--int-option={intOption}");
                console.Out.WriteLine($"--string-option={stringOption}");
            }
        }
    }

    public static class InvocationExtensions
    {
        public static CommandLineBuilder UseMethodAsCommandHandler(this CommandLineBuilder builder, string methodName)
        {
            builder.UseMiddleware(context =>
            {
                if (context.ParseResult.CommandResult.Command is Command command &&
                    command.Handler == null)
                {
                    var type = command.GetType();

                    var invokeMethod = type.GetMethod(methodName, BindingFlags.Instance | BindingFlags.Public);

                    if (invokeMethod != null)
                    {
                        command.Handler = CommandHandler.Create(invokeMethod, command);
                    }
                }
            }, MiddlewareOrder.Configuration);

            return builder;
        }
    }

If you wanted to mix DI into this, one approach that can work is inside the middleware, call context.BindingContext.AddService. That interface could use a little refinement.

One thing that I like about this approach is that both the DI setup and the reflection are lazy, meaning you never pay a performance cost for it app-wide, but only for the command that was actually invoked.

Thoughts?

[KathleenDollard]

I like the idea of what @jonsequitur proposed. Some possible tweaks:

I think the name should be optional, although I'm a bit unclear how we know whether the handler is async:

.UseMethodAsCommandHandler()

That may not be the right name, and I think an optional argument to state the arg name if you want something other than Invoke or InvokeAsync is reasonable. But I'd like to see a stronger convention here.

Which then leads to the question of whether this is the right gesture to set up the handler. We could:

• Do what's proposed
• Have different method name like .UseDeclaredHandler()
• Have an interface with Invoke and another with InvokeAsync, and if the command class also implements the interface, assume you want the wire up and have an opt out like .NoDefaultCommandHandlers()

[jonsequitur]

I didn't think much about the name, just wanted to demonstrate the approach.

The reason an interface doesn't work is that the parameters are user-defined, much like an ASP.NET controller. But I do think this general approach could reduce the ceremony of having to invoke CommandHandler.Create directly. Although opt-out or opt-in to a convention like this could certainly be signaled by a marker interface or even just the the fact that you have a class derived from Command.

[KathleenDollard]

I missed that there were parameters. Can we make the common case simpler? This would allow an interface (with a more involved parameter list). Do we support this? It would require reflection in the constructor of the base class, so I added an intermediate class as that would be undesirable for Command.

        public class One : InferredCommand
        {
            int int_option;
            string string_option;

            public async Task InvokeAsync(IConsole console, <other stuff>)
            {
                console.Out.WriteLine($"--int-option={int_Option}");
                console.Out.WriteLine($"--string-option={string_Option}");
            }
        }

[jonsequitur]

I merged an improved approach to custom model binding that should make this a little easier: #848.

@KathleenDollard, I think we almost have the interface you're speaking of, which is ICommandHandler:

public interface ICommandHandler
{
    Task<int> InvokeAsync(InvocationContext context);
}

An implementation of it that carried state, e.g. the binding targets for the arguments, could look something like this:

public class Handler : ICommandHandler
{
    // ... properties
    Task<int> InvokeAsync(InvocationContext context);
}

This scales pretty well if people are in the habit of binding complex objects rather than a bunch of primitive parameters.

There would need to be a way to point to the handler from the command, presumably, maybe something like a Command.HandlerType property?

So putting that all together, an inherited Command could look like this:

public class MyCommand : Command
{
    public MyCommand() : base(name: "mycommand")
    {
        AddOption(new Option<int>("--int-option")); // or nameof(Handler.IntOption).ToKebabCase() if you don't like the string literal
        AddOption(new Option<string>("--string-option"));
    }

    public override Type HandlerType => typeof(Handler);

    public class Handler : ICommandHandler
    {
        public int IntOption { get; set; } // bound from option
        public string StringOption { get; set; } // bound from option
        public IConsole Console { get; set; } // bound from DI

        public async Task<int> InvokeAsync(InvocationContext context) 
        {
            this.Console.Out.WriteLine(IntOption);
            this.Console.Out.WriteLine(StringOption);
        }
    }

}

Thoughts?

[Gronex]

I added the approach of using inheritance, and allowing the command to indicate what the handler type should be.

In this implementation things injected through the host is only able to resolve using constructor injection, and the commandLineApi resolution happens on the propperties.

I am not quite sure if that is a good approach though as it could be confusing why things like IConsole will resolve as a property, while MyService will not.

Any thoughts on that?

I was also wondering if there is some way to enforce that Command.HandlerType is in fact implementing ICommandHandler

[jonsequitur]

Thanks. I'll take a look with your questions in mind.

[jonsequitur]

@Gronex Would you mind doing a rebase against master and a force push so the diff is clearer?

[Gronex]

Sorry about that. Should be better now.

[zpittmansf]

Looking forward to this change. Will this get merged soon or is there some other way to gain similar functionality?

[roshangautam]

Not Sure if this helps but I am tackling DI in my command classes as follows. But It would be really helpful if constructor injection works for command classes to avoid manually pulling dependencies from container

    public class ExportCommand: Command
    {
        private const string CommandName = "export";
        private const string CommandDescription = "Export solution from a power platform environment";
        private ILogger<ExportCommand> _logger;
        private Settings _cdsSolutionSettings;
        private CdsServiceClient _cdsServiceClient;

        public ExportCommand() : base(CommandName, CommandDescription)
        {
            Handler = CommandHandler.Create<IHost>(Execute);
        }

        private void Execute(IHost host)
        {
            _logger = host.Services.Get<ILogger<ExportCommand>>();
            _cdsSolutionSettings = host.Services.Get<IOptions<Settings>>().Value;
            _cdsServiceClient = host.Services.Get<ICrmClientFactory>().Manufacture();
             // DO YOUR THING
        }
    }

[zpittmansf]

Not Sure if this helps but I am tackling DI in my command classes as follows. But It would be really helpful if constructor injection works for command classes to avoid manually pulling dependencies from container

    public class ExportCommand: Command
    {
        private const string CommandName = "export";
        private const string CommandDescription = "Export solution from a power platform environment";
        private ILogger<ExportCommand> _logger;
        private Settings _cdsSolutionSettings;
        private CdsServiceClient _cdsServiceClient;

        public ExportCommand() : base(CommandName, CommandDescription)
        {
            Handler = CommandHandler.Create<IHost>(Execute);
        }

        private void Execute(IHost host)
        {
            _logger = host.Services.Get<ILogger<ExportCommand>>();
            _cdsSolutionSettings = host.Services.Get<IOptions<Settings>>().Value;
            _cdsServiceClient = host.Services.Get<ICrmClientFactory>().Manufacture();
             // DO YOUR THING
        }
    }

Thanks @roshangautam. I think that is a good step in preparation of the handler being constructed and injected separately from the command class!

[zpittmansf]

Hi @jonsequitur ! Is this PR abandoned for some other effort at this point?

[zpittmansf]

Could this HandlerType be removed with usage like ICommandHandler::Handler.GetType() instead?

[jonsequitur]

Not if Handler is null.

[zpittmansf]

Right, so on line 95 just do a handler is null check and call gettype when handler is not null. I don't see any other usage of handler type outside the context of this scope:

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

Line 95 in 631b88e

if (command.Handler == null && command.HandlerType != null)

[jonsequitur]

I paused on this for a while so we could make improvements to the model binding code (e.g. #1018) related to app model work.

Reviewing it again, I don't think the Command.HandlerType property is ideal. It's only there for the purpose of an external component (System.CommandLine.Hosting) to choose a handler, something that can be done by other means, and it's internally inconsistent, e.g. I can do this:

public class MyCommand : Command
{
  public override Type HandlerType => typeof(MyHandlerType);
}

// ...

var command = new MyCommand
{
  Handler = new NotMyHandlerType()
};

I suggested a couple of ideas above for wiring up handlers by convention during invocation time.

An important difference between that approach and the one enabled here is whether DI happens when the parser is created versus during command invocation. Using a DI container during parser creation means I'm building the handler object graphs eagerly for every command, which is less performant than doing it lazily on invocation for a single command, or no command in the case of completions.

[zpittmansf]

I paused on this for a while so we could make improvements to the model binding code (e.g. #1018) related to app model work.

Reviewing it again, I don't think the Command.HandlerType property is ideal. It's only there for the purpose of an external component (System.CommandLine.Hosting) to choose a handler, something that can be done by other means, and it's internally inconsistent, e.g. I can do this:

public class MyCommand : Command
{
  public override Type HandlerType => typeof(MyHandlerType);
}

// ...

var command = new MyCommand
{
  Handler = new NotMyHandlerType()
};

I suggested a couple of ideas above for wiring up handlers by convention during invocation time.

An important difference between that approach and the one enabled here is whether DI happens when the parser is created versus during command invocation. Using a DI container during parser creation means I'm building the handler object graphs eagerly for every command, which is less performant than doing it lazily on invocation for a single command, or no command in the case of completions.

I was wondering if performance was a concern there. That makes sense. I think I better understand why allowing for a HandlerType specification too, I misread the code. I agree overall though that as a framework the HandlerType property allows you to do things that would be inconsistent, from a DI perspective (only use case as well) it doesn't feel natural.

That said, I think what is current is a very workable solution. Is there something else that is needed at this point to complete the PR?

[Gronex]

How would you feel about an interface that looked something like:

var parser = new CommandLineBuilder(new MyCommand())
  .UseHost(host =>
  {
      host.ConfigureServices(services =>
      {
          services.AddTransient<MyService>();
      })
      .UseCommandHandler<MyCommand, MyCommand.MyHandler>();
  })
  .Build();

So instead of the command telling who can handle it, it is configured when you set up the host.
This way we can get rid of the HandlerType which i don't think anyone really likes.

[Gronex]

@zpittmansf I can't really replicate your issue.. I attempted copying all your code into the main project of HostingPlayground after unsuccessfully making a test that would give the same result, but still with no luck.

What is the output you are getting from the WriteLine call?

[zpittmansf]

@zpittmansf I can't really replicate your issue.. I attempted copying all your code into the main project of HostingPlayground after unsuccessfully making a test that would give the same result, but still with no luck.

What is the output you are getting from the WriteLine call?

Both the properties One and Two are null so you get " & ".

I think I see the problem... There isn't a way to just extend the modelbinder behavior to call UpdateInstance(). I had only copied over the extensions and didn't have the updated modelbinder.cs. Apologies.

[gokussx4]

@zpittmansf I can't really replicate your issue.. I attempted copying all your code into the main project of HostingPlayground after unsuccessfully making a test that would give the same result, but still with no luck.

What is the output you are getting from the WriteLine call?

Just a follow up. I was attempting other methods of extending the current command line api without having to change the source but I couldn't see a way to do that without calling UpdateInstance as you did in your fork. Tested the fork and it works great for me locally.

[gokussx4]

@Gronex @jonsequitur thanks so much for working on this feature, can't wait for it to be available!

[Gronex]

@jonsequitur I merged the lastest main branch, and given some changes in the way the dependencies are resolved changed the way the handler is resolved.
I am not sure if there is anything else holding this pull request back.

[jonsequitur]

I wonder if the fact that this is an exact type match, as opposed to allowing for matching on subtypes, will be less intuitive.

[Gronex]

I am unsure if it risks causing more confusion if a command and a more specific command is registered, then the handler for the command might end up matching with the more specific command if i am understanding the scenario correctly.

[jonsequitur]

It looks like it might also be redundant with the following line.

Is there a way to ask the container if it can create an instance of handlerType, so we don't implement logic that's inconsistent with the container configuration?

[Gronex]

Are you thinking testing if the container already has the handler type registered? Or verifying we are not parsed something the container cannot possibly new up?

[jonsequitur]

For example, the container might be configured to map ISomeCommand to MyCommand. The current code would block that from working because of the command.GetType() == commandType check. So in effect we're reimplementing a type matching check event though the resolved handler might work just fine.

[Gronex]

The problem is that the check is there to ensure that the command is the one that the caller configured to be handeled by that commandHandler.

Is it even possible to have the command as something not inheriting from Command like an interface?

[zpittmansf]

@jonsequitur I tried using the daily builds nuget feed but the latest beta of system.commandline.hosting (2.0.0-beta1.20074.1) does not include these changes it seems. With this PR being merged into main how does one get a nuget build of main latest?

[zpittmansf]

@jonsequitur I tried using the daily builds nuget feed but the latest beta of system.commandline.hosting (2.0.0-beta1.20074.1) does not include these changes it seems. With this PR being merged into main how does one get a nuget build of main latest?

I was able to find it. Although 2.0.0-beta * is ok for a version of system.commandline, in order to see the latest for system.commandline.hosting you need to target 0.3.0-alpha. @jonsequitur I don't know if that was intended or not. It seems odd that the dev next is in a previous unreleased alpha version when system.commandline.hosting is already 2 major versions released. I could just be misunderstanding the strategy here though.

Thanks for getting this change in place!

[jonsequitur]

This is deliberate if a little confusing. There was an older 1.* released version of System.CommandLine, so we needed to choose a higher number for that. We didn't move all of the other libraries up because they're less mature.