Skip to content

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