Skip to content

Add support for stripping chars while binding to properties using ConfigurationBinder #37378

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

Closed
daniel-white opened this issue Jun 4, 2020 · 3 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Configuration
Milestone

Comments

@daniel-white
Copy link

daniel-white commented Jun 4, 2020

Background and Motivation

When using IOptions<T>, the property names must match case-insensitive with the keys stored in the instance of IConfiguration/IConfigurationProvider based on the rules defined in ConfigurationBinder. This can cause some readability concerns with environment variables that have single underscores (as not to conflict with the delimiter with 2 underscores) and all caps names as is a common convention. There is no way to override the behavior by configuration or reflection other that to change how the keys are stored in the IConfigurationProviders.

For instance, it maybe advantageous to allow applications to take keys such as My_Configuration_Key and bind it to a property named MyConfigurationKey in a model for IOptions<T> or other uses that the ConfigurationBinder provides.

I believe the logic should be contained in ConfigurationBinder and done procedurally (perhaps with some sort of caching mechanism) vs. reflection with a custom attribute declared on the properties of the options model classes or adjusting IConfigurationProviders to implement similar logic.

Proposed API

I propose a new property for BinderOptions: KeyCharsToStrip, defaulting to null indicating no stripping of characters from the key:

namespace Microsoft.Extensions.Configuration
{
    public class BinderOptions
    {
+       public char[]? KeyCharsToStrip { get; set; } = null;
    }

When BinderOptions was designed, it was designed to be extensible and further refine the behavior of the ConfigurationBinder.

Usage Examples

When calling IConfiguration.Bind there exist overloads that allows one to pass a lambda of type Action<BinderOptions> to configure the ConfigurationBinder. Hence, the KeyCharsToStrip property would be exposed there from an application perspective.

IServiceCollection services = ...;
char[] myReusableStripChars = new [] { '-', '_' };
services.Configure<MyOptions>(options => 
    Configuration.Bind(options, o => o.KeyCharsToStrip = myReusableStripChars)
);

Internally to ConfigurationBinder, there would be some sort of normalization step while attempting to locate the key to match the property name.

// in ConfigurationBinder
if (options.KeyCharsToStrip != null)
{
    // do a comparison of properties and keys while "removing" the chars listed in KeyCharsToStrip 
}

Alternative Designs

One alternative solution to normalizing the configuration keys would be to allow applications to declare an attribute on the property they wish to map much like JsonProperty:

namespace Microsoft.Extensions.Configuration
{
+    [AttributeUsage(AttributeTargets.Property)]
+    public class ConfigurationKeyAttribute : Attribute
+    {
+        public ConfigurationKeyAttribute(string key) { ... }
+        ...
+    }

The application could consume it this way:

class MyOptions
{
    [ConfigurationKey("My_Environment_Variable")]
    public string MyEnvironmentVariable { get; set; }
}

However, this is complicated by a few reasons that I can think of off the top of my head:

  1. Does is this prescribed key in the attribute make the binding case-sensitive?
  2. This seems to put configuration details into a model class and cannot be adjusted at runtime like how the current extension methods for configuration off IServiceCollection work.
  3. Reading this does it mean it is bound to the root of configuration or the branch that was provided at the call to .Bind?

Another alternative solution would be to add the option to each of the IConfigurationProviders' registration. Here are a few concerns with this approach:

  1. Multiple potential implementations in many of the configuration providers with mostly the same code.
  2. This to me appears to be at the next level after storing the configuration which IConfiguration does. It is more of a concern of the "view" that ConfigurationBinder provides.

Risks

  1. Since this property in BinderOptions is an additive change, no breaking changes claimed.
  2. Depending on the implementation in ConfigurationBinder, there will be a hit to memory/performance depending on the size of the inputs (keys and the chars in KeyCharsToStrip. If the option is not set, the logic will simply skip the costly operation of calculating the key to property name binding.
@daniel-white daniel-white added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 4, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-Meta untriaged New issue has not been triaged by the area owner labels Jun 4, 2020
@ericstj ericstj removed the untriaged New issue has not been triaged by the area owner label Jul 7, 2020
@ericstj ericstj added this to the 6.0.0 milestone Jul 7, 2020
@ericstj
Copy link
Member

ericstj commented Jul 7, 2020

It seems we have a number of issues around key handling to deal with the constrained value space in configuration sources. Similar to #35989. Perhaps we should consider some holistic solution to dealing with these.

@daniel-white
Copy link
Author

@ericstj im fine with either. I'm no longer with the employer when I created this, but I still see some benefit treating it holistically.

@maryamariyan
Copy link
Member

maryamariyan commented Oct 22, 2020

Closing issue in favor of #36010

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Extensions-Configuration
Projects
None yet
Development

No branches or pull requests

4 participants