Skip to content

Fix Configuration Binding with Instantiated Objects #81050

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

Merged
tarekgh merged 9 commits into from
Jan 26, 2023
Merged

Fix Configuration Binding with Instantiated Objects #81050

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8d7d920
Fix Configuration Binding with Instantiated Objects
tarekgh Jan 23, 2023
1493935
Add Extra test
tarekgh Jan 23, 2023
ea81eba
Fix test build break
tarekgh Jan 23, 2023
a7761d5
Scoping the fix to settable collections only
tarekgh Jan 24, 2023
36c25ca
Address the feedback
tarekgh Jan 24, 2023
438a77c
Add log messages to the test to get more info about CI failure
tarekgh Jan 25, 2023
b8c73f9
Add logs to test
tarekgh Jan 25, 2023
4f7a43c
Exclude ISET<T> from the test trimming
tarekgh Jan 25, 2023
c047d04
Fix the trimming file to point at correct assembly
tarekgh Jan 26, 2023
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
Next Next commit
Fix Configuration Binding with Instantiated Objects
  • Loading branch information
@tarekgh
tarekgh committed Jan 23, 2023
commit 8d7d92042964eef26a918d9607866dca306b4ca8
86 changes: 41 additions & 45 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 @@ -302,10 +302,12 @@ private static void BindInstance(

if (config != null && config.GetChildren().Any())
{
// for arrays and read-only list-like interfaces, we concatenate on to what is already there, if we can
if (type.IsArray || IsImmutableArrayCompatibleInterface(type))
// for arrays we concatenate on to what is already there, if we can
// for read-only list-like interfaces, we do the same only if the bindingPoint.Value is null.
bool isImmutableArrayCompatibleInterface = IsImmutableArrayCompatibleInterface(type);
if (type.IsArray || isImmutableArrayCompatibleInterface)
{
if (!bindingPoint.IsReadOnly)
if (!bindingPoint.IsReadOnly && (!isImmutableArrayCompatibleInterface || bindingPoint.Value is null))
{
bindingPoint.SetValue(BindArray(type, (IEnumerable?)bindingPoint.Value, config, options));
}
Expand All @@ -317,7 +319,7 @@ private static void BindInstance(
// for sets and read-only set interfaces, we clone what's there into a new collection, if we can
Copy link
Member

@eerhardt eerhardt Jan 30, 2023

Choose a reason for hiding this comment

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

Is this comment now incorrect?

Copy link
Member Author

@tarekgh tarekgh Jan 30, 2023

Choose a reason for hiding this comment

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

We need to remove the word sets from the comment.

Copy link
Member

@eerhardt eerhardt Jan 30, 2023

Choose a reason for hiding this comment

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

I think the comment needs to be beefed up a bit to describe the whole behavior.

if (TypeIsASetInterface(type) && !bindingPoint.IsReadOnly)
{
object? newValue = BindSet(type, (IEnumerable?)bindingPoint.Value, config, options);
object? newValue = BindSet(type, bindingPoint.Value, config, options);
if (newValue != null)
{
bindingPoint.SetValue(newValue);
Expand Down Expand Up @@ -530,33 +532,26 @@ private static bool CanBindToTheseConstructorParameters(ParameterInfo[] construc
return null;
}

Type genericType = typeof(Dictionary<,>).MakeGenericType(keyType, valueType);
MethodInfo addMethod = genericType.GetMethod("Add", DeclaredOnlyLookup)!;

Type kvpType = typeof(KeyValuePair<,>).MakeGenericType(keyType, valueType);
PropertyInfo keyMethod = kvpType.GetProperty("Key", DeclaredOnlyLookup)!;
PropertyInfo valueMethod = kvpType.GetProperty("Value", DeclaredOnlyLookup)!;

object dictionary = Activator.CreateInstance(genericType)!;

var orig = source as IEnumerable;
object?[] arguments = new object?[2];
MethodInfo? addMethod = dictionaryType.GetMethod("Add", DeclaredOnlyLookup);
if (addMethod is null && source is not null)
{
// Instantiated IReadOnlyDictionary cannot be mutated by the configuration.
return null;
}

if (orig != null)
if (source is null)
{
foreach (object? item in orig)
{
object? k = keyMethod.GetMethod!.Invoke(item, null);
object? v = valueMethod.GetMethod!.Invoke(item, null);
arguments[0] = k;
arguments[1] = v;
addMethod.Invoke(dictionary, arguments);
}
dictionaryType = typeof(Dictionary<,>).MakeGenericType(keyType, valueType);
source = Activator.CreateInstance(dictionaryType);
addMethod ??= dictionaryType.GetMethod("Add", DeclaredOnlyLookup)!;
}

BindDictionary(dictionary, genericType, config, options);
Debug.Assert(source is not null);
Debug.Assert(addMethod is not null);

BindDictionary(source, dictionaryType, config, options);

return dictionary;
return source;
}

// Binds and potentially overwrites a dictionary object.
Expand Down Expand Up @@ -733,36 +728,37 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co

[RequiresDynamicCode(DynamicCodeWarningMessage)]
[RequiresUnreferencedCode("Cannot statically analyze what the element type is of the Array so its members may be trimmed.")]
private static object? BindSet(Type type, IEnumerable? source, IConfiguration config, BinderOptions options)
private static object? BindSet(Type type, object? source, IConfiguration config, BinderOptions options)
{
Type elementType = type.GetGenericArguments()[0];

Type keyType = type.GenericTypeArguments[0];
bool keyTypeIsEnum = elementType.IsEnum;
Copy link
Member

@eerhardt eerhardt Jan 30, 2023

Choose a reason for hiding this comment

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

keyType is no longer used. Should this be changed to:

bool elementTypeIsEnum = elementType.IsEnum;

Copy link
Member Author

@tarekgh tarekgh Jan 30, 2023

Choose a reason for hiding this comment

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

yes.


bool keyTypeIsEnum = keyType.IsEnum;

if (keyType != typeof(string) && !keyTypeIsEnum)
if (elementType != typeof(string) && !keyTypeIsEnum)
{
// We only support string and enum keys
return null;
}

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

MethodInfo addMethod = genericType.GetMethod("Add", DeclaredOnlyLookup)!;

object?[] arguments = new object?[1];
MethodInfo? addMethod = type.GetMethod("Add", DeclaredOnlyLookup);
if (addMethod is null && source is not null)
{
// It is readonly Set interface. We bind the readonly interfaces only when its value is not instantiated.
return null;
}

if (source != null)
if (source is null)
{
foreach (object? item in source)
{
arguments[0] = item;
addMethod.Invoke(instance, arguments);
}
Type genericType = typeof(HashSet<>).MakeGenericType(elementType);
source = Activator.CreateInstance(genericType)!;
addMethod ??= genericType.GetMethod("Add", DeclaredOnlyLookup)!;
}

Debug.Assert(source is not null);
Debug.Assert(addMethod is not null);

object?[] arguments = new object?[1];

foreach (IConfigurationSection section in config.GetChildren())
{
var itemBindingPoint = new BindingPoint();
Expand All @@ -777,7 +773,7 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co
{
arguments[0] = itemBindingPoint.Value;

addMethod.Invoke(instance, arguments);
addMethod.Invoke(source, arguments);
}
}
catch (Exception ex)
Expand All @@ -790,7 +786,7 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co
}
}

return instance;
return source;
}

[RequiresUnreferencedCode(TrimmingWarningMessage)]
Expand Down
45 changes: 19 additions & 26 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 @@ -835,9 +835,8 @@ public void CanBindInstantiatedIReadOnlySet()

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

Assert.Equal(2, options.InstantiatedIReadOnlySet.Count);
Assert.Equal("Yo1", options.InstantiatedIReadOnlySet.ElementAt(0));
Assert.Equal("Yo2", options.InstantiatedIReadOnlySet.ElementAt(1));
// Instantiated IReadOnlySet cannot get mutated.
Assert.Equal(0, options.InstantiatedIReadOnlySet.Count);
}

[Fact]
Expand All @@ -855,11 +854,10 @@ public void CanBindInstantiatedIReadOnlyWithSomeValues()

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

Assert.Equal(4, options.InstantiatedIReadOnlySetWithSomeValues.Count);
// Instantiated IReadOnlySet doesn't get mutated.
Assert.Equal(2, options.InstantiatedIReadOnlySetWithSomeValues.Count);
Assert.Equal("existing1", options.InstantiatedIReadOnlySetWithSomeValues.ElementAt(0));
Assert.Equal("existing2", options.InstantiatedIReadOnlySetWithSomeValues.ElementAt(1));
Assert.Equal("Yo1", options.InstantiatedIReadOnlySetWithSomeValues.ElementAt(2));
Assert.Equal("Yo2", options.InstantiatedIReadOnlySetWithSomeValues.ElementAt(3));
}

[Fact]
Expand Down Expand Up @@ -932,13 +930,10 @@ public void CanBindInstantiatedReadOnlyDictionary2()

var options = config.Get<Foo>()!;

Assert.Equal(4, options.Items.Count);
// Readonly dictionary with instantiated value cannot get mutated by the configuration.
Assert.Equal(2, options.Items.Count);
Assert.Equal(1, options.Items["existing-item1"]);
Assert.Equal(2, options.Items["existing-item2"]);
Assert.Equal(3, options.Items["item3"]);
Assert.Equal(4, options.Items["item4"]);


}

[Fact]
Expand All @@ -956,14 +951,14 @@ public void BindInstantiatedIReadOnlyDictionary_CreatesCopyOfOriginal()

var options = config.Get<ConfigWithInstantiatedIReadOnlyDictionary>()!;

Assert.Equal(3, options.Dictionary.Count);
// Readonly dictionary with instantiated value cannot be mutated by the configuration
Assert.Equal(2, options.Dictionary.Count);

// does not overwrite original
Assert.Equal(1, ConfigWithInstantiatedIReadOnlyDictionary._existingDictionary["existing-item1"]);

Assert.Equal(666, options.Dictionary["existing-item1"]);
Assert.Equal(1, options.Dictionary["existing-item1"]);
Assert.Equal(2, options.Dictionary["existing-item2"]);
Assert.Equal(3, options.Dictionary["item3"]);
}

[Fact]
Expand Down Expand Up @@ -1027,11 +1022,11 @@ public void CanBindInstantiatedReadOnlyDictionary()
var options = config.Get<ComplexOptions>()!;

var resultingDictionary = options.InstantiatedReadOnlyDictionaryWithWithSomeValues;
Assert.Equal(4, resultingDictionary.Count);

// Readonly dictionary with instantiated value cannot be mutated by the configuration.
Assert.Equal(2, resultingDictionary.Count);
Assert.Equal(1, resultingDictionary["existing-item1"]);
Assert.Equal(2, resultingDictionary["existing-item2"]);
Assert.Equal(3, resultingDictionary["item3"]);
Assert.Equal(4, resultingDictionary["item4"]);
}

[Fact]
Expand Down Expand Up @@ -1376,9 +1371,8 @@ public void CanBindInstantiatedIEnumerableWithItems()

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

Assert.Equal(2, options.InstantiatedIEnumerable.Count());
Assert.Equal("Yo1", options.InstantiatedIEnumerable.ElementAt(0));
Assert.Equal("Yo2", options.InstantiatedIEnumerable.ElementAt(1));
// Instantiated readonly IEnumerable which cannot be mutated by the configuration
Assert.Equal(0, options.InstantiatedIEnumerable.Count());
}

[Fact]
Expand Down Expand Up @@ -1456,9 +1450,9 @@ public void CanBindInstantiatedIReadOnlyCollectionWithItems()

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

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

// Instantiated readonly collection which cannot be mutated by the configuration
Assert.Equal(0, options.InstantiatedIReadOnlyCollection.Count);
}

[Fact]
Expand All @@ -1477,9 +1471,8 @@ public void CanBindInstantiatedIEnumerableWithNullItems()

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

Assert.Equal(2, options.InstantiatedIEnumerable.Count());
Assert.Equal("Yo1", options.InstantiatedIEnumerable.ElementAt(0));
Assert.Equal("Yo2", options.InstantiatedIEnumerable.ElementAt(1));
// Instantiated IEnumerable is a readonly collection which cannot be mutated by the configuration
Assert.Equal(0, options.InstantiatedIEnumerable.Count());
}

[Fact]
Expand Down
Loading