Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Simplified usage of BindingContext resolved command handlers #1358

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Simplified usage of BindingContext resolved command handlers #1358

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d556325
Simplified usage of BindingContext resolved command handlers
fredrikhr Jul 20, 2021
2f2796b
Add AddService<TSvc, TImpl>() to BindingContext
fredrikhr Jul 21, 2021
b811921
Argument validation for AddService methods
fredrikhr Jul 21, 2021
b050e7a
Add additional test case for ConfigureBindingContext
fredrikhr Jul 22, 2021
5db4df8
Create own test for returned wrapper instance when calling CommandHan…
fredrikhr Aug 8, 2021
765d3a5
resolve https://github.com/dotnet/command-line-api/pull/1358/files/b0…
fredrikhr Aug 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 97 additions & 1 deletion src/System.CommandLine.Tests/Invocation/CommandHandlerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.CommandLine.Binding;
using System.CommandLine.Builder;
using System.CommandLine.Invocation;
using System.CommandLine.IO;
using System.CommandLine.Parsing;
Expand Down Expand Up @@ -322,7 +323,6 @@ public async Task Method_parameters_of_type_InvocationContext_receive_the_curren
boundContext.ParseResult.ValueForOption(option).Should().Be(123);
}


private class ExecuteTestClass
{
public string boundName = default;
Expand Down Expand Up @@ -507,5 +507,101 @@ public class OverridenVirtualTestCommandHandler : VirtualTestCommandHandler
public override Task<int> InvokeAsync(InvocationContext context)
=> Task.FromResult(41);
}

[Fact]
public static void FromBindingContext_forwards_invocation_to_bound_handler_type()
{
var command = new RootCommand
{
Handler = CommandHandler.FromBindingContext<BindingContextResolvedCommandHandler>()
};
command.Handler.Should().NotBeOfType<BindingContextResolvedCommandHandler>();
fredrikhr marked this conversation as resolved.
Show resolved Hide resolved
var parser = new CommandLineBuilder(command)
.ConfigureBindingContext(context => context.AddService<BindingContextResolvedCommandHandler>())
.Build();

var console = new TestConsole();
parser.Invoke(Array.Empty<string>(), console);
console.Out.ToString().Should().Be(typeof(BindingContextResolvedCommandHandler).FullName);
}

[Fact]
public static void Subsequent_call_to_configure_overrides_service_registration()
{
BindingContextCommandHandlerAction action = (handler, Console) =>
{
handler.Should().BeOfType<BindingContextCommandHandler2>();
fredrikhr marked this conversation as resolved.
Show resolved Hide resolved
};
var parser = new CommandLineBuilder(new RootCommand
{
Handler = CommandHandler.FromBindingContext<IBindingContextCommandHandlerInterface>()
})
.ConfigureBindingContext(context => context.AddService(_ => action))
.ConfigureBindingContext(context => context.AddService<IBindingContextCommandHandlerInterface, BindingContextCommandHandler1>())
.ConfigureBindingContext(context => context.AddService<IBindingContextCommandHandlerInterface, BindingContextCommandHandler2>())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One concern I have with this approach is that all handler registrations for all subcommands will happen regardless of which subcommand was used, when only one subcommand is ever used. This can be a bit wasteful and is part of why we've generally steered away from the common pattern for longer-lived apps where DI is configured up front but the cost is amortized over a longer lifetime. This is very often not the case for command-line apps.

Copy link
Contributor Author

@fredrikhr fredrikhr Aug 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonsequitur In this case (using the lazily bound registration) only the registration is added, no instance of BindingContextCommandHandler1 nor BindingContextCommandHandler2 is actually created at this point. And neither does the call to CommandHandler.FromBindingContext. The actual instance is first created when the command handler for the choses command is invoked.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. At invocation time though, all of these registrations happen on the BindingContext, and coupled with the addition of non-lazy AddService calls, this encourages the pattern I mentioned above.

.Build();
parser.Invoke(Array.Empty<string>(), new TestConsole());
}

public class BindingContextResolvedCommandHandler : ICommandHandler
{
public BindingContextResolvedCommandHandler(IConsole console)
{
Console = console;
}

public IConsole Console { get; }

public Task<int> InvokeAsync(InvocationContext context)
{
Console.Out.Write(GetType().FullName);
return Task.FromResult(0);
}
}

public interface IBindingContextCommandHandlerInterface : ICommandHandler
{
}

public class BindingContextCommandHandler1 : IBindingContextCommandHandlerInterface
{
private readonly BindingContextCommandHandlerAction invokeAction;

public BindingContextCommandHandler1(IConsole console,
BindingContextCommandHandlerAction invokeAction)
{
Console = console;
this.invokeAction = invokeAction;
}

public IConsole Console { get; }
public Task<int> InvokeAsync(InvocationContext context)
{
invokeAction(this, Console);
return Task.FromResult(0);
}
}

public class BindingContextCommandHandler2 : IBindingContextCommandHandlerInterface
{
private readonly BindingContextCommandHandlerAction invokeAction;

public BindingContextCommandHandler2(IConsole console,
BindingContextCommandHandlerAction invokeAction)
{
Console = console;
this.invokeAction = invokeAction;
}

public IConsole Console { get; }

public Task<int> InvokeAsync(InvocationContext context)
{
invokeAction(this, Console);
return Task.FromResult(0);
}
}

public delegate void BindingContextCommandHandlerAction(ICommandHandler handler, IConsole console);
}
}
35 changes: 27 additions & 8 deletions src/System.CommandLine/Binding/BindingContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public IConsole Console

internal ServiceProvider ServiceProvider { get; }

public void AddModelBinder(ModelBinder binder) =>
public void AddModelBinder(ModelBinder binder) =>
_modelBindersByValueDescriptor.Add(binder.ValueDescriptor.ValueType, binder);

public ModelBinder GetModelBinder(IValueDescriptor valueDescriptor)
Expand All @@ -65,19 +65,38 @@ public ModelBinder GetModelBinder(IValueDescriptor valueDescriptor)

public void AddService(Type serviceType, Func<IServiceProvider, object> factory)
{
_ = serviceType ?? throw new ArgumentNullException(nameof(serviceType));
_ = factory ?? throw new ArgumentNullException(nameof(factory));
ServiceProvider.AddService(serviceType, factory);
}

public void AddService<T>(Func<IServiceProvider, T> factory)
{
if (factory is null)
_ = factory ?? throw new ArgumentNullException(nameof(factory));
ServiceProvider.AddService(typeof(T), s => factory(s));
}

public void AddService(Type serviceType, Type? implementationType = null)
{
_ = serviceType ?? throw new ArgumentNullException(nameof(serviceType));
implementationType ??= serviceType;
object factory(IServiceProvider serviceProvider)
{
throw new ArgumentNullException(nameof(factory));
var bindingContext =
serviceProvider.GetService(typeof(BindingContext)) as BindingContext
?? this;
var valueDescriptor = new ModelBinder.AnonymousValueDescriptor(implementationType);
var modelBinder = bindingContext.GetModelBinder(valueDescriptor);
return modelBinder.CreateInstance(bindingContext)!;
}

ServiceProvider.AddService(typeof(T), s => factory(s));
AddService(serviceType, factory);
}

public void AddService<TService, TImplementation>() =>
AddService(typeof(TService), typeof(TImplementation));

public void AddService<T>() => AddService<T, T>();

internal bool TryGetValueSource(
IValueDescriptor valueDescriptor,
[MaybeNullWhen(false)] out IValueSource valueSource)
Expand Down Expand Up @@ -108,8 +127,8 @@ internal bool TryBindToScalarValue(
else
{
var parsed = ArgumentConverter.ConvertObject(
valueDescriptor as IArgument ?? new Argument(valueDescriptor.ValueName),
valueDescriptor.ValueType,
valueDescriptor as IArgument ?? new Argument(valueDescriptor.ValueName),
valueDescriptor.ValueType,
value,
resources);

Expand Down
12 changes: 12 additions & 0 deletions src/System.CommandLine/Builder/CommandLineBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,18 @@ public static CommandLineBuilder ConfigureConsole(
return builder;
}

public static CommandLineBuilder ConfigureBindingContext(
this CommandLineBuilder builder,
Action<BindingContext> configureBindingContext)
{
builder.AddMiddleware((context, next) =>
{
configureBindingContext?.Invoke(context.BindingContext);
return next(context);
}, default(MiddlewareOrder));
return builder;
}

public static CommandLineBuilder EnableDirectives(
this CommandLineBuilder builder,
bool value = true)
Expand Down
4 changes: 4 additions & 0 deletions src/System.CommandLine/Invocation/CommandHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,10 @@ public static ICommandHandler Create<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T1
Func<T1, T2, T3, T4, T5, T6, T7, T8, T9, T10, T11, T12, T13, T14, T15, T16, Task<int>> action) =>
HandlerDescriptor.FromDelegate(action).GetCommandHandler();

public static ICommandHandler FromBindingContext<THandler>()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be helpful for the naming to reflect that the actual handler will be created during binding. One not great idea to open up discussion: DuringBindingCreateHandlerOfType<T>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CreateLazyBoundHandler (to match with the existing Create)?

where THandler : ICommandHandler =>
Create((InvocationContext context, THandler handler) => handler.InvokeAsync(context));

internal static async Task<int> GetExitCodeAsync(object value, InvocationContext context)
{
switch (value)
Expand Down