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.

Sign up for GitHub

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

Jump to bottom

Ensure Bind can handle null from GetSection #92384

Merged
merged 3 commits into from
Sep 22, 2023
Merged

Ensure Bind can handle null from GetSection #92384

merged 3 commits into from
Sep 22, 2023

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Sep 20, 2023

Fixes #92213

IConfiguration instances may return a null value from GetSection. We were not handling this and would throw a NullReferenceException.

@ericstj
Ensure Bind can handle null from GetSection
181a558 
IConfiguration instances may return a null value from GetSection.
We were not handling this and would throw a NullReferenceException.
@ericstj ericstj requested review from krwq, layomia and tarekgh September 20, 2023 22:44
@ghost ghost assigned ericstj Sep 20, 2023
@ghost
Copy link

ghost commented Sep 20, 2023

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

Issue Details

Fixes #92213

IConfiguration instances may return a null value from GetSection. We were not handling this and would throw a NullReferenceException.

Author: ericstj
Assignees: ericstj
Labels:

area-Extensions-Configuration
Milestone: -

ericstj
ericstj commented Sep 20, 2023
View reviewed changes
src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs Outdated
@@ -298,6 +298,11 @@ private static void BindProperty(PropertyInfo property, object instance, IConfig
return;
}

if (config == null)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously the entire block below was wrapped in a null check - 35f043f#diff-2e6f75e1d4f577e1c8c1b6a32d158ce7812a38ad8fcd3b4d762157b92c03e890L308-R315

Since this method already has multiple returns, I find it more readable to early-exit rather than count curly-brackets/indentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good to me.

@RussKie
Copy link
Member

RussKie commented Sep 21, 2023

Thank you!

tarekgh
tarekgh approved these changes Sep 21, 2023
View reviewed changes
Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added minor comment/suggestion, LGTM otherwise. Thanks for fixing this.

@ericstj
Copy link
Member Author

ericstj commented Sep 21, 2023

@RussKie - if you wanted to workaround this you can register a mock section as the default return value for GetSection as follows:

mockConfSection.Setup(m => m.GetSection(It.IsAny<string>())).Returns(emptySection.Object);

That would ensure that GetSection doesn't return null. We definitely want to fix this though, since it's a regression.

@RussKie
Copy link
Member

RussKie commented Sep 21, 2023

@RussKie Igor Velikorossov FTE - if you wanted to workaround this you can register a mock section as the default return value for GetSection as follows:

mockConfSection.Setup(m => m.GetSection(It.IsAny<string>())).Returns(emptySection.Object);

That would ensure that GetSection doesn't return null. We definitely want to fix this though, since it's a regression.

👍

@build-analysis build-analysis bot mentioned this pull request Sep 21, 2023
Closed
@ericstj
Copy link
Member Author

ericstj commented Sep 22, 2023

Remaining legs are outer loop build incorrectly triggered for all PRs

@ericstj ericstj merged commit 1ea868f into dotnet:main Sep 22, 2023
117 of 123 checks passed
@build-analysis build-analysis bot mentioned this pull request Sep 22, 2023
Closed
@ericstj
Copy link
Member Author

ericstj commented Sep 22, 2023

/backport to release/8.0

@github-actions
Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6268341143

This was referenced Sep 22, 2023
Open
Closed
@github-actions github-actions bot mentioned this pull request Sep 22, 2023
Closed
@ericstj
Copy link
Member Author

ericstj commented Sep 22, 2023

/backport to release/8.0-rc2

@github-actions
Copy link
Contributor

Started backporting to release/8.0-rc2: https://github.com/dotnet/runtime/actions/runs/6275041479

@github-actions github-actions bot mentioned this pull request Sep 22, 2023
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Oct 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@RussKie RussKie RussKie left review comments

@stephentoub stephentoub stephentoub left review comments

@krwq krwq krwq left review comments

@tarekgh tarekgh tarekgh approved these changes

@layomia layomia Awaiting requested review from layomia

Assignees

@ericstj ericstj

Labels
area-Extensions-Configuration
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

NRE in ConfigurationBinder.BindInstance
5 participants
@ericstj @RussKie @krwq @stephentoub @tarekgh