PATCH with ValidateModelState enabled and [Required] attribute

[milosloub]

If I provide ValidateModelState =true, then every PATCH failed on validation
on properties with [Required] attribute.
For example I have three properties with [Required], but sending patch to one of them only. Then
two of them failed on validation.

I found that AspNetCore uses JsonPatchDocument for this use case, but maybe I missed something.
Do you know how to deal with it?

• JsonApiDotNetCore Version: 3.0.1-alpha4

[jaredcnance]

@milosloub I don't have a solution for this off the top of my head. But, to be clear, the desired behavior is that for [Required] properties should only be validated for the attributes being PATCHed, ensuring their new values are not null or empty? It might be worth looking into migrating to a version of JsonPatchDocument<T> if that's what they use. Its API is a bit nicer than shoving the values into Dictionary<AttrAttribute, object> AttributesToUpdate on JsonApiContext. What do you think?

The nice part is that we might be able to implement this as a non-breaking change with C# 8's default interface methods.

[milosloub]

@jaredcnance yes, that's exactly that case - validating only PATCHed attributes. After few tries I discovered that JsonPatchDocument also doesn't solve this issue. Maybe I missed something general with [Required] and there must be some custom validation. So now I don't know, I'm back at the beginning

Ach sooo, this is the case for default interface methods (I was trying to find real world usage 😄)

[jaredcnance]

It looks like it should work but I’ll have to dig in more. It may be better to have a custom validation implementation instead of parsing into JsonPatchDocument which seems to be unnecessarily coupled to the JSON PATCH specification.

That said, I think there’s a good deal we can learn from their API design. In the future I would prefer to migrate to something similar instead of passing the metadata through the context.

[milosloub]

Yeah, it looks like it should work. I test the folowing
Controller

        // POST api/values
        [HttpPatch("{id}")]
        public IActionResult Patch(int id, [FromBody] JsonPatchDocument<MyModel> patchData)
        {
            var model = new MyModel();
            patchData.ApplyTo(model);

            if (!TryValidateModel(model))              
                return BadRequest(ModelState.Select(i => i.Value.Errors));

            return Ok();
        }

Model

    public class MyModel
    {
        [Required]
        public string FirstName { get; set; }

        [Required]
        public string LastName { get; set; }

        [Required]
        public int Age { get; set; }
    }

and request with Content-Type: application/json-patch+json

Still no luck.
But yes, I agree that custom validation approach would be better for JADNC

[milosloub]

Ah, I found my bad approach of testing this. It is required, that you have full object that proceed through POST and it's validation. In my case I only created new instance of empty model, than validation failed. If I use model with correctly filled properties, then solution above works as expected.

So instead of
var model = new MyModel();
I used

var model = new MyModel
{
    FirstName = "Ben",
    LastName = "Foster",
    Age = 30,
};

But still I think, that custom validation should be more effective than this "heavy" object approach. What do you think?

[jaredcnance]

Agreed. I don't want to re-invent the wheel, so it'd be interesting to see how JsonPatchDocument is handled. I'm not super familiar with the internals of the default model validation, but I wouldn't expect it to be too hard to figure out.

[medokin]

I found another problem maybe related to this.
Let's take this for example:

    public class Person
    {
        public int Id { get; set; }

        public string FirstName { get; set; }

        public Pet Pet { get; set; }
    }

    public class Pet
    {
        public int Id { get; set; }

        [Required]
        public string Name { get; set; }
    }

Now if try to post a new Person with an already existent Pet Id, the validation fails with Pet.Name - The Name field is required.

[mjartz]

Hi, I am also experiencing issues with this scenario.

Most of my resources contain basic validations that are checked in the BaseJsonApiController Post and Patch methods when the option to ValidateModelState is true. As above, if I patch a subset of fields and there are other fields that are flagged as required, I am getting a validation error stating as such, even though I am not modifying those fields.

I also came across a scenario where Patch is failing because I have validations that are dependent on multiple fields in the resource. If I patch only one of those fields, the validations will fail because dependent data is not present, even though the eventual update would be valid.

E.g.
A user entity where both names must be either null or not null together.
If there is an existing user which has both a first and last name and I update the first name only to a different value, the validation will fail because it thinks the last name is null even though in the DB it is not.

public class User : Identifiable<string>, IValidatableObject
{
    [MaxLength(50)]
    public string FirstName { get; set; }
    [MaxLength(50)]
    public string LastName { get; set; }
    
    public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
    {
        var errors = new List<ValidationResult>();
        if ((FirstName != null && LastName == null) || (FirstName == null && LastName != null))
            errors.Add(new ValidationResult("First and last name must both be supplied."));
        return errors;
    }
}

I did play around with overriding the PatchAsync method in a custom controller where I retrieved the entity from the DB and patched in the values not in the targeted fields list to then be able to validate a 'completed entity'. This worked ok for basic scenarios, but i suspect there might be issues or constraints with this method, particularly with relationships / included resources.

I guess the only other option would be to turn off ValidateModelState and delay and manage the validations in my DbContext prior to SaveChanges where all entities should be populated?

Regards,