Issue 36015 new BinderOption to throw on missing configuration by SteveDunn · Pull Request #53852 · dotnet/runtime

SteveDunn · 2021-06-08T06:42:28Z

ghost · 2021-06-08T06:42:34Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2021-06-08T06:42:35Z

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

Issue Details

Author:	SteveDunn
Assignees:	-
Labels:	`area-Extensions-Configuration`, `new-api-needs-documentation`
Milestone:	-

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

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

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

…rget. Remove the BindingException as that'd need another API review

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

src/libraries/Microsoft.Extensions.Primitives/src/Resources/Strings.resx

safern · 2021-06-14T20:46:46Z

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

            if (instance != null)
            {
-                foreach (PropertyInfo property in GetAllProperties(instance.GetType()))
+                List<PropertyInfo> modelProperties = GetAllProperties(instance.GetType()).ToList();


Why do you need the ToList()?

I see it is to enumerate the list twice. Maybe we can void it and check this as we bind the properties, aggregate the missing names on a list if the flag to throw is enabled and then throw?

If performance is the driving force behind this suggestion, then enumerating just once would be the best thing to do. I'm guessing though, that binding configuration won't be on a hot path. Hence, in my opinion, it is more beneficial this way as:

the list of properties has already been gathered (we're just enumerating twice, not building it twice)

it is logically self-contained; a distinct step before any binding is attempted

it fails fast: if we check as we bind we could end up with an object which has some properties that are set and some which aren't set

if we check as we bind, then the enumeration of property names in config would happen on BindProperty, which would result in more enumerations than we currently have

Those are valid points. Sounds good. I think since this is opt-in, I don't mind an extra enumeration, also Binding usually just happens once in a lifetime of the application.

What do you think of removing Linq usages? We have been trying to do that on these libraries.

I'm happy to remove Linq usages if that fits in with the plan for libraries. May I ask why though? I've certainly seen, in my day job, overuse of Linq especially in tight loops, which cause a lot of overhead for the GC with temporary objects. But outside of tight loops or hot paths, I think the declarative nature of Linq makes the code easier to understand.

I just looked into doing this imperatively by creating an empty List to hold the missing property names and found that we could end up with a sparse array, which I estimate would counter any gains achieved by switching from Linq. One the one hand, we create a zero-sized List, in which case the size doubles for every addition potentially leading to a sparse array, or we could create the List with the same size as the amount of children and assume everything is missing, in which case it's likely that not everything is missing and we'll again end up with a sparse array.

I don't think these scenarios are tragic though; whether it's the overhead of a Linq enumerator, or a few dozen empty slots in an array, this usually happens just once.

Bearing this in mind, I think it's no less efficient to use Linq, but If you'd like me to switch, then I'm happy to do so.

I don't mind an extra enumeration, also Binding usually just happens once in a lifetime of the application.

I think if you use IOptionsSnapshot for example then binding could happen once per request

Also, Microsoft.Extensions.Configuration has already used Linq to build the collection that we further 'compose'

The reason reducing Linq was brought up, has to do with this issue #44598, for example https://github.com/dotnet/runtime/pull/44825/files

I found this post by Andrew Lock:
https://andrewlock.net/the-dangers-and-gotchas-of-using-scoped-services-when-configuring-options-in-asp-net-core/

It says:

Note: As the strongly-typed settings are re-built with every request, and the binding relies on reflection under the hood, you should bear performance in mind. There is currently an open issue on GitHub to investigate performance.

It links to aspnet/Configuration#604

Seeing as Reflection is the slowest part of the process, I think the extra enumeration is fairly insignificant.

GetAllProperties is a private method that's typed as returning IEnumerable<PropertyInfo>, but its implementation is actually constructing and returning a List<PropertyInfo>. The right change here then is to just change that private method to return a List<PropertyInfo> and remove the ToList call from the call site.

safern · 2021-06-14T20:47:19Z

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

+                        .Select(mp => $"'{mp.Key}'")
+                        .ToList();
+
+                    if (missingPropertyNames.Any())


You are creating a List for these missing properties, you could use Length instead and avoid Linq.

I've used the Count property. After about 15 years of .NET, I too still cannot remember whether it's .Length or .Count :) Luckily, ReSharper knows!

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

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

safern

Thanks!

safern · 2021-06-17T00:06:51Z

runtime pipeline for this PR: https://dnceng.visualstudio.com/public/_build/results?buildId=1191246&view=results
linker tests for this PR: https://dnceng.visualstudio.com/public/_build/results?buildId=1191249&view=results
dev innerloop for this PR: https://dnceng.visualstudio.com/public/_build/results?buildId=1191247&view=results

safern · 2021-06-17T04:16:33Z

Thank you very much @SteveDunn! Feel free to keep picking issues and we'll be happy to help and merge your contributions.

ghost added area-Extensions-Configuration new-api-needs-documentation labels Jun 8, 2021

SteveDunn mentioned this pull request Jun 8, 2021

Add BinderOption to allow callers to specify that configuration binding should throw an exception upon any failure. #36015

Closed

safern requested review from maryamariyan and safern June 11, 2021 22:22

maryamariyan reviewed Jun 12, 2021

View reviewed changes

SteveDunn force-pushed the issue-36015-binderoption-to-throw-on-missing-configuration branch 2 times, most recently from 8b53c47 to ae0ae28 Compare June 14, 2021 06:31

SteveDunn added 6 commits June 14, 2021 07:33

Implementation

5847a71

Tidy up

b1efdb9

Revert some of the auto-generated ref.cs changes made by the build ta…

5550d3c

…rget. Remove the BindingException as that'd need another API review

Doc update

8d6dcc4

Use string resources instead of literals

03d8643

PR feedback

89de0f3

SteveDunn force-pushed the issue-36015-binderoption-to-throw-on-missing-configuration branch from ae0ae28 to 89de0f3 Compare June 14, 2021 06:33

maryamariyan suggested changes Jun 14, 2021

View reviewed changes

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

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

PR feedback

75a5f48

safern reviewed Jun 14, 2021

View reviewed changes

PR feedback

3892805

maryamariyan reviewed Jun 16, 2021

View reviewed changes

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

PR feedback

cb0e155

maryamariyan approved these changes Jun 16, 2021

View reviewed changes

safern reviewed Jun 16, 2021

View reviewed changes

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

safern approved these changes Jun 16, 2021

View reviewed changes

Remove ToList() call.

dd7929e

safern merged commit 33cf378 into dotnet:main Jun 17, 2021

ghost locked as resolved and limited conversation to collaborators Jul 17, 2021

Conversation

SteveDunn commented Jun 8, 2021 • edited by maryamariyan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 8, 2021

Uh oh!

ghost commented Jun 8, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveDunn Jun 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

safern left a comment

Choose a reason for hiding this comment

Uh oh!

safern commented Jun 17, 2021

Uh oh!

safern commented Jun 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SteveDunn commented Jun 8, 2021 •

edited by maryamariyan

Loading

SteveDunn Jun 15, 2021 •

edited

Loading