Simplified usage of BindingContext resolved command handlers

[fredrikhr]

This PR adds convenience helper methods to simplify the usage of services resolved by the BindingContext.

• Add static helper FromBindingContext to CommandHandler.
  Creates an ICommandHandler where the specified command handler type is filled in at runtime by the BindingContext. This requires that the BindingContext knows how to instantiate the handler type (requires service registration).
• Add a ConfigureBindingContext middleware.
  Middleware simply takes an Action<BindingContext> parameter. In this callback the developer can call AddModelBinder and AddService. This simplifies service-registration on the BindingContext.
• Add no-parameter AddService instance method on BindingContext.
  Simplified service registration for trivial types. Uses a ModelBinder to auto-construct the service.

inspired by discussion in #1344.

/cc @leonardochaia

[fredrikhr]

I added 2f2796b following the suggestion from @leonardochaia

[leonardochaia]

Awesome! It looks like I would be able to remove my DI workaround with this changes.

This allows to override injected dependencies for testing purposes!

Can we please add this last test case? :)

var parser = new CommandLineBuilder(command)
   .ConfigureBindingContext(context => context.AddService<IGreeter, GreeterImpl>())
   .ConfigureBindingContext(context => context.AddService<IGreeter>(sp=> new GreeterMock())) // would use Moq or something here
   .Build();

IGreeter should be of type GreeterMock
I think this should work since the IServiceProvider impl has a Dictionary with a Type for the key as far as I remember.

[fredrikhr]

@leonardochaia I don't understand what you mean? The first one will be done by this PR, the second one you could always do already?

[leonardochaia]

Sorry, should have been clearer.

I'm just trying to be sure that the second ConfigureBindingContext will override the first one (and don't throw, or silently ignore it)

That's why I would assert that

IGreeter should be of type GreeterMock

[fredrikhr]

The second one thows??? Really? What exception do you get? 🤔

[leonardochaia]

I'm sorry (again). Didn't mean to say it is throwing right now. My point is that, if the expected API is that multiple calls to ConfigureBindingContext should be supported, and the latter call should be able to override things configured on previous calls, we should have a test that makes sure that that behavior is not gonna be broken in the future by someone i.e changing the inner code to throw when a service is already registered or something like that.

So that's why I propose we add a test that represents the actual use case of overriding an already registered service.

Something like Registered_services_can_be_overriden_by_subsequent_calls_to_ConfigurreBindingContext

EDIT: I'm not saying we should test the AddService methods work. We know they do by the previous tests as you mentioned. I'm interested in testing that subsequent calls to ConfigureBindingContext will allow to override services registered on earlier calls, since that's the API I think we need for testing purposes

[leonardochaia]

This looks awesome! 🎉

[jonsequitur]

The new AddService overloads increase the scope of the DI features in ways that might be misleading and will likely lead to additional issues being opened when people misunderstand the goal. The pre-existing AddService method accepted a Func so that you can know when it's called and control things like lifetime management, singleton versus transient instances, and recursive resolution, none of which System.CommandLine's service provider currently does for external types.

[jonsequitur]

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>

[fredrikhr]

CreateLazyBoundHandler (to match with the existing Create)?

[jonsequitur]

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.

[fredrikhr]

@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.

[jonsequitur]

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.

[fredrikhr]

@jonsequitur

The new AddService overloads increase the scope of the DI features in ways that might be misleading and will likely lead to additional issues being opened when people misunderstand the goal.

Yeah, I can see that. Basically, all service registrations are technically Transient services (they all have a factory registration that is called each time the service is resolved), but of course the factory for the basic services returns singleton instances. However, the no-func overloads that I added really are Transient. Maybe add the Transient suffix? Would that help?

[jonsequitur]

I'd prefer to avoid increasing the scope of the existing DI implementation at all, which originally lacked a public AddService method of any kind.

A safer approach might be to allow people to bring their own IServiceProvider implementation and therefore be able to completely control object lifetimes.

[fredrikhr]

@jonsequitur Hmm, okay, so we could just remove all the overloads to AddService. So, what about CommandHandler.CreateLazyFromModelBinder? If I get a ModelBinder that would be able to create an instance regardless of whether it was added to the BindingContext, no? So we don't actually need to add Handlers as BindingContext services? (Provided paramless-ctor)

[jonsequitur]

The way we create command handlers is undergoing some rethinking so that we can eventually remove all of the name-based matching and a lot of model binding complexity. If you're interested in the source generator approach we're looking at, take a look at this PR: #1362.