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
merged 9 commits into from
Jan 26, 2023
Merged

Fix Configuration Binding with Instantiated Objects #81050

Show file tree
Hide file tree
Changes from all commits
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
99 changes: 58 additions & 41 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 @@ -315,12 +315,15 @@ 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

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

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

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)
if (TypeIsASetInterface(type))
{
object? newValue = BindSet(type, (IEnumerable?)bindingPoint.Value, config, options);
if (newValue != null)
if (!bindingPoint.IsReadOnly || bindingPoint.Value is not null)
{
bindingPoint.SetValue(newValue);
object? newValue = BindSet(type, (IEnumerable?)bindingPoint.Value, config, options);
if (!bindingPoint.IsReadOnly && newValue != null)
{
bindingPoint.SetValue(newValue);
}
}

return;
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't the if (TypeIsADictionaryInterface(type) section below symmetrical with the above if (TypeIsASetInterface(type)? It feels wrong that these two checks are different.

Expand Down Expand Up @@ -530,33 +533,41 @@ 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];

if (orig != null)
// addMethod can only be null if dictionaryType is IReadOnlyDictionary<TKey, TValue> rather than IDictionary<TKey, TValue>.
MethodInfo? addMethod = dictionaryType.GetMethod("Add", DeclaredOnlyLookup);
if (addMethod is null || source is null)
{
foreach (object? item in orig)
dictionaryType = typeof(Dictionary<,>).MakeGenericType(keyType, valueType);
var dictionary = Activator.CreateInstance(dictionaryType);
Copy link
Member

Choose a reason for hiding this comment

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

object? dictionary = Activator.CreateInstance(dictionaryType);

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, make sense.

addMethod = dictionaryType.GetMethod("Add", DeclaredOnlyLookup);

var orig = source as IEnumerable;
if (orig is not null)
{
object? k = keyMethod.GetMethod!.Invoke(item, null);
object? v = valueMethod.GetMethod!.Invoke(item, null);
arguments[0] = k;
arguments[1] = v;
addMethod.Invoke(dictionary, arguments);
Type kvpType = typeof(KeyValuePair<,>).MakeGenericType(keyType, valueType);
PropertyInfo keyMethod = kvpType.GetProperty("Key", DeclaredOnlyLookup)!;
PropertyInfo valueMethod = kvpType.GetProperty("Value", DeclaredOnlyLookup)!;
object?[] arguments = new object?[2];

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);
}
}

source = dictionary;
}

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 @@ -737,32 +748,38 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co
{
Type elementType = type.GetGenericArguments()[0];

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

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

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];

if (source != null)
// addMethod can only be null if type is IReadOnlySet<T> rather than ISet<T>.
MethodInfo? addMethod = type.GetMethod("Add", DeclaredOnlyLookup);
if (addMethod is null || source is null)
{
foreach (object? item in source)
Type genericType = typeof(HashSet<>).MakeGenericType(elementType);
object instance = Activator.CreateInstance(genericType)!;
addMethod = genericType.GetMethod("Add", DeclaredOnlyLookup);

if (source != null)
{
arguments[0] = item;
addMethod.Invoke(instance, arguments);
foreach (object? item in source)
{
arguments[0] = item;
addMethod!.Invoke(instance, arguments);
}
}

source = (IEnumerable)instance;
}

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

foreach (IConfigurationSection section in config.GetChildren())
{
var itemBindingPoint = new BindingPoint();
Expand All @@ -777,7 +794,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 +807,7 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co
}
}

return instance;
return source;
}

[RequiresUnreferencedCode(TrimmingWarningMessage)]
Expand Down
6 changes: 3 additions & 3 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 @@ -812,7 +812,7 @@ public void CanBindISetNoSetter()

var config = configurationBuilder.Build();

var options = config.Get<ComplexOptions>()!;
var options = config.Get<ComplexOptions>(o => o.ErrorOnUnknownConfiguration = true)!;
Copy link
Member

Choose a reason for hiding this comment

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

What was this change for?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added this to get the exception thrown instead of skipping the configuration. Just to help in the case of the test failure.


Assert.Equal(2, options.ISetNoSetter.Count);
Assert.Equal("Yo1", options.ISetNoSetter.ElementAt(0));
Expand Down Expand Up @@ -937,8 +937,6 @@ public void CanBindInstantiatedReadOnlyDictionary2()
Assert.Equal(2, options.Items["existing-item2"]);
Assert.Equal(3, options.Items["item3"]);
Assert.Equal(4, options.Items["item4"]);


}

[Fact]
Expand All @@ -956,6 +954,7 @@ public void BindInstantiatedIReadOnlyDictionary_CreatesCopyOfOriginal()

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

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

// does not overwrite original
Expand Down Expand Up @@ -1027,6 +1026,7 @@ public void CanBindInstantiatedReadOnlyDictionary()
var options = config.Get<ComplexOptions>()!;

var resultingDictionary = options.InstantiatedReadOnlyDictionaryWithWithSomeValues;

Assert.Equal(4, resultingDictionary.Count);
Assert.Equal(1, resultingDictionary["existing-item1"]);
Assert.Equal(2, resultingDictionary["existing-item2"]);
Expand Down
Loading