-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Issue 36015 new BinderOption to throw on missing configuration #53852
Merged
safern
merged 10 commits into
dotnet:main
from
SteveDunn:issue-36015-binderoption-to-throw-on-missing-configuration
Jun 17, 2021
Merged
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
5847a71
Implementation
SteveDunn b1efdb9
Tidy up
SteveDunn 5550d3c
Revert some of the auto-generated ref.cs changes made by the build ta…
SteveDunn 8d6dcc4
Doc update
SteveDunn 03d8643
Use string resources instead of literals
SteveDunn 89de0f3
PR feedback
SteveDunn 75a5f48
PR feedback
SteveDunn 3892805
PR feedback
SteveDunn cb0e155
PR feedback
SteveDunn dd7929e
Remove ToList() call.
safern File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need the
ToList()
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
BindProperty
, which would result in more enumerations than we currently haveThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-sizedList
, in which case the size doubles for every addition potentially leading to a sparse array, or we could create theList
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you use
IOptionsSnapshot
for example then binding could happen once per requestThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason reducing Linq was brought up, has to do with this issue #44598, for example https://github.com/dotnet/runtime/pull/44825/files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
It links to aspnet/Configuration#604
Seeing as Reflection is the slowest part of the process, I think the extra enumeration is fairly insignificant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetAllProperties is a private method that's typed as returning
IEnumerable<PropertyInfo>
, but its implementation is actually constructing and returning aList<PropertyInfo>
. The right change here then is to just change that private method to return aList<PropertyInfo>
and remove theToList
call from the call site.