JsonPatchDocument.ApplyTo With ModelState Throws Exceptions

[RehanSaeed]

The overload of ApplyTo with ModelState throws exceptions:

• ArgumentException when the patch document is invalid e.g. empty op property or typo in the op property.
• InvalidOperationException when using a value of test for the op property.

I expected it to add an error into the ModelState instead. Right now, I'm having to wrap it in a try-catch and add ModelState errors manually.

I've resorted to writing the following extension method to add errors instead of throw exceptions:

public static class JsonPatchDocumentExtensions
{
    public static void SafeApplyTo<T>(this JsonPatchDocument<T> patch, T value, ModelStateDictionary modelState)
        where T : class
    {
        try
        {
            patch.ApplyTo(value, modelState);
        }
        catch (ArgumentException argumentException)
        {
            modelState.AddModelError(
                typeof(T).Name,
                argumentException.Message);
        }
        catch (NotSupportedException notSupportedException)
        {
            modelState.AddModelError(
                typeof(T).Name,
                notSupportedException.Message);
        }
    }
}

Unit tests:

public class JsonPatchDocumentExtensionsTest
{
    [Fact]
    public void SafeApplyTo_ValidPatchParameters_NoErrorsAdded()
    {
        var patch = new JsonPatchDocument<Model>();
        patch.Operations.Add(new Operation<Model>("replace", "/Property", null, "Foo"));
        var model = new Model();
        var modelState = new ModelStateDictionary();

        patch.SafeApplyTo(model, modelState);

        Assert.Equal(0, modelState.ErrorCount);
    }

    [Theory]
    [InlineData("replace", "/PropertyNotFound", null, "Foo", "The property at path '/PropertyNotFound' could not be removed.")]
    [InlineData("test", "/Property", null, "Foo", "The test operation is not supported.")]
    [InlineData("invalid", "/Property", null, "Foo", "Requested value 'invalid' was not found.")]
    [InlineData("", "/Property", null, "Foo", "Must specify valid information for parsing in the string.")]
    public void SafeApplyTo_PatchParameters_AddsModelStateError(
        string op,
        string path,
        string from,
        string value,
        string error)
    {
        var patch = new JsonPatchDocument<Model>();
        patch.Operations.Add(new Operation<Model>(op, path, from, value));
        var model = new Model();
        var modelState = new ModelStateDictionary();

        patch.SafeApplyTo(model, modelState);

        Assert.Equal(1, modelState.ErrorCount);
        Assert.Equal(nameof(Model), modelState.First().Key);
        Assert.Equal(1, modelState.First().Value.Errors.Count);
        Assert.Equal(error, modelState.First().Value.Errors.First().ErrorMessage);
    }

    public class Model
    {
        public string Property { get; set; }
    }
}

[rynowak]

Thanks for opening this. I agree, we should not be throwing based on user input, and that's what this does.

[kichalla]

aspnet/JsonPatch@81931e7