Refactor GetPath in JsonPatchDocumentOfT (#107)

Addresses #98

src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocumentOfT.cs

@@ -58,7 +58,7 @@ namespace Microsoft.AspNetCore.JsonPatch
 
             Operations.Add(new Operation<TModel>(
                 "add",
-                GetPath(path),
+                GetPath(path, null),
                 from: null,
                 value: value));
 
@@ -85,7 +85,7 @@ namespace Microsoft.AspNetCore.JsonPatch
 
             Operations.Add(new Operation<TModel>(
                 "add",
-                GetPath(path) + "/" + position,
+                GetPath(path, position.ToString()),
                 from: null,
                 value: value));
 
@@ -93,7 +93,7 @@ namespace Microsoft.AspNetCore.JsonPatch
         }
 
         /// <summary>
-        /// At value at end of list
+        /// Add value to the end of the list
         /// </summary>
         /// <typeparam name="TProp">value type</typeparam>
         /// <param name="path">target location</param>
@@ -108,7 +108,7 @@ namespace Microsoft.AspNetCore.JsonPatch
 
             Operations.Add(new Operation<TModel>(
                 "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<TModel>("remove", GetPath(path), from: null));
+            Operations.Add(new Operation<TModel>("remove", GetPath(path, null), from: null));
 
             return this;
         }
@@ -149,7 +149,7 @@ namespace Microsoft.AspNetCore.JsonPatch
 
             Operations.Add(new Operation<TModel>(
                 "remove",
-                GetPath(path) + "/" + position,
+                GetPath(path, position.ToString()),
                 from: null));
 
             return this;
@@ -170,7 +170,7 @@ namespace Microsoft.AspNetCore.JsonPatch
 
             Operations.Add(new Operation<TModel>(
                 "remove",
-                GetPath(path) + "/-",
+                GetPath(path, "-"),
                 from: null));
 
             return this;
@@ -192,7 +192,7 @@ namespace Microsoft.AspNetCore.JsonPatch
 
             Operations.Add(new Operation<TModel>(
                 "replace",
-                GetPath(path),
+                GetPath(path, null),
                 from: null,
                 value: value));
 
@@ -217,7 +217,7 @@ namespace Microsoft.AspNetCore.JsonPatch
 
             Operations.Add(new Operation<TModel>(
                 "replace",
-                GetPath(path) + "/" + position,
+                GetPath(path, position.ToString()),
                 from: null,
                 value: value));
 
@@ -240,7 +240,7 @@ namespace Microsoft.AspNetCore.JsonPatch
 
             Operations.Add(new Operation<TModel>(
                 "replace",
-                GetPath(path) + "/-",
+                GetPath(path, "-"),
                 from: null,
                 value: value));
 
@@ -270,8 +270,8 @@ namespace Microsoft.AspNetCore.JsonPatch
 
             Operations.Add(new Operation<TModel>(
                 "move",
-                GetPath(path),
+                GetPath(path, null),
-                GetPath(from)));
+                GetPath(from, null)));
 
             return this;
         }
@@ -301,8 +301,8 @@ namespace Microsoft.AspNetCore.JsonPatch
 
             Operations.Add(new Operation<TModel>(
                 "move",
-                GetPath(path),
+                GetPath(path, null),
-                GetPath(from) + "/" + positionFrom));
+                GetPath(from, positionFrom.ToString())));
 
             return this;
         }
@@ -332,8 +332,8 @@ namespace Microsoft.AspNetCore.JsonPatch
 
             Operations.Add(new Operation<TModel>(
                 "move",
-                GetPath(path) + "/" + positionTo,
+                GetPath(path, positionTo.ToString()),
-                GetPath(from)));
+                GetPath(from, null)));
 
             return this;
         }
@@ -365,8 +365,8 @@ namespace Microsoft.AspNetCore.JsonPatch
 
             Operations.Add(new Operation<TModel>(
                 "move",
-                GetPath(path) + "/" + positionTo,
+                GetPath(path, positionTo.ToString()),
-                GetPath(from) + "/" + positionFrom));
+                GetPath(from, positionFrom.ToString())));
 
             return this;
         }
@@ -396,8 +396,8 @@ namespace Microsoft.AspNetCore.JsonPatch
 
             Operations.Add(new Operation<TModel>(
                 "move",
-                GetPath(path) + "/-",
+                GetPath(path, "-"),
-                GetPath(from) + "/" + positionFrom));
+                GetPath(from, positionFrom.ToString())));
 
             return this;
         }
@@ -425,8 +425,8 @@ namespace Microsoft.AspNetCore.JsonPatch
 
             Operations.Add(new Operation<TModel>(
                 "move",
-                GetPath(path) + "/-",
+                GetPath(path, "-"),
-                GetPath(from)));
+                GetPath(from, null)));
 
             return this;
         }
@@ -454,8 +454,8 @@ namespace Microsoft.AspNetCore.JsonPatch
 
             Operations.Add(new Operation<TModel>(
                 "copy",
-                GetPath(path)
+                GetPath(path, null),
-              , GetPath(from)));
+                GetPath(from, null)));
 
             return this;
         }
@@ -485,8 +485,8 @@ namespace Microsoft.AspNetCore.JsonPatch
 
             Operations.Add(new Operation<TModel>(
                 "copy",
-                GetPath(path),
+                GetPath(path, null),
-                GetPath(from) + "/" + positionFrom));
+                GetPath(from, positionFrom.ToString())));
 
             return this;
         }
@@ -516,8 +516,8 @@ namespace Microsoft.AspNetCore.JsonPatch
 
             Operations.Add(new Operation<TModel>(
                 "copy",
-                GetPath(path) + "/" + positionTo,
+                GetPath(path, positionTo.ToString()),
-                GetPath(from)));
+                GetPath(from, null)));
 
             return this;
         }
@@ -549,8 +549,8 @@ namespace Microsoft.AspNetCore.JsonPatch
 
             Operations.Add(new Operation<TModel>(
                 "copy",
-                GetPath(path) + "/" + positionTo,
+                GetPath(path, positionTo.ToString()),
-                GetPath(from) + "/" + positionFrom));
+                GetPath(from, positionFrom.ToString())));
 
             return this;
         }
@@ -580,8 +580,8 @@ namespace Microsoft.AspNetCore.JsonPatch
 
             Operations.Add(new Operation<TModel>(
                 "copy",
-                GetPath(path) + "/-",
+                GetPath(path, "-"),
-                GetPath(from) + "/" + positionFrom));
+                GetPath(from, positionFrom.ToString())));
 
             return this;
         }
@@ -609,8 +609,8 @@ namespace Microsoft.AspNetCore.JsonPatch
 
             Operations.Add(new Operation<TModel>(
                 "copy",
-                GetPath(path) + "/-",
+                GetPath(path, "-"),
-                GetPath(from)));
+                GetPath(from, null)));
 
             return this;
         }
@@ -705,61 +705,57 @@ namespace Microsoft.AspNetCore.JsonPatch
             return allOps;
         }
 
-        private string GetPath<TProp>(Expression<Func<TModel, TProp>> expr)
+        // Internal for testing
+        internal string GetPath<TProp>(Expression<Func<TModel, TProp>> 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<string> GetPathSegments(Expression expr)
         {
+            var listOfSegments = new List<string>();
             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
-                return (expressionType == ExpressionType.ArrayIndex
+                || expressionType == ExpressionType.Convert
-                       || expressionType == ExpressionType.Call
+                || expressionType == ExpressionType.MemberAccess);
-                       || 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)
+        // 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<Func<object, object>>(converted, fakeParameter);
-            Func<object, object> func;
+            var func = lambda.Compile();
-
-            func = lambda.Compile();
 
             return Convert.ToString(func(null), CultureInfo.InvariantCulture);
         }

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")]

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);
 
+        /// <summary>
+        /// The expression '{0}' is not supported.
+        /// </summary>
+        internal static string ExpressionTypeNotSupported
+        {
+            get => GetString("ExpressionTypeNotSupported");
+        }
+
+        /// <summary>
+        /// The expression '{0}' is not supported.
+        /// </summary>
+        internal static string FormatExpressionTypeNotSupported(object p0)
+            => string.Format(CultureInfo.CurrentCulture, GetString("ExpressionTypeNotSupported"), p0);
+
         /// <summary>
         /// The index value provided by path segment '{0}' is out of bounds of the array size.
         /// </summary>
@@ -136,6 +150,20 @@ namespace Microsoft.AspNetCore.JsonPatch
         internal static string FormatInvalidJsonPatchOperation(object p0)
             => string.Format(CultureInfo.CurrentCulture, GetString("InvalidJsonPatchOperation"), p0);
 
+        /// <summary>
+        /// The provided path segment '{0}' cannot be converted to the target type.
+        /// </summary>
+        internal static string InvalidPathSegment
+        {
+            get => GetString("InvalidPathSegment");
+        }
+
+        /// <summary>
+        /// The provided path segment '{0}' cannot be converted to the target type.
+        /// </summary>
+        internal static string FormatInvalidPathSegment(object p0)
+            => string.Format(CultureInfo.CurrentCulture, GetString("InvalidPathSegment"), p0);
+
         /// <summary>
         /// The provided string '{0}' is an invalid path.
         /// </summary>
@@ -206,20 +234,6 @@ namespace Microsoft.AspNetCore.JsonPatch
         internal static string FormatPatchNotSupportedForNonGenericLists(object p0)
             => string.Format(CultureInfo.CurrentCulture, GetString("PatchNotSupportedForNonGenericLists"), p0);
 
-        /// <summary>
-        /// The provided path segment '{0}' cannot be converted to the target type.
-        /// </summary>
-        internal static string InvalidPathSegment
-        {
-            get => GetString("InvalidPathSegment");
-        }
-
-        /// <summary>
-        /// The provided path segment '{0}' cannot be converted to the target type.
-        /// </summary>
-        internal static string FormatInvalidPathSegment(object p0)
-            => string.Format(CultureInfo.CurrentCulture, GetString("InvalidPathSegment"), p0);
-
         /// <summary>
         /// The target location specified by path segment '{0}' was not found.
         /// </summary>

src/Microsoft.AspNetCore.JsonPatch/Resources.resx

@@ -132,6 +132,9 @@
   <data name="CannotUpdateProperty" xml:space="preserve">
     <value>The property at path '{0}' could not be updated.</value>
   </data>
+  <data name="ExpressionTypeNotSupported" xml:space="preserve">
+    <value>The expression '{0}' is not supported. Supported expressions include member access and indexer expressions.</value>
+  </data>
   <data name="IndexOutOfBounds" xml:space="preserve">
     <value>The index value provided by path segment '{0}' is out of bounds of the array size.</value>
   </data>

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<SimpleObjectWithNestedObject>();
+
+            // 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<int[]>();
+
+            // Act
+            var path = patchDocument.GetPath(p => p[3], null);
+
+            // Assert
+            Assert.Equal("/3", path);
+        }
+
+        [Fact]
+        public void ExpressionType_Call()
+        {
+            // Arrange
+            var patchDocument = new JsonPatchDocument<Dictionary<string, int>>();
+
+            // 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<SimpleObject>();
+
+            // Act
+            var path = patchDocument.GetPath(p => p, null);
+
+            // Assert
+            Assert.Equal("/", path);
+        }
+
+        [Fact]
+        public void ExpressionType_Parameter_WithPosition()
+        {
+            // Arrange
+            var patchDocument = new JsonPatchDocument<SimpleObject>();
+
+            // Act
+            var path = patchDocument.GetPath(p => p, "-");
+
+            // Assert
+            Assert.Equal("/-", path);
+        }
+
+        [Fact]
+        public void ExpressionType_Convert()
+        {
+            // Arrange
+            var patchDocument = new JsonPatchDocument<NestedObjectWithDerivedClass>();
+
+            // 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<SimpleObject>();
+
+            // Act
+            var exception = Assert.Throws<InvalidOperationException>(() =>
+            {
+                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
+    {
+    }
+}

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<List<JsonPropertyObject>>();
+            patchDoc.Add(p => p, new JsonPropertyObject());
+
+            var serialized = JsonConvert.SerializeObject(patchDoc);
+            var deserialized =
+                JsonConvert.DeserializeObject<JsonPatchDocument<JsonPropertyWithAnotherNameObject>>(serialized);
+
+            // get path
+            var pathToCheck = deserialized.Operations.First().path;
+            Assert.Equal("/-", pathToCheck);
+        }
+
+        [Fact]
+        public void Add_ToRoot_OfListOfObjects_AtGivenPosition()
+        {
+            var patchDoc = new JsonPatchDocument<List<JsonPropertyObject>>();
+            patchDoc.Add(p => p[3], new JsonPropertyObject());
+
+            var serialized = JsonConvert.SerializeObject(patchDoc);
+            var deserialized =
+                JsonConvert.DeserializeObject<JsonPatchDocument<JsonPropertyWithAnotherNameObject>>(serialized);
+
+            // get path
+            var pathToCheck = deserialized.Operations.First().path;
+            Assert.Equal("/3", pathToCheck);
+        }
+
         [Fact]
         public void Add_WithExpression_RespectsJsonPropertyName_ForModelProperty()
         {

test/Microsoft.AspNetCore.JsonPatch.Test/Microsoft.AspNetCore.JsonPatch.Test.csproj

@@ -14,4 +14,8 @@
     <PackageReference Include="Moq" />
   </ItemGroup>
 
+  <ItemGroup>
+    <Service Include="{82a7f48d-3b50-4b1e-b82e-3ada8210c358}" />
+  </ItemGroup>
+
 </Project>