From e8452821b9a7689853e36c9a10ad4b8c6281817c Mon Sep 17 00:00:00 2001 From: Kiran Challa Date: Wed, 28 Dec 2016 13:35:15 -0800 Subject: [PATCH] [Fixes #50] JsonPatchDocument.Replace() yields invalid path when [JsonProperty] is used (1.1.0) --- .../Internal/ExpressionHelpers.cs | 120 ------------- .../JsonPatchDocumentOfT.cs | 170 ++++++++++++++---- ...nPatchDocumentJsonPropertyAttributeTest.cs | 113 +++++++++++- 3 files changed, 245 insertions(+), 158 deletions(-) delete mode 100644 src/Microsoft.AspNetCore.JsonPatch/Internal/ExpressionHelpers.cs diff --git a/src/Microsoft.AspNetCore.JsonPatch/Internal/ExpressionHelpers.cs b/src/Microsoft.AspNetCore.JsonPatch/Internal/ExpressionHelpers.cs deleted file mode 100644 index c978330e5c..0000000000 --- a/src/Microsoft.AspNetCore.JsonPatch/Internal/ExpressionHelpers.cs +++ /dev/null @@ -1,120 +0,0 @@ -// 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.Globalization; -using System.Linq.Expressions; -using System.Reflection; -using Newtonsoft.Json; - -namespace Microsoft.AspNetCore.JsonPatch.Internal -{ - public static class ExpressionHelpers - { - public static string GetPath(Expression> expr) where TModel : class - { - return "/" + GetPath(expr.Body, true); - } - - private static string GetPath(Expression expr, bool firstTime) - { - switch (expr.NodeType) - { - case ExpressionType.ArrayIndex: - var binaryExpression = (BinaryExpression)expr; - - 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; - - 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); - case ExpressionType.MemberAccess: - var memberExpression = expr as MemberExpression; - - 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; - default: - return string.Empty; - } - } - - private static string GetPropertyNameFromMemberExpression(MemberExpression memberExpression) - { - // if there's a JsonProperty attribute, we must return the PropertyName - // from the attribute rather than the member name - var jsonPropertyAttribute = - memberExpression.Member.GetCustomAttribute( - typeof(JsonPropertyAttribute), true); - - if (jsonPropertyAttribute == null) - { - return memberExpression.Member.Name; - } - // get value - var castedAttribute = (JsonPropertyAttribute)jsonPropertyAttribute; - return castedAttribute.PropertyName; - } - - private static bool ContinueWithSubPath(ExpressionType expressionType, bool firstTime) - { - 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); - } - } - - private static string GetIndexerInvocation(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(); - - return Convert.ToString(func(null), CultureInfo.InvariantCulture); - } - } -} \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocumentOfT.cs b/src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocumentOfT.cs index fc660b2b45..c81c4e4d6c 100644 --- a/src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocumentOfT.cs +++ b/src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocumentOfT.cs @@ -2,7 +2,9 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; +using System.Linq; using System.Collections.Generic; +using System.Globalization; using System.Linq.Expressions; using Microsoft.AspNetCore.JsonPatch.Adapters; using Microsoft.AspNetCore.JsonPatch.Converters; @@ -66,7 +68,7 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "add", - ExpressionHelpers.GetPath(path).ToLowerInvariant(), + GetPath(path), from: null, value: value)); @@ -93,7 +95,7 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "add", - ExpressionHelpers.GetPath(path).ToLowerInvariant() + "/" + position, + GetPath(path) + "/" + position, from: null, value: value)); @@ -116,7 +118,7 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "add", - ExpressionHelpers.GetPath(path).ToLowerInvariant() + "/-", + GetPath(path) + "/-", from: null, value: value)); @@ -136,7 +138,7 @@ namespace Microsoft.AspNetCore.JsonPatch throw new ArgumentNullException(nameof(path)); } - Operations.Add(new Operation("remove", ExpressionHelpers.GetPath(path).ToLowerInvariant(), from: null)); + Operations.Add(new Operation("remove", GetPath(path), from: null)); return this; } @@ -157,7 +159,7 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "remove", - ExpressionHelpers.GetPath(path).ToLowerInvariant() + "/" + position, + GetPath(path) + "/" + position, from: null)); return this; @@ -178,7 +180,7 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "remove", - ExpressionHelpers.GetPath(path).ToLowerInvariant() + "/-", + GetPath(path) + "/-", from: null)); return this; @@ -200,7 +202,7 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "replace", - ExpressionHelpers.GetPath(path).ToLowerInvariant(), + GetPath(path), from: null, value: value)); @@ -225,7 +227,7 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "replace", - ExpressionHelpers.GetPath(path).ToLowerInvariant() + "/" + position, + GetPath(path) + "/" + position, from: null, value: value)); @@ -248,7 +250,7 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "replace", - ExpressionHelpers.GetPath(path).ToLowerInvariant() + "/-", + GetPath(path) + "/-", from: null, value: value)); @@ -278,8 +280,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "move", - ExpressionHelpers.GetPath(path).ToLowerInvariant(), - ExpressionHelpers.GetPath(from).ToLowerInvariant())); + GetPath(path), + GetPath(from))); return this; } @@ -309,8 +311,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "move", - ExpressionHelpers.GetPath(path).ToLowerInvariant(), - ExpressionHelpers.GetPath(from).ToLowerInvariant() + "/" + positionFrom)); + GetPath(path), + GetPath(from) + "/" + positionFrom)); return this; } @@ -340,8 +342,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "move", - ExpressionHelpers.GetPath(path).ToLowerInvariant() + "/" + positionTo, - ExpressionHelpers.GetPath(from).ToLowerInvariant())); + GetPath(path) + "/" + positionTo, + GetPath(from))); return this; } @@ -373,8 +375,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "move", - ExpressionHelpers.GetPath(path).ToLowerInvariant() + "/" + positionTo, - ExpressionHelpers.GetPath(from).ToLowerInvariant() + "/" + positionFrom)); + GetPath(path) + "/" + positionTo, + GetPath(from) + "/" + positionFrom)); return this; } @@ -404,8 +406,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "move", - ExpressionHelpers.GetPath(path).ToLowerInvariant() + "/-", - ExpressionHelpers.GetPath(from).ToLowerInvariant() + "/" + positionFrom)); + GetPath(path) + "/-", + GetPath(from) + "/" + positionFrom)); return this; } @@ -433,8 +435,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "move", - ExpressionHelpers.GetPath(path).ToLowerInvariant() + "/-", - ExpressionHelpers.GetPath(from).ToLowerInvariant())); + GetPath(path) + "/-", + GetPath(from))); return this; } @@ -462,8 +464,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "copy", - ExpressionHelpers.GetPath(path).ToLowerInvariant() - , ExpressionHelpers.GetPath(from).ToLowerInvariant())); + GetPath(path) + , GetPath(from))); return this; } @@ -493,8 +495,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "copy", - ExpressionHelpers.GetPath(path).ToLowerInvariant(), - ExpressionHelpers.GetPath(from).ToLowerInvariant() + "/" + positionFrom)); + GetPath(path), + GetPath(from) + "/" + positionFrom)); return this; } @@ -524,8 +526,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "copy", - ExpressionHelpers.GetPath(path).ToLowerInvariant() + "/" + positionTo, - ExpressionHelpers.GetPath(from).ToLowerInvariant())); + GetPath(path) + "/" + positionTo, + GetPath(from))); return this; } @@ -557,8 +559,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "copy", - ExpressionHelpers.GetPath(path).ToLowerInvariant() + "/" + positionTo, - ExpressionHelpers.GetPath(from).ToLowerInvariant() + "/" + positionFrom)); + GetPath(path) + "/" + positionTo, + GetPath(from) + "/" + positionFrom)); return this; } @@ -588,8 +590,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "copy", - ExpressionHelpers.GetPath(path).ToLowerInvariant() + "/-", - ExpressionHelpers.GetPath(from).ToLowerInvariant() + "/" + positionFrom)); + GetPath(path) + "/-", + GetPath(from) + "/" + positionFrom)); return this; } @@ -617,8 +619,8 @@ namespace Microsoft.AspNetCore.JsonPatch Operations.Add(new Operation( "copy", - ExpressionHelpers.GetPath(path).ToLowerInvariant() + "/-", - ExpressionHelpers.GetPath(from).ToLowerInvariant())); + GetPath(path) + "/-", + GetPath(from))); return this; } @@ -712,5 +714,107 @@ namespace Microsoft.AspNetCore.JsonPatch return allOps; } + + private string GetPath(Expression> expr) + { + return "/" + GetPath(expr.Body, true).ToLowerInvariant(); + } + + private string GetPath(Expression expr, bool firstTime) + { + switch (expr.NodeType) + { + case ExpressionType.ArrayIndex: + var binaryExpression = (BinaryExpression)expr; + + 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; + + 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); + case ExpressionType.MemberAccess: + var memberExpression = expr as MemberExpression; + + 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; + default: + return string.Empty; + } + } + + private string GetPropertyNameFromMemberExpression(MemberExpression memberExpression) + { + var jsonObjectContract = ContractResolver.ResolveContract(memberExpression.Expression.Type) as JsonObjectContract; + if (jsonObjectContract != null) + { + return jsonObjectContract.Properties + .First(jsonProperty => jsonProperty.UnderlyingName == memberExpression.Member.Name) + .PropertyName; + } + + return null; + } + + private static bool ContinueWithSubPath(ExpressionType expressionType, bool firstTime) + { + 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); + } + } + + private static string GetIndexerInvocation(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(); + + return Convert.ToString(func(null), CultureInfo.InvariantCulture); + } } } \ No newline at end of file diff --git a/test/Microsoft.AspNetCore.JsonPatch.Test/JsonPatchDocumentJsonPropertyAttributeTest.cs b/test/Microsoft.AspNetCore.JsonPatch.Test/JsonPatchDocumentJsonPropertyAttributeTest.cs index 0ec0c18afd..182cf1cd6d 100644 --- a/test/Microsoft.AspNetCore.JsonPatch.Test/JsonPatchDocumentJsonPropertyAttributeTest.cs +++ b/test/Microsoft.AspNetCore.JsonPatch.Test/JsonPatchDocumentJsonPropertyAttributeTest.cs @@ -1,8 +1,10 @@ // 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 Newtonsoft.Json; using System.Linq; +using System.Reflection; +using Newtonsoft.Json; +using Newtonsoft.Json.Serialization; using Xunit; namespace Microsoft.AspNetCore.JsonPatch @@ -26,13 +28,47 @@ namespace Microsoft.AspNetCore.JsonPatch Assert.Equal(pathToCheck, "/anothername"); } + [Fact] + public void Add_WithExpressionOnStringProperty_FallsbackToPropertyName_WhenJsonPropertyName_IsEmpty() + { + // Arrange + var patchDoc = new JsonPatchDocument(); + patchDoc.Add(m => m.StringProperty, "Test"); + var serialized = JsonConvert.SerializeObject(patchDoc); + + // Act + var deserialized = + JsonConvert.DeserializeObject>(serialized); + + // Assert + var pathToCheck = deserialized.Operations.First().path; + Assert.Equal("/stringproperty", pathToCheck); + } + + [Fact] + public void Add_WithExpressionOnArrayProperty_FallsbackToPropertyName_WhenJsonPropertyName_IsEmpty() + { + // Arrange + var patchDoc = new JsonPatchDocument(); + patchDoc.Add(m => m.ArrayProperty, "James"); + var serialized = JsonConvert.SerializeObject(patchDoc); + + // Act + var deserialized = + JsonConvert.DeserializeObject>(serialized); + + // Assert + var pathToCheck = deserialized.Operations.First().path; + Assert.Equal("/arrayproperty/-", pathToCheck); + } + [Fact] public void Add_WithExpression_RespectsJsonPropertyName_WhenApplyingToDifferentlyTypedClassWithPropertyMatchingJsonPropertyName() { var patchDocToSerialize = new JsonPatchDocument(); patchDocToSerialize.Add(p => p.Name, "John"); - // the patchdoc will deserialize to "anothername". We should thus be able to apply + // the patchdoc will deserialize to "anothername". We should thus be able to apply // it to a class that HAS that other property name. var doc = new JsonPropertyWithAnotherNameDTO() { @@ -81,7 +117,7 @@ namespace Microsoft.AspNetCore.JsonPatch { Name = "InitialValue" }; - + // serialization should serialize to "AnotherName" var serialized = "[{\"value\":\"Kevin\",\"path\":\"/AnotherName\",\"op\":\"add\"}]"; var deserialized = @@ -151,9 +187,9 @@ namespace Microsoft.AspNetCore.JsonPatch var doc = new JsonPropertyComplexNameDTO() { FooSlashBars = "InitialName", - FooSlashTilde = new SimpleDTO + FooSlashTilde = new SimpleDTO { - StringProperty = "Initial Value" + StringProperty = "Initial Value" } }; @@ -167,5 +203,72 @@ namespace Microsoft.AspNetCore.JsonPatch Assert.Equal("Kevin", doc.FooSlashBars); Assert.Equal("Final Value", doc.FooSlashTilde.StringProperty); } + + [Fact] + public void Move_WithExpression_FallsbackToPropertyName_WhenJsonPropertyName_IsEmpty() + { + // Arrange + var patchDoc = new JsonPatchDocument(); + patchDoc.Move(m => m.StringProperty, m => m.StringProperty2); + var serialized = JsonConvert.SerializeObject(patchDoc); + + // Act + var deserialized = + JsonConvert.DeserializeObject>(serialized); + + // Assert + var fromPath = deserialized.Operations.First().from; + Assert.Equal("/stringproperty", fromPath); + var toPath = deserialized.Operations.First().path; + Assert.Equal("/stringproperty2", toPath); + } + + [Fact] + public void Add_WithExpression_AndCustomContractResolver_UsesPropertyName_SetByContractResolver() + { + // Arrange + var patchDoc = new JsonPatchDocument(); + patchDoc.ContractResolver = new CustomContractResolver(); + patchDoc.Add(m => m.SSN, "123-45-6789"); + var serialized = JsonConvert.SerializeObject(patchDoc); + + // Act + var deserialized = + JsonConvert.DeserializeObject>(serialized); + + // Assert + var path = deserialized.Operations.First().path; + Assert.Equal("/socialsecuritynumber", path); + } + + private class JsonPropertyWithNoPropertyName + { + [JsonProperty] + public string StringProperty { get; set; } + + [JsonProperty] + public string[] ArrayProperty { get; set; } + + [JsonProperty] + public string StringProperty2 { get; set; } + + [JsonProperty] + public string SSN { get; set; } + } + + private class CustomContractResolver : DefaultContractResolver + { + protected override JsonProperty CreateProperty(MemberInfo member, MemberSerialization memberSerialization) + { + var jsonProperty = base.CreateProperty(member, memberSerialization); + + if (jsonProperty.PropertyName == "SSN") + { + jsonProperty.PropertyName = "SocialSecurityNumber"; + } + + return jsonProperty; + } + } } }