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.

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

OptionsServiceCollectionExtensions.ConfigureDynamic #45294

Open
macsux opened this issue Nov 28, 2020 · 5 comments
Open

OptionsServiceCollectionExtensions.ConfigureDynamic #45294

macsux opened this issue Nov 28, 2020 · 5 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Options
Milestone

Comments

@macsux
Copy link
Contributor

macsux commented Nov 28, 2020

Background and motivation

It is sometimes desired to defined multiple named sections of repeating config blocks in the configuration file. Consider the following configuration which is used to define OAuth2 clients:

clients:
  google:
    clientId: foo
    secret: bar
  microsoft:
    clientId: dotnet
    secret: rocks

The current approach would require explicitly calling Configure on for each named section, which would require a code change for each new named registration. This new API would allow the entire section to be registered with each entry becoming a named instance. This API is important for dynamically configuring this like authentication schemes.

Proposed API

namespace Microsoft.Extensions.DependencyInjection
{
     public static class OptionsConfigurationServiceCollectionExtensions
     {
+        public static void ConfigureNamedSection<TOptions>(this IServiceCollection services, IConfiguration config) where TOptions : class
+        {
+            services.AddOptions<TOptions>();
+            services.AddSingleton<IConfigureOptions<TOptions>>(new NamedConfigureFromConfigurationSectionNamesOptions<TOptions>(config));
+        }
using Microsoft.Extensions.Configuration;

namespace Microsoft.Extensions.Options.ConfigurationExtensions
{
    public class NamedConfigureFromConfigurationSectionNamesOptions<TOptions> : IConfigureNamedOptions<TOptions> 
        where TOptions : class
    {
        private readonly IConfiguration _config;
        public NamedConfigureFromConfigurationSectionNamesOptions(IConfiguration config)
        {
            _config = config;
        }
        public void Configure(string name, TOptions options) => _config.GetSection(name).Bind(options);

        public void Configure(TOptions options) => Configure(Options.DefaultName, options);
    }
}

Usage Examples

Given the config section above, the registration could look like the following:

services.ConfigureNamedSection<OAuth2Options>(_configuration.GetSection("clients"));

Alternative Designs

None considered beyond alternate names for this new API. The naming of the new method can potentially be improved to be more intuitive about what it does.

Risks

Introduction of the extra API may cause confusion on its intended usage since it implies a specific structure of the IConfiguration passed in.

@macsux macsux added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 28, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-Extensions-Options untriaged New issue has not been triaged by the area owner labels Nov 28, 2020
@ghost
Copy link

ghost commented Nov 28, 2020

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

It is sometimes desired to defined multiple named sections of repeating config blocks in the configuration file. Consider the following configuration which is used to define OAuth2 clients:

clients:
  google:
    clientId: foo
    secret: bar
  microsoft:
    clientId: dotnet
    secret: rocks

The current approach would require explicitly calling Configure on for each named section, which would require a code change for each new named registration. This new API would allow the entire section to be registered with each entry becoming a named instance. This API is important for dynamically configuring this like authentication schemes.

Proposed API

namespace Microsoft.Extensions.DependencyInjection
{
     public static class OptionsConfigurationServiceCollectionExtensions
     {
+        public static void ConfigureNamedSection<TOptions>(this IServiceCollection services, IConfiguration config) where TOptions : class
+        {
+            services.AddOptions<TOptions>();
+            services.AddSingleton<IConfigureOptions<TOptions>>(new NamedConfigureFromConfigurationSectionNamesOptions<TOptions>(config));
+        }
using Microsoft.Extensions.Configuration;

namespace Microsoft.Extensions.Options.ConfigurationExtensions
{
    public class NamedConfigureFromConfigurationSectionNamesOptions<TOptions> : IConfigureNamedOptions<TOptions> 
        where TOptions : class
    {
        private readonly IConfiguration _config;
        public NamedConfigureFromConfigurationSectionNamesOptions(IConfiguration config)
        {
            _config = config;
        }
        public void Configure(string name, TOptions options) => _config.GetSection(name).Bind(options);

        public void Configure(TOptions options) => Configure(Options.DefaultName, options);
    }
}

Usage Examples

Given the config section above, the registration could look like the following:

services.ConfigureNamedSection<OAuth2Options>(_configuration.GetSection("clients"));

Alternative Designs

None considered beyond alternate names for this new API. The naming of the new method can potentially be improved to be more intuitive about what it does.

Risks

Introduction of the extra API may cause confusion on its intended usage since it implies a specific structure of the IConfiguration passed in.

Author: macsux
Assignees: -
Labels:

api-suggestion, area-Extensions-Options, untriaged

Milestone: -

@davidfowl
Copy link
Member

I think I would prefer to continue adding API to the AddOptions builder:

services.AddOptions<TOptions>()
        .BindNameConfiguration("clients");

@macsux
Copy link
Contributor Author

macsux commented Nov 28, 2020

Based on the feedback from @davidfowl, changing the proposal to look as following:
Updated: to handle empty path

namespace Microsoft.Extensions.DependencyInjection
{
    public static class OptionsBuilderConfigurationExtensions
    {
        public static OptionsBuilder<TOptions> BindNameConfiguration<TOptions>(
            this OptionsBuilder<TOptions> optionsBuilder,
            string configSectionPath)
            where TOptions : class
        {
            _ = optionsBuilder ?? throw new ArgumentNullException(nameof(optionsBuilder));
            _ = configSectionPath ?? throw new ArgumentNullException(nameof(configSectionPath));

            optionsBuilder.Services.AddSingleton<IConfigureOptions<TOptions>>(sp =>
            {
                IConfiguration config = sp.GetRequiredService<IConfiguration>();
                IConfiguration section = string.Equals("", configSectionPath, StringComparison.OrdinalIgnoreCase)
                    ? config
                    : config.GetSection(configSectionPath);
                return new BindNameConfigurationOptions<TOptions>(section);
            });
            return optionsBuilder;
        }
namespace Microsoft.Extensions.Options.ConfigurationExtensions
{
    public class BindNameConfigurationOptions<TOptions> : IConfigureNamedOptions<TOptions>
        where TOptions : class
    {
        private readonly IConfiguration _config;
        public BindNameConfigurationOptions(IConfiguration config)
        {
            _config = config;
        }
        public void Configure(string name, TOptions options) => _config.GetSection(name).Bind(options);

        public void Configure(TOptions options) => Configure(Options.DefaultName, options);
    }
}

@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Apr 8, 2021
@maryamariyan maryamariyan added this to the 6.0.0 milestone Apr 8, 2021
@ghost ghost moved this from Untriaged to 6.0.0 in ML, Extensions, Globalization, etc, POD. Apr 8, 2021
@macsux
Copy link
Contributor Author

macsux commented Jun 8, 2021

@davidfowl After trying to use the API as you suggested, I found that it doesn't really fit well together as an extension on OptionsBuilder. The reason is that OptionsBuilder inherently is tied to a single named instance of options. This creates a logical disconnect as:

  1. Starting an OptionsBuilder requires a name. We're kinda skipping name in what you proposed because it's dynamically derived, but it doesn't feel intuitive as the builder is already supposed to be tied to a single named instance
  2. Subsequent calls to OptionsBuilder such as the Validate method only make sense if they are applied to all option instances.

Seems like the logic in Validation, Postconfigure, and other things invoked by OptionsBuilder is built in such a way that if the name is null then it's assumed to apply to all instances, but there's no way to create OptionsBuilder with Name == null. If we want to proceed down this path then one solution would be to add a way to reset OptionBuilder.Name property to null after BindNameConfiguration is called. The easiest way to do it is to move BindNameConfiguration off the extension method on to OptionBuilder itself and add a private setter for Name.

@eerhardt
Copy link
Member

6.0 is now feature complete, this won't be completed for that release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Options
Projects
None yet
Development

No branches or pull requests

5 participants