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

Consider the impact of C# 11's required properties on model binding / validation #40099

Open
pranavkm opened this issue Feb 9, 2022 · 4 comments
Assignees
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Milestone

Comments

@pranavkm
Copy link
Contributor

pranavkm commented Feb 9, 2022

Feature: https://github.com/dotnet/csharplang/blob/main/proposals/required-members.md

Using this keyword appears as an attribute in the generated code. We could envision reading it and treated it as the equivalent of BindRequiredAttribute (this property must have a value).

@pranavkm pranavkm added the area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Feb 9, 2022
@rafikiassumani-msft rafikiassumani-msft added this to the .NET 7 Planning milestone Feb 10, 2022
@ghost
Copy link

ghost commented Feb 10, 2022

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@rafikiassumani-msft rafikiassumani-msft added the Needs: Design This issue requires design work before implementating. label Feb 10, 2022
@ghost
Copy link

ghost commented Oct 11, 2022

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@captainsafia
Copy link
Member

Another thing to add to the list now that we have support for AsParameters in minimal is to make sure that the required keyword is respected on properties that are passed in with the optionality checks that are provided in the framework.

@captainsafia
Copy link
Member

captainsafia commented Nov 9, 2022

@brunolins16 and I just met to discuss this.

One thing we noted in our investigation is that it is important to have nullability disabled while assessing the impacted of the required property, since our support for inferring requiredness from nullability can mask behaviors.

Some other notes include:

  • We need to add a check for RequiredMember in the PropertyAsParameterInfo implementation.
  • We discovered a bug in MVC model binding where when nullability is enabled and the validation layer infers that a parameters is required, model binding will still assume it is option. This is unrelated to the required members support but is a bug in our binding logic in general. @brunolins16 will file an issue.
  • For FromBody arguments in MVC, model binding will fail appropriately when the user fails to provide a required member because the serializer will fail.
    • Update: we decided that we should have more first class support for RequiredMember in the binder so we're not relying on the behavior of the serializer to respect the property.
  • For FromQuery and FromRoute arguments in a model, MVC will not respect the RequriedMember attribute.

@captainsafia captainsafia removed the Needs: Design This issue requires design work before implementating. label Nov 9, 2022
@captainsafia captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates and removed area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jun 20, 2023
@captainsafia captainsafia modified the milestones: .NET 8 Planning, Backlog Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
No open projects
Status: No status
Development

No branches or pull requests

3 participants