Configuration > Binder - Support for binding by custom attribute names.

[Coderrob]

Background and Motivation

I am trying to use JSON configuration settings that have names such as "some.flag" that get loaded into the IConfiguration dictionary. The key itself is mapped via JsonPropertyNameAttribute on my classes. The Binder project does not know of anything outside of the objects property names when it attempts to lookup the value by property name in the configuration dictionary.

What would be useful is to add custom binding attribute to identify a "known" name within the configuration dictionary that may not match the actual property name.

These lookups can be enabled by a new custom binding options attribute.

Proposed API

Naming I was leaning towards ConfigurationKeyNameAttribute to also map with class file names within the project. Within the context of configuration binder it could also be fitting for BindingKeyNameAttribute. Naming is hard.

namespace Microsoft.Extensions.Configuration
{
+    [AttributeUsage(AttributeTargets.Field | AttributeTargets.Property)]
+    public sealed class ConfigurationKeyNameAttribute : Attribute
+    {
+        public string Name { get; }
+
+        public ConfigurationKeyNameAttribute(string name)
+        {
+            Name = name;
+        }
+    }
}

        private static void BindProperty(PropertyInfo property, object instance, IConfiguration config, BinderOptions options)
        {
            // We don't support set only, non public, or indexer properties
            if (property.GetMethod == null ||
                (!options.BindNonPublicProperties && !property.GetMethod.IsPublic) ||
                property.GetMethod.GetParameters().Length > 0)
            {
                return;
            }

            object propertyValue = property.GetValue(instance);
            bool hasSetter = property.SetMethod != null && (property.SetMethod.IsPublic || options.BindNonPublicProperties);

            if (propertyValue == null && !hasSetter)
            {
                // Property doesn't have a value and we cannot set it so there is no
                // point in going further down the graph
                return;
            }

-            propertyValue = BindInstance(property.PropertyType, propertyValue, config.GetSection(property.Name), options);
+            propertyValue = GetPropertyValue(property, instance, config, options);

            if (propertyValue != null && hasSetter)
            {
                property.SetValue(instance, propertyValue);
            }
        }

+        private static object GetPropertyValue(PropertyInfo property, object instance, IConfiguration config, BinderOptions options)
+        {
+            object propertyValue;
+            var propertyName = GetPropertyAttributeKeyName(property);
+
+            if (string.IsNullOrEmpty(propertyName))
+                propertyName = property.Name;
+
+            return BindInstance(property.PropertyType, propertyValue, config.GetSection(propertyName), options);
+        }
+
+       private static string GetPropertyAttributeKeyName(MemberInfo property)
+       {
+            if (property == null)
+                return string.Empty;
+
+            foreach (var attributeData in property.GetCustomAttributesData())
+            {
+                if (attributeData.AttributeType != typeof(ConfigurationKeyNameAttribute)) continue;
+                var constructorArgument = attributeData.ConstructorArguments.FirstOrDefault();
+                if (constructorArgument.ArgumentType == typeof(string))
+                    return constructorArgument.Value?.ToString() ?? string.Empty;
+            }
+
+            return string.Empty;
+       }

Usage Examples

Currently if there exists a class the behavior of the binder is to look at the dictionary created from external data sources like XML or typically JSON. A somewhat typical example of a .Net class with custom JSON property attributes:

public class Person
{
    [ConfigurationKeyName("first_name")]
    [JsonPropertyName("last_name")]
    public string LastName { get; set; }
}

Decorating the new property with the custom binding attribute I've proposed the binder will be able to map the dictionary name to the custom property name during the binding operation.

        public class ComplexOptions
        {
            [ConfigurationKeyName(Name = "Named_Property")]
            public string NamedProperty { get; set; }
        }

        public class DerivedOptions : ComplexOptions
        {
        }

        [Fact]
        public void CanBindAttributesIConfigurationSection()
        {
            var dic = new Dictionary<string, string>
            {
                {"Section:Named_Property", "Yo"},
            };
            var configurationBuilder = new ConfigurationBuilder();
            configurationBuilder.AddInMemoryCollection(dic);
            var config = configurationBuilder.Build();
            var options = config.Get<ConfigurationInterfaceOptions>();
            var childOptions = options.Section.Get<DerivedOptions>(binderOptions =>
                binderOptions.BindPropertyUsingAttributeNames = true);
            Assert.Equal("Yo", childOptions.NamedProperty);
        }

Alternative Designs

Alternative is to use private properties that act as a setter for the desired properties that have the same matching name from JSON. The period in the name doesn't help in my case.

Risks

Existing behavior is preserved. New property to attempt to bind using custom property names is an opt-in behavior. Input or property info sanitation checks would be needed to ensure no binding lookup causes any unexpected exceptions.

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

[Coderrob]

@maryamariyan Originally opened a PR in the dotnet/extensions repo for this work. Not sure if that helps. Applied DataMember attribute property info lookup to support custom data binding names agnostic of Xml/Json specific uses from the extensions internal configuration dictionary.

dotnet/extensions#2810

[maryamariyan]

Thanks @Coderrob were you planning on moving over the diff to runtime repo and opening a PR against this new repo instead?

[Coderrob]

Thanks @Coderrob were you planning on moving over the diff to runtime repo and opening a PR against this new repo instead?

Sure, I’ll see what I can do this weekend. I found that ability to be incredibly useful in binding. We’ll see what happens.

[maryamariyan]

@Coderrob thanks I saw the update on the PR. But as discussed in the PR the best next step on this would be to prepare the API proposal, usage, etc. in the issue page here (following https://github.com/dotnet/runtime/blob/master/docs/project/api-review-process.md).

Since usually API reviews might take a little churn you might end up wanting to close the PR at some point and reopen once this issue gets API approved.

[Coderrob]

@maryamariyan updated this issue per the API proposal template.

[Coderrob]

Thank you for the help @maryamariyan :)

[safern]

Hey @Coderrob

Thanks for the suggestion. One question that comes into mind is why would we want to match any attribute that contains a named parameter called "Name" and then have a Property in the BinderOptions that has to be set to true in order to enable this.

Wouldn't it make sense to follow what other binders/deserializers do, and add a well known attribute, and then when binding properties, if the property contains this attribute, then use whatever argument was passed to the attribute as the name to bind to, if not just use the property name? i.e, adding: [ConfigurationBinderPropertyName("My_Property")] ?

[Coderrob]

Hey @Coderrob

Thanks for the suggestion. One question that comes into mind is why would we want to match any attribute that contains a named parameter called "Name" and then have a Property in the BinderOptions that has to be set to true in order to enable this.

Wouldn't it make sense to follow what other binders/deserializers do, and add a well known attribute, and then when binding properties, if the property contains this attribute, then use whatever argument was passed to the attribute as the name to bind to, if not just use the property name? i.e, adding: [ConfigurationBinderPropertyName("My_Property")] ?

@safern

The only concern I have on the configuration binding attribute would be:

• it creates attributes w/ duplicated text values
• configuration binding concerns start to bleed out into the objects

Those two things may not be big concerns. Just making note. I was hoping to do two birds w/ one option flag. The common use of DataMember / JsonPropertyName both share the use of a name property to drive their lookup. This is what is causing a problem after the configuration source loads the json data into the dictionary.

At this point the key and the property names themselves do not match. I figured scanning attributes to identify names contained within the key of the dictionary would be unique enough that conflicts and order of operations would be limited.

In either case whether my proposed binding options or as a standalone configuration specific attribute would solve the problem.

[safern]

Thanks, @Coderrob. What would happen on the case where there are 2 attributes with a Name parameter to the constructor?

I think I'm still leaning towards a new attribute as that is what other binders do, however I'd like opinions from @davidfowl, @maryamariyan and @ericstj, if they have one.

[Coderrob]

@safern How I originally built it in the PR it iterates the attributes name parameter value, checks the dictionary, and if nothing is found it moves on to the next.

I'm with you, and good idea. I think having a custom binding attribute would be the safest explicit way to handle the lookup. :) Either way I'd be happy if it was implemented.

[safern]

I'm with you, and good idea. I think having a custom binding attribute would be the safest explicit way to handle the lookup. :) Either way I'd be happy if it was implemented.

Great 😄 -- we can definitely get this working. Would you mind updating the API proposal to focus on the attribute addition instead?

[safern]

@Coderrob any progress on updating the proposal?

[Coderrob]

@Coderrob any progress on updating the proposal?

@safern whoops, sorry for the drop-off. I'll update the proposal this weekend with the suggest change to approach. Thanks for following up. Life... uh... finds a way.... to distract. 🦖

[Coderrob]

@safern Updated API proposal. If you can spare a cycle to review I'd be most appreciative.

Took a bit to get spare time to devote to it. I ran through a few different ideas of names, but names are hard so your input there would be greatly appreciated as well.

I also thought leaving a function open for getting the property attribute's key name would be helpful. If in the future the binding were to support wildcard prefixes or who knows what. Took a swag at a KISS solution above in the proposal that solves the need.

Hope that helps. :)

[davidfowl]

Can we use an existing attribute instead (like data annotations)

[Coderrob]

Can we use an existing attribute instead (like data annotations)

@davidfowl looking through the current DataAnnotations attributes to see if there is anything closely matching the intent. Only thing that is similar in naming at least seems to be the KeyAttribute. We could add two constructors. Empty and one where a key name is defined? There is also the DisplayAttribute where a name is defined. The comments are very UI focused but it does say "Gets or sets the Name attribute property, which may be a resource key string."... so maybe a reuse point for configuration?

My previous proposal was to iterate the custom attribute data and look for any attribute with a defined "Name" property to do a key lookup in the configuration dictionary. Talking with @safern it made it more explicit with an attribute so that was the approach we agreed to.

We could piggy back off the data annotation KeyAttribute/DisplayAttribute, create a new data annotation attribute for this need, or should we keep binding specific attribute closer to its library?

[Coderrob]

@safern was there any updates needed to the binding attribute API recommendation?

[safern]

@Coderrob based on your summary I still incline towards adding a new attribute for this purpose as the others seem pretty UI focused, but since @davidfowl proposed using the existing like DataAnnotations attributes, I would like to know his take to see if there is another attribute we might be missing that he is thinking of.

[SomeoneIsWorking]

The fact that this doesn't exist already is really making me confused

[Coderrob]

@safern is this attribute still being considered for implementation? Don’t know if I should do anything else for it.

[davidfowl]

@safern Seems threre's nothing that hits the mark besides DisplayName (and that's wrong). I guess we can go with the original proposal.

[safern]

Thanks @davidfowl -- I'll set it as ready for review.

[terrajobst]

Video

• Looks good as proposed
  • Does this need an analyzer?

namespace Microsoft.Extensions.Configuration
{
    [AttributeUsage(AttributeTargets.Field | AttributeTargets.Property)]
    public sealed class ConfigurationKeyNameAttribute : Attribute
    {
        public ConfigurationKeyNameAttribute(string name);
        public string Name { get; }
    }
}

[Coderrob]

New binding support implemented. Closing issue.