JsonPatchDocument.ApplyTo<T> should validate target model

[akalcik]

When ModelState.IsValid is called on model which is provided as a input parameter and the model isn't valid, ProblemDetails is generated and provided as a body output by all operations with status code >= 400.

        [HttpPost]
        public async Task<ActionResult<ToDoModel>> Create(ToDoInsertModel toDoToInsert)
        {
            if (!ModelState.IsValid || toDoToInsert == null) { return BadRequest(); }

            var toDo = _mapper.Map<ToDoModel>(toDoToInsert);
            await _toDoAppDbContext.ToDos.AddAsync(toDo);
            await _toDoAppDbContext.SaveChangesAsync();

            return CreatedAtAction(nameof(Get), new {id = toDo.Id}, toDo);
        }

In case above when ToDoInsertModel validation fails because Description is null, BadRequest returns following response:

HTTP/1.1 400 Bad Request
Transfer-Encoding: chunked
Content-Type: application/json; charset=utf-8
Server: Microsoft-IIS/10.0
X-SourceFiles: =?UTF-8?B?QzpcUHJvamVjdHNcYW50b25rYWxjaWstYXZhbmFkZS52aXN1YWxzdHVkaW8uY29tXFRvRG9BcHBcc3JjXFRvRG9BcHBTb2x1dGlvblxUb0RvQXBwLkFwaVx0b2Rvc1w0MA==?=
X-Powered-By: ASP.NET
Date: Thu, 31 Jan 2019 15:30:12 GMT

{
  "errors": {
    "Description": [
      "The Description field is required."
    ]
  },
  "title": "One or more validation errors occurred.",
  "status": 400,
  "traceId": "800000b1-0002-f100-b63f-84710c7967bb"
}

Now implementation of HttpPatch Method with JsonPatchDocument.ApplyTo<T>.

        [Route("{id:int}")]
        [HttpPatch]
        public async Task<ActionResult<ToDoModel>> Modify(int id, JsonPatchDocument<ToDoModifyModel> patch)
        {
            ToDoModel toDoDatabaseEntry = await _toDoAppDbContext.ToDos.SingleOrDefaultAsync(toDo => toDo.Id == id);

            if (toDoDatabaseEntry == null) { return NotFound(); }

            var toDoFromDatabase = _mapper.Map<ToDoModifyModel>(toDoDatabaseEntry);
            patch.ApplyTo(toDoFromDatabase, ModelState);
            TryValidateModel(toDoFromDatabase);

            if (!ModelState.IsValid) { return BadRequest(ModelState); }

            _toDoAppDbContext.Entry(toDoDatabaseEntry).CurrentValues.SetValues(toDoFromDatabase);
            await _toDoAppDbContext.SaveChangesAsync();

            return toDoDatabaseEntry;
        }

The output is different.

HTTP/1.1 400 Bad Request
Transfer-Encoding: chunked
Content-Type: application/json; charset=utf-8
Server: Microsoft-IIS/10.0
X-SourceFiles: =?UTF-8?B?QzpcUHJvamVjdHNcYW50b25rYWxjaWstYXZhbmFkZS52aXN1YWxzdHVkaW8uY29tXFRvRG9BcHBcc3JjXFRvRG9BcHBTb2x1dGlvblxUb0RvQXBwLkFwaVx0b2Rvc1w2?=
X-Powered-By: ASP.NET
Date: Thu, 31 Jan 2019 15:33:15 GMT

{
  "Description": [
    "The Description field is required."
  ]
}

The reason for this is that patch.ApplyTo(toDoFromDatabase, ModelState); doesn't invoke validation on the target model, which should be expected behavior. Because of this TryValidateModel(toDoFromDatabase); need to be called explicitly, and ModeState object must be provided as an input for BadRequest object, which leads to inconsistent output. Need to notice when you don't provide ModeState you don't get any validation details errors.

[akalcik]

Seems to be relevant #6077

[milosloub]

Same problem here. I'm also using overload patch.ApplyTo(model, ModelState); and if(!ModelState.IsValid) leads to unexpected behavior

[mkArtakMSFT]

Thanks for contacting us, @akalcik. Feel free to send a PR for this and we'll happily consider it. As this will potentially be a breaking change, this should be backed by some compatibility option.

[TanayParikh]

Related to: https://github.com/dotnet/aspnetcore/issues/19170

[rafikiassumani-msft]

Assigned to @pranavkm for further conversations on JsonPatch support.

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.