Binding to non-null IEnumerable doesn't work
brockallen opened this issue · 24 comments
AB#1277574
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. For example:
For this json:
{
"config": {
"data": [ "a", "b" ]
}
}
and this application code:
public class Config
{
IEnumerable<string> _data = new List<string>();
public IEnumerable<string> Data
{
get
{
return _data;
}
set
{
_data = value;
}
}
}
public class Program
{
public static void Main(string[] args)
{
var config = new ConfigurationBuilder()
.SetBasePath(Directory.GetCurrentDirectory())
.AddJsonFile("test.json")
.Build();
var val = config.GetSection("config").Get<Config>();
Console.WriteLine(val?.Data?.Count());
}
}
The output is:
0
Press any key to continue . . .
I'd expect it to print "2".
I'd argue the issue is in BindInstance and explicit handling for IEnumerable should be made (much like the check for IsArray https://github.com/aspnet/Configuration/blob/dev/src/Microsoft.Extensions.Configuration.Binder/ConfigurationBinder.cs#L281).
I ran into this issue today and came to the same conclusion as @brockallen
We could treat a non-null but empty collection as something that we add to. This way an empty collection would be treated the same as a null collection.
Can you handle the non-null and non empty collection case too?
e.g. IEnumerable _data = new[] { "c" };
The Config data can have default value if it does not exist in the JSON and can be overwritten if it exists in the JSON.
I find this problem when I try to bind the IdentityServer4 Client
For version 1.5.2, the allowedGrantTypes is IEnumerable and has default value (it is changed to ICollection in latest dev version). I try to put the client settings in the JSON file but the AllowedGrantTypes can't be overwritten because it is IEnumerable with default value. It works find if the IEnumberable is null or if it is ICollection
private IEnumerable<string> _allowedGrantTypes = GrantTypes.Implicit;
@ajcvickers - any updates on when this is going to be fixed? Thank you!
@lbogdanov It's currently in the backlog, which means it's not going to be fixed in an upcoming release. Beyond that, it will depend on prioritization against other work.
The correct behaviour is to overwrite. Not append.
This is how other collections behave, and its generally how a json parser behaves regarding default values on a POCO.
For .NET 5.0: https://github.com/dotnet/runtime/blob/master/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs#L327
For .NET Core 3.1.8: https://github.com/dotnet/extensions/blob/v3.1.8/src/Configuration/Config.Binder/src/ConfigurationBinder.cs#L328
@lbogdanov basically this is never going to be fixed because the team doesn't care enough to do it. Instead of fixing bugs they would rather just keep piling on new features, because bugs are boring and don't get blog posts written about them.
Basically, why don't we check if the instance runtime type is some ICollection<T> instead of checking its declared type in https://github.com/dotnet/runtime/blob/master/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs#L334? If it is, then its declared type is either some ICollection<T> or IEnumerable<T>, and binding should be fine.
collectionInterface = FindOpenGenericInterface(typeof(ICollection<>), instance.GetType());
if (collectionInterface != null)
{
BindCollection(instance, collectionInterface, config, options);
}I've checked this on both empty and not empty initialized IEnumerable<T>s and it seems to work, all tests are also passed. If that's okay, I'd like to open PR with this change.
It seems reasonable to treat an empty enumerable in the same way as null. I'm less inclined to examine the runtime type unless we have some precedent for that. Additionally, it doesn't fix the cases where folks backed the property with Enumerable.Empty or an Array. cc @eerhardt @maryamariyan
I'd like to look at this if it's still 'up-for-grabs'?
I believe it's still up for grabs, and something that we'd like to fix for 6.0.
Thanks, @SteveDunn, I've assigned you the issue. Let me know how I can help.
@safern thank you for your kind offer of help - I will take you up on that; any idea re. this? https://discord.com/channels/732297728826277939/732297728826277942/839396094618238996
This is my first PR on this repo and I'm stumbling around like a newborn Giraffe!
This is my first PR on this repo and I'm stumbling around like a newborn Giraffe!
Not a problem. I've pinged you in discord, also feel free to post your questions here if you rather keep them in the open.
@maryamariyan I'm going through 1st party asks. From what I can see, this is still open, we're still interested in doing this, but it can't fit this release any more?
If so please change milestone. Or close if appropriate.
For triage, will discuss with @halter73 on next steps for this.
I have the same problem, and use this workaround to avoid analyzer complaint:
public class Config
{
public IEnumerable<string> Data { get; set; } = null!;
}But I agree to solve this. No one would understand to make it work without complain from analyzer.
I have the same problem, and use this workaround to avoid analyzer complaint:
public class Config { public IEnumerable<string> Data { get; set; } = null!; }But I agree to solve this. No one would understand to make it work without complain from analyzer.
@maryamariyan - do you know if there's been any more thought on this one?
Here's a summary of where we're at with this issue:
When we set this issue up for grabs the first time, the desired fix was not clearly defined.
As part of closing the original PR we uncovered that there is a subtle difference between configuration binding and System.Text.Json deserialization. Because with configuration we have to append data from multiple sources but deserialization has a single source of truth and that is why we don't think of configuration binding as a deserializer. (In a way for the configuration case we are mapping what is already been de-serialized to a POCO.)
This comment covers where we left off from the original PR: #52514 (comment)
We decided since then that the proper fix here for this issue would be very similar to BindArray:
@SteveDunn we could set it up for grabs if you're open to making the above fix. Thanks again for staying engaged on this.
/cc @halter73 @davidfowl
Hi @maryamariyan . Yes very happy to have another crack at this one. Will be great to get it done!
closed via @66131