Microsoft.Extensions.Configuration App binding adding to the initialized Collections, not replacing

[Cazzar]

Background and Motivation

When defining a configuration that has some defaulted options that are a List<T> or an array type we might want to have the ability to provide a default list, but completely replaces the enumerable's contents with what is in the configuration file.

Proposed API

namespace Microsoft.Extensions.Configuration
{
    public class BinderOptions
    {
        public bool BindNonPublicProperties { get; set; }
+        public bool OverrideLists { get; set; }
    }
}

Usage Examples

Expanding upon my original repo as a base

var config = new ConfigurationBuilder().AddNewtonsoftJsonStream(new MemoryStream(Encoding.ASCII.GetBytes("{ \"Example\": [ \"Item 2\" ] }"))).Build();
var obj = config.Get<ConfigType>(options => options.OverrideLists = true);

Alternative Designs

This gives a similar action to Newtonsoft.JSON's ObjectCreationHandling option with the value of Replace which when handling an Object when deserializing to a class, it uses a new class with no contents at all.

---

Original issue

Description

Example Repo: https://github.com/Cazzar/ConfigurationArrayTest
To show the issue itself, simply running dotnet run should work

The issue itself seems to occur that if you have an initialization for an IEnumerable instead of having the option to Replace the contents on deserialization, it will append to the contents, making it impossible to use a strongly typed configuration instance with defaults in the case that they are not provided.

Testing this, it seems to show both a pattern within both System.Text.Json and the Newtonsoft.Json versions of configuration loading.

Configuration

• Windows 10 Build 19042.685 NET 5.0.1 x64
• Doesn't seem to be configuration specific.

Other information

Example output from my repo above:

Newtonsoft.JSON
Item Count: 2
Items in array: Item 1, Item 2

System.Net.Json
Item Count: 2
Items in array: Item 1, Item 2

In this case, I would expect only Item 2 to exist in the arrays, effectively overwriting the bound object with the contents of the configuration.

From further speculation, this might not be specifically bound to the input type but by the Binding options not having the option to replace
An initial thought would be for this using Add without clearing the collection first:

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

Lines 423 to 447 in 796cbee

	private static void BindCollection(object collection, Type collectionType, IConfiguration config, BinderOptions options)
	{
	// ICollection<T> is guaranteed to have exactly one parameter
	Type itemType = collectionType.GenericTypeArguments[0];
	MethodInfo addMethod = collectionType.GetMethod("Add", DeclaredOnlyLookup);
	foreach (IConfigurationSection section in config.GetChildren())
	{
	try
	{
	object item = BindInstance(
	type: itemType,
	instance: null,
	config: section,
	options: options);
	if (item != null)
	{
	addMethod.Invoke(collection, new[] { item });
	}
	}
	catch
	{
	}
	}
	}

[ghost]

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

Issue Details

---

Description

Example Repo: https://github.com/Cazzar/ConfigurationArrayTest
To show the issue itself, simply running dotnet run should work

The issue itself seems to occur that if you have an initialization for an IEnumerable instead of having the option to Replace the contents on deserialization, it will append to the contents, making it impossible to use a strongly typed configuration instance with defaults in the case that they are not provided.

Testing this, it seems to show both a pattern within both System.Net.Json and the Newtonsoft.Json versions of configuration loading.

Configuration

• Windows 10 Build 19042.685
• NET 5.0.1
• x64
• Doesn't seem to be configuration specific.

Other information

Example output from my repo above:

Newtonsoft.JSON
Item Count: 2
Items in array: Item 1, Item 2

System.Net.Json
Item Count: 2
Items in array: Item 1, Item 2

In this case, I would expect only Item 2 to exist in the arrays, effectively overwriting the bound object with the contents of the configuration.

From further speculation, this might not be specifically bound to the input type but by the Binding options not having the option to replace
An initial thought would be for this using Add without clearing the collection first:

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

Lines 423 to 447 in 796cbee

	private static void BindCollection(object collection, Type collectionType, IConfiguration config, BinderOptions options)
	{
	// ICollection<T> is guaranteed to have exactly one parameter
	Type itemType = collectionType.GenericTypeArguments[0];
	MethodInfo addMethod = collectionType.GetMethod("Add", DeclaredOnlyLookup);
	foreach (IConfigurationSection section in config.GetChildren())
	{
	try
	{
	object item = BindInstance(
	type: itemType,
	instance: null,
	config: section,
	options: options);
	if (item != null)
	{
	addMethod.Invoke(collection, new[] { item });
	}
	}
	catch
	{
	}
	}
	}

Author:	Cazzar
Assignees:	-
Labels:	area-System.Configuration, untriaged
Milestone:	-

[CodeBlanch]

@Cazzar Funny I just ran into this too. It seems to be working as designed given there are unit tests for the behavior, for example:

runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs

Lines 305 to 332 in e3247ad

	[Fact]
	public void AlreadyInitializedListBinding()
	{
	var input = new Dictionary<string, string>
	{
	{"AlreadyInitializedList:0", "val0"},
	{"AlreadyInitializedList:1", "val1"},
	{"AlreadyInitializedList:2", "val2"},
	{"AlreadyInitializedList:x", "valx"}
	};
	var configurationBuilder = new ConfigurationBuilder();
	configurationBuilder.AddInMemoryCollection(input);
	var config = configurationBuilder.Build();
	var options = new OptionsWithLists();
	config.Bind(options);
	var list = options.AlreadyInitializedList;
	Assert.Equal(5, list.Count);
	Assert.Equal("This was here before", list[0]);
	Assert.Equal("val0", list[1]);
	Assert.Equal("val1", list[2]);
	Assert.Equal("val2", list[3]);
	Assert.Equal("valx", list[4]);
	}

What I ran into is in the case of IReadOnlyCollection...

        public class Test
        {
            public IReadOnlyCollection<string> ReadOnlyValues { get; set; } = new[] { "default" };
        }

...configuration is completely ignored because you can't add to an existing IReadOnlyCollection.

My expectation was that "ReadOnlyValues" would be completely replaced if something was specified in the configuration.

[Cazzar]

@CodeBlanch Interesting, even if it was a binding option that might be a reasonable solution, since if you were to use raw JSON parsing, it is possible, at least with Newtonsoft.JSON that you can replace instead of update.

I've got a potential workaround idea that I will try later today and update here for the solution.

Given that it is an expected pattern, breaking the current API would not be a good idea, I wouldn't be against some sort of binding options.

[ghost]

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

Issue Details

---

Description

Example Repo: https://github.com/Cazzar/ConfigurationArrayTest
To show the issue itself, simply running dotnet run should work

The issue itself seems to occur that if you have an initialization for an IEnumerable instead of having the option to Replace the contents on deserialization, it will append to the contents, making it impossible to use a strongly typed configuration instance with defaults in the case that they are not provided.

Testing this, it seems to show both a pattern within both System.Net.Json and the Newtonsoft.Json versions of configuration loading.

Configuration

• Windows 10 Build 19042.685
• NET 5.0.1
• x64
• Doesn't seem to be configuration specific.

Other information

Example output from my repo above:

Newtonsoft.JSON
Item Count: 2
Items in array: Item 1, Item 2

System.Net.Json
Item Count: 2
Items in array: Item 1, Item 2

In this case, I would expect only Item 2 to exist in the arrays, effectively overwriting the bound object with the contents of the configuration.

From further speculation, this might not be specifically bound to the input type but by the Binding options not having the option to replace
An initial thought would be for this using Add without clearing the collection first:

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

Lines 423 to 447 in 796cbee

	private static void BindCollection(object collection, Type collectionType, IConfiguration config, BinderOptions options)
	{
	// ICollection<T> is guaranteed to have exactly one parameter
	Type itemType = collectionType.GenericTypeArguments[0];
	MethodInfo addMethod = collectionType.GetMethod("Add", DeclaredOnlyLookup);
	foreach (IConfigurationSection section in config.GetChildren())
	{
	try
	{
	object item = BindInstance(
	type: itemType,
	instance: null,
	config: section,
	options: options);
	if (item != null)
	{
	addMethod.Invoke(collection, new[] { item });
	}
	}
	catch
	{
	}
	}
	}

Author:	Cazzar
Assignees:	-
Labels:	area-Extensions-Configuration, area-System.Configuration, untriaged
Milestone:	-

[safern]

@Cazzar this definitely looks like by design and it would be a breaking change to "fix" this. What I might suggest if we wanted to support this scenario would be adding an option to BindingOptions that is something like: OverrideLists or something like that, that would trigger the behavior you're suggesting.

Anyway, were you able to find a workaround for this? If not I can help finding one. I think we might be able to close this issue?

[Cazzar]

@safern I, unfortunately, couldn't find something that worked well enough for my use-case (though it may also relate to the need of the configuration being serialised back in place which was a dirty bit of code, to begin with). Since I was using it with IOptions, I had a fair bit to try including late-initializing the properties and ended up moving to the previous setup we had internally.

Given it does look like intended functionality as I mentioned in #46988 (comment) an option within BindingOptions would probably be a good idea in the long run.

[safern]

Given it does look like intended functionality as I mentioned in #46988 (comment) an option within BindingOptions would probably be a good idea in the long run.

Would you mind submitting a formal API proposal to add that BindingOption as part of this or a new issue?

Here are the instructions on how to do that: https://github.com/dotnet/runtime/blob/master/docs/project/api-review-process.md

[Cazzar]

Because I would have just referenced this issue, I have updated the above description based off the process @safern

[safern]

Thank you, @Cazzar. Based on the proposed API, would it be reasonable that the flag is named something like: UseDefaultLists or something that has to be set to true explicitly to trigger this behavior? Currently overriding the list is the default behavior, so I would like a flag that is explicit about turning on a new feature vs turning off an old behavior, does that make sense?

[CodeBlanch]

@safern The behavior right now is to add to the list, if possible, otherwise it does nothing (the readonly case). The behavior we want to enable is to override whatever is there.

[safern]

@safern The behavior right now is to add to the list, if possible, otherwise it does nothing (the readonly case). The behavior we want to enable is to override whatever is there.

Sorry I mixed up things. You're right. The proposal looks good.

[terrajobst]

• How important is this?
  • This is getting into the realm of the complicated serialization behavior.
  • It seems default values in collections are a bit of fringe behavior.
  • Let's close it for now until there is more evidence that we need this.
• General feedback:
  • It seems a bit weird to ask developers to make their collections settable; it would be more natural if the binder would call a Clear() method.
  • If the collection implements both IList and is settable, preferring the setter seems reasonable.

namespace Microsoft.Extensions.Configuration
{
    public partial class BinderOptions
    {
        // Already exists:
        // public bool BindNonPublicProperties { get; set; }
        public bool OverrideLists { get; set; }
    }
}