Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 5.6k
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.
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
8d7d9201493935ea81ebaa7761d536c25ca438a77cb8c73f94f7a43cc047d04File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| 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; | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why isn't the | ||
| @@ -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); | ||
tarekgh marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| if (addMethod is null || source is null) | ||
| { | ||
| foreach (object? item in orig) | ||
| dictionaryType = typeof(Dictionary<,>).MakeGenericType(keyType, valueType); | ||
| var dictionary = Activator.CreateInstance(dictionaryType); | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. object?dictionary=Activator.CreateInstance(dictionaryType);MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
| @@ -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; | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
boolelementTypeIsEnum=elementType.IsEnum;MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
tarekgh marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| 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(); | ||
| @@ -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) | ||
| @@ -790,7 +807,7 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co | ||
| } | ||
| } | ||
| return instance; | ||
| return source; | ||
| } | ||
| [RequiresUnreferencedCode(TrimmingWarningMessage)] | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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)!; | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What was this change for? MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
| @@ -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] | ||
| @@ -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 | ||
| @@ -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"]); | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
setsfrom the comment.There was a problem hiding this comment.
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.