From 81931e75d48370cf9163254523b1f6c4bcc92acb Mon Sep 17 00:00:00 2001 From: Kiran Challa Date: Mon, 31 Oct 2016 15:06:25 -0700 Subject: [PATCH] Handle exceptions for invalid operation types Related to https://github.com/aspnet/Mvc/issues/5463 --- .../Adapters/ObjectAdapter.cs | 26 +++--- .../Internal/ErrorReporter.cs | 16 ++++ .../JsonPatchDocument.cs | 18 +++- .../JsonPatchDocumentOfT.cs | 18 +++- .../Operations/OperationBase.cs | 10 +- .../Operations/OperationOfT.cs | 3 +- .../Properties/Resources.Designer.cs | 16 ++++ .../Resources.resx | 3 + .../JsonPatchDocumentTest.cs | 91 +++++++++++++++++++ 9 files changed, 182 insertions(+), 19 deletions(-) create mode 100644 src/Microsoft.AspNetCore.JsonPatch/Internal/ErrorReporter.cs create mode 100644 test/Microsoft.AspNetCore.JsonPatch.Test/JsonPatchDocumentTest.cs diff --git a/src/Microsoft.AspNetCore.JsonPatch/Adapters/ObjectAdapter.cs b/src/Microsoft.AspNetCore.JsonPatch/Adapters/ObjectAdapter.cs index ee01fbeba7..0ac2ebbe56 100644 --- a/src/Microsoft.AspNetCore.JsonPatch/Adapters/ObjectAdapter.cs +++ b/src/Microsoft.AspNetCore.JsonPatch/Adapters/ObjectAdapter.cs @@ -150,14 +150,14 @@ namespace Microsoft.AspNetCore.JsonPatch.Adapters if (!visitor.TryVisit(ref target, out adapter, out errorMessage)) { var error = CreatePathNotFoundError(objectToApplyTo, path, operation, errorMessage); - ReportError(error); + ErrorReporter(error); return; } if (!adapter.TryAdd(target, parsedPath.LastSegment, ContractResolver, value, out errorMessage)) { var error = CreateOperationFailedError(objectToApplyTo, path, operation, errorMessage); - ReportError(error); + ErrorReporter(error); return; } } @@ -259,14 +259,14 @@ namespace Microsoft.AspNetCore.JsonPatch.Adapters if (!visitor.TryVisit(ref target, out adapter, out errorMessage)) { var error = CreatePathNotFoundError(objectToApplyTo, path, operationToReport, errorMessage); - ReportError(error); + ErrorReporter(error); return; } if (!adapter.TryRemove(target, parsedPath.LastSegment, ContractResolver, out errorMessage)) { var error = CreateOperationFailedError(objectToApplyTo, path, operationToReport, errorMessage); - ReportError(error); + ErrorReporter(error); return; } } @@ -312,14 +312,14 @@ namespace Microsoft.AspNetCore.JsonPatch.Adapters if (!visitor.TryVisit(ref target, out adapter, out errorMessage)) { var error = CreatePathNotFoundError(objectToApplyTo, operation.path, operation, errorMessage); - ReportError(error); + ErrorReporter(error); return; } if (!adapter.TryReplace(target, parsedPath.LastSegment, ContractResolver, operation.value, out errorMessage)) { var error = CreateOperationFailedError(objectToApplyTo, operation.path, operation, errorMessage); - ReportError(error); + ErrorReporter(error); return; } } @@ -401,29 +401,25 @@ namespace Microsoft.AspNetCore.JsonPatch.Adapters if (!visitor.TryVisit(ref target, out adapter, out errorMessage)) { var error = CreatePathNotFoundError(objectToGetValueFrom, fromLocation, operation, errorMessage); - ReportError(error); + ErrorReporter(error); return false; } if (!adapter.TryGet(target, parsedPath.LastSegment, ContractResolver, out propertyValue, out errorMessage)) { var error = CreateOperationFailedError(objectToGetValueFrom, fromLocation, operation, errorMessage); - ReportError(error); + ErrorReporter(error); return false; } return true; } - private void ReportError(JsonPatchError error) + private Action ErrorReporter { - if (LogErrorAction != null) + get { - LogErrorAction(error); - } - else - { - throw new JsonPatchException(error); + return LogErrorAction ?? Internal.ErrorReporter.Default; } } diff --git a/src/Microsoft.AspNetCore.JsonPatch/Internal/ErrorReporter.cs b/src/Microsoft.AspNetCore.JsonPatch/Internal/ErrorReporter.cs new file mode 100644 index 0000000000..76b55a6144 --- /dev/null +++ b/src/Microsoft.AspNetCore.JsonPatch/Internal/ErrorReporter.cs @@ -0,0 +1,16 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using Microsoft.AspNetCore.JsonPatch.Exceptions; + +namespace Microsoft.AspNetCore.JsonPatch.Internal +{ + internal static class ErrorReporter + { + public static readonly Action Default = (error) => + { + throw new JsonPatchException(error); + }; + } +} diff --git a/src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocument.cs b/src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocument.cs index 72e93e0302..875134ed9d 100644 --- a/src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocument.cs +++ b/src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocument.cs @@ -5,6 +5,7 @@ using System; using System.Collections.Generic; using Microsoft.AspNetCore.JsonPatch.Adapters; using Microsoft.AspNetCore.JsonPatch.Converters; +using Microsoft.AspNetCore.JsonPatch.Exceptions; using Microsoft.AspNetCore.JsonPatch.Internal; using Microsoft.AspNetCore.JsonPatch.Operations; using Newtonsoft.Json; @@ -170,7 +171,22 @@ namespace Microsoft.AspNetCore.JsonPatch throw new ArgumentNullException(nameof(objectToApplyTo)); } - ApplyTo(objectToApplyTo, new ObjectAdapter(ContractResolver, logErrorAction)); + var adapter = new ObjectAdapter(ContractResolver, logErrorAction); + foreach (var op in Operations) + { + try + { + op.Apply(objectToApplyTo, adapter); + } + catch (JsonPatchException jsonPatchException) + { + var errorReporter = logErrorAction ?? ErrorReporter.Default; + errorReporter(new JsonPatchError(objectToApplyTo, op, jsonPatchException.Message)); + + // As per JSON Patch spec if an operation results in error, further operations should not be executed. + break; + } + } } /// diff --git a/src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocumentOfT.cs b/src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocumentOfT.cs index afb5eb3ff1..fc660b2b45 100644 --- a/src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocumentOfT.cs +++ b/src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocumentOfT.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; using System.Linq.Expressions; using Microsoft.AspNetCore.JsonPatch.Adapters; using Microsoft.AspNetCore.JsonPatch.Converters; +using Microsoft.AspNetCore.JsonPatch.Exceptions; using Microsoft.AspNetCore.JsonPatch.Internal; using Microsoft.AspNetCore.JsonPatch.Operations; using Newtonsoft.Json; @@ -648,7 +649,22 @@ namespace Microsoft.AspNetCore.JsonPatch throw new ArgumentNullException(nameof(objectToApplyTo)); } - ApplyTo(objectToApplyTo, new ObjectAdapter(ContractResolver, logErrorAction)); + var adapter = new ObjectAdapter(ContractResolver, logErrorAction); + foreach (var op in Operations) + { + try + { + op.Apply(objectToApplyTo, adapter); + } + catch (JsonPatchException jsonPatchException) + { + var errorReporter = logErrorAction ?? ErrorReporter.Default; + errorReporter(new JsonPatchError(objectToApplyTo, op, jsonPatchException.Message)); + + // As per JSON Patch spec if an operation results in error, further operations should not be executed. + break; + } + } } /// diff --git a/src/Microsoft.AspNetCore.JsonPatch/Operations/OperationBase.cs b/src/Microsoft.AspNetCore.JsonPatch/Operations/OperationBase.cs index 5f421f41c7..eb35fa7e9d 100644 --- a/src/Microsoft.AspNetCore.JsonPatch/Operations/OperationBase.cs +++ b/src/Microsoft.AspNetCore.JsonPatch/Operations/OperationBase.cs @@ -2,6 +2,7 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using Microsoft.AspNetCore.JsonPatch.Exceptions; using Newtonsoft.Json; namespace Microsoft.AspNetCore.JsonPatch.Operations @@ -13,7 +14,14 @@ namespace Microsoft.AspNetCore.JsonPatch.Operations { get { - return (OperationType)Enum.Parse(typeof(OperationType), op, true); + OperationType result; + if (!Enum.TryParse(op, ignoreCase: true, result: out result)) + { + throw new JsonPatchException( + Resources.FormatInvalidJsonPatchOperation(op), + innerException: null); + } + return result; } } diff --git a/src/Microsoft.AspNetCore.JsonPatch/Operations/OperationOfT.cs b/src/Microsoft.AspNetCore.JsonPatch/Operations/OperationOfT.cs index c41454b7ef..5af4f5d3f9 100644 --- a/src/Microsoft.AspNetCore.JsonPatch/Operations/OperationOfT.cs +++ b/src/Microsoft.AspNetCore.JsonPatch/Operations/OperationOfT.cs @@ -3,6 +3,7 @@ using System; using Microsoft.AspNetCore.JsonPatch.Adapters; +using Microsoft.AspNetCore.JsonPatch.Exceptions; namespace Microsoft.AspNetCore.JsonPatch.Operations { @@ -73,7 +74,7 @@ namespace Microsoft.AspNetCore.JsonPatch.Operations adapter.Copy(this, objectToApplyTo); break; case OperationType.Test: - throw new NotSupportedException(Resources.TestOperationNotSupported); + throw new JsonPatchException(new JsonPatchError(objectToApplyTo, this, Resources.TestOperationNotSupported)); default: break; } diff --git a/src/Microsoft.AspNetCore.JsonPatch/Properties/Resources.Designer.cs b/src/Microsoft.AspNetCore.JsonPatch/Properties/Resources.Designer.cs index 9e00cc2f00..ed567cf003 100644 --- a/src/Microsoft.AspNetCore.JsonPatch/Properties/Resources.Designer.cs +++ b/src/Microsoft.AspNetCore.JsonPatch/Properties/Resources.Designer.cs @@ -122,6 +122,22 @@ namespace Microsoft.AspNetCore.JsonPatch return string.Format(CultureInfo.CurrentCulture, GetString("InvalidJsonPatchDocument"), p0); } + /// + /// Invalid JsonPatch operation '{0}'. + /// + internal static string InvalidJsonPatchOperation + { + get { return GetString("InvalidJsonPatchOperation"); } + } + + /// + /// Invalid JsonPatch operation '{0}'. + /// + internal static string FormatInvalidJsonPatchOperation(object p0) + { + return string.Format(CultureInfo.CurrentCulture, GetString("InvalidJsonPatchOperation"), p0); + } + /// /// The provided string '{0}' is an invalid path. /// diff --git a/src/Microsoft.AspNetCore.JsonPatch/Resources.resx b/src/Microsoft.AspNetCore.JsonPatch/Resources.resx index a7b50520ac..7381ad02fc 100644 --- a/src/Microsoft.AspNetCore.JsonPatch/Resources.resx +++ b/src/Microsoft.AspNetCore.JsonPatch/Resources.resx @@ -138,6 +138,9 @@ The type '{0}' was malformed and could not be parsed. + + Invalid JsonPatch operation '{0}'. + The provided string '{0}' is an invalid path. diff --git a/test/Microsoft.AspNetCore.JsonPatch.Test/JsonPatchDocumentTest.cs b/test/Microsoft.AspNetCore.JsonPatch.Test/JsonPatchDocumentTest.cs new file mode 100644 index 0000000000..723ef15b64 --- /dev/null +++ b/test/Microsoft.AspNetCore.JsonPatch.Test/JsonPatchDocumentTest.cs @@ -0,0 +1,91 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using Microsoft.AspNetCore.JsonPatch.Exceptions; +using Newtonsoft.Json; +using Xunit; + +namespace Microsoft.AspNetCore.JsonPatch.Test +{ + public class JsonPatchDocumentTest + { + [Fact] + public void TestOperation_ThrowsException_CallsIntoLogErrorAction() + { + // Arrange + var serialized = "[{\"value\":\"John\",\"path\":\"/Name\",\"op\":\"test\"}]"; + var jsonPatchDocument = JsonConvert.DeserializeObject>(serialized); + var model = new Customer(); + var expectedErrorMessage = "The test operation is not supported."; + string actualErrorMessage = null; + + // Act + jsonPatchDocument.ApplyTo(model, (jsonPatchError) => + { + actualErrorMessage = jsonPatchError.ErrorMessage; + }); + + // Assert + Assert.Equal(expectedErrorMessage, actualErrorMessage); + } + + [Fact] + public void TestOperation_NoLogErrorAction_ThrowsJsonPatchException() + { + // Arrange + var serialized = "[{\"value\":\"John\",\"path\":\"/Name\",\"op\":\"test\"}]"; + var jsonPatchDocument = JsonConvert.DeserializeObject>(serialized); + var model = new Customer(); + var expectedErrorMessage = "The test operation is not supported."; + + // Act + var jsonPatchException = Assert.Throws(() => jsonPatchDocument.ApplyTo(model)); + + // Assert + Assert.Equal(expectedErrorMessage, jsonPatchException.Message); + } + + [Fact] + public void InvalidOperation_ThrowsException_CallsIntoLogErrorAction() + { + // Arrange + var operationName = "foo"; + var serialized = "[{\"value\":\"John\",\"path\":\"/Name\",\"op\":\"" + operationName + "\"}]"; + var jsonPatchDocument = JsonConvert.DeserializeObject>(serialized); + var model = new Customer(); + var expectedErrorMessage = $"Invalid JsonPatch operation '{operationName}'."; + string actualErrorMessage = null; + + // Act + jsonPatchDocument.ApplyTo(model, (jsonPatchError) => + { + actualErrorMessage = jsonPatchError.ErrorMessage; + }); + + // Assert + Assert.Equal(expectedErrorMessage, actualErrorMessage); + } + + [Fact] + public void InvalidOperation_NoLogErrorAction_ThrowsJsonPatchException() + { + // Arrange + var operationName = "foo"; + var serialized = "[{\"value\":\"John\",\"path\":\"/Name\",\"op\":\"" + operationName + "\"}]"; + var jsonPatchDocument = JsonConvert.DeserializeObject>(serialized); + var model = new Customer(); + var expectedErrorMessage = $"Invalid JsonPatch operation '{operationName}'."; + + // Act + var jsonPatchException = Assert.Throws(() => jsonPatchDocument.ApplyTo(model)); + + // Assert + Assert.Equal(expectedErrorMessage, jsonPatchException.Message); + } + + private class Customer + { + public string Name { get; set; } + } + } +}