Skip to content

Commit

Permalink
Fix invalid processing of empty Dictionary node (dotnet#66229).
Browse files Browse the repository at this point in the history
  • Loading branch information
@Maximys
Maksim Golev authored and Maximys committed May 20, 2023
1 parent 0096ba5 commit a8944dd
Show file tree
Hide file tree
Showing 3 changed files with 169 additions and 87 deletions.
191 changes: 104 additions & 87 deletions src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public static class ConfigurationBinder
var options = new BinderOptions();
configureOptions?.Invoke(options);
var bindingPoint = new BindingPoint();
BindInstance(type, bindingPoint, config: configuration, options: options);
BindInstance(type, bindingPoint, config: configuration, options: options, isParentCollection: false);
return bindingPoint.Value;
}

Expand Down Expand Up @@ -137,7 +137,7 @@ public static void Bind(this IConfiguration configuration, object? instance, Act
var options = new BinderOptions();
configureOptions?.Invoke(options);
var bindingPoint = new BindingPoint(instance, isReadOnly: true);
BindInstance(instance.GetType(), bindingPoint, configuration, options);
BindInstance(instance.GetType(), bindingPoint, configuration, options, false);
}
}

Expand Down Expand Up @@ -260,7 +260,8 @@ private static void BindProperty(PropertyInfo property, object instance, IConfig
property.PropertyType,
propertyBindingPoint,
config.GetSection(GetPropertyName(property)),
options);
options,
false);

// For property binding, there are some cases when HasNewValue is not set in BindingPoint while a non-null Value inside that object can be retrieved from the property getter.
// As example, when binding a property which not having a configuration entry matching this property and the getter can initialize the Value.
Expand All @@ -277,7 +278,8 @@ private static void BindInstance(
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type type,
BindingPoint bindingPoint,
IConfiguration config,
BinderOptions options)
BinderOptions options,
bool isParentCollection)
{
// if binding IConfigurationSection, break early
if (type == typeof(IConfigurationSection))
Expand All @@ -300,112 +302,122 @@ private static void BindInstance(
return;
}

if (config != null && config.GetChildren().Any())
if (config != null)
{
// for arrays and read-only list-like interfaces, we concatenate on to what is already there, if we can
if (type.IsArray || IsImmutableArrayCompatibleInterface(type))
if (config.GetChildren().Any())
{
if (!bindingPoint.IsReadOnly)
// for arrays and read-only list-like interfaces, we concatenate on to what is already there, if we can
if (type.IsArray || IsImmutableArrayCompatibleInterface(type))
{
bindingPoint.SetValue(BindArray(type, (IEnumerable?)bindingPoint.Value, config, options));
}
if (!bindingPoint.IsReadOnly)
{
bindingPoint.SetValue(BindArray(type, (IEnumerable?)bindingPoint.Value, config, options));
}

// for getter-only collection properties that we can't add to, nothing more we can do
return;
}
// for getter-only collection properties that we can't add to, nothing more we can do
return;
}

// -----------------------------------------------------------------------------------------------------------------------------
// | bindingPoint | bindingPoint |
// Interface | Value | IsReadOnly | Behavior
// -----------------------------------------------------------------------------------------------------------------------------
// ISet<T> | not null | true/false | Use the Value instance to populate the configuration
// ISet<T> | null | false | Create HashSet<T> instance to populate the configuration
// ISet<T> | null | true | nothing
// IReadOnlySet<T> | null/not null | false | Create HashSet<T> instance, copy over existing values, and populate the configuration
// IReadOnlySet<T> | null/not null | true | nothing
// -----------------------------------------------------------------------------------------------------------------------------
if (TypeIsASetInterface(type))
{
if (!bindingPoint.IsReadOnly || bindingPoint.Value is not null)
// -----------------------------------------------------------------------------------------------------------------------------
// | bindingPoint | bindingPoint |
// Interface | Value | IsReadOnly | Behavior
// -----------------------------------------------------------------------------------------------------------------------------
// ISet<T> | not null | true/false | Use the Value instance to populate the configuration
// ISet<T> | null | false | Create HashSet<T> instance to populate the configuration
// ISet<T> | null | true | nothing
// IReadOnlySet<T> | null/not null | false | Create HashSet<T> instance, copy over existing values, and populate the configuration
// IReadOnlySet<T> | null/not null | true | nothing
// -----------------------------------------------------------------------------------------------------------------------------
if (TypeIsASetInterface(type))
{
object? newValue = BindSet(type, (IEnumerable?)bindingPoint.Value, config, options);
if (!bindingPoint.IsReadOnly && 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;
}
return;
}

// -----------------------------------------------------------------------------------------------------------------------------
// | bindingPoint | bindingPoint |
// Interface | Value | IsReadOnly | Behavior
// -----------------------------------------------------------------------------------------------------------------------------
// IDictionary<T> | not null | true/false | Use the Value instance to populate the configuration
// IDictionary<T> | null | false | Create Dictionary<T> instance to populate the configuration
// IDictionary<T> | null | true | nothing
// IReadOnlyDictionary<T> | null/not null | false | Create Dictionary<K,V> instance, copy over existing values, and populate the configuration
// IReadOnlyDictionary<T> | null/not null | true | nothing
// -----------------------------------------------------------------------------------------------------------------------------
if (TypeIsADictionaryInterface(type))
{
if (!bindingPoint.IsReadOnly || bindingPoint.Value is not null)
// -----------------------------------------------------------------------------------------------------------------------------
// | bindingPoint | bindingPoint |
// Interface | Value | IsReadOnly | Behavior
// -----------------------------------------------------------------------------------------------------------------------------
// IDictionary<T> | not null | true/false | Use the Value instance to populate the configuration
// IDictionary<T> | null | false | Create Dictionary<T> instance to populate the configuration
// IDictionary<T> | null | true | nothing
// IReadOnlyDictionary<T> | null/not null | false | Create Dictionary<K,V> instance, copy over existing values, and populate the configuration
// IReadOnlyDictionary<T> | null/not null | true | nothing
// -----------------------------------------------------------------------------------------------------------------------------
if (TypeIsADictionaryInterface(type))
{
object? newValue = BindDictionaryInterface(bindingPoint.Value, type, config, options);
if (!bindingPoint.IsReadOnly && newValue != null)
if (!bindingPoint.IsReadOnly || bindingPoint.Value is not null)
{
bindingPoint.SetValue(newValue);
object? newValue = BindDictionaryInterface(bindingPoint.Value, type, config, options);
if (!bindingPoint.IsReadOnly && newValue != null)
{
bindingPoint.SetValue(newValue);
}
}
}

return;
}
return;
}

// If we don't have an instance, try to create one
if (bindingPoint.Value is null)
{
// if the binding point doesn't let us set a new instance, there's nothing more we can do
if (bindingPoint.IsReadOnly)
// If we don't have an instance, try to create one
if (bindingPoint.Value is null)
{
return;
// if the binding point doesn't let us set a new instance, there's nothing more we can do
if (bindingPoint.IsReadOnly)
{
return;
}

Type? interfaceGenericType = type.IsInterface && type.IsConstructedGenericType ? type.GetGenericTypeDefinition() : null;

if (interfaceGenericType is not null &&
(interfaceGenericType == typeof(ICollection<>) || interfaceGenericType == typeof(IList<>)))
{
// For ICollection<T> and IList<T> we bind them to mutable List<T> type.
Type genericType = typeof(List<>).MakeGenericType(type.GenericTypeArguments[0]);
bindingPoint.SetValue(Activator.CreateInstance(genericType));
}
else
{
bindingPoint.SetValue(CreateInstance(type, config, options));
}
}

Type? interfaceGenericType = type.IsInterface && type.IsConstructedGenericType ? type.GetGenericTypeDefinition() : null;
Debug.Assert(bindingPoint.Value is not null);

// At this point we know that we have a non-null bindingPoint.Value, we just have to populate the items
// using the IDictionary<> or ICollection<> interfaces, or properties using reflection.
Type? dictionaryInterface = FindOpenGenericInterface(typeof(IDictionary<,>), type);

if (interfaceGenericType is not null &&
(interfaceGenericType == typeof(ICollection<>) || interfaceGenericType == typeof(IList<>)))
if (dictionaryInterface != null)
{
// For ICollection<T> and IList<T> we bind them to mutable List<T> type.
Type genericType = typeof(List<>).MakeGenericType(type.GenericTypeArguments[0]);
bindingPoint.SetValue(Activator.CreateInstance(genericType));
BindDictionary(bindingPoint.Value, dictionaryInterface, config, options);
}
else
{
bindingPoint.SetValue(CreateInstance(type, config, options));
Type? collectionInterface = FindOpenGenericInterface(typeof(ICollection<>), type);
if (collectionInterface != null)
{
BindCollection(bindingPoint.Value, collectionInterface, config, options);
}
else
{
BindProperties(bindingPoint.Value, config, options);
}
}
}

Debug.Assert(bindingPoint.Value is not null);

// At this point we know that we have a non-null bindingPoint.Value, we just have to populate the items
// using the IDictionary<> or ICollection<> interfaces, or properties using reflection.
Type? dictionaryInterface = FindOpenGenericInterface(typeof(IDictionary<,>), type);

if (dictionaryInterface != null)
{
BindDictionary(bindingPoint.Value, dictionaryInterface, config, options);
}
else
{
Type? collectionInterface = FindOpenGenericInterface(typeof(ICollection<>), type);
if (collectionInterface != null)
{
BindCollection(bindingPoint.Value, collectionInterface, config, options);
}
else
if (isParentCollection)
{
BindProperties(bindingPoint.Value, config, options);
bindingPoint.TrySetValue(CreateInstance(type, config, options));
}
}
}
Expand Down Expand Up @@ -647,7 +659,8 @@ private static void BindDictionary(
type: valueType,
bindingPoint: valueBindingPoint,
config: child,
options: options);
options: options,
true);
if (valueBindingPoint.HasNewValue)
{
indexerProperty.SetValue(dictionary, valueBindingPoint.Value, new object[] { key });
Expand Down Expand Up @@ -685,7 +698,8 @@ private static void BindCollection(
type: itemType,
bindingPoint: itemBindingPoint,
config: section,
options: options);
options: options,
true);
if (itemBindingPoint.HasNewValue)
{
addMethod?.Invoke(collection, new[] { itemBindingPoint.Value });
Expand Down Expand Up @@ -740,7 +754,8 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co
type: elementType,
bindingPoint: itemBindingPoint,
config: section,
options: options);
options: options,
isParentCollection: true);
if (itemBindingPoint.HasNewValue)
{
list.Add(itemBindingPoint.Value);
Expand Down Expand Up @@ -808,7 +823,8 @@ private static Array BindArray(Type type, IEnumerable? source, IConfiguration co
type: elementType,
bindingPoint: itemBindingPoint,
config: section,
options: options);
options: options,
true);
if (itemBindingPoint.HasNewValue)
{
arguments[0] = itemBindingPoint.Value;
Expand Down Expand Up @@ -995,7 +1011,8 @@ private static List<PropertyInfo> GetAllProperties([DynamicallyAccessedMembers(D
parameter.ParameterType,
propertyBindingPoint,
config.GetSection(parameterName),
options);
options,
false);

if (propertyBindingPoint.Value is null)
{
Expand Down
20 changes: 20 additions & 0 deletions ...soft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using System.Text.Json.Serialization;
using Microsoft.Extensions.Configuration;
using Xunit;

Expand Down Expand Up @@ -580,6 +581,25 @@ public class ClassWithIndirectSelfReference
public List<ClassWithIndirectSelfReference> MyList { get; set; }
}

public class DistributedQueueConfig
{
public List<QueueNamespaces> Namespaces { get; set; }
}

public class QueueNamespaces
{
public string Namespace { get; set; }

public Dictionary<string, QueueProperties> Queues { get; set; } = new();
}

public class QueueProperties
{
public DateTimeOffset? CreationDate { get; set; }

public DateTimeOffset? DequeueOnlyMarkedDate { get; set; } = default(DateTimeOffset);
}

public record RecordWithPrimitives
{
public bool Prop0 { get; set; }
Expand Down
Loading

0 comments on commit a8944dd

Please sign in to comment.