dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
34.87k stars 9.85k forks source link

JsonPatchDocument.ApplyTo<T> should respect validation attributes on the target model #19170

Open jhardin-aptos opened 4 years ago

jhardin-aptos commented 4 years ago

We're adding JSON Patch support to an existing ASP.NET Core 2.2 REST API.

The models already have extensive use of custom validation attributes to validate the bodies of normal REST object-oriented HTTP POST and PATCH requests. This is working well.

However, when applying changes to a model object via JSON Patch JsonPatchDocument.ApplyTo<T>(), the validation attributes on the classes are ignored and invalid values can be applied to the object via a Replace operation, or to a new subobject via an Add operation, without any error being generated.

Note that this is distinct from checking the model state after the changes are applied, because depending on how the model classes are coded providing an invalid value for a property will not result in an invalid model. For example, in our model code providing an invalid value to an enum-based property does not cause the property value to change from the existing (valid) value, and does not cause the model class property setter to emit an exception, because if we did throw an exception there we'd get a generic error from the JSON deserializer rather than a specific error from the custom validation attribute we wrote for enumerations; the invalid data in the request does not invalidate the model.

Ideally, JsonPatchDocument.ApplyTo<T>() should respect any validation attributes on the properties of the object (and subobject(s)) being patched, and should echo any validation failure errors into the JsonPatchError collection it emits so they can be returned to the caller if desired.

This was already suggested in https://github.com/aspnet/JsonPatch/issues/18 and https://github.com/dotnet/aspnetcore/issues/7677 and the arguments against it were (in my opinion) weak. Manually coding an extra explicit TryValidateObject() should not be needed. Manually coding an extra explicit TryValidateModel() should not be needed (and again, may not work depending on how the class behaves when given invalid data, so https://github.com/dotnet/aspnetcore/issues/7158 is not a complete solution).

The assumption that the validation attributes will be automatically applied by JSON Patch the same way they are applied by a HTTP PATCH or POST of that model is reasonable, and should be supported.

mkArtakMSFT commented 4 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to happen for the coming release. We will reassess the backlog following the current release and consider this item at that time. However, keep in mind that there are many other high priority features with which it will be competing for resources.

ghost commented 1 year ago

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.