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

Update 7.0.md with implicit Required for ASP.NET Core 7 model binding #35007

Closed
wants to merge 1 commit into from

Conversation

johnkors
Copy link
Contributor

@johnkors johnkors commented Apr 14, 2023

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

📄 File 🔗 Preview link
docs/core/compatibility/7.0.md Breaking changes in .NET 7

@johnkors johnkors requested a review from gewarren as a code owner April 14, 2023 12:26
@dotnet-bot dotnet-bot added this to the April 2023 milestone Apr 14, 2023
@ghost ghost added the community-contribution Indicates PR is created by someone from the .NET community. label Apr 14, 2023
@gewarren
Copy link
Contributor

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?

@Rick-Anderson
Copy link
Contributor

Must have been missing
<Nullable>enable</Nullable>

See Non-nullable reference types and [Required] attribute

This changed in 6.0, not 7.0

@johnkors
Copy link
Contributor Author

johnkors commented Apr 19, 2023

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.

@Rick-Anderson
Copy link
Contributor

Rick-Anderson commented Apr 19, 2023

@johnkors Thanks for the awesome test project, we'll get right on this.

@serpent5 @fiyazbinhasan check out https://github.com/johnkors/aspnetcore7-compat-repro

@serpent5
Copy link
Contributor

serpent5 commented Apr 20, 2023

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 [Required]:

// 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 NullabilityInfoContext, which declares that the type is nullable and adds the implicit [Required]. I believe that decision is made here:

var isOptional = nullability != null && nullability.ReadState != NullabilityState.NotNull;

I was able to prove, in a local project, that nullability.ReadState is set to NullabilityState.NotNull, which evaluates to the attribute being added.

@brunolins16
Copy link
Member

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 [Required]:

// 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 NullabilityInfoContext, which declares that the type is nullable and adds the implicit [Required]. I believe that decision is made here:

var isOptional = nullability != null && nullability.ReadState != NullabilityState.NotNull;

I was able to prove, in a local project, that nullability.ReadState is set to NullabilityState.NotNull, which evaluates to the attribute being added.

@serpent5 you are totally right. We have updated the docs months ago.
dotnet/AspNetCore.Docs#25840

@johnkors
Copy link
Contributor Author

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.

@gewarren
Copy link
Contributor

Closing this PR in favor of dotnet/AspNetCore.Docs#29059.

@gewarren gewarren closed this Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution Indicates PR is created by someone from the .NET community. dotnet-fundamentals/svc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants