Skip to content

ModelState validation changes when adding a new property to an existing model #41687

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

Closed
1 task done
bkoelman opened this issue May 14, 2022 · 1 comment
Closed
1 task done
Labels
feature-model-binding old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels

Comments

@bkoelman
Copy link

bkoelman commented May 14, 2022

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

Adding a new property to an existing model changes the required-ness of another existing (unmodified) property when nullable reference types are enabled. As a result, the outcome of ModelState validation changes.

Expected Behavior

Adding a new property should not change the validation of another existing (unmodified) property.

Steps To Reproduce

  • In Visual Studio 2022, create a new API project: File > New > Project > ASP.NET Core Web API (accept defaults).
  • Add the following code to a new file in the project:
    public abstract class Identifiable<TId>
    {
        public virtual TId Id { get; set; } = default!;
    
        //public string? Version { get; set; }
    }
    
    public class Customer : Identifiable<string?>
    {
        public string Name { get; set; } = null!;
    
        [Range(1, 150)]
        public int Age { get; set; }
    }
  • Add the next method to Controllers\WeatherForecastController.cs:
    [HttpPost]
    public virtual IActionResult Post([FromBody] Customer customer)
    {
        if (!ModelState.IsValid)
        {
            return BadRequest(ModelState);
        }
    
        return Ok($"Hello '{customer.Name}' with ID '{customer.Id}'.");
    }
  • Run the application.
  • From the Swagger UI in the browser, expand "POST WeatherForecast" and click Try it out. Replace the request body with {} and click Execute.
  • Note the response contains two errors:
      "errors": {
        "Age": [
          "The field Age must be between 1 and 150."
        ],
        "Name": [
          "The Name field is required."
        ]
      }
  • Stop the application and uncomment the Version property from the first code snippet.
  • Run the application and execute the same POST request from Swagger UI in the browser.
  • Note the response now contains three errors:
      "errors": {
        "Id": [
          "The Id field is required."
        ],
        "Age": [
          "The field Age must be between 1 and 150."
        ],
        "Name": [
          "The Name field is required."
        ]
      }

Exceptions (if any)

No response

.NET Version

6.0.203

Anything else?

I traced this down to DataAnnotationsMetadataProvider. Uncommenting the Version property makes the C# compiler switch from [NullableAttribute] to [NullableContextAttribute], causing the metadata provider to bail out early due to the use of generics here (or gets it wrong here, I don't remember exactly).

This appears to be fixed in #39219. So I've tested with .NET 7 Preview4 and can confirm the problem no longer occurs there.

Feel free to close this issue, as the problem is already solved. I just thought it would be good to log this for awareness and traceability.

I don't think it makes sense to backport this to .NET 6, because NullabilityInfoContext doesn't behave correctly in the .NET 6 runtime version:

var property = typeof(Customer).GetProperty("Id")!;

var nullContext = new NullabilityInfoContext();
var nullability = nullContext.Create(property);
var isOptional = nullability.ReadState != NullabilityState.NotNull;

// .NET 6 v6.0.300:
// class Customer : Identifiable<string> => isOptional: true
// class Customer : Identifiable<string?> => isOptional: true

// .NET 7 Preview 4:
// class Customer : Identifiable<string> => isOptional: false
// class Customer : Identifiable<string?> => isOptional: true
@brunolins16
Copy link
Member

@bkoelman Thanks for reporting the issue. As you mentioned this behavior was changed in .NET 7 when we moved to NullabilityInfoContext instead of the attributes lookup and we should have this issue fixed.

Feel free to reopen or create a new issue if you see any other related problems.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-model-binding old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
Development

No branches or pull requests

3 participants