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] Fix nullable annotation for Validator.TryValidateValue and ValidateValue #91293

Merged
merged 4 commits into from
Aug 31, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Aug 29, 2023

Backport of #91286 to release/8.0

/cc @jeffhandley

Customer Impact

System.ComponentModel.DataAnnotations.Validator has methods for validating individual values against a list of validation attributes; they are ValidateValue and TryValidateValue. Those two methods are annotated as not allowing null values to be validated, and the XML doc comments indicate the value cannot be null as well. The annotation and comment were incorrect though, with the XML doc comment likely copied and pasted from the Object validation members long ago.

Customers haven't historically called these methods directly, with it being more of a framework-level concern to orchestrate validation. With the Options Validation source generator introduced in .NET 8 though, we are now generating code into customers' projects that calls these methods directly so that Reflection can be bypassed at runtime. With the incorrect nullable annotation, we have to generate code that includes a ! on the argument value. Taking this fix in .NET 8, the source generator can potentially stop emitting the ! into the generated code when targeting net8+. (That change is being considered but is not included in this PR).

Testing

Compilation and unit tests succeed. A search across other dotnet org repos found no call sites to update.

Risk

Low. Only affects nullable annotation behavior at design-time, and this relaxes an erroneous annotation in a non-breaking way.

@dotnet-issue-labeler dotnet-issue-labeler bot added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners new-api-needs-documentation labels Aug 29, 2023
@dotnet-issue-labeler
Copy link

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, 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.

@carlossanlop carlossanlop added area-System.ComponentModel.DataAnnotations and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Aug 29, 2023
@ghost
Copy link

ghost commented Aug 29, 2023

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

Issue Details

Backport of #91286 to release/8.0

/cc @jeffhandley

Customer Impact

System.ComponentModel.DataAnnotations.Validator has methods for validating individual values against a list of validation attributes; they are ValidateValue and TryValidateValue. Those two methods are annotated as not allowing null values to be validated, and the XML doc comments indicate the value cannot be null as well. The annotation and comment were incorrect though, with the XML doc comment likely copied and pasted from the Object validation members long ago.

Customers haven't historically called these methods directly, with it being more of a framework-level concern to orchestrate validation. With the Options Validation source generator introduced in .NET 8 though, we are now generating code into customers' projects that calls these methods directly so that Reflection can be bypassed at runtime. With the incorrect nullable annotation, we have to generate code that includes a ! on the argument value. Taking this fix in .NET 8, we can stop generating the superfluous ! as customers adopt the source generator.

Testing

Pending: #91286 (comment)

Risk

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

area-System.ComponentModel.DataAnnotations, new-api-needs-documentation

Milestone: -

@jeffhandley jeffhandley added the Servicing-approved Approved for servicing release label Aug 31, 2023
@jeffhandley
Copy link
Member

@carlossanlop This one's ready for release/8.0

@carlossanlop carlossanlop merged commit 9c422ed into release/8.0 Aug 31, 2023
106 of 107 checks passed
@carlossanlop carlossanlop deleted the backport/pr-91286-to-release/8.0 branch August 31, 2023 22:26
@radical radical mentioned this pull request Sep 26, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants