ConfigurationBinder doesn't honor pre-existing ISet when binding

[ericstj]

Description

When binding to a property that exposes an ISet with a setter and initializes the property with some collection, that collection is ignored and instead a new HashTable is created. This could change the comparator or otherwise result in different behavior than the pre-allocated collection.

Reproduction Steps

using Configuration.Binder.Working;
using Microsoft.Extensions.Configuration;


IConfiguration config = new ConfigurationBuilder()
 .AddInMemoryCollection()
 .Build();

config["MySet:0"] = "HelloWorld";
TestObject obj = config.Get<TestObject>()!;

Console.WriteLine($"Test object contains helloworld: {obj.MySet.Contains("helloworld")}");

// workaround
obj.MySet = new HashSet<string>(obj.MySet, StringComparer.OrdinalIgnoreCase);
Console.WriteLine($"Test object contains helloworld: {obj.MySet.Contains("helloworld")}");

public class TestObject
{
 public ISet<string> MySet { get; set; } = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
}

Expected behavior

This should succeed, using the user-initialized collection with case insensitive comparer.

Actual behavior

The first call fails because ConfigurationBinder recreated the ISet<string> as a new HashSet<string>() using case-sensitive comparer.

Regression?

Yes, from 6.0.0 due to this PR #68133

Known Workarounds

One can recreate the collection after binding.

obj.MySet = new HashSet<string>(obj.MySet, StringComparer.OrdinalIgnoreCase);

Or hide the setter, to prevent ConfigurationBinder from resetting the instance.

Configuration

No response

Other information

No response

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

When binding to a property that exposes an ISet with a setter and initializes the property with some collection, that collection is ignored and instead a new HashTable is created. This could change the comparator or otherwise result in different behavior than the pre-allocated collection.

Reproduction Steps

using Configuration.Binder.Working;
using Microsoft.Extensions.Configuration;


IConfiguration config = new ConfigurationBuilder()
 .AddInMemoryCollection()
 .Build();

config["MySet:0"] = "HelloWorld";
TestObject obj = config.Get<TestObject>()!;

Console.WriteLine($"Test object contains helloworld: {obj.MySet.Contains("helloworld")}");

// workaround
obj.MySet = new HashSet<string>(obj.MySet, StringComparer.OrdinalIgnoreCase);
Console.WriteLine($"Test object contains helloworld: {obj.MySet.Contains("helloworld")}");

public class TestObject
{
 public ISet<string> MySet { get; set; } = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
}

Expected behavior

This should succeed, using the user-initialized collection with case insensitive comparer.

Actual behavior

The first call fails because ConfigurationBinder recreated the ISet<string> as a new HashSet<string>() using case-sensitive comparer.

Regression?

Yes, from 6.0.0 due to this PR #68133

Known Workarounds

One can recreate the collection after binding.

obj.MySet = new HashSet<string>(obj.MySet, StringComparer.OrdinalIgnoreCase);

Or hide the setter, to prevent ConfigurationBinder from resetting the instance.

Configuration

No response

Other information

No response

Author:	ericstj
Assignees:	-
Labels:	area-Extensions-Configuration
Milestone:	-

[layomia]

Thanks for the repro. Since this a regression we should address sooner than later.

[ericstj]

Thank you to @michasHL reported this.

we should address sooner than later.

Yes, for servicing in 7.0 if it needs to be fixed. FWIW we've already shipped two fixes in 7.0.1 and 7.0.2 that were regressions caused by this same change. We need to be extra careful in churning this to be certain we don't regress further.

The problem here appears to be that BindSet doesn't honor use an existing instance:

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

Lines 747 to 748 in 97a51cc

	Type genericType = typeof(HashSet<>).MakeGenericType(keyType);
	object instance = Activator.CreateInstance(genericType)!;

We should look at how other types are handled to find precedent. I would expect that this should be very similar to the dictionary case as well. We should make sure that collections are handled consistently with respect to the when existing instances are honored vs new instances are created.

edit: Actually it looks like IDictionary is broken in the same way by this change. The binder will also replace a dictionary instance with a new (different) instance if the setter is exposed in 7.0 when it didn't in 6.0.

[tarekgh]

I'll look at this when I am back. @ericstj how did you find it? :-)

[tarekgh]

CC @SteveDunn @eerhardt

[SteveDunn]

I'm happy to take a look over Christmas. There's certainly a few edge cases here. I do remember a discussion point around whether we should mutate the original collection or recreate it in a new collection and mutate that new collection with the items in config.
The equality comparer is a good example of a 'customisation' that is relatively simple (and understood), but I think it'd be extremely difficult to recreate any user provided custom collections with multiple constructors.

[eerhardt]

Why do we need to create a new instance in this case? Why can't we just add the configuration values to the existing collection?

[tarekgh]

@eerhardt that is what I was going to look at. I'll try to look and ensure we'll get the .NET 6.0 behavior as possible in such case.

[SteveDunn]

Apologies, I didn't get a chance to look at this. If it's not picked up by the weekend, I'll definitely get around to it then.

[michasHL]

@SteveDunn did you get a chance to take a look at this issue?
We ran into this again after continuing our rollout of .NET 7.