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

Fix nullability of TryUpdateModelAsync #41959

Merged
merged 3 commits into from
Jun 17, 2022
Merged

Fix nullability of TryUpdateModelAsync #41959

merged 3 commits into from
Jun 17, 2022

Conversation

halter73
Copy link
Member

@halter73 halter73 commented Jun 1, 2022

This will be source-breaking (especially since arrays and Expressions are not covariant), so we should make an announcement if we change this. That said, the nu.labilty was wrong before and we have generally decided to fix nullability mistakes.

We strive to get annotations correct the "first time" and do due-diligence in an attempt to do so. However, we acknowledge that we are likely to need to augment and change some annotations in the future:

  • Mistakes. Given the sheer number of APIs being reviewed and annotated, we are likely to make some errors, and we'd like to be able to fix them so that long-term customers get the greatest benefit.

https://github.com/dotnet/runtime/blob/8a043bf7adb0fbf5e60a8dd557c98686bc0a8377/docs/coding-guidelines/api-guidelines/nullability.md#breaking-change-guidance

Fixes #41886

This will also address the nullability fix described in #41929. We're keeping that open to track the breaking change announcements though.

Edit: Just kidding. Our public API check complains of all of the following because the nullable setter has nothing in the annotation indicating it's nullable. So you have to leave it as-is or try "removing" and readding it in PublicAPI.Unshipped.txt. Neither works.

  • "WebRootPath.set -> void' appears more than once in the public API files"
  • "WebRootPath.set' doesn't match implicitly implemented member 'void IHostingEnvironment.WebRootPath.set' (possibly because of nullability attributes)"
  • "WebRootPath.set -> void' is marked as removed but it isn't deleted in source code"

I'll submit a separate PR and see if we can force it through in PublicAPI.Shipped.txt or something.

@halter73 halter73 added feature-model-binding area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jun 1, 2022
@halter73 halter73 requested review from brunolins16 and a team June 1, 2022 01:27
@halter73 halter73 requested a review from a team as a code owner June 1, 2022 01:27
Copy link
Member

@brunolins16 brunolins16 left a comment

Choose a reason for hiding this comment

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

LGTM! Just need to fix the PublicAPI.txt issues. 🚀

@halter73 halter73 merged commit 375f5a4 into main Jun 17, 2022
@halter73 halter73 deleted the halter73/41886 branch June 17, 2022 07:04
@ghost ghost added this to the 7.0-preview6 milestone Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels feature-model-binding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TryUpdateModelAsync() doesn't account for Nullable Reference Types in navigation properties of the model
3 participants