Support immutable types with configuration binding #67258

SteveDunn · 2022-03-28T20:22:58Z

Fixes #43662 Support immutable types with configuration binding

ghost · 2022-03-28T20:23:04Z

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #43662 Support immutable types with configuration binding

Author:	SteveDunn
Assignees:	-
Labels:	`area-Extensions-Configuration`
Milestone:	-

SteveDunn · 2022-03-28T20:41:28Z

This PR supersedes #67161 due to several git mishaps in a row!

src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

SteveDunn · 2022-03-31T21:07:48Z

@eerhardt / @davidfowl - how does this PR look? Is there anything that still looks unresolved? There's a couple of things that are unclear and need resolving from what I can tell:

With the PR, we can now bind to records and other immutable classes. ~~But not structs (Support immutable types with configuration binding #67258 (comment))~~ NEW! Now with structs!
There is a potentially unused method (Support immutable types with configuration binding #67258 (comment)). It relates to the existing attribute named ConfigurationKeyNameAttribute. This can be applied to properties, but I think it might be useful if it was also applied to parameters. What do you think?

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

maryamariyan · 2022-04-04T21:48:11Z

@SteveDunn did you notice the CI build failure in https://github.com/dotnet/runtime/pull/67258/checks?check_run_id=5824336121?

SteveDunn · 2022-04-04T21:59:50Z

@madelson - I just merged in your changes from main. Would you mind giving this PR a quick look-over?

SteveDunn · 2022-04-04T22:01:43Z

@SteveDunn did you notice the CI build failure in https://github.com/dotnet/runtime/pull/67258/checks?check_run_id=5824336121?

@maryamariyan - yes, I just resolved the conflicts with @madelson 's change. It now builds and passes the tests, although I think it would be good if @madelson could give this PR a quick once-over.

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

maryamariyan · 2022-04-04T23:30:31Z

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

@@ -703,10 +759,10 @@ private static string GetPropertyName(MemberInfo property!!)
                    .Value?
                    .ToString();

-                return !string.IsNullOrWhiteSpace(name) ? name : property.Name;


There is a potentially unused method (#67258 (comment)). It relates to the existing attribute named ConfigurationKeyNameAttribute. This can be applied to properties, but I think it might be useful if it was also applied to parameters. What do you think?

We could add this support in a separate PR. The idea would be to allow usage of ConfigurationKeyName attribute over on constructor parameters, not just over a property as it is defined today here.

Sample usage:

With records:

public record Poco([ConfigurationKeyName="my_string"] string myString);

Workaround without this would be to add the annotation over the property as shown below:

public class Poco { [ConfigurationKeyName="my_string"] public string MyString { get; init; } public Poco(string myString) { .. } }

TODO:

Add API proposal for this change (outside the scope for this PR).

Thanks @maryamariyan - is the task for me, or for somebody else?

I will take this task to API review.

Should I create a branch off this branch and implement it?

Sure, but we'd be able to add that only after the API review gets approved.

OK. I think I've addressed all the issues in this PR then. Happy to respond to any more.

src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

madelson · 2022-04-05T14:18:01Z

I think it would be good if @madelson could give this PR a quick once-over.

@SteveDunn happy to do so; left a few quick comments but should hopefully be able to look in more depth later this week.

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

madelson · 2022-04-07T12:30:46Z

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

+                        continue;
+                    }
+
+                    if (chosenParameters is null || constructorParameters.Length > chosenParameters.Length)


Is it a problem that ties are broken based on constructor order which is inherently undefined? I wonder if it would be better if ties resulted in an exception.

In general, it feels like one of the most important things for this issue is to clearly define the constructor selection policy. Once selected, changing it would break compat. Just to summarize some of the considerations:

Public default constructors are preferred for compat

Are non-public constructors allowed (e. g. internal)?

Are in and ref parameters allowed?

Do we populate parameters with no default value and no configuration key (I think we shouldn't)?

Do the available configuration keys matter for constructor selection (e. g. if I have just "a": "2" in my config and we're choosing between constructors Options(int a) and Options(string x, string y), which one should it choose?

What do we do if 2 constructors are equally eligible for selection?

One policy I might suggest is select the public constructor with the most bindable parameters, where bindable means not ref or out, and either has a matching configuration key or has a default value. Prefer default constructors over any others for compat.

A more conservative approach could be Select the default constructor if there is one. Otherwise, if there is a single public constructor with no ref/out parameters use that. Otherwise, throw. The advantage of this more conservative approach is that we can amend it later without breaking compat.

We should look at what JSON does here. There’s prior art

I took a look at the STJ source and this article. It chooses the default public parameterless constructor, or, if there is one, a constructor (doesn't have to be public) decorated with a JsonConstructor attribute. There's other ways to coerce the use of other constructors, such as using contract resolvers and the like.
STJ disallows non-public constructors.

Re. these points:

Public default constructors are preferred for compat
Yes, I think that makes sense. It currently does this.

Are non-public constructors allowed (e. g. internal)?
Currently, no. As above, this is consistent with System.Text.Json

Are in and ref parameters allowed?
In STJ, both ref and in parameters are disallowed, resulting in
Unhandled exception. System.ArgumentException: The type 'System.String&' may not be used as a type argument. at System.RuntimeType.ThrowIfTypeNeverValidGenericArgument(RuntimeType type)
Should we do the same? Or should we silently not consider the constructor?

Do we populate parameters with no default value and no configuration key (I think we shouldn't)?
Currently we do. I've added a test for this current behaviour (CanBindImmutableClass_PicksLargestConstructorEvenIfItHasExtraneousParameters)

Do the available configuration keys matter for constructor selection (e. g. if I have just "a": "2" in my config and we're choosing between constructors Options(int a) and Options(string x, string y), which one should it choose?
The available keys are not considered. It currently picks the largest constructor in terms of parameters. As above, I added a test for this current behaviour (CanBindImmutableClass_PicksLargestConstructorEvenIfItHasExtraneousParameters)

What do we do if 2 constructors are equally eligible for selection?
We pick first one. I added a test to cover this behaviour (CanBindImmutableClass_PicksFirstOfAnyAmbiguousConstructors)

You make some very valid points, and I think we should add constraints to avoid ambiguity and confusion. I think a model closer to STJ makes sense (except the use of an attribute):

Use the default public constructor if it has one

Use the only other parameterised constructor if there is one

Disallow in and ref

Re. the parameterised constructor: I think we should leave this fairly tolerant with regards to the relationship between the parameters and the properties. It will allow config and classes to evolve separately, and will allow users to add 'synthetic' fields at bind time. What do we think of this?

Any thoughts on these suggestions? Making the behaviour match closer to STJ seems like the right choice.

Generally those look like the right choices to me. Lets do it. Are we going to have an attribute to disambiguate ctors?

I exercised some initiative and decided to go with the System.Text.Json way of binding constructors. This means that the behaviour now is this:

Use the default public parameterless constructor if it has one

Use the only other parameterised constructor if there is one

Disallow in, out, and ref parameters on that constructor by throwing an InvalidOperationException

Throw is there are multiple parameterised constructors

How odd - I must've typed my last comment while you were typing yours!

I think that if we don't consider non-public constructors, then we won't need an attribute to disambiguate any other constructor. That's because we use the public parameterless constructor, if there is one, or the only other public parameterised constructor.
We could consider adding an attribute, but that decision is based more on the question of 'Should we allow non-public constructors?'

I think that if we don't consider non-public constructors, then we won't need an attribute to disambiguate any other constructor.

Structs always have a default constructor. What if you want to make config prefer your non-default constructor? Otherwise the following would always bind to X = 0, Y = 0 because there's a default constructor with no parameters but the properties aren't settable.

public struct Point { public int X { get; } public int Y { get; } // An attribute would be nice here to tell Configuration not to use the default constructor public Point(int x, int y) => (X, Y) = (x, y); }

Also, are we going to use default values for constructor parameters that are missing?

public struct Point { // ... public Point(int x, int y = 0) => (X, Y) = (x, y); }

https://github.com/dotnet/runtime/blob/0e18cfda13a8136103c83442e1a649cdecc237b0/src/libraries/System.Text.Json/docs/ParameterizedCtorSpec.md#feature-behavior

Good points. I'll add some more tests. If we go with an attribute, will that have to go through an API Proposal? And will that (should that) be part of another task and PR?

Actually, binding to structs do work, and there is a test for it (CanBindStructOptions). The logic for constructors (currently) is:

if it's not a value type && there are no constructors- then throw an exception

if there is more than one constructor && none of them are the parameterless constructor, then throw and exception

if it has just one constructor && that constructor is not the parameterless constructor, then try to bind to constructor parameters

otherwise, use Activator.CreateInstance with the default constructor (no parameter binding)

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

halter73 · 2022-04-21T22:00:20Z

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

+                    parameterValues[index] = BindParameter(parameters[index], config, options);
+                }
+
+                return constructor.Invoke(parameterValues);


@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?

Did you see this #67258 (comment)?

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

halter73 · 2022-05-04T17:30:51Z

Another thing to consider here is that the following test that used to pass now fails. This is because we'd use the default parameterless constructor for structs before, but now we try to pass null into the single-parameter constructor that doesn't expect a null value resulting in an NullReferenceException:

public struct MutableStructWithConstructor
{
    public MutableStructWithConstructor(string randomParameter)
    {
        Color = randomParameter;
        Length = randomParameter.Length;
    }

    public string Color { get; set; }
    public int Length { get; set;  }
}

[Fact]
public void CanBindMutableStructWithConstructor()
{
    var dic = new Dictionary<string, string>
    {
        {"Length", "42"},
        {"Color", "Green"},
    };
    var configurationBuilder = new ConfigurationBuilder();
    configurationBuilder.AddInMemoryCollection(dic);
    var config = configurationBuilder.Build();

    var options = config.Get<MutableStructWithConstructor>();
    Assert.Equal(42, options.Length);
    Assert.Equal("Green", options.Color);
}

I don't think we want to regress this scenario.

halter73 · 2022-05-04T17:41:53Z

This brings up the larger issue that we should probably not just be passing null into parameters that are missing unless they have a default value or are nullable.

We should also add tests for structs that are populated partially with a constructor and partially with properties since that's a thing we now support with this PR. Do we do this for JSON? Also, I don't think I saw a response this this question.

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?

#67258 (comment)

SteveDunn · 2022-05-06T05:44:05Z

This brings up the larger issue that we should probably not just be passing null into parameters that are missing unless they have a default value or are nullable.

We should also add tests for structs that are populated partially with a constructor and partially with properties since that's a thing we now support with this PR. Do we do this for JSON? Also, I don't think I saw a response this this question.

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?

#67258 (comment)

Re. the comment:

We should also add tests for structs that are populated partially with a constructor and partially with properties

This is already covered in a test that I added named CanBindSemiImmutableClass that binds 2 constructor parameters and 1 property.

Re. the STJ question - "If a constructor parameter does not match a property": for binding, on one hand, it would make sense to throw if there is a constructor and we don't have any properties that match any parameters. But on the other hand, a user might want a best-effort approach, e.g. I have a constructor that "I" use, and I'd like binding to bind to whatever properties it can. But there will have to be a trade-off for this flexibility; either the user will ensure that the constructor behaves nicely with unspecified properties from config, or they'll need to ensure that the properties are specified.

SteveDunn · 2022-05-16T04:31:06Z

This is one of a few of my PRs laying dormant. Is there anything I need to do to progress them?

eerhardt

This looks really close. Thanks for the great work here @SteveDunn. I just have a few comments.

src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

src/libraries/Microsoft.Extensions.Configuration.Binder/src/Resources/Strings.resx

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs

…t have corresponding config items.

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

davidfowl · 2022-06-04T07:00:25Z

Is this resolved?

SteveDunn · 2022-06-04T07:08:13Z

Is this resolved?

I believe that everything is now resolved in this PR. It's certainly been a journey, but a very satisfying one!

davidfowl · 2022-06-04T07:39:22Z

@SteveDunn I would send you some .NET swag for your patience with us on this PR 😄

SteveDunn · 2022-06-05T04:13:39Z

@SteveDunn I would send you some .NET swag for your patience with us on this PR 😄

Very kind - I'd take anything that's free! 😆

eerhardt

This LGTM. Thanks for all your work here, @SteveDunn!

It would be good to get @halter73 and @maryamariyan to sign off as well.

halter73

@SteveDunn I would send you some .NET swag for your patience with us on this PR 😄

Agreed! Great work.

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

src/libraries/Microsoft.Extensions.Configuration.Binder/src/Resources/Strings.resx

…sources/Strings.resx

dotnet-issue-labeler bot added the area-Extensions-Configuration label Mar 28, 2022

ghost added the community-contribution Indicates that the PR has been added by a community member label Mar 28, 2022

SteveDunn mentioned this pull request Mar 28, 2022

Support binding to immutable types #43662 #67161

Closed

SteveDunn commented Mar 28, 2022

View reviewed changes

src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs Outdated Show resolved Hide resolved

src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs Show resolved Hide resolved

SteveDunn commented Mar 28, 2022

View reviewed changes

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Outdated Show resolved Hide resolved

eerhardt reviewed Apr 1, 2022

View reviewed changes

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Outdated Show resolved Hide resolved

madelson reviewed Apr 4, 2022

View reviewed changes

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Outdated Show resolved Hide resolved

maryamariyan reviewed Apr 4, 2022

View reviewed changes

madelson reviewed Apr 4, 2022

View reviewed changes

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Outdated Show resolved Hide resolved

madelson reviewed Apr 4, 2022

View reviewed changes

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Outdated Show resolved Hide resolved

madelson reviewed Apr 5, 2022

View reviewed changes

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Outdated Show resolved Hide resolved

madelson reviewed Apr 7, 2022

View reviewed changes

halter73 reviewed Apr 21, 2022

View reviewed changes

runfoapp bot mentioned this pull request Apr 21, 2022

Read_Archive_File failing due to access denied to tar/pax/file.tar #68357

Closed

halter73 reviewed May 4, 2022

View reviewed changes

eerhardt reviewed May 18, 2022

View reviewed changes

halter73 reviewed May 21, 2022

View reviewed changes

SteveDunn added 2 commits May 21, 2022 06:03

Implementation

b2c6255

PR feedback: refactor out duplication getting name from attributes

1d8d70b

Reword and rename exception resource text

4e5f912

halter73 reviewed May 24, 2022

View reviewed changes

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Outdated Show resolved Hide resolved

src/libraries/Microsoft.Extensions.Configuration.Binder/tests/ConfigurationBinderTests.cs Outdated Show resolved Hide resolved

SteveDunn added 4 commits May 26, 2022 22:16

Disallow constructor parameters that do not match properties or do no…

2918063

…t have corresponding config items.

PR feedback

d7754c2

Check for null parameter name

59ed6d3

Check for null parameter name

ede16aa

eerhardt reviewed May 27, 2022

View reviewed changes

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Show resolved Hide resolved

SteveDunn mentioned this pull request May 28, 2022

System.Text.Json cannot bind to fields #69936

Closed

Also check for fields when binding parameters

0c558bc

SteveDunn mentioned this pull request May 28, 2022

Support immutable types with configuration binding #43662

Closed

Remove check that constructor binding checks presense of fields.

5029862

eerhardt reviewed Jun 1, 2022

View reviewed changes

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Outdated Show resolved Hide resolved

eerhardt reviewed Jun 1, 2022

View reviewed changes

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Outdated Show resolved Hide resolved

eerhardt reviewed Jun 1, 2022

View reviewed changes

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Outdated Show resolved Hide resolved

runfoapp bot mentioned this pull request Jun 1, 2022

Sha512ManagedTests.Sha512_Fips180_3_Async is flaky #70058

Closed

SteveDunn added 3 commits June 2, 2022 16:17

Use TryGetDefaultValue from common

6aa72ad

Remove unused local and method for fields

dee80fd

Remove linq

2a59c97

eerhardt approved these changes Jun 6, 2022

View reviewed changes

halter73 approved these changes Jun 7, 2022

View reviewed changes

maryamariyan approved these changes Jun 7, 2022

View reviewed changes

src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Outdated Show resolved Hide resolved

src/libraries/Microsoft.Extensions.Configuration.Binder/src/Resources/Strings.resx Outdated Show resolved Hide resolved

eerhardt and others added 2 commits June 8, 2022 09:16

Revert unnecessary change.

888672c

Update src/libraries/Microsoft.Extensions.Configuration.Binder/src/Re…

24cd5a4

…sources/Strings.resx

maryamariyan merged commit d78f364 into dotnet:main Jun 9, 2022

SteveDunn deleted the bind-to-immutable-types branch June 9, 2022 19:50

ghost locked as resolved and limited conversation to collaborators Jul 10, 2022

Support immutable types with configuration binding #67258

Support immutable types with configuration binding #67258

Conversation

SteveDunn commented Mar 28, 2022

ghost commented Mar 28, 2022

SteveDunn commented Mar 28, 2022

SteveDunn commented Mar 31, 2022 • edited Loading

maryamariyan commented Apr 4, 2022

SteveDunn commented Apr 4, 2022

SteveDunn commented Apr 4, 2022

maryamariyan Apr 4, 2022 • edited Loading

Choose a reason for hiding this comment

Sample usage:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveDunn Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

maryamariyan Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madelson commented Apr 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveDunn Apr 9, 2022 • edited Loading

Choose a reason for hiding this comment

SteveDunn Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveDunn Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

SteveDunn Apr 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveDunn Apr 28, 2022 • edited Loading

Choose a reason for hiding this comment

halter73 Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halter73 commented May 4, 2022

halter73 commented May 4, 2022

SteveDunn commented May 6, 2022 • edited Loading

SteveDunn commented May 16, 2022 • edited Loading

eerhardt left a comment

Choose a reason for hiding this comment

davidfowl commented Jun 4, 2022

SteveDunn commented Jun 4, 2022

davidfowl commented Jun 4, 2022 • edited Loading

SteveDunn commented Jun 5, 2022

eerhardt left a comment

Choose a reason for hiding this comment

halter73 left a comment

Choose a reason for hiding this comment

SteveDunn commented Mar 31, 2022 •

edited

Loading

maryamariyan Apr 4, 2022 •

edited

Loading

SteveDunn Apr 5, 2022 •

edited

Loading

maryamariyan Apr 5, 2022 •

edited

Loading

SteveDunn Apr 9, 2022 •

edited

Loading

SteveDunn Apr 14, 2022 •

edited

Loading

SteveDunn Apr 21, 2022 •

edited

Loading

SteveDunn Apr 22, 2022 •

edited

Loading

SteveDunn Apr 28, 2022 •

edited

Loading

halter73 Apr 21, 2022 •

edited

Loading

SteveDunn commented May 6, 2022 •

edited

Loading

SteveDunn commented May 16, 2022 •

edited

Loading

davidfowl commented Jun 4, 2022 •

edited

Loading