From bf5c043de39d1f415baedc38f559db23ebc4d2e2 Mon Sep 17 00:00:00 2001 From: Jass Bagga Date: Mon, 11 Sep 2017 09:54:29 -0700 Subject: [PATCH] Refactor GetPath in JsonPatchDocumentOfT (#107) Addresses #98 --- .../JsonPatchDocumentOfT.cs | 167 ++++++++---------- .../Properties/AssemblyInfo.cs | 7 + .../Properties/Resources.Designer.cs | 42 +++-- .../Resources.resx | 3 + .../JsonPatchDocumentGetPathTest.cs | 124 +++++++++++++ ...nPatchDocumentJsonPropertyAttributeTest.cs | 31 ++++ ...Microsoft.AspNetCore.JsonPatch.Test.csproj | 4 + 7 files changed, 274 insertions(+), 104 deletions(-) create mode 100644 src/Microsoft.AspNetCore.JsonPatch/Properties/AssemblyInfo.cs create mode 100644 test/Microsoft.AspNetCore.JsonPatch.Test/JsonPatchDocumentGetPathTest.cs diff --git a/src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocumentOfT.cs b/src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocumentOfT.cs index d669970407..33e1e5033f 100644 --- a/src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocumentOfT.cs +++ b/src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocumentOfT.cs @@ -58,7 +58,7 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "add", - GetPath(path), + GetPath(path, null), from: null, value: value)); @@ -85,7 +85,7 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "add", - GetPath(path) + "/" + position, + GetPath(path, position.ToString()), from: null, value: value)); @@ -93,7 +93,7 @@ namespace Microsoft.AspNetCore.JsonPatch } /// - /// At value at end of list + /// Add value to the end of the list /// /// value type /// target location @@ -108,7 +108,7 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "add", - GetPath(path) + "/-", + GetPath(path, "-"), from: null, value: value)); @@ -128,7 +128,7 @@ namespace Microsoft.AspNetCore.JsonPatch throw new ArgumentNullException(nameof(path)); } - Operations.Add(new Operation("remove", GetPath(path), from: null)); + Operations.Add(new Operation("remove", GetPath(path, null), from: null)); return this; } @@ -149,7 +149,7 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "remove", - GetPath(path) + "/" + position, + GetPath(path, position.ToString()), from: null)); return this; @@ -170,7 +170,7 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "remove", - GetPath(path) + "/-", + GetPath(path, "-"), from: null)); return this; @@ -192,7 +192,7 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "replace", - GetPath(path), + GetPath(path, null), from: null, value: value)); @@ -217,7 +217,7 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "replace", - GetPath(path) + "/" + position, + GetPath(path, position.ToString()), from: null, value: value)); @@ -240,7 +240,7 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "replace", - GetPath(path) + "/-", + GetPath(path, "-"), from: null, value: value)); @@ -270,8 +270,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "move", - GetPath(path), - GetPath(from))); + GetPath(path, null), + GetPath(from, null))); return this; } @@ -301,8 +301,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "move", - GetPath(path), - GetPath(from) + "/" + positionFrom)); + GetPath(path, null), + GetPath(from, positionFrom.ToString()))); return this; } @@ -332,8 +332,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "move", - GetPath(path) + "/" + positionTo, - GetPath(from))); + GetPath(path, positionTo.ToString()), + GetPath(from, null))); return this; } @@ -365,8 +365,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "move", - GetPath(path) + "/" + positionTo, - GetPath(from) + "/" + positionFrom)); + GetPath(path, positionTo.ToString()), + GetPath(from, positionFrom.ToString()))); return this; } @@ -396,8 +396,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "move", - GetPath(path) + "/-", - GetPath(from) + "/" + positionFrom)); + GetPath(path, "-"), + GetPath(from, positionFrom.ToString()))); return this; } @@ -425,8 +425,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "move", - GetPath(path) + "/-", - GetPath(from))); + GetPath(path, "-"), + GetPath(from, null))); return this; } @@ -454,8 +454,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "copy", - GetPath(path) - , GetPath(from))); + GetPath(path, null), + GetPath(from, null))); return this; } @@ -485,8 +485,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "copy", - GetPath(path), - GetPath(from) + "/" + positionFrom)); + GetPath(path, null), + GetPath(from, positionFrom.ToString()))); return this; } @@ -516,8 +516,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "copy", - GetPath(path) + "/" + positionTo, - GetPath(from))); + GetPath(path, positionTo.ToString()), + GetPath(from, null))); return this; } @@ -549,8 +549,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "copy", - GetPath(path) + "/" + positionTo, - GetPath(from) + "/" + positionFrom)); + GetPath(path, positionTo.ToString()), + GetPath(from, positionFrom.ToString()))); return this; } @@ -580,8 +580,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "copy", - GetPath(path) + "/-", - GetPath(from) + "/" + positionFrom)); + GetPath(path, "-"), + GetPath(from, positionFrom.ToString()))); return this; } @@ -609,8 +609,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "copy", - GetPath(path) + "/-", - GetPath(from))); + GetPath(path, "-"), + GetPath(from, null))); return this; } @@ -705,61 +705,57 @@ namespace Microsoft.AspNetCore.JsonPatch return allOps; } - private string GetPath(Expression> expr) + // Internal for testing + internal string GetPath(Expression> expr, string position) { - return "/" + GetPath(expr.Body, true).ToLowerInvariant(); + var segments = GetPathSegments(expr.Body); + var path = String.Join("/", segments); + if (position != null) + { + path += "/" + position; + if (segments.Count == 0) + { + return path.ToLowerInvariant(); + } + } + + return "/" + path.ToLowerInvariant(); } - private string GetPath(Expression expr, bool firstTime) + private List GetPathSegments(Expression expr) { + var listOfSegments = new List(); switch (expr.NodeType) { case ExpressionType.ArrayIndex: var binaryExpression = (BinaryExpression)expr; + listOfSegments.AddRange(GetPathSegments(binaryExpression.Left)); + listOfSegments.Add(binaryExpression.Right.ToString()); + return listOfSegments; - if (ContinueWithSubPath(binaryExpression.Left.NodeType, false)) - { - var leftFromBinaryExpression = GetPath(binaryExpression.Left, false); - return leftFromBinaryExpression + "/" + binaryExpression.Right.ToString(); - } - else - { - return binaryExpression.Right.ToString(); - } case ExpressionType.Call: var methodCallExpression = (MethodCallExpression)expr; + listOfSegments.AddRange(GetPathSegments(methodCallExpression.Object)); + listOfSegments.Add(EvaluateExpression(methodCallExpression.Arguments[0])); + return listOfSegments; - if (ContinueWithSubPath(methodCallExpression.Object.NodeType, false)) - { - var leftFromMemberCallExpression = GetPath(methodCallExpression.Object, false); - return leftFromMemberCallExpression + "/" + - GetIndexerInvocation(methodCallExpression.Arguments[0]); - } - else - { - return GetIndexerInvocation(methodCallExpression.Arguments[0]); - } case ExpressionType.Convert: - return GetPath(((UnaryExpression)expr).Operand, false); + listOfSegments.AddRange(GetPathSegments(((UnaryExpression)expr).Operand)); + return listOfSegments; + case ExpressionType.MemberAccess: var memberExpression = expr as MemberExpression; + listOfSegments.AddRange(GetPathSegments(memberExpression.Expression)); + // Get property name, respecting JsonProperty attribute + listOfSegments.Add(GetPropertyNameFromMemberExpression(memberExpression)); + return listOfSegments; - if (ContinueWithSubPath(memberExpression.Expression.NodeType, false)) - { - var left = GetPath(memberExpression.Expression, false); - // Get property name, respecting JsonProperty attribute - return left + "/" + GetPropertyNameFromMemberExpression(memberExpression); - } - else - { - // Get property name, respecting JsonProperty attribute - return GetPropertyNameFromMemberExpression(memberExpression); - } case ExpressionType.Parameter: // Fits "x => x" (the whole document which is "" as JSON pointer) - return firstTime ? string.Empty : null; + return listOfSegments; + default: - return string.Empty; + throw new InvalidOperationException(Resources.FormatExpressionTypeNotSupported(expr)); } } @@ -776,33 +772,24 @@ namespace Microsoft.AspNetCore.JsonPatch return null; } - private static bool ContinueWithSubPath(ExpressionType expressionType, bool firstTime) + private static bool ContinueWithSubPath(ExpressionType expressionType) { - if (firstTime) - { - return (expressionType == ExpressionType.ArrayIndex - || expressionType == ExpressionType.Call - || expressionType == ExpressionType.Convert - || expressionType == ExpressionType.MemberAccess - || expressionType == ExpressionType.Parameter); - } - else - { - return (expressionType == ExpressionType.ArrayIndex - || expressionType == ExpressionType.Call - || expressionType == ExpressionType.Convert - || expressionType == ExpressionType.MemberAccess); - } + return (expressionType == ExpressionType.ArrayIndex + || expressionType == ExpressionType.Call + || expressionType == ExpressionType.Convert + || expressionType == ExpressionType.MemberAccess); + } - private static string GetIndexerInvocation(Expression expression) + // Evaluates the value of the key or index which may be an int or a string, + // or some other expression type. + // The expression is converted to a delegate and the result of executing the delegate is returned as a string. + private static string EvaluateExpression(Expression expression) { var converted = Expression.Convert(expression, typeof(object)); var fakeParameter = Expression.Parameter(typeof(object), null); var lambda = Expression.Lambda>(converted, fakeParameter); - Func func; - - func = lambda.Compile(); + var func = lambda.Compile(); return Convert.ToString(func(null), CultureInfo.InvariantCulture); } diff --git a/src/Microsoft.AspNetCore.JsonPatch/Properties/AssemblyInfo.cs b/src/Microsoft.AspNetCore.JsonPatch/Properties/AssemblyInfo.cs new file mode 100644 index 0000000000..08e6f5e01c --- /dev/null +++ b/src/Microsoft.AspNetCore.JsonPatch/Properties/AssemblyInfo.cs @@ -0,0 +1,7 @@ + +// 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.Runtime.CompilerServices; + +[assembly: InternalsVisibleTo("Microsoft.AspNetCore.JsonPatch.Test, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")] diff --git a/src/Microsoft.AspNetCore.JsonPatch/Properties/Resources.Designer.cs b/src/Microsoft.AspNetCore.JsonPatch/Properties/Resources.Designer.cs index 896a3d2ad4..432fab2cbf 100644 --- a/src/Microsoft.AspNetCore.JsonPatch/Properties/Resources.Designer.cs +++ b/src/Microsoft.AspNetCore.JsonPatch/Properties/Resources.Designer.cs @@ -80,6 +80,20 @@ namespace Microsoft.AspNetCore.JsonPatch internal static string FormatCannotUpdateProperty(object p0) => string.Format(CultureInfo.CurrentCulture, GetString("CannotUpdateProperty"), p0); + /// + /// The expression '{0}' is not supported. + /// + internal static string ExpressionTypeNotSupported + { + get => GetString("ExpressionTypeNotSupported"); + } + + /// + /// The expression '{0}' is not supported. + /// + internal static string FormatExpressionTypeNotSupported(object p0) + => string.Format(CultureInfo.CurrentCulture, GetString("ExpressionTypeNotSupported"), p0); + /// /// The index value provided by path segment '{0}' is out of bounds of the array size. /// @@ -136,6 +150,20 @@ namespace Microsoft.AspNetCore.JsonPatch internal static string FormatInvalidJsonPatchOperation(object p0) => string.Format(CultureInfo.CurrentCulture, GetString("InvalidJsonPatchOperation"), p0); + /// + /// The provided path segment '{0}' cannot be converted to the target type. + /// + internal static string InvalidPathSegment + { + get => GetString("InvalidPathSegment"); + } + + /// + /// The provided path segment '{0}' cannot be converted to the target type. + /// + internal static string FormatInvalidPathSegment(object p0) + => string.Format(CultureInfo.CurrentCulture, GetString("InvalidPathSegment"), p0); + /// /// The provided string '{0}' is an invalid path. /// @@ -206,20 +234,6 @@ namespace Microsoft.AspNetCore.JsonPatch internal static string FormatPatchNotSupportedForNonGenericLists(object p0) => string.Format(CultureInfo.CurrentCulture, GetString("PatchNotSupportedForNonGenericLists"), p0); - /// - /// The provided path segment '{0}' cannot be converted to the target type. - /// - internal static string InvalidPathSegment - { - get => GetString("InvalidPathSegment"); - } - - /// - /// The provided path segment '{0}' cannot be converted to the target type. - /// - internal static string FormatInvalidPathSegment(object p0) - => string.Format(CultureInfo.CurrentCulture, GetString("InvalidPathSegment"), p0); - /// /// The target location specified by path segment '{0}' was not found. /// diff --git a/src/Microsoft.AspNetCore.JsonPatch/Resources.resx b/src/Microsoft.AspNetCore.JsonPatch/Resources.resx index 0b67084233..bf07e4a31e 100644 --- a/src/Microsoft.AspNetCore.JsonPatch/Resources.resx +++ b/src/Microsoft.AspNetCore.JsonPatch/Resources.resx @@ -132,6 +132,9 @@ The property at path '{0}' could not be updated. + + The expression '{0}' is not supported. Supported expressions include member access and indexer expressions. + The index value provided by path segment '{0}' is out of bounds of the array size. diff --git a/test/Microsoft.AspNetCore.JsonPatch.Test/JsonPatchDocumentGetPathTest.cs b/test/Microsoft.AspNetCore.JsonPatch.Test/JsonPatchDocumentGetPathTest.cs new file mode 100644 index 0000000000..dcde4b442d --- /dev/null +++ b/test/Microsoft.AspNetCore.JsonPatch.Test/JsonPatchDocumentGetPathTest.cs @@ -0,0 +1,124 @@ +// 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 System.Collections.Generic; +using Xunit; + +namespace Microsoft.AspNetCore.JsonPatch +{ + public class JsonPatchDocumentGetPathTest + { + [Fact] + public void ExpressionType_MemberAccess() + { + // Arrange + var patchDocument = new JsonPatchDocument(); + + // Act + var path = patchDocument.GetPath(p => p.SimpleObject.IntegerList, "-"); + + // Assert + Assert.Equal("/simpleobject/integerlist/-", path); + } + + [Fact] + public void ExpressionType_ArrayIndex() + { + // Arrange + var patchDocument = new JsonPatchDocument(); + + // Act + var path = patchDocument.GetPath(p => p[3], null); + + // Assert + Assert.Equal("/3", path); + } + + [Fact] + public void ExpressionType_Call() + { + // Arrange + var patchDocument = new JsonPatchDocument>(); + + // Act + var path = patchDocument.GetPath(p => p["key"], "3"); + + // Assert + Assert.Equal("/key/3", path); + } + + [Fact] + public void ExpressionType_Parameter_NullPosition() + { + // Arrange + var patchDocument = new JsonPatchDocument(); + + // Act + var path = patchDocument.GetPath(p => p, null); + + // Assert + Assert.Equal("/", path); + } + + [Fact] + public void ExpressionType_Parameter_WithPosition() + { + // Arrange + var patchDocument = new JsonPatchDocument(); + + // Act + var path = patchDocument.GetPath(p => p, "-"); + + // Assert + Assert.Equal("/-", path); + } + + [Fact] + public void ExpressionType_Convert() + { + // Arrange + var patchDocument = new JsonPatchDocument(); + + // Act + var path = patchDocument.GetPath(p => (BaseClass)p.DerivedObject, null); + + // Assert + Assert.Equal("/derivedobject", path); + } + + [Fact] + public void ExpressionType_NotSupported() + { + // Arrange + var patchDocument = new JsonPatchDocument(); + + // Act + var exception = Assert.Throws(() => + { + patchDocument.GetPath(p => p.IntegerValue >= 4, null); + }); + + // Assert + Assert.Equal( + string.Format("The expression '(p.IntegerValue >= 4)' is not supported. Supported expressions include member access and indexer expressions."), + exception.Message); + } + } + + internal class DerivedClass : BaseClass + { + public DerivedClass() + { + } + } + + internal class NestedObjectWithDerivedClass + { + public DerivedClass DerivedObject { get; set; } + } + + internal class BaseClass + { + } +} diff --git a/test/Microsoft.AspNetCore.JsonPatch.Test/JsonPatchDocumentJsonPropertyAttributeTest.cs b/test/Microsoft.AspNetCore.JsonPatch.Test/JsonPatchDocumentJsonPropertyAttributeTest.cs index 3948b79484..50ab6a9f5b 100644 --- a/test/Microsoft.AspNetCore.JsonPatch.Test/JsonPatchDocumentJsonPropertyAttributeTest.cs +++ b/test/Microsoft.AspNetCore.JsonPatch.Test/JsonPatchDocumentJsonPropertyAttributeTest.cs @@ -1,6 +1,7 @@ // 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.Collections.Generic; using System.Linq; using System.Reflection; using Newtonsoft.Json; @@ -11,6 +12,36 @@ namespace Microsoft.AspNetCore.JsonPatch { public class JsonPatchDocumentJsonPropertyAttributeTest { + [Fact] + public void Add_ToRoot_OfListOfObjects_AtEndOfList() + { + var patchDoc = new JsonPatchDocument>(); + patchDoc.Add(p => p, new JsonPropertyObject()); + + var serialized = JsonConvert.SerializeObject(patchDoc); + var deserialized = + JsonConvert.DeserializeObject>(serialized); + + // get path + var pathToCheck = deserialized.Operations.First().path; + Assert.Equal("/-", pathToCheck); + } + + [Fact] + public void Add_ToRoot_OfListOfObjects_AtGivenPosition() + { + var patchDoc = new JsonPatchDocument>(); + patchDoc.Add(p => p[3], new JsonPropertyObject()); + + var serialized = JsonConvert.SerializeObject(patchDoc); + var deserialized = + JsonConvert.DeserializeObject>(serialized); + + // get path + var pathToCheck = deserialized.Operations.First().path; + Assert.Equal("/3", pathToCheck); + } + [Fact] public void Add_WithExpression_RespectsJsonPropertyName_ForModelProperty() { diff --git a/test/Microsoft.AspNetCore.JsonPatch.Test/Microsoft.AspNetCore.JsonPatch.Test.csproj b/test/Microsoft.AspNetCore.JsonPatch.Test/Microsoft.AspNetCore.JsonPatch.Test.csproj index 2e4f68b538..6d7b403a79 100644 --- a/test/Microsoft.AspNetCore.JsonPatch.Test/Microsoft.AspNetCore.JsonPatch.Test.csproj +++ b/test/Microsoft.AspNetCore.JsonPatch.Test/Microsoft.AspNetCore.JsonPatch.Test.csproj @@ -14,4 +14,8 @@ + + + +