InvocationContext

[tmds]

With the current API, the command handlers are passed an instance of ParseResult.

https://github.com/dotnet/command-line-api/blob/d18f598068fad37791e9f401a05d5f6a0591fa0e/src/System.CommandLine/CliAction.cs#L19

This doesn't allow injecting services that a command handler might need. It also doesn't enable splitting up a command handler in different steps, which store their intermediate results on the context that get passed between steps.

These are known patterns from ASP.NET, where an HttpContext is passed between middleware.

System.CommandLine can support this pattern also by (re-)introducing the InvocationContext as the class that gets passed to the command handlers, and which can be derived by the user.

For example:

var command = new RootCommand();
command.SetAction(async (context, ct) => 
{
    var appContext = (AppInvocationContext)context;
    await appContext.AppService.UseServiceAsync(ct);
    return 0;
});

var config = new CommandLineConfiguration(command);
config.ContextFactory = parseResult => new AppInvocationContext(parseResult);

class AppInvocationContext : InvocationContext
{
   public MyInvocationContext(ParseResult parseResult) : base(parseResult) { }
   public IAppService AppService { get; } = AppService.DefaultInstance;
}

[adamsitnik]

It seems that InvocationContext is on its way out.

It is, for the reasons described here: https://github.com/dotnet/command-line-api/issues/2046#issuecomment-1432009844

which enables extending what gets passed on the invocation

Could you please elaborate more on that? Why do you need it, what kind of problem are you trying to solve? Do you have any links to OSS repos that would benefit from it?

[tmds]

It is, for the reasons described here: https://github.com/dotnet/command-line-api/issues/2046#issuecomment-1432009844

Yes, as a sealed class it no longer has any purpose.

As a non-sealed class, it allows a user to extend it in any way needed.

Could you please elaborate more on that? Why do you need it, what kind of problem are you trying to solve?

I'm trying to add DI.

And I'm trying to split up handlers into steps so I can re-use some steps across several handlers. These steps need some way of storing their information.

If I can make S.CL create an instance of the derived class I have (which is my request in https://github.com/dotnet/command-line-api/issues/2087), I choose how I implement DI, and how I track this data between steps.

When I created that issue, I hadn't noticed yet you intend to remove the class.

Is it clear what I'm trying to do? I don't have any code I can share at the moment. If my use-case is not clear, imagine how you would build a handler with an injectable IAppService.

with this, you can do:

class AppInvocationContext : InvocationContext
{
    public IAppService AppService { get; }
}

and in the handler:

IAppService appService = (context as AppInvocationContext).AppService;

[adamsitnik]

We are currently working on making System.CommandLine part of BCL. We want to make the APIs super simple, intuitive to use, performant and AOT friendly.

But we also want to make sure that the APIs don’t encourage .NET developers to fall into any traps. I strongly believe that configuration of DI before knowing the parse result is an anti-pattern from performance perspective and it needs to be avoided, or at least not recommended/encouraged.

Example: the application builds DI container, defines a command with a required Option and then performs the parsing. It turns out that the required option is missing, it prints an error and quits. Why would it need to build DI container up front? The same goes for requesting help: we just want to print and exit.

Another performance trap is building a very huge DI container up front, for all the commands, while each of them requires only a subset of registered types/instances.

So far System.CommandLine was encouraging such bad practices, if you take a look at our samples you can find HostingPlayground.

What it does?

It uses DI to register a single type that implements IGreeter interface:

https://github.com/dotnet/command-line-api/blob/15a45d30410aafed67f231421733143e8f9168ab/samples/HostingPlayground/Program.cs#L19-L22

the interface comes with a default implementation:

https://github.com/dotnet/command-line-api/blob/15a45d30410aafed67f231421733143e8f9168ab/samples/HostingPlayground/IGreeter.cs#L5-L7

and the type is empty:

https://github.com/dotnet/command-line-api/blob/15a45d30410aafed67f231421733143e8f9168ab/samples/HostingPlayground/Greeter.cs#L3-L6

it creates a command with required Option (so nothing is executed when it's not provided):

https://github.com/dotnet/command-line-api/blob/15a45d30410aafed67f231421733143e8f9168ab/samples/HostingPlayground/Program.cs#L30-L34

then it uses reflection-heavy optional package for creating a handler:

https://github.com/dotnet/command-line-api/blob/15a45d30410aafed67f231421733143e8f9168ab/samples/HostingPlayground/Program.cs#L35

just to log something to the output:

https://github.com/dotnet/command-line-api/blob/15a45d30410aafed67f231421733143e8f9168ab/samples/HostingPlayground/Program.cs#L46-L48

One could argue that it's an artificial example and nobody writes real software like this.

Yesterday I was wrapping my head around System.CommandLine.Hosting existence and I googled for "System.CommandLine.Hosting samples"

The first link led me to this blog post

which falls into all traps from our sample:

• using hosting for an app that is supposed to perform a single task and just quit
• using DI to register only two things
• creating an interface with a single implementation and no tests
• using reflection-based handler registration

We really need to stop promoting such practices. That is why I strongly believe that the input for handler/action should be only the ParseResult.

And if somebody is still willing to pass DI to their handler, they can very easily do that with lambdas by capturing an implicit closure:

```cs static int Main(string[] args) { using MyDependencyContainer ioc = new(); Option boolOption = new("--bool"); Option stringOption = new("--string"); RootCommand command = new() { boolOption, stringOption }; command.SetHandler(parseResult => { bool boolean = parseResult.GetValue(boolOption); string text = parseResult.GetValue(stringOption); // ioc is not an argument of the handler, but it still can be used here ioc.Resolve(); return 0; }); return command.Parse(args).Invoke(); } ```

[adamsitnik]

BTW #2071 does not mention that, but we are also going to provide a possibility to specify a custom behavior without using lambdas.

```cs class CommandWithDC : Command { MyDependencyContainer DC { get; } public CommandWithDC(string name, MyDependencyContainer dc) : base(name) => DC = dc; public override int Execute(ParseResult parseResult) { // custom handler without lambdas } } ```

[tmds]

I strongly believe that configuration of DI before knowing the parse result is an anti-pattern from performance perspective and it needs to be avoided, or at least not recommended/encouraged.

I want to use DI to make my app testable.

Is that an anti-pattern that should be discouraged?

[tmds]

I don't see how you can make a command line run against some mock object without DI.

Also: DI doesn't imply bad performance.

[jonsequitur]

@tmds, do you think that the CLI Actions proposal (#2071) provides a usable hook for DI via custom CliAction-derived types?

Also: DI doesn't imply bad performance.

I agree with this. The nuance that we're focused on here is that DI (with most current container libraries) usually implies a bit of startup cost in terms of allocations before any object resolutions are performed. This startup cost is less important in longer-running apps than in shorter-running command line apps.

[tmds]

@tmds, do you think that the CLI Actions proposal (https://github.com/dotnet/command-line-api/issues/2071) provides a usable hook for DI via custom CliAction-derived types?

I'd like to include in tests what happens before ParseResult and mock the services the handlers use.

The design doesn't permit it: I can do what I want after I have the ParseResult.

Let me compare my request for InvocationContext with ASP.NET Core: ParseResult is to InvocationContext what HttpRequest is to HttpContext.

HttpContext can be subclassed, and its properties like Items, Features, and RequestServices allow separate parts of the app to provide and consume things.

Having an InvocationContext that can be subclassed, enables the same pattern for S.CL.

[tmds]

I've just put the sources of my app on github.

Anything in https://github.com/tmds/dotnet-shift/tree/main/src/dotnet-shift/Cli is System.CommandLine related.

AppContext is what would be the derived InvocationContext.

This is built against the latest ci packages of System.CommandLine.

Feel free to take a look. I hope you don't spot too many anti-patterns. :smile:

You can close this issue when it is no longer useful.

[adamsitnik]

I don't see how you can make a command line run against some mock object without DI.

@tmds First of all, apologies for delays in responding. I am preparing for my first parental leave (the due date is 7th of April) and I am currently trying to make as much progress as I can with API feedback (#1891) that we have received from @bartonjs, our expert in API design.

My plan is to merge all big changes until the end of this week and next week write a lot of samples for things like how to test commands, how and when to configure CI. And create a blog post with a list of breaking changes and the reasoning behind them. We aim for releasing a preview package in early April, get feedback and finally release a non-preview package with .NET 8.

If there is any scenario that is important and the new API makes it impossible/too hard to implement, we are going to adjust the API in May-June.

[tmds]

I am preparing for my first parental leave (the due date is 7th of April)

Congrats! Enjoy this special time!

If there is any scenario that is important and the new API makes it impossible/too hard to implement

The current design doesn't allow to add anything to be passed to the invocation handlers.

For example, in my project I'm using Spectre.Console for its nice prompting and rendering capabilities. I want the command handlers to have access to an IAnsiConsole, and same for the ExceptionHandler.

Being able to derive the InvocationContext and add a IAnsiConsole would be a natural extension point imo.

Now, I'm just hacking my way around this limitation.

[tmds]

I've updated the top-level comment based on the current API of S.CL.

[jonsequitur]

One approach that can work that looks pretty similar to what you're looking for might be to derive from CommandLineConfiguration:

var command = new RootCommand();

command.SetAction(async (parseResult, ct) =>
{
    var appConfig = (AppConfiguration)parseResult.Configuration;

    // access custom config properties

    return 0;
});

var config = new AppConfiguration(command);

config.Invoke("");

class AppConfiguration : CommandLineConfiguration
{
    public AppConfiguration(Command rootCommand) : base(rootCommand)
    {
    }

    // put what you need here
}

We're still contemplating some changes to CommandLineConfiguration and RootCommand but this might be a reasonable injection point for certain cases.

Custom CliAction types might be another, which I'm currently experimenting with.

[tmds]

One approach that can work that looks pretty similar to what you're looking for might be to derive from CommandLineConfiguration:

Yes, I can use that. As a bonus, it goes everywhere the ParseResult goes, so it's also usable from a CompletionContext for example.

[jonsequitur]

I'll be adding a test to help mark this as an intentional scenario and prevent CliConfiguration from becoming sealed later.

[jonsequitur]

@tmds, do you feel like this can be closed out or is there more here you'd like to capture?

[maxandriani]

First of all thankyou to this amazing lib. If I may, I'ld like to agree with @tmds on the Service Injection situation. I undertand that it may cause bad design practices by user side, but you folks can't just ignore the fact that users are asking for that and they (me included) will try to use an IOC container with this lib.

I saw that @jonsequitur commented someware that users can create an IOC container outside the lib and with some few global/static variables they can use it on the SetHandler lambdas. On top of that solution and with a few extension methods I wrote something like that.

The point is to just configure the ioc container in the same api-style as S.CL. But actually create the container only in the handler lambda. I know the HttpClients are configured in all handlers and not all handlers will user that. But 98% of my app will use the custom HttpClients so I feel fine to pay the bill.

@jonsequitur I think if you guys just provide a few exemples that what is the "best practices" way to use IOC people (including me) will stop fighting with/ the S.CL api.

Last but not least, rather than using complex Binding objects to map parameters and options, what do you think about just allow a configuration lambda and let the user resolve the bindings?

// Program.cs

var configGlobalOption = new Option<FileInfo>(new string[] { "--config", "-c" });
var verbosityGlobalOptions = new Option<Verbosity>(new string[] { "--verbosity", "-v" };

var cmdA = new Command(...);
cmdA.SetHandler<CmdAHandler>()
     .ConfigureServices((services, config, parseResult) => // IServiceCollection, IConfiguration, ParseResult
    {
        services.AddSingleton(...);
    });

var cmdWithArgs = new Command(...);
cmdWithArgs.SetHandler<CmdWithArgsHandler>(parseResult => new CmdBArgs(parseResult.GetValueForOption(configGlobalOption))); // user binding resolution...

var root = new RootCommand("some api cli");
root.AddGlobalOption(configGlobalOption.AddCompletions());
root.AddGlobalOption(configGlobalOption.AddCompletions());
root.Add(cmdA);
root.Add(cmdB);

var parser = new CommandLineBuilder(root)
    .UseAppContext()
    .UseConfiguration(configGlobalOption)
    .UseServices((services, config, parseResult) => // IServiceCollection, IConfiguration, ParseResult
    {
        services.AddHttpClient<ITokenService>();
        services.AddHttpClient("keycloak");
    })
    .Build();

await parser.InvokeAsync(args);

// somewhereelse.cs
public interface ICommandHandlerAsync<TArgs>
{
    Task<int> InvokeAsync(TArgs args, CancellationToken cancellationToken);
}

public interface ICommandHandlerAsync
{
    Task<int> InvokeAsync(CancellationToken cancellationToken);
}

public static class CommandExtensions
{
    public static ISetHandlerConfigurationBuilder SetHandler<THandler>(this Command command)
        where THandler : class, IAppCommandHandler
    {
        var handlerConfigBuilder = new SetHandlerConfigurationBuilder();
        command.SetHandler(async (context) =>
        {
            var appBuilder = context.BindingContext.GetRequiredService<IAppHostBuilder>();
            handlerConfigBuilder.ApplyServiceConfiguration(appBuilder);
            var app = appBuilder.Build(context.ParseResult);
            var handler = ActivatorUtilities.CreateInstance<THandler>(app.Services);
            var result = await handler.ExecuteAsync(context.GetCancellationToken());
            context.ExitCode = result;
        });
        return handlerConfigBuilder;
    }

    public static ISetHandlerConfigurationBuilder SetHandler<THandler, TArgs>(this Command command, Func<ParseResult, TArgs> parser)
        where THandler : IAppCommandHandler<TArgs>
    {
        var handlerConfigBuilder = new SetHandlerConfigurationBuilder();
        command.SetHandler(async (context) =>
        {
            var appBuilder = context.BindingContext.GetRequiredService<IAppHostBuilder>();
            handlerConfigBuilder.ApplyServiceConfiguration(appBuilder);
            var app = appBuilder.Build(context.ParseResult);
            var handler = ActivatorUtilities.CreateInstance<THandler>(app.Services);
            var result = await handler.ExecuteAsync(parser(context.ParseResult), context.GetCancellationToken());
            context.ExitCode = result;
        });
        return handlerConfigBuilder;
    }
}