Runtime: API Proposal: Add BindConfiguration extension method for OptionsBuilder

[fredrikhr]

Summary

• Add BindConfiguration extension method for OptionsBuilder<TOptions>.

Description

There are situations in which the IConfiguration instance from which to configure options has not yet been materialized when a method like ConfigureServices is being executed. For example, the .NET Core Generic Host Builder offers the ConfigureServices method which does not offer access to the Application Configuration instance that will materialize when the Host is built. (Note: A fully materialized host configuration instance can be obtained from the HostBuilderContext instance that is passed to the callback, however, this host configuration can differ from the Application configuration and may not offer the desired behaviour.)

In such situations, the Bind extension methods on the OptionsBuilder<TOptions> cannot be called, since they all require a fully materialized IConfiguration instance. Instead, you need to call the Configure extension method, specifying IConfiguration as a DI dependency for configuration and then call the IConfiguration.Bind extension method in the specifying configuration callback action.

Since binding from a DI-provided IConfiguration is a fairly usual scenario when using the .NET Generic Host (i.e. without using a Startup class), it would be advantageous to simplify this scenario by providing 1^st class support in form of an extension method that performs the necessary steps.

Proposal

• Add BindConfiguration extension method for OptionsBuilder<TOptions>.

namespace Microsoft.Extensions.DependencyInjection
{
// Existing extensions holder type
public static class OptionsBuilderConfigurationExtensions
{
   // New API
   public static OptionsBuilder<TOptions> BindConfiguration<TOptions>(
            this OptionsBuilder<TOptions> optionsBuilder,
            string configSectionPath = null,
            Action<BinderOptions> configureBinder = null)
            where TOptions : class => null;
}
}

Usage

partial class Startup
{
       public static void ConfigureServices(IServiceCollection services)
        {
            services.AddOptions<MyOptions>()
                .BindConfiguration();
        }
}

Previous implementation

The proposed API is a convenience wrapper around existing APIs. The code above could previously be implemented as follows:

partial class Startup
{
       public static void ConfigureServices(IServiceCollection services)
        {
            services.AddOptions<MyOptions>()
                .Configure<IConfiguration>((opts, config) => config.Bind(opts));
        }
}

Comments

The name BindConfiguration was chosen, since the arguments of the extension method do not directly conway how binding is performed. The existing Bind extension methods all take an IConfiguration instance as argument, and in these cases the binding is implied from the argument type. Conceptually, the proposed API can be thought of as an overload to the existing Bind extension methods.

Implementation

34191 specifies a possible implementation for this API

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.

[maryamariyan]

/cc: @davidfowl

[davidfowl]

This is a good idea! I would rename sectionName to path (since you want to support A:B:C)

cc @HaoK

[HaoK]

Sounds like a great idea to me as well!

[fredrikhr]

I would rename sectionName to path (since you want to support A:B:C)

@davidfowl The Bind extension method on IConfiguration uses "key" as the paramter name. Either "key" or "path" seem good names to me?...

[davidfowl]

I would be explicit and call it configPath. Also thinking about this more, it's not clear to me where you'd use this without the section

[fredrikhr]

It's not clear to me where you'd use this without the section

@davidfowl What do you mean by that? Are you referring to the nullability of the argument? I agree that you almost never want to have a null configuration path, but since we'd implement that over the IConfiguration.Bind(string key, object instance) extension method, null is technically a valid/possible key (which would use the Configuration root, empty string is also valid.

But I agree that it is most nonsensical to do so, I have no problems with making it a required parameter.

[fredrikhr]

Updated proposal above in reaction to https://github.com/dotnet/runtime/issues/36209#issuecomment-657081587 and https://github.com/dotnet/runtime/issues/36209#issuecomment-657201618.

• Method praramater renamed to configPath.
• Method parameter marked as required and non-null.

[terrajobst]

Video

• Looks reasonable
• configPath looks like a file system path; we should name it configSectionPath, unless we use configPath elsewhere already.

C# namespace Microsoft.Extensions.DependencyInjection { public static class OptionsBuilderConfigurationExtensions { public static OptionsBuilder<TOptions> BindConfiguration<TOptions>( this OptionsBuilder<TOptions> optionsBuilder, string configSectionPath, Action<BinderOptions> configureBinder = null) where TOptions : class => null; } }

[fredrikhr]

@maryamariyan I have opened #39825 which implements this proposal.

@maryamariyan, @davidfowl, while I was implementing the API, I noticed why I had originally proposed the configSectionPath parameter to be nullable: In the other Bind methods we also use the Name property of optionsBuilder as a section name to the IConfiguration. Therefore, defaulting configSectionPath could make sense.

[Joelius300]

Not sure if this is the appropriate place to ask but I'm uncertain if I found a bug or misunderstood something. In the original issue it was said

A fully materialized host configuration instance can be obtained from the HostBuilderContext instance that is passed to the callback, however, this host configuration can differ from the Application configuration and may not offer the desired behaviour

Until recently, I only used the generic host as part of ASP.NET and didn't have to think much about this. However, I just got started with a console application using Microsoft.Extensions.Hosting and just did it like I was used to from using Startup (so calling AddOptions<T>().Bind(config.GetSection("Whatever"))). Then I realized BindConfiguration was added and tried using it (since it seemed more appropriate) but even though it gets the correct values initially the reload-on-change functionality doesn't work anymore.

Is this expected or a bug?

If this isn't a short and obvious answer I can open a standalone issue for it but I thought I'd ask here first.

---

To reproduce just create a new .NET 5 console application, add Microsoft.Extensions.Hosting and replace the contents of Program.cs with the following.

using System.Threading.Tasks;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;

namespace Generic_Host_Configuration_Issue
{
    public static class Program
    {
        public static Task Main(string[] args) => Host.CreateDefaultBuilder(args)
                                                      .ConfigureServices(ConfigureServices)
                                                      .RunConsoleAsync();

        private static void ConfigureServices(HostBuilderContext hostBuilderContext, IServiceCollection services)
        {
            services.AddHostedService<SampleHostedService>();

            // This works as expected and updates when the file is changed
            services.Configure<SampleOptions>(hostBuilderContext.Configuration.GetSection("Sample"));

            // This also works
            //services.AddOptions<SampleOptions>()
            //        .Bind(hostBuilderContext.Configuration.GetSection("Sample"));

            // But this doesn't
            //services.AddOptions<SampleOptions>()
            //        .BindConfiguration("Sample");
        }
    }
}

Add the following classes.

public class SampleOptions
{
    public string MyText { get; set; }
    public int MyInt { get; set; }
}

public class SampleHostedService : BackgroundService
{
    private readonly ILogger<SampleHostedService> _logger;
    private readonly IOptionsMonitor<SampleOptions> _optionsMonitor;

    public SampleHostedService(ILogger<SampleHostedService> logger, IOptionsMonitor<SampleOptions> optionsMonitor)
    {
        _logger = logger;
        _optionsMonitor = optionsMonitor;
    }

    protected override async Task ExecuteAsync(CancellationToken stoppingToken)
    {
        while (!stoppingToken.IsCancellationRequested)
        {
            var current = _optionsMonitor.CurrentValue;
            _logger.LogInformation($"MyText: {current.MyText} | MyInt: {current.MyInt}");

            await Task.Delay(3000, stoppingToken).ConfigureAwait(false);
        }
    }
}

Now the only thing left is an appsettings.json file which gets copied to the output.

{
  "Sample": {
    "MyText": "Hello World!",
    "MyInt": 42
  }
}

Now you can run the application and make changes in the appsettings.json file (the one in the build-folder of course) to see it change (or not 😉).

[fredrikhr]

@Joelius300 hmmm, both your example Bind from Configuration on the HostBuilderContext. My extension actually binds from the fully materialized Configuration that also takes AddConfiguration calls within Startup into account.

Can you test what happens if you call:
Configure((opts, config) => config.Bind(section, opts))

[fredrikhr]

@Joelius300 so what I am saying is that it is rather a bug in the Chained Configuration Source, not actually in this extension method. That being said, I don't know whether they wanted this behaviour in the chained configuration or whether it is a bug. Could you open up a new issue detailing this (and tag me, since I too would like to know 😂)?

[Joelius300]

@fredrikhr

My extension actually binds from the fully materialized Configuration that also takes AddConfiguration calls within Startup into account

I know but does that prevent the reload-on-change functionality? That just doesn't seem inteded to me.

Can you test what happens if you call:
Configure((opts, config) => config.Bind(section, opts))

How would I call that? Didn't manage to find that function on IServiceCollection or OptionsBuilder 😕

so what I am saying is that it is rather a bug in the Chained Configuration Source, not actually in this extension method

Maybe but I thought I'd ask here first 😅

[fredrikhr]

@Joelius300 huh? Configure() should d be on OptionsBuilder? 🤔

[Joelius300]

@fredrikhr

I can do this

services.AddOptions<SampleOptions>()
        .Configure<IConfiguration>((options, config) => config.Bind(options));

but that just gives me an empty option (all default values).

[davidfowl]

https://github.com/dotnet/runtime/pull/39825#issuecomment-731360273

[davidfowl]

@maryamariyan we should fix this one.

[Joelius300]

@davidfowl a-ha! 😄

Should I open a separate issue to document it or do these comments here suffice?

[HaoK]

Hrm, so generally bind doesn't take part in registering for notifications, in the past this gets hooked up via a Configure, but you can always just directly add the change source for reload to work:

https://github.com/dotnet/runtime/blob/master/src/libraries/Microsoft.Extensions.Options.ConfigurationExtensions/src/OptionsConfigurationServiceCollectionExtensions.cs#L71

[HaoK]

Since basically change notifications is orthogonal, its needing that change token source that's hooked up to config.reload, and turning on reloadOnChanges on the config file itself

[HaoK]

Ah I see, this is named a BindConfiguration (but its really a Configure), so yeah that method should also register the change source, should be an easy fix