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

[release/8.0] Ensure Bind can handle null from GetSection #92450

Closed
wants to merge 3 commits into from

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 22, 2023

Backport of #92384 to release/8.0

/cc @ericstj

Customer Impact

NullReferenceException when an IConfiguration instance returns null from GetSection. No implementations in the product do this, and the interface is not annotated to return null, however we received a customer report of this regression from mocking the interface. When using moq to mock an interface it will return null for unspecified cases.

Testing

Tested customer reported case manually using Moq. Added automated test for both runtime binder and source gen to catch this behavior.

Risk

Very low - we are only adding back a null check that was removed from a previous change in RC2.

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

ghost commented Sep 22, 2023

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

Issue Details

Backport of #92384 to release/8.0

/cc @ericstj

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-Extensions-Configuration

Milestone: -

@carlossanlop
Copy link
Member

@ericstj this is targeting release/8.0. Intended? We could try to get it in to RC2.

@ericstj
Copy link
Member

ericstj commented Sep 22, 2023

@ericstj this is targeting release/8.0. Intended? We could try to get it in to RC2.

I see - I was thinking it might be tight to get it in RC2 and this isn't a major blocker since our implementations of configuration will never return null - the interface itself is not annotated to return null.

I'll open an RC2 PR which we can merge if it's ready in time and doesn't reset anything. Otherwise we'll use this one.

@ericstj ericstj added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Sep 22, 2023
@ericstj
Copy link
Member

ericstj commented Sep 22, 2023

Marked this as no-merge for now. We'll only use this PR if we decide not to take in RC2.

@carlossanlop
Copy link
Member

The RC2 PR was merged, so this will flow to RTM automatically. We can close this now. #92477

@carlossanlop carlossanlop deleted the backport/pr-92384-to-release/8.0 branch September 22, 2023 17:26
@ericstj
Copy link
Member

ericstj commented Sep 22, 2023

Thanks @carlossanlop

@ghost ghost locked as resolved and limited conversation to collaborators Oct 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Configuration NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants