-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Update 7.0.md with implicit Required for ASP.NET Core 7 model binding #35007
Conversation
Could it be that nullable context wasn't enabled when you were targeting .NET 6? @Rick-Anderson or @brunolins16 If this model validation change is considered a breaking change, can you create a breaking change issue as per the usual workflow? |
Must have been missing See Non-nullable reference types and [Required] attribute This changed in 6.0, not 7.0 |
Nope, I have a repro here. This contains a project targeting both .NET 6 and .NET 7, running the same test suite against an ASP.NET Core site showing a successful test run in .NET 6, whilst failiing in .NET 7 for the exact same app. https://github.com/johnkors/aspnetcore7-compat-repro Filed this ticket at ASP.NET Core. |
@johnkors Thanks for the awesome test project, we'll get right on this. @serpent5 @fiyazbinhasan check out https://github.com/johnkors/aspnetcore7-compat-repro |
The following example controller also demonstrates the issue, which appears to be specific to the type being generic: [ApiController]
[Route("/")]
public class HomeController : ControllerBase
{
[HttpPost]
public IActionResult Post(TestModel<object> testModel)
=> Ok(testModel);
}
public class TestModel<T>
{
public object Value { get; set; }
} The same example, without generics, is consistent between .NET 6 and .NET 7: [ApiController]
[Route("/")]
public class HomeController : ControllerBase
{
[HttpPost]
public IActionResult Post(TestModel testModel)
=> Ok(testModel);
}
public class TestModel
{
public object Value { get; set; }
} From an initial investigation, I believe this may have changed in dotnet/aspnetcore#39219. Specifically, the .NET 6 code has this, which decides that the type isn't nullable, and therefore doesn't need an implicit // For generic types, inspecting the nullability requirement additionally requires
// inspecting the nullability constraint on generic type parameters. This is fairly non-triviial
// so we'll just avoid calculating it. Users should still be able to apply an explicit [Required]
// attribute on these members.
if (containingType.IsGenericType)
{
return false;
} In .NET 7, the approach changes to use var isOptional = nullability != null && nullability.ReadState != NullabilityState.NotNull; I was able to prove, in a local project, that |
@serpent5 you are totally right. We have updated the docs months ago. |
Nice! That note is exactly what I didn't find from the "breaking changes" sections. If it could be included, it would be helpful for us & others migrating. |
Closing this PR in favor of dotnet/AspNetCore.Docs#29059. |
Summary
This hit us when going from ASP.NET Core 6 to 7.
Nullability: enabled
A model sent to an API controller containing a
string
field was not treated as [Required] in ASP.NET Core 6, but was implicitly [Required] in 7.Mark: The docs say this is already present in ASP.NET Core 6, but this only hit us after migrating to 7, so something does not add up.
Internal previews