ConfigurationBinder handles ISet<>

[SteveDunn]

Fixes #66141

[ghost]

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

Issue Details

---

Fixes #66141

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

[tarekgh]

@SteveDunn could you please resolve the file conflicts?

CC @maryamariyan

[SteveDunn]

@SteveDunn could you please resolve the file conflicts?

CC @maryamariyan

Rebased from main and resolved conflicts.

[tarekgh]

LGTM. Thanks @SteveDunn

@maryamariyan do you want to have a quick look?

[halter73]

The ArgumentException when trying to bind to ISet shows there's a more general bug in our collection binding logic. This PR fixes ISet by special-casing ISet before we get to the buggy logic, but we should have never tried to bind to the ISet property using a List in the first place. We should have always been able to recognize our inability to construct a valid implementation of an interface and skip over it when binding.

It might end up that we test for ISet assignability just as this PR already does after fixing the more general bug, but I think there's good chance that rethinking our collection binding logic to avoid assignments that could never work might also change how we test for `ISet.

I would like a fix to FindOpenGenericInterface like I suggested in #66141 (comment) so the SkipsCustomCollection test below also passes.

public interface ICustomCollection<out T> : IEnumerable<T>
{
}
public class MyClassWithCustomCollection
{
    public ICustomCollection<string> CustomCollection { get; set; }
}
[Fact]
public void SkipsCustomCollection()
{
    var configurationBuilder = new ConfigurationBuilder();
    configurationBuilder.AddInMemoryCollection( new Dictionary<string, string>
    {
        ["CustomCollection:0"] = "Yo!",
    });
    var config = configurationBuilder.Build();
    var instance = config.Get<MyClassWithCustomCollection>()!;
    Assert.Null(instance.CustomCollection);
}

[SteveDunn]

Thanks for the suggestion @halter73 . I've fixed the issues mentioned, and the example test case you provided is now included. I also added similar cases for custom dictionaries.

[halter73]

Upon further review I think we should throw for custom interfaces implementing known collection interface, it just shouldn't be an ArgumentException thrown when trying to set a property value, it should be an InvalidOperationException thrown while trying to activate the type like we do for other interfaces.

In my last comment I link to a diff to your PR that has the behavior I want as far as that goes. I didn't go ahead and make all the changes I suggested. For example, we still need to limit sets to strings and enums and write tests for that, but please feel free to cherry-pick these changes.

[SteveDunn]

I added the above test and got it passing, but there was no way for this to remain green whilst all others remained green.
I then noticed some existing tests that asserts that an exception is thrown for custom collections deriving from IEnumerable<>, e.g. a test called ThrowsForCustomIEnumerableCollection:

Do we want to change this behaviour from throwing this exception to just skipping the custom collections (enumerables, sets, dictionaries)?

[SteveDunn]

This is one of a few of my PRs laying dormant. Is there anything I need to do to progress them?

[halter73]

Do we want to change this behaviour from throwing this exception to just skipping the custom collections (enumerables, sets, dictionaries)?

No. It's better to stay consistent and throw from all custom collections. That's why I changed all the SkipCustom... tests to ThrowsForCustom... tests in dotnet:fcddddc...dotnet:dbd9dcd.

[halter73]

Sorry for the slow turnaround getting back to this. It looks really good. Thanks for sticking with this so long!

I still do want to add a test for a collection without a public Add method since we nearly regressed this. I asked for this, but it was tucked away in a longer comment.

We should add a test that binds to an explicitly implemented ICollection<string> to verify we don't regress this in the future.

[eerhardt]

I still do want to add a test for a collection without a public Add method since we nearly regressed this.

@SteveDunn - I think this is the last feedback to address on this PR, and then it can be merged.

[eerhardt]

I guess I had 2 more comments.

[SteveDunn]

I think everything has now been addressed for this. And I also just rebased.

[eerhardt]

I don't think this fits in with the name of the method: IsArrayCompatibleReadOnlyInterface - ICollection<> is not "read-only".

Also, later in the BindInstance method we are checking for ICollection<>, does it need to be in both places?

[SteveDunn]

I think the method is answering the question 'Should the type be treated as immutable?" - e.g. if it's one of these types, then return true to say not to append to the source, but clone it to a new collection.
I've renamed it as such, although I do think it could be clearer. Do you have any thoughts?

[eerhardt]

Why do we need to check for ICollection<> in both places?

[SteveDunn]

I think the second check for ICollection<> is looking for an Add method to determine whether to set (add) members of the collection, or whether to bind to properties (line 358).

Do you have any thoughts on how this could be made clearer? To be perfectly honest, I do find this method rather confusing!

[SteveDunn]

I'll take one last look and then merge.

@SteveDunn could you please resolve the conflicts one last time? Sorry for the delay!

No worries at all, you must be busy (if you need an additional member of the team, I'd love to join!)

[tarekgh]

The failure in Windows x64 Debug run is unrelated and tracked by #72428