Skip to content

Support immutable types with configuration binding #67258

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
maryamariyan merged 41 commits into from
Jun 9, 2022
Merged

Support immutable types with configuration binding #67258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
b2c6255
Implementation
SteveDunn Mar 28, 2022
1d8d70b
PR feedback: refactor out duplication getting name from attributes
SteveDunn Apr 2, 2022
a51db1a
PR feedback
SteveDunn Apr 4, 2022
0bdd214
PR feedback: allow creation of value types
SteveDunn Apr 4, 2022
d71d14a
Post merge fixes
SteveDunn Apr 4, 2022
a90ae61
Post merge fixes and remove unused method.
SteveDunn Apr 4, 2022
9bb4cbf
PR feedback: use explicit value when binding to constructor params
SteveDunn Apr 5, 2022
e59e8ef
PR feedback: filter out `out` and `ref`
SteveDunn Apr 5, 2022
94e5c2b
Refactor
SteveDunn Apr 5, 2022
13f6c37
Refactor
SteveDunn Apr 5, 2022
4d04a4b
PR feedback: test for readonly struct
SteveDunn Apr 5, 2022
a9e5eb7
PR feedback: tests for sem-immutable class with init property
SteveDunn Apr 5, 2022
98be745
PR feedback: added tests for `record struct` and `readonly record str…
SteveDunn Apr 5, 2022
d690b95
PR feedback
SteveDunn Apr 5, 2022
eaa0ccb
PR feedback
SteveDunn Apr 8, 2022
2fb15ee
Add tests for current behaviour discussed in the PR
SteveDunn Apr 9, 2022
b948152
PR feedback. Now behaves more like System.Text.Json
SteveDunn Apr 21, 2022
4dcc89f
Slightly simplify the constructor picker logic
SteveDunn Apr 28, 2022
3f10fcc
Update src/libraries/Microsoft.Extensions.Configuration.Binder/src/Co…
SteveDunn May 5, 2022
7b95565
Fix build
SteveDunn May 7, 2022
88985ca
Update src/libraries/Microsoft.Extensions.Configuration.Binder/src/Re…
SteveDunn May 21, 2022
41a7119
Corrected 'parameterized' (was 'parameterised' - sorry, muscle memory!)
SteveDunn May 21, 2022
6d54f27
Corrected test type
SteveDunn May 21, 2022
fa8c4b5
Changed comment to be more accurate
SteveDunn May 21, 2022
11c7808
Changed more instances of 'parameterised' to 'parameterized'
SteveDunn May 21, 2022
f2db596
Treat structs as always having a parameterless constructor: https://g…
SteveDunn May 21, 2022
2d57891
Invalid parameters (ref/out/in) are reported by name rather than type
SteveDunn May 21, 2022
b41c603
Revert refactored method as it is no longer used in multiple places
SteveDunn May 21, 2022
99b3329
Added tests for behaviour of setting parameters followed by properties
SteveDunn May 21, 2022
4e5f912
Reword and rename exception resource text
SteveDunn May 24, 2022
2918063
Disallow constructor parameters that do not match properties or do no…
SteveDunn May 26, 2022
d7754c2
PR feedback
SteveDunn May 26, 2022
59ed6d3
Check for null parameter name
SteveDunn May 27, 2022
ede16aa
Check for null parameter name
SteveDunn May 27, 2022
0c558bc
Also check for fields when binding parameters
SteveDunn May 28, 2022
5029862
Remove check that constructor binding checks presense of fields.
SteveDunn Jun 1, 2022
6aa72ad
Use TryGetDefaultValue from common
SteveDunn Jun 2, 2022
dee80fd
Remove unused local and method for fields
SteveDunn Jun 2, 2022
2a59c97
Remove linq
SteveDunn Jun 2, 2022
888672c
Revert unnecessary change.
eerhardt Jun 8, 2022
24cd5a4
Update src/libraries/Microsoft.Extensions.Configuration.Binder/src/Re…
maryamariyan Jun 8, 2022
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
138 changes: 127 additions & 11 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 @@ -9,6 +9,7 @@
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Reflection;
using Microsoft.Extensions.Internal;

namespace Microsoft.Extensions.Configuration
{
Expand Down Expand Up @@ -328,9 +329,10 @@ private static object BindToCollection(Type type, IConfiguration config, BinderO

[RequiresUnreferencedCode(TrimmingWarningMessage)]
private static void BindInstance(
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
Type type,
BindingPoint bindingPoint, IConfiguration config, BinderOptions options)
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Type type,
BindingPoint bindingPoint,
IConfiguration config,
BinderOptions options)
{
// if binding IConfigurationSection, break early
if (type == typeof(IConfigurationSection))
Expand Down Expand Up @@ -381,7 +383,7 @@ private static void BindInstance(
return; // We are already done if binding to a new collection instance worked
}

bindingPoint.SetValue(CreateInstance(type));
bindingPoint.SetValue(CreateInstance(type, config, options));
}

// See if it's a Dictionary
Expand All @@ -407,23 +409,63 @@ private static void BindInstance(
}
}

[RequiresUnreferencedCode("In case type is a Nullable<T>, cannot statically analyze what the underlying type is so its members may be trimmed.")]
private static object CreateInstance([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)] Type type)
[RequiresUnreferencedCode(
"In case type is a Nullable<T>, cannot statically analyze what the underlying type is so its members may be trimmed.")]
private static object CreateInstance(
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors |
DynamicallyAccessedMemberTypes.NonPublicConstructors)]
Type type,
IConfiguration config,
BinderOptions options)
{
Debug.Assert(!type.IsArray);

if (type.IsAbstract)
if (type.IsInterface || type.IsAbstract)
{
throw new InvalidOperationException(SR.Format(SR.Error_CannotActivateAbstractOrInterface, type));
}

if (!type.IsValueType)
ConstructorInfo[] constructors = type.GetConstructors(BindingFlags.Public | BindingFlags.Instance);

bool hasParameterlessConstructor =
type.IsValueType || constructors.Any(ctor => ctor.GetParameters().Length == 0);

if (!type.IsValueType && constructors.Length == 0)
{
throw new InvalidOperationException(SR.Format(SR.Error_MissingPublicInstanceConstructor, type));
}

if (constructors.Length > 1 && !hasParameterlessConstructor)
{
bool hasDefaultConstructor = type.GetConstructors(DeclaredOnlyLookup).Any(ctor => ctor.IsPublic && ctor.GetParameters().Length == 0);
if (!hasDefaultConstructor)
throw new InvalidOperationException(SR.Format(SR.Error_MultipleParameterizedConstructors, type));
}

if (constructors.Length == 1 && !hasParameterlessConstructor)
{
ConstructorInfo constructor = constructors[0];
ParameterInfo[] parameters = constructor.GetParameters();

if (!CanBindToTheseConstructorParameters(parameters, out string nameOfInvalidParameter))
{
throw new InvalidOperationException(SR.Format(SR.Error_CannotBindToConstructorParameter, type, nameOfInvalidParameter));
}


List<PropertyInfo> properties = GetAllProperties(type);

if (!DoAllParametersHaveEquivalentProperties(parameters, properties, out string nameOfInvalidParameters))
{
throw new InvalidOperationException(SR.Format(SR.Error_ConstructorParametersDoNotMatchProperties, type, nameOfInvalidParameters));
}

object?[] parameterValues = new object?[parameters.Length];

for (int index = 0; index < parameters.Length; index++)
{
throw new InvalidOperationException(SR.Format(SR.Error_MissingParameterlessConstructor, type));
parameterValues[index] = BindParameter(parameters[index], type, config, options);
}

return constructor.Invoke(parameterValues);
Copy link
Member

@halter73 halter73 Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@layomia Can you take a look at this since S.T.J has to deal with the very similar problem of deserializing custom types using parameterized constructors?

I would like the behavior of Microsoft.Extensions.Configuraiton and S.T.J to align as much is possible. I realize there are already many difference for various reasons. Default case-insensitivity is an example.

In the doc describing the S.T.J behavior, I see the following:

If a constructor parameter does not match with a property, InvalidOperationException will be thrown if deserialization is attempted.

Do you think that would make sense to do here? We could always relax the restriction later. Are there other behaviors or restrictions that would make sense to copy?

Copy link
Member

@davidfowl davidfowl Apr 22, 2022

Choose a reason for hiding this comment

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

Did you see this #67258 (comment)?

}

object? instance;
Expand All @@ -439,6 +481,46 @@ private static object CreateInstance([DynamicallyAccessedMembers(DynamicallyAcce
return instance ?? throw new InvalidOperationException(SR.Format(SR.Error_FailedToActivate, type));
}

private static bool DoAllParametersHaveEquivalentProperties(ParameterInfo[] parameters,
List<PropertyInfo> properties, out string missing)
{
HashSet<string> propertyNames = new(StringComparer.OrdinalIgnoreCase);
foreach (PropertyInfo prop in properties)
{
propertyNames.Add(prop.Name);
}

List<string> missingParameters = new();

foreach (ParameterInfo parameter in parameters)
{
string name = parameter.Name!;
if (!propertyNames.Contains(name))
{
missingParameters.Add(name);
}
}

missing = string.Join(",", missingParameters);

return missing.Length == 0;
}

private static bool CanBindToTheseConstructorParameters(ParameterInfo[] constructorParameters, out string nameOfInvalidParameter)
{
nameOfInvalidParameter = string.Empty;
foreach (ParameterInfo p in constructorParameters)
{
if (p.IsOut || p.IsIn || p.ParameterType.IsByRef)
{
nameOfInvalidParameter = p.Name!; // never null as we're not passed return value parameters: https://docs.microsoft.com/en-us/dotnet/api/system.reflection.parameterinfo.name?view=net-6.0#remarks
return false;
}
}

return true;
}

[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,
Expand Down Expand Up @@ -687,6 +769,40 @@ private static List<PropertyInfo> GetAllProperties(
return allProperties;
}

[RequiresUnreferencedCode(PropertyTrimmingWarningMessage)]
private static object? BindParameter(ParameterInfo parameter, Type type, IConfiguration config,
BinderOptions options)
{
string? parameterName = parameter.Name;

if (parameterName is null)
{
throw new InvalidOperationException(SR.Format(SR.Error_ParameterBeingBoundToIsUnnamed, type));
}

var propertyBindingPoint = new BindingPoint(initialValue: config.GetSection(parameterName).Value, isReadOnly: false);

if (propertyBindingPoint.Value is null)
{
if (ParameterDefaultValue.TryGetDefaultValue(parameter, out object? defaultValue))
{
propertyBindingPoint.SetValue(defaultValue);
}
else
{
throw new InvalidOperationException(SR.Format(SR.Error_ParameterHasNoMatchingConfig, type, parameterName));
}
}

BindInstance(
parameter.ParameterType,
propertyBindingPoint,
config.GetSection(parameterName),
options);

return propertyBindingPoint.Value;
}

private static string GetPropertyName(MemberInfo property)
{
ThrowHelper.ThrowIfNull(property);
Expand Down
9 changes: 7 additions & 2 deletions ...soft.Extensions.Configuration.Binder/src/Microsoft.Extensions.Configuration.Binder.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,19 @@

<ItemGroup>
<ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.Configuration.Abstractions\src\Microsoft.Extensions.Configuration.Abstractions.csproj" />
<Compile Include="$(CommonPath)System\ThrowHelper.cs"
Link="Common\System\ThrowHelper.cs" />
<Compile Include="$(CommonPath)Extensions\ParameterDefaultValue\ParameterDefaultValue.cs" Link="Common\src\Extensions\ParameterDefaultValue\ParameterDefaultValue.cs" />
<Compile Include="$(CommonPath)System\ThrowHelper.cs" Link="Common\System\ThrowHelper.cs" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'">
<Compile Include="$(CommonPath)Extensions\ParameterDefaultValue\ParameterDefaultValue.netstandard.cs" Link="Common\src\Extensions\ParameterDefaultValue\ParameterDefaultValue.netstandard.cs" />
<Compile Include="$(CoreLibSharedDir)System\Diagnostics\CodeAnalysis\DynamicallyAccessedMembersAttribute.cs" />
<Compile Include="$(CoreLibSharedDir)System\Diagnostics\CodeAnalysis\DynamicallyAccessedMemberTypes.cs" />
<Compile Include="$(CoreLibSharedDir)System\Diagnostics\CodeAnalysis\RequiresUnreferencedCodeAttribute.cs" />
<Compile Include="$(CoreLibSharedDir)System\Diagnostics\CodeAnalysis\UnconditionalSuppressMessageAttribute.cs" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
<Compile Include="$(CommonPath)Extensions\ParameterDefaultValue\ParameterDefaultValue.netcoreapp.cs" Link="Common\src\Extensions\ParameterDefaultValue\ParameterDefaultValue.netcoreapp.cs" />
</ItemGroup>
</Project>
19 changes: 17 additions & 2 deletions src/libraries/Microsoft.Extensions.Configuration.Binder/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,12 @@
<data name="Error_CannotActivateAbstractOrInterface" xml:space="preserve">
<value>Cannot create instance of type '{0}' because it is either abstract or an interface.</value>
</data>
<data name="Error_CannotBindToConstructorParameter" xml:space="preserve">
<value>Cannot create instance of type '{0}' because one or more parameters cannot be bound to. Constructor parameters cannot be declared as in, out, or ref. Invalid parameters are: '{1}'</value>
</data>
<data name="Error_ConstructorParametersDoNotMatchProperties" xml:space="preserve">
<value>Cannot create instance of type '{0}' because one or more parameters cannot be bound to. Constructor parameters must have corresponding properties. Fields are not supported. Missing properties are: '{1}'</value>
</data>
<data name="Error_FailedBinding" xml:space="preserve">
<value>Failed to convert configuration value at '{0}' to type '{1}'.</value>
</data>
Expand All @@ -129,8 +135,17 @@
<data name="Error_MissingConfig" xml:space="preserve">
<value>'{0}' was set on the provided {1}, but the following properties were not found on the instance of {2}: {3}</value>
</data>
<data name="Error_MissingParameterlessConstructor" xml:space="preserve">
<value>Cannot create instance of type '{0}' because it is missing a public parameterless constructor.</value>
<data name="Error_MissingPublicInstanceConstructor" xml:space="preserve">
<value>Cannot create instance of type '{0}' because it is missing a public instance constructor.</value>
</data>
<data name="Error_MultipleParameterizedConstructors" xml:space="preserve">
<value>Cannot create instance of type '{0}' because it has multiple public parameterized constructors.</value>
</data>
<data name="Error_ParameterBeingBoundToIsUnnamed" xml:space="preserve">
<value>Cannot create instance of type '{0}' because one or more parameters are unnamed.</value>
</data>
<data name="Error_ParameterHasNoMatchingConfig" xml:space="preserve">
<value>Cannot create instance of type '{0}' because parameter '{1}' has no matching config. Each parameter in the constructor that does not have a default value must have a corresponding config entry.</value>
</data>
<data name="Error_UnsupportedMultidimensionalArray" xml:space="preserve">
<value>Cannot create instance of type '{0}' because multidimensional arrays are not supported.</value>
Expand Down
Loading