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.

Sign up for GitHub

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

Jump to bottom

Enable binding to non-null IEnumerable properties. #52514

Closed
wants to merge 1 commit into from
Closed

Enable binding to non-null IEnumerable properties. #52514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ private static object BindInstance(Type type, object instance, IConfiguration co
instance = CreateInstance(type);
}

// See if its a Dictionary
// See if it's a Dictionary
Type collectionInterface = FindOpenGenericInterface(typeof(IDictionary<,>), type);
if (collectionInterface != null)
{
Expand All @@ -330,16 +330,31 @@ private static object BindInstance(Type type, object instance, IConfiguration co
}
else
{
// See if its an ICollection
// See if it's an ICollection
collectionInterface = FindOpenGenericInterface(typeof(ICollection<>), type);
if (collectionInterface != null)
{
BindCollection(instance, collectionInterface, config, options);
}
// Something else
else
{
BindNonScalar(config, instance, options);
// see if it's an IEnumerable that has been set with a broader collection
collectionInterface = FindOpenGenericInterface(typeof(IEnumerable<>), type);

if (collectionInterface != null && instance != null)
{
collectionInterface = FindOpenGenericInterface(typeof(ICollection<>), instance.GetType());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the idea was to actually create a new List<T> and call the the IEnumerable<T> setter with that.

It looks like this is effectively as-casting by checking if the IEnumerable<T> was really initialized with an object implementing ICollection<T> and adding the items to the existing object.

I do not think we should be mutating objects declared to be IEnumerable<T> .

Copy link
Contributor Author

@SteveDunn SteveDunn May 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @halter73 . Thank you for the feedback!

I thought the idea was to actually create a new List and call the the IEnumerable setter with that.

My interpretation of the original issue is that if the type of the property is an IEnumerable<T> and is non-null, and is a mutable collection (i.e. has an Add method, à la ICollection<T>), then it should be set. This is from the issue:

When using the binding feature in the config system, for IEnumerable collections that have a default, non-null value, the collection is never assigned into the target model...

Re. mutating:

I do not think we should be mutating objects declared to be IEnumerable .

Objectively, In this case, we're not mutating the object per se, rather, we're checking the runtime type to see if it's a mutable collection, and using what's already there; the Add method.
But subjectively, this is clearly debatable. From reading the comments on the issue (assuming my interpretation is correct), this has previously been debated.

This is my first contribution to this repo, so I wanted to be sure I was fixing the right thing in the right place. To ensure the right thing aspect, I created a separate stand-alone exe and consumed the new DLL containing my fix. I used the exact DTO and config from the issue itself. But as I say, this is my first contribution, and it is entirely possible that I have misinterpreted the original issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the issue, it's not very clear. I don't want to speak for everyone, but I thought the idea was to have ConfigurationBinder treat settable properties declared to be IEnumerable<T> similarly to T[]. That is to say that if the property already has an initial IEnumerable<T> value, ConfigurationBinder would allocate a new List<T> copy over the items from the initial IEnumerable<T> to the new List<T> and then copy over the config items afterwards.

This would be very similar to BindArray:

runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

Lines 449 to 482 in 0ee10e9

private static Array BindArray(Array source, IConfiguration config, BinderOptions options)
{
IConfigurationSection[] children = config.GetChildren().ToArray();
int arrayLength = source.Length;
Type elementType = source.GetType().GetElementType();
var newArray = Array.CreateInstance(elementType, arrayLength + children.Length);
// binding to array has to preserve already initialized arrays with values
if (arrayLength > 0)
{
Array.Copy(source, newArray, arrayLength);
}
for (int i = 0; i < children.Length; i++)
{
try
{
object item = BindInstance(
type: elementType,
instance: null,
config: children[i],
options: options);
if (item != null)
{
newArray.SetValue(item, arrayLength + i);
}
}
catch
{
}
}
return newArray;
}

I think that's what we agreed on. Is that right @maryamariyan @ericstj @safern @davidfowl?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

System.Text.Json and most of the deserializers out there overwrite the collection on the POCO, so IMHO, I think we should be consistent with what other deserializers do. However we at the moment append to a non-null (for Collections, for Array we overwrite) and we had a feature request to support this: #46988.
So I think that would be a breaking change, but I think it is worth making it?

Maybe we could leave the append behavior behind a flag?

Copy link
Contributor Author

@SteveDunn SteveDunn May 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this comment below relates to issue #46988; that issue and the issue that this PR addresses are intertwined so of course the comment is perfectly apposite:

That is to say that if the property already has an initial IEnumerable value, ConfigurationBinder would allocate a new List copy over the items from the initial IEnumerable to the new List and then copy over the config items afterwards.

I don't think that'd work as we can't assume that a property declared as IEnumerable<T> could be set with a List<T>. Take for example this scenario:

    public class Config {
        private IEnumerable<string> _data = new FunkyList {"item1"};
        
        public IEnumerable<string> Data {
            get => _data;
            set => _data = value;
        }
    }

    class FunkyList : IList<string>  {
        private readonly List<string> _list = new();
        
        public void Add(string item) => _list.Add($"YO {item}");

        // .. other implementing methods
    }

(yes, the setter for Data does allow another collection type, but I'd be miffed if something changed it after I'd explicitly set it, and it could potentially break anything that depended on the behaviour of the funky list)

As mentioned above re. two similar issues: there does appear to be some disparity between them. Paraphrasing from #46988: 'if someone provides a list with some items, we should overwrite those items'. The issue fixed here is 'if someone provides an IEnumerable property which has a runtime type that is a mutable collection, then populate that collection'

These two issues certainly seem to be at odds with each other and seem to be mutually exclusive; we can't fix one without breaking the other.

Just my $0.02: a corollary from the juxtaposition of items #36390 and #46988 is the concept of augmentation. Augmentation goes beyond basic binding of types (IMO). I want a type which contains data which is set at runtime (like in the constructor), but I don't want a straight 'blat whatever I set at runtime with whatever is in cofig', Instead, I'd like to augment with static data from config. #46988 touches on this by mentioning a flag specific to lists, but I don't think it goes far enough. Perhaps we need something like:

public enum AugmentationChoices {
    OverrideListsThatHaveBeenSetByCode,
    OverridePropertiesThatHaveBeenSetByCode,
    AskMeForMyChoiceForEachProperty // user provides a callback with a property name or PropertyInfo
}

Anyway, that was just an idea as the term 'binding' seems rather vague (or more kindly, incomplete) given the complexities and choices that are highlighted by these two issues.

I suppose we are left needing to answer a few questions:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

System.Text.Json and most of the deserializers out there overwrite the collection on the POCO, so IMHO, I think we should be consistent with what other deserializers do. However we at the moment append to a non-null (for Collections, for Array we overwrite) and we had a feature request to support this: #46988.
So I think that would be a breaking change, but I think it is worth making it?

Maybe we could leave the append behavior behind a flag?

Maybe something to cogitate is what this library offers. The mention of deserializers and their mostly common behaviours could hint at a leaky abstraction. From a users perspective, I provide the library some json and a Type and I'd like the library to populate that type with the contents of the json. I think the word populate here is ambiguous though as it could mean 'overwrite' or could mean (as mentioned in my comment above) 'augment'.

From an API design perspective, the entry point is via this:
var val = config.GetSection("config").Get<Config>();

Perhaps, instead of Get we had either DeserializeTo or AugmentTo?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are really valid points. Thanks, @SteveDunn for your thoughts. I think we should focus on fixing the real issue here which is being able to bind to non-null empty collection.

Then if there is real need to change the behavior and to override always, we could reopen: #46988 and build a real scenario to bring it back to discussion with the API Review crew. @halter73 does that make sense? It seems like we keep looping on what we should do, I think we should keep it "simple" now and fix the reported issue that is affecting customers.

Copy link
Member

@halter73 halter73 May 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should focus on fixing the real issue here which is being able to bind to non-null empty collection.

What's the real issue? Does fixing the real issue require mutating an object that's declared to be IEnumerable? I think that's a change for the worse not the better.

I don't see what makes empty settable IEnumerables special, especially when empty arrays and empty ICollections are not. I'm fine taking this back to API review because it is a behavioral change. I thought what I suggested was what we agreed on in API review last time, but that clearly wasn't recorded.

I would argue that my suggested behavioral change is far less surprising and more consistent than the current behavior and could be treated as a simple bug fix. I'd be surprised to find code relying on settable IEnumerables not being set by config. This appears to be mutating non-settable IEnumerables which seems to me much more likely to be breaking.

Copy link
Contributor Author

@SteveDunn SteveDunn May 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the real issue? Does fixing the real issue require mutating an object that's declared to be IEnumerable? I think that's a change for the worse not the better.

The type is IEnumerable, but the object that the user provides is a mutable collection. That distinction is relevant as this is the convention used by binding when it comes across a null enumerable (it mutates it to a list).

I don't see what makes empty settable IEnumerables special, especially when empty arrays and empty ICollections are not.

I think this is a really important observation and really describes the issue! Being new to the party, I was on the fence about this issue and picked it up because it looked fairly easy and would be a nice entry point to what I hope will be many contributions. But by asking asking that question, I now believe that fixing this issue is the correct approach:

Why are empty enumerables special, but empty arrays and collections are not special...
Below are two examples, both, in the current implementation (net5.0): the first one showing that nothing is special and everything is consistent; the second one showing that empty enumerables are special and things are not consistent.
With this config:

{
    "config": {
      "theArray": [ "a", "b" ],
      "theCollection": [ "a", "b" ],
      "theEnumerable": [ "a", "b" ]
    }
}

... and this POCO:

public class Config
{
    public string[] TheArray { get; set; }
    public ICollection<string> TheCollection { get; set; }
    public IEnumerable<string> TheEnumerable { get; set; }
}

.. running this (again, this is net5.0) produces:

System.String[]: 2
a
b

System.Collections.Generic.List`1[System.String]: 2
a
b

System.Collections.Generic.List`1[System.String]: 2
a
b

Empty arrays, empty collections, and empty enumerables are not special. So far so good, we're consistent.

But given this: POCO

public class Config
{
    public string[] TheArray { get; set; } = Array.Empty<string>();
    public ICollection<string> TheCollection { get; set; } = new List<string>();
    public IEnumerable<string> TheEnumerable { get; set; } = new List<string>();
}

... we get:

System.String[]: 2
a
b

System.Collections.Generic.List`1[System.String]: 2
a
b

System.Collections.Generic.List`1[System.String]: 0

Now, empty arrays and collection are not special, but empty enumerables are special.

So, going back to the question, we can now ask it again with additional emphasis:

Why ARE empty enumerables special, but empty arrays and collections are not

Clearly, they shouldn't be, but to answer that question...

In the first run
A convention was applied. That convention turned the enumerable into a list. That's fair enough as it's likely what the user wanted and is pretty safe from the immutability standpoint.

In the second run
The user manually applied that convention (of treating an enumerable as a list). The outcome is still the same: the enumerable property is backed by a list and is represented by an enumerable, and most importantly, it's still safe from the immutability standpoint.

So, if either way, in both example, we have an enumerable backed by a list and the only difference being one is set by convention and one is being set by the user. So the question is now why does functionality differ?

Perhaps if the issue behind this PR was worded something like:

"When manually backing an enumerable as a collection, binding does not populate that collection"

... then it would be more clear-cut.

Copy link
Member

@maryamariyan maryamariyan May 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this issue offline today.

System.Text.Json and most of the deserializers out there overwrite the collection on the POCO

It was also discussed that a big difference between deserializing and configuration binding was there are multiple sources in binder.

There is a lack of clarity because there was not enough information upfront in the issue before setting this as up-for-grabs.

It is best if we close this PR and do it ourselves.

Thanks for your contribution @SteveDunn.

cc: @HaoK


if (collectionInterface != null)
{
BindCollection(instance, collectionInterface, config, options);
}
}
// Something else
else
{
BindNonScalar(config, instance, options);
}
}
}
}
Expand Down
25 changes: 25 additions & 0 deletions src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.IO;
using System.Linq;
using System.Reflection;
using System.Text;
using Xunit;

namespace Microsoft.Extensions.Configuration.Binder.Test
Expand Down Expand Up @@ -47,6 +50,8 @@ public string ReadOnly
{
get { return null; }
}

public IEnumerable<string> InstantiatedCovariantIEnumerable { get; set; } = new List<string>();
}

public class NestedOptions
Expand Down Expand Up @@ -204,6 +209,26 @@ public void CanBindIConfigurationSectionWithDerivedOptionsSection()
Assert.Null(options.Section.Value);
}

[Fact]
public void CanBindInstantiatedCovariantIEnumerable()
{
var dic = new Dictionary<string, string>
{
{"InstantiatedCovariantIEnumerable:0", "Yo1"},
{"InstantiatedCovariantIEnumerable:1", "Yo2"},
};
var configurationBuilder = new ConfigurationBuilder();
configurationBuilder.AddInMemoryCollection(dic);

var config = configurationBuilder.Build();

var options = config.Get<ComplexOptions>();

Assert.Equal(2, options.InstantiatedCovariantIEnumerable.Count());
Assert.Equal("Yo1", options.InstantiatedCovariantIEnumerable.ElementAt(0));
Assert.Equal("Yo2", options.InstantiatedCovariantIEnumerable.ElementAt(1));
}

[Fact]
public void CanBindConfigurationKeyNameAttributes()
{
Expand Down