Options builder to chain Configure calls of the same named TOptions #238

jdom · 2017-10-09T19:58:17Z

Addresses #237

This allows to add a named service to the application, as well as allow the end-user to configure its options, without the need to discover what are the TOptions it needs to chain calls to, nor having to pass the name on every .Configure<TOptions>(name, ...) call.

This results in the user being able to do things like this for extensions that use the builder:

services.AddNamedXyz("name", optionsBuilder => 
   optionsBuilder.Configure(Configuration.GetSection("Something"))
   optionsBuilder.Configure(options => options.SomeSetting = "value for name"));

Notice how the 2 calls to the Configure method automatically infer the TOptions type, and do not need to pass the name (even thought they are configuring a named option.

Also, the author of the AddNamedXyz extension method does not need to reference the Microsoft.Extensions.Options.ConfigurationExtension nuget package just in case the end user cares to use the IConfiguration overload. If the end user references it, then the relevant Configure overload will automatically light up on the options builder.

dnfclas · 2017-10-09T19:58:21Z

@jdom,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

jdom · 2017-10-09T20:00:40Z

src/Microsoft.Extensions.Options/IOptionsBuilder.cs

+    /// Represents a builder that configures a named TOptions instance.
+    /// </summary>
+    /// <typeparam name="TOptions">The type of options being requested.</typeparam>
+    public interface IOptionsBuilder<TOptions> where TOptions : class


Is IOptionsBuilder a good name? Because it is meant to forward calls to the IServiceCollection. An alternative is to make it agnostic of it, and then have the concrete implementation flush the calls to the service collection, but I would think that's a little less intuitive in case you mix using the builder and using Configure directly on the service collection

I'd prefer if the only thing you can do with this builder is call Configure as opposed to allowing arbitrary access to IServiceCollection.

i.e. the only operations that would be allowed on the builder would be Configure/PostConfigure (or anything else specifically targeting the builder)

Is there any major benefit to having an interface for this as opposed to only the OptionsBuilder class itself, you can still add extension methods in the same way. There should be nothing that is worth replacing in the base class really.

As this is all sugar on top of existing IServiceCollection methods

There isn't really a benefit for the interface. As it is, it's just syntactic sugar on top of IServiceCollection, which is the main challenge I'm addressing. I started this thinking I would store delegates and then have an explicit call that would bind them to the IServiceCollection, but that's what I though would be un-intuitive with no real gain (other than decoupling the builder from the DI container temporarily, which IMO is not useful).
So I can change this to be just a public type for us to hook up extension methods.
Still, the specific extension methods would need to access the IServiceCollection to be able to forward calls to it, or do you suggest some other approach?

Yeah that's fine, we typically hang onto the IServiceCollection in our builders too, and you will definitely have to hang onto the Name as well.

The main benefit I see is strongly tying a specific named options instance to this builder, so you can just call Configure/PostConfigure without the generic or specifying the name.

I'm still trying to see if we could use this instead of things like ConfigureApplicationCookie that we have today.

Hehe, configuring the cookie is the only example I saw of using named options in the wild, and I'm not terribly fond of the approach, and hence this proposal. In Orleans we'll have many cases of configuring named services/options.
But even for our singleton service abstractions that can be fulfilled with 1 of different implementations, where each of them might have slightly different typed options, this can be of a lot of use, as the end user will not have to discover which strongly typed options to use, as well as giving the flexibility to chain multiple calls (and not just pick only 1 of the Action<TOptions> or IConfiguration overloads, in case we provide the extension methods with these overloads).

HaoK · 2017-10-09T20:06:56Z

test/Microsoft.Extensions.Options.Test/OptionsBuilderTests.cs

+        public void CanSupportDefaultName()
+        {
+            var services = new ServiceCollection().AddOptions();
+            var builder = services.CreateOptionsBuilder<FakeOptions>();


Nit: maybe shorten CreateOptionsBuilder => BuildOptions?

davidfowl · 2017-10-10T05:37:33Z

src/Microsoft.Extensions.Options/OptionsServiceCollectionExtensions.cs

+        /// <typeparam name="TOptions">The options type to be configured.</typeparam>
+        /// <param name="services">The <see cref="IServiceCollection"/> to add the services to.</param>
+        /// <returns>The <see cref="OptionsBuilder{TOptions}"/> so that configure calls can be chained in it.</returns>
+        public static OptionsBuilder<TOptions> BuildOptions<TOptions>(this IServiceCollection services) where TOptions : class


I'm not sure about this name.

What about these alternatives?

Configure<TOptions>() (as it would be the only of these overloads that don't take in an extra argument, but it might be confusing that they have different return values)

CreateOptionsBuilder<TOptions>()

ConfigureOptions<TOptions>() (although there is already a non-generic overload with this name that takes a type, so it might be very confusing)

BuildOptions<TOptions>() (just putting it here for contrasting)

ConfiguratorFor<TOptions>() (probably might make sense to rename OptionsBuilder to OptionsConfigurator if we choose this extension method name, and maybe even regardless, as this helper doesn't build from scratch, just helps configure).

UseConfigurator<TOptions>()

BTW, reflecting on this, my preference would be option 5.

jdom · 2017-10-28T01:47:40Z

Any preferences for naming, or further feedback on the approach?

HaoK · 2017-11-03T18:48:46Z

So I thought about this a bit more, and I think we can maybe do something more drastic, akin to what we did with authentication...

Where we add OptionsBuilder, which has OptionsBuilder<T> Build<T>(string name, Action<OptionsBuilder<T>> build) method.

So startup would look more like:

  services.BuildOptions()
    .Build<Options1>("name", o => o.Configure(config)) // named instance
    .Build<Options1>(o => o.Configure(config)) // no name

This lets us scope options apis to OptionsBuilder and not pollute IServiceCollection, and we can still hang the apis specific to a particular options type (hiding the name for example) to an instance of OptionsBuilder<T>

Thoughts @davidfowl @ajcvickers @jdom ?

jdom · 2017-11-03T20:05:47Z

@HaoK your suggestion will work for us, so we'll gladly take it, although I'm not sure it is better IMO. The current approach just adds a single extension method to IServiceCollection, with an overload for taking the name (so technically 2 methods with the same name).
Your suggestion has only 1 total, but you have to chain it to be useful, otherwise BuildOptions() by itself doesn't add value. Also, at least in our scenarios, it's not that the end user will start configuring a bunch of named options in one go... they will do it while they are adding concrete service instances, so it makes sense that they they get the options builder/configurator at the time they add that specific service. For that I mean they'll do this:

services
    .AddFooService("service1", ob => ob.Configure(config).Configure(o => o.FooSetting = 3))
    .AddBarService("service2", ob => ob.Configure(config).Configure(o => o.BarSetting = "bar"))
    .AddBarService("service3", ob => ob.Configure(config));

While your approach would favor more this:

services
    .AddFooService("service1")
    .AddBarService("service2")
    .AddBarService("service3")
    .BuildOptions()
         .Build<FooServiceOptions>("service1", ob => ob.Configure(config).Configure(o => o.FooSetting = 3)
         .Build<BarServiceOptions>("service2", ob => ob.Configure(config).Configure(o => o.BarSetting = "bar"))
         .Build<BarServiceOptions>("service3", ob => ob.Configure(config)));

Granted, it doesn't prevent the original code, but you can see that where it's most useful is not by chaining different option builders together, but chaining the addition of services with their options, and the user doesn't need to discover which options to configure, or what name to correlate with the named service.

HaoK · 2017-11-03T20:12:48Z

So I'd expect you to just have AddFooService return OptionsBuilder<FooOptions>, and then you'd still be able to do something like:

services.AddFooService("service1").Configure(config).Configure(o => o.FooSetting = 3))

HaoK · 2017-11-03T20:15:16Z

The BuildOptions block would be more for if you wanted to configure all the options in once place. AddXyz should still be free (and encouraged) to return the appropriate sugar to make it easy to configure things.

HaoK · 2017-11-06T23:13:13Z

@jdom I'm going to prototype the OptionsBuilder in this PR: #239 as we can have these new Configure overloads return an OptionsBuilder<T> instead of IServiceCollection

AddOptions<T> now calls AddOptions() implicitly.

This reverts commit a47db5a.

HaoK · 2017-11-07T22:00:01Z

Looks good to me, any concerns @davidfowl @ajcvickers ?

HaoK · 2017-11-09T19:48:41Z

...icrosoft.Extensions.Options.ConfigurationExtensions/OptionsBuilderConfigurationExtensions.cs

+        /// <param name="optionsBuilder">The options builder to add the services to.</param>
+        /// <param name="config">The configuration being bound.</param>
+        /// <returns>The <see cref="OptionsBuilder{TOptions}"/> so that additional calls can be chained.</returns>
+        public static OptionsBuilder<TOptions> Configure<TOptions>(this OptionsBuilder<TOptions> optionsBuilder, IConfiguration config) where TOptions : class


Actually now that we have a Builder to hang this off of, we don't have to call it Configure, we should call this method Bind to give it more meaning

I would gladly do that change, but let's clarify the bigger picture first. Does this also apply to the overload that takes and Action<TOptions>. And would we rename PostConfigure to PostBind?
Or maybe those overloads should be renamed to Apply and PostApply? Or Update?

Yeah we probably should switch to something different, its going to be too insane to keep adding all these overloads as longer term I wouldn't be surprised if there were more lifecycles either, like Validation.

Which probably would mean something closer to:

Bind(IConfiguration, OptionsLifecycle.[Configure/PostConfigure/Validation]).

To level we'd still have
Configure
PostConfigure
(Validate in the future)

It looks like we don't have PostConfigure(IConfiguration) today anyways, so I don't think you need to worry about it for the purposes of this PR... We can tackle this in a different issue/PR

Its just sugar for AddOptions<T>.PostConfigure(o => config.Bind(o)) anyways.

ajcvickers · 2017-11-10T18:36:38Z

@HaoK Looks okay to me.

HaoK · 2017-11-10T23:04:43Z

Thanks @jdom I'll merge this into dev now

Options builder to chain Configure calls of the same named TOptions

ad13dfc

dnfclas added the cla-already-signed label Oct 9, 2017

jdom mentioned this pull request Oct 9, 2017

OptionsBuilder to chain configuration calls #237

Closed

jdom commented Oct 9, 2017

View reviewed changes

HaoK reviewed Oct 9, 2017

View reviewed changes

jdom added 3 commits October 9, 2017 14:10

Rename CreateOptionsBuilder to BuildOptions

ba9b973

Remove IOptionsBuilder interface

d56e59b

Add more test cases that chain calls to Configure and PostConfigure

14fa691

davidfowl reviewed Oct 10, 2017

View reviewed changes

jdom mentioned this pull request Nov 7, 2017

Add Configure<TOption, TDep1, TDep2> sugar #239

Closed

jdom added 5 commits November 7, 2017 11:00

Merge remote-tracking branch 'remotes/upstream/dev' into options-builder

115f170

Rename OptionsBuilder to OptionsConfigurator

a47db5a

Rename BuildOptions to AddOptions

3581b03

AddOptions<T> now calls AddOptions() implicitly.

Move basic extension methods to the OptionsConfigurator class

ce123b9

Revert "Rename OptionsBuilder to OptionsConfigurator"

effb942

This reverts commit a47db5a.

HaoK reviewed Nov 9, 2017

View reviewed changes

jdom added 2 commits November 9, 2017 12:57

Rename Configure(IConfiguration) overload to Bind

92140a0

Add test coverage for the IConfiguration extension method

e4a7fe7

HaoK merged commit fe3f1b1 into aspnet:dev Nov 10, 2017

jdom mentioned this pull request Nov 21, 2017

Use OptionsBuilder to allow chaining Configure calls for providers dotnet/orleans#3696

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Options builder to chain Configure calls of the same named TOptions #238

Options builder to chain Configure calls of the same named TOptions #238

jdom commented Oct 9, 2017

dnfclas commented Oct 9, 2017

jdom Oct 9, 2017

HaoK Oct 9, 2017

HaoK Oct 9, 2017

HaoK Oct 9, 2017

jdom Oct 9, 2017

HaoK Oct 9, 2017

jdom Oct 9, 2017

HaoK Oct 9, 2017

davidfowl Oct 10, 2017

jdom Oct 12, 2017

jdom Nov 2, 2017

jdom commented Oct 28, 2017

HaoK commented Nov 3, 2017

jdom commented Nov 3, 2017

HaoK commented Nov 3, 2017

HaoK commented Nov 3, 2017

HaoK commented Nov 6, 2017

HaoK commented Nov 7, 2017

HaoK Nov 9, 2017

jdom Nov 9, 2017

HaoK Nov 9, 2017 •

edited

Loading

HaoK Nov 9, 2017

HaoK Nov 9, 2017

ajcvickers commented Nov 10, 2017

HaoK commented Nov 10, 2017

Options builder to chain Configure calls of the same named TOptions #238

Options builder to chain Configure calls of the same named TOptions #238

Conversation

jdom commented Oct 9, 2017

dnfclas commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdom commented Oct 28, 2017

HaoK commented Nov 3, 2017

jdom commented Nov 3, 2017

HaoK commented Nov 3, 2017

HaoK commented Nov 3, 2017

HaoK commented Nov 6, 2017

HaoK commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HaoK Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajcvickers commented Nov 10, 2017

HaoK commented Nov 10, 2017

HaoK Nov 9, 2017 •

edited

Loading