From 96ef7439d388b69a4d3f723dbce74fc5bbc916b4 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Mon, 28 Nov 2022 19:28:22 -0800 Subject: [PATCH 1/4] Fix configuration binding with types implementing IDictionary --- .../src/ConfigurationBinder.cs | 9 +++++---- .../tests/ConfigurationCollectionBindingTests.cs | 9 ++++++++- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs index 39b90faf65206a..4deabbf084822c 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs @@ -589,12 +589,13 @@ private static void BindConcreteDictionary( Debug.Assert(dictionary is not null); - Type dictionaryObjectType = dictionary.GetType(); + Type? iDictionaryObjectType = FindOpenGenericInterface(typeof(IDictionary<,>), dictionary.GetType()); - MethodInfo tryGetValue = dictionaryObjectType.GetMethod("TryGetValue", BindingFlags.Public | BindingFlags.Instance)!; + Debug.Assert(iDictionaryObjectType is not null); + + MethodInfo tryGetValue = iDictionaryObjectType.GetMethod("TryGetValue", DeclaredOnlyLookup)!; + PropertyInfo? setter = iDictionaryObjectType.GetProperty("Item", DeclaredOnlyLookup); - // dictionary should be of type Dictionary<,> or of type implementing IDictionary<,> - PropertyInfo? setter = dictionaryObjectType.GetProperty("Item", BindingFlags.Public | BindingFlags.Instance); if (setter is null || !setter.CanWrite) { // Cannot set any item on the dictionary object. diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs index 66bc1402bdd8c5..b4e4e95452b539 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs @@ -1171,7 +1171,7 @@ public void CanBindInitializedCustomIndirectlyDerivedIEnumerableList() } [Fact] - public void CanBindInitializedIReadOnlyDictionaryAndDoesNotMofifyTheOriginal() + public void CanBindInitializedIReadOnlyDictionaryAndDoesNotModifyTheOriginal() { // A field declared as IEnumerable that is instantiated with a class // that indirectly implements IEnumerable is still bound, but with @@ -1672,6 +1672,13 @@ public class ImplementerOfIDictionaryClass : IDictionary _dict.TryGetValue(key, out value); System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() => _dict.GetEnumerator(); + + // The following are members which has same names as the IDictionary<,> members. + // Adding these to the test to ensure not getting System.Reflection.AmbiguousMatchException when we bind the dictionary. + private string? v; + public string? this[string key] { get => v; set => v = value; } + public bool TryGetValue() { return true; } + } public class ExtendedDictionary : Dictionary From 08ba1faddaf679ccb98d2b2074a2f9c29c0ddade Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Tue, 29 Nov 2022 08:45:29 -0800 Subject: [PATCH 2/4] Apply suggestions from code review Co-authored-by: Steve Dunn --- .../tests/ConfigurationCollectionBindingTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs index b4e4e95452b539..b604f3c054d7d2 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationCollectionBindingTests.cs @@ -1673,8 +1673,8 @@ public class ImplementerOfIDictionaryClass : IDictionary _dict.GetEnumerator(); - // The following are members which has same names as the IDictionary<,> members. - // Adding these to the test to ensure not getting System.Reflection.AmbiguousMatchException when we bind the dictionary. + // The following are members which have the same names as the IDictionary<,> members. + // The following members test that there's no System.Reflection.AmbiguousMatchException when binding to the dictionary. private string? v; public string? this[string key] { get => v; set => v = value; } public bool TryGetValue() { return true; } From a1512e632894b36858d952f8458722417e832539 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Tue, 29 Nov 2022 13:39:13 -0800 Subject: [PATCH 3/4] Address the feedback --- .../src/ConfigurationBinder.cs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs index 4deabbf084822c..0002310228a5e5 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs @@ -367,7 +367,7 @@ private static void BindInstance( if (dictionaryInterface != null) { - BindConcreteDictionary(bindingPoint.Value!, dictionaryInterface, config, options); + BindDictionary(bindingPoint.Value!, dictionaryInterface, config, options); } else { @@ -549,7 +549,7 @@ private static bool CanBindToTheseConstructorParameters(ParameterInfo[] construc } } - BindConcreteDictionary(dictionary, dictionaryType, config, options); + BindDictionary(dictionary, genericType, config, options); return dictionary; } @@ -562,12 +562,15 @@ private static bool CanBindToTheseConstructorParameters(ParameterInfo[] construc // in their config class, then it is cloned to a new dictionary, the same way as other collections. [RequiresDynamicCode(DynamicCodeWarningMessage)] [RequiresUnreferencedCode("Cannot statically analyze what the element type is of the value objects in the dictionary so its members may be trimmed.")] - private static void BindConcreteDictionary( + private static void BindDictionary( object? dictionary, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.NonPublicProperties)] Type dictionaryType, IConfiguration config, BinderOptions options) { + Debug.Assert(dictionaryType.IsGenericType && + (dictionaryType.GetGenericTypeDefinition() == typeof(IDictionary<,>) || dictionaryType.GetGenericTypeDefinition() == typeof(Dictionary<,>))); + Type keyType = dictionaryType.GenericTypeArguments[0]; Type valueType = dictionaryType.GenericTypeArguments[1]; bool keyTypeIsEnum = keyType.IsEnum; @@ -589,12 +592,9 @@ private static void BindConcreteDictionary( Debug.Assert(dictionary is not null); - Type? iDictionaryObjectType = FindOpenGenericInterface(typeof(IDictionary<,>), dictionary.GetType()); - - Debug.Assert(iDictionaryObjectType is not null); - MethodInfo tryGetValue = iDictionaryObjectType.GetMethod("TryGetValue", DeclaredOnlyLookup)!; - PropertyInfo? setter = iDictionaryObjectType.GetProperty("Item", DeclaredOnlyLookup); + MethodInfo tryGetValue = dictionaryType.GetMethod("TryGetValue", DeclaredOnlyLookup)!; + PropertyInfo? setter = dictionaryType.GetProperty("Item", DeclaredOnlyLookup); if (setter is null || !setter.CanWrite) { From e5db199911cd82823623d3b3523eacd59a0a2916 Mon Sep 17 00:00:00 2001 From: Tarek Mahmoud Sayed Date: Tue, 29 Nov 2022 14:19:56 -0800 Subject: [PATCH 4/4] more feedback --- .../src/ConfigurationBinder.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs index 0002310228a5e5..256bcc15883956 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs @@ -554,7 +554,7 @@ private static bool CanBindToTheseConstructorParameters(ParameterInfo[] construc return dictionary; } - // Binds and potentially overwrites a concrete dictionary. + // Binds and potentially overwrites a dictionary object. // This differs from BindDictionaryInterface because this method doesn't clone // the dictionary; it sets and/or overwrites values directly. // When a user specifies a concrete dictionary or a concrete class implementing IDictionary<,> @@ -563,7 +563,7 @@ private static bool CanBindToTheseConstructorParameters(ParameterInfo[] construc [RequiresDynamicCode(DynamicCodeWarningMessage)] [RequiresUnreferencedCode("Cannot statically analyze what the element type is of the value objects in the dictionary so its members may be trimmed.")] private static void BindDictionary( - object? dictionary, + object dictionary, [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.NonPublicProperties)] Type dictionaryType, IConfiguration config, BinderOptions options) @@ -594,9 +594,9 @@ private static void BindDictionary( MethodInfo tryGetValue = dictionaryType.GetMethod("TryGetValue", DeclaredOnlyLookup)!; - PropertyInfo? setter = dictionaryType.GetProperty("Item", DeclaredOnlyLookup); + PropertyInfo? indexerProperty = dictionaryType.GetProperty("Item", DeclaredOnlyLookup); - if (setter is null || !setter.CanWrite) + if (indexerProperty is null || !indexerProperty.CanWrite) { // Cannot set any item on the dictionary object. return; @@ -624,7 +624,7 @@ private static void BindDictionary( options: options); if (valueBindingPoint.HasNewValue) { - setter.SetValue(dictionary, valueBindingPoint.Value, new object[] { key }); + indexerProperty.SetValue(dictionary, valueBindingPoint.Value, new object[] { key }); } } catch(Exception ex)