Skip to content
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

Configuration binding with nested types and readonly properties not working #77677

Closed
Tracked by #77390
glucaci opened this issue Oct 31, 2022 · 11 comments · Fixed by #77693
Closed
Tracked by #77390

Configuration binding with nested types and readonly properties not working #77677

glucaci opened this issue Oct 31, 2022 · 11 comments · Fixed by #77693

Comments

@glucaci
Copy link

glucaci commented Oct 31, 2022

Description

With #67258 the binding of immutable types is working but with nested types seams not to work.

Reproduction Steps

public record RootConfig(NestedConfig Nested);

public record NestedConfig(string MyProp);

public RootConfig GetConfiguration()
{
    IConfiguration configuration = new ConfigurationBuilder()
        .AddInMemoryCollection(new Dictionary<string, string?>
        {
            { "Nested:MyProp", "Dummy" }
        })
        .Build();

    return configuration.Get<RootConfig>();            
}

In the above case the following exception will be thrown:

Cannot create instance of type 'RootConfig' because parameter 'Nested' has no matching config. 
Each parameter in the constructor that does not have a default value must have a corresponding config entry.

If the Nested property is declared with init modifier like bellow it will work.

public record RootConfig
{
    public NestedConfig Nested { get; init; }
}

Expected behavior

No exception.

Actual behavior

Throws InvalidOperationException.

Regression?

No response

Known Workarounds

Declare the Nested property with init modifier.

Configuration

.NET 7 RC2

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 31, 2022
@ghost
Copy link

ghost commented Oct 31, 2022

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

Issue Details

Description

With #67258 the binding of immutable types is working but with nested types seams not to work.

Reproduction Steps

public record RootConfig(NestedConfig Nested);

public record NestedConfig(string MyProp);

public RootConfig GetConfiguration()
{
    IConfiguration configuration = new ConfigurationBuilder()
        .AddInMemoryCollection(new Dictionary<string, string?>
        {
            { "Nested:MyProp", "Dummy" }
        })
        .Build();

    return configuration.Get<RootConfig>();            
}

In the above case the following exception will be thrown:

Cannot create instance of type 'RootConfig' because parameter 'Nested' has no matching config. 
Each parameter in the constructor that does not have a default value must have a corresponding config entry.

If the Nested property is declared with init modifier like bellow it will work.

public record RootConfig
{
    public NestedConfig Nested { get; init; }
}

Expected behavior

No exception.

Actual behavior

Throws InvalidOperationException.

Regression?

No response

Known Workarounds

Declare the Nested property with init modifier.

Configuration

.NET 7 RC2

Other information

No response

Author: glucaci
Assignees: -
Labels:

area-Extensions-Configuration

Milestone: -

@tarekgh
Copy link
Member

tarekgh commented Oct 31, 2022

@SteveDunn will you have some spare time to look at this one?

@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Oct 31, 2022
@maryamariyan maryamariyan added this to the 8.0.0 milestone Oct 31, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Oct 31, 2022
@SteveDunn
Copy link
Contributor

SteveDunn commented Oct 31, 2022

@tarekgh / @glucaci - hi - I just got main and added this test and it passed without the exception described above

image

Admittedly, this was a very quick exercise as my time is rather limited with my day job.

Have I missed something?

(PR added - please feel free to modify it so that it breaks, and I'll attempt a fix when time permits - #77693)

@tarekgh
Copy link
Member

tarekgh commented Oct 31, 2022

@SteveDunn thanks for the follow up. I am trying that with .NET 7.0 rc2 and I am seeing it failing. I'll try the rtm bits to see if it reproduces there.

FrameworkDescription: .NET 7.0.0-rc.2.22472.3
RuntimeIdentifier: win10-x64

Unhandled exception. System.InvalidOperationException: Cannot create instance of type 'NetCoreApp.Program+RootConfig' because parameter 'Nested' has no matching config. Each parameter in the constructor that does not have a default value must have a corresponding config entry.
   at Microsoft.Extensions.Configuration.ConfigurationBinder.BindParameter(ParameterInfo parameter, Type type, IConfiguration config, BinderOptions options)
   at Microsoft.Extensions.Configuration.ConfigurationBinder.CreateInstance(Type type, IConfiguration config, BinderOptions options)
   at Microsoft.Extensions.Configuration.ConfigurationBinder.BindInstance(Type type, BindingPoint bindingPoint, IConfiguration config, BinderOptions options)
   at Microsoft.Extensions.Configuration.ConfigurationBinder.Get(IConfiguration configuration, Type type, Action`1 configureOptions)
   at Microsoft.Extensions.Configuration.ConfigurationBinder.Get[T](IConfiguration configuration, Action`1 configureOptions)
   at Microsoft.Extensions.Configuration.ConfigurationBinder.Get[T](IConfiguration configuration)
   at NetCoreApp.Program.BindWithNestedTypesWithReadOnlyProperties() in C:\Temp\NetCoreApp\Program.cs:line 114
   at NetCoreApp.Program.Main(String[] args) in C:\Temp\NetCoreApp\Program.cs:line 159

@tarekgh
Copy link
Member

tarekgh commented Nov 1, 2022

Interesting, it looks something has been changed in .NET 8.0 which fixed the issue. We need to understand what has changed.

@SteveDunn
Copy link
Contributor

SteveDunn commented Nov 1, 2022

It is this:

image

Left is RC2, right is Main as of yesterday (31st October '22)

The commit on the right is ca0d3e4d by @Saalvage

I'm happy if you want to close this PR.

@SteveDunn
Copy link
Contributor

Actually, it might be worth merging in as the test is useful as a regression.

@glucaci
Copy link
Author

glucaci commented Nov 1, 2022

It is this:

image

Left is RC2, right is Main as of yesterday (31st October '22)

The commit on the right is ca0d3e4d by @Saalvage

I'm happy if you want to close this PR.

@tarekgh could this be cherry-picked into .NET 7?

@tarekgh
Copy link
Member

tarekgh commented Nov 1, 2022

Actually, it might be worth merging in as the test is useful as a regression.

Agree. Thanks for pointing at the change that fixed this issue.

could this be cherry-picked into .NET 7?

.NET 7.0 is done. It is going to be released in a few days. We may consider servicing this later if proven it is a blocker to important scenarios. Considering the workaround is trivial and it is not a regression, this will be difficult to meet the servicing bar.

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 1, 2022
@aradalvand
Copy link

aradalvand commented Nov 23, 2022

Just encountered the same problem; such a shame that the fix didn't make it to 7.0...

Considering the workaround is trivial

What is the workaround, please?

@tarekgh
Copy link
Member

tarekgh commented Nov 23, 2022

@aradalvand

In the issue description, it mentioned the following

Known Workarounds
Declare the Nested property with init modifier.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants