Add BindConfiguration extension method for OptionsBuilder

[fredrikhr]

Implements new API proposed in #36209. The proposal was reviewed and approved by @terrajobst on 2020-07-21T18:33:00Z, ref.: #36209 (comment).

Details

The method calls the Configure<TDep> method on the passed optionsBuilder instance, specifying IConfiguration as an injected dependency.

The configureOptions argument for the call to the Configure method is given as a lambda that accepts the IConfiguration root instance from DI. The lambda is now responsible for extracting the appropriate section from that root. This is done by

1. If configSectionPath is a non-empty string, IConfiguration.GetSection is called with configSectionPath as the argument.
2. If configSectionPath is null or an empty string, the Name property of the optionsBuilder parameter is used.
3. As a final fallback the configuration root is used, if both strings above are null or empty.

When the appropriate configuration section has been obtained, the Bind extension method is called, perfoming the binding agrainst the TOptions instance. The configureBinder parameter is specified as nullable, and is passed directly to the Bind extension method. According to the current implementation of ConfigurationBinder.Bind, null is an acceptable value.

Returns

The method returns the value of the optionsBuilder this-parameter to allow for fluid chained method calls.

Exceptions

• If optionsBuilder is null the method immediately throws a new ArgumentNullException.

fixes #36209, duplicate of dotnet/extensions#3020 and #34191

[fredrikhr]

3. As a final fallback the configuration root is used, if both strings above are null or empty.

@maryamariyan, @davidfowl, maybe we should discuss this behaviour? Should I maybe throw InvalidOperationException or ArgumentException instead?

[fredrikhr]

I also wanted to add test-cases for this new API, but I realized after some searching that the Microsoft.Extensions.Options.ConfigurationExtensions library simply does not have its pwn test-project. Should I just create a new project (modelled after one of the test-projects from other libraries), or is there something else I should consider?

[ghost]

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

[maryamariyan]

@fredrikhr are you adding tests for coverage of this API as well?

Perhaps you could add them in https://github.com/dotnet/runtime/blob/master/src/libraries/Microsoft.Extensions.Options/tests/OptionsBuilderTest.cs

Ideally though, if you could add a new test project for Microsoft.Extensions.Options.ConfigurationExtensions and place your tests there it would be better. Because if the test class was under
src/libraries/Microsoft.Extensions.Options.ConfigurationExtensions/tests
folder then you could then check code coverage with: (ignore -c Release if not building in Release) using:

> dotnet build src\libraries\Microsoft.Extensions.Options.ConfigurationExtensions\tests\ /t:test /p:coverage=true -c Release

but for now since all Options tests are under src/libraries/Microsoft.Extensions.Options/tests folder you can add them there instead if you like.

[fredrikhr]

@maryamariyan I'll start working on a new unit testing projects and add tests.

[fredrikhr]

@maryamariyan okay I have added a new Unit test project for MS.Ext.Options.ConfigExt can you look at if what I did is okay? I took inspiration from how the unit test project for MS.Ext.Options work.

For the new API I also enabled nullable annotations using #nullable enable/restore (to avoid touching the existing APIs) in
14598c1. Not that it matters incredibly much in this case, but if I understand correctly this is the way to move forward with new APIs, so why not? We could probably enable for the whole project, but I figured that would be out of scope for this PR.

[maryamariyan]

For the new API I also enabled nullable annotations using #nullable enable/restore (to avoid touching the existing APIs) in
14598c1. Not that it matters incredibly much in this case, but if I understand correctly this is the way to move forward with new APIs, so why not? We could probably enable for the whole project, but I figured that would be out of scope for this PR.

I think it's better to add the API without nullable annotations. They would get covered once the entire assembly is annotated. It could make it harder to review later otherwise.

[fredrikhr]

@maryamariyan okay your call :)

[maryamariyan]

LGTM thanks @fredrikhr.

[maryamariyan]

Nit: just making a quick comparison style-wise with other sln files in the repo, and we don't seem to have "dependencies" like what you are adding here. e.g. Primitives.

Thanks for adding the test project!

[fredrikhr]

Okay, I removed the dependencies solution folder and moved all together into test. ddb583b

[maryamariyan]

@davidfowl @HaoK seems like your feedback are addressed as well.

[maryamariyan]

Thanks @fredrikhr

[fredrikhr]

@maryamariyan merged into master. Does that mean this will actually be part of .NET 5?

[maryamariyan]

@maryamariyan merged into master. Does that mean this will actually be part of .NET 5?

sure thing :)

[davidfowl]

I think we missed wiring up change notifications as part of this change 😢