Fix for #2280 - Cannot do an "add" patch to IList<Anything>

Added unit tests

src/Microsoft.AspNet.JsonPatch/Adapters/ObjectAdapter.cs

@@ -3,10 +3,13 @@
 
 using System;
 using System.Collections;
+using System.Collections.Generic;
 using System.Reflection;
 using Microsoft.AspNet.JsonPatch.Exceptions;
 using Microsoft.AspNet.JsonPatch.Helpers;
 using Microsoft.AspNet.JsonPatch.Operations;
+using Microsoft.Framework.Internal;
+using Newtonsoft.Json;
 using Newtonsoft.Json.Serialization;
 
 namespace Microsoft.AspNet.JsonPatch.Adapters
@@ -124,17 +127,16 @@ namespace Microsoft.AspNet.JsonPatch.Adapters
             }
             else
             {
-                positionAsInteger = PropertyHelpers.GetNumericEnd(path);
+                positionAsInteger = GetNumericEnd(path);
 
                 if (positionAsInteger > -1)
                 {
                     actualPathToProperty = path.Substring(0,
-                        path.IndexOf('/' + positionAsInteger.ToString()));
+                        path.LastIndexOf('/' + positionAsInteger.ToString()));
                 }
             }
 
-            var patchProperty = PropertyHelpers
+            var patchProperty = FindPropertyAndParent(objectToApplyTo, actualPathToProperty);
-                .FindPropertyAndParent(objectToApplyTo, actualPathToProperty, ContractResolver);
 
             // does property at path exist?
             if (!CheckIfPropertyExists(patchProperty, objectToApplyTo, operationToReport, path))
@@ -148,13 +150,12 @@ namespace Microsoft.AspNet.JsonPatch.Adapters
             if (appendList || positionAsInteger > -1)
             {
                 // what if it's an array but there's no position??
-                if (IsNonStringArray(patchProperty))
+                if (IsNonStringArray(patchProperty.Property.PropertyType))
                 {
-                    // now, get the generic type of the enumerable
+                    // now, get the generic type of the IList<> from Property type.
-                    var genericTypeOfArray = PropertyHelpers.GetEnumerableType(
+                    var genericTypeOfArray = GetIListType(patchProperty.Property.PropertyType);
-                        patchProperty.Property.PropertyType);
 
-                    var conversionResult = PropertyHelpers.ConvertToActualType(genericTypeOfArray, value);
+                    var conversionResult = ConvertToActualType(genericTypeOfArray, value);
 
                     if (!CheckIfPropertyCanBeSet(conversionResult, objectToApplyTo, operationToReport, path))
                     {
@@ -198,7 +199,7 @@ namespace Microsoft.AspNet.JsonPatch.Adapters
             }
             else
             {
-                var conversionResultTuple = PropertyHelpers.ConvertToActualType(
+                var conversionResultTuple = ConvertToActualType(
                     patchProperty.Property.PropertyType,
                     value);
 
@@ -244,7 +245,7 @@ namespace Microsoft.AspNet.JsonPatch.Adapters
             var positionAsInteger = -1;
             var actualFromProperty = operation.from;
 
-            positionAsInteger = PropertyHelpers.GetNumericEnd(operation.from);
+            positionAsInteger = GetNumericEnd(operation.from);
 
             if (positionAsInteger > -1)
             {
@@ -252,8 +253,7 @@ namespace Microsoft.AspNet.JsonPatch.Adapters
                     operation.from.IndexOf('/' + positionAsInteger.ToString()));
             }
 
-            var patchProperty = PropertyHelpers
+            var patchProperty = FindPropertyAndParent(objectToApplyTo, actualFromProperty);
-                .FindPropertyAndParent(objectToApplyTo, actualFromProperty, ContractResolver);
 
             // does property at from exist?
             if (!CheckIfPropertyExists(patchProperty, objectToApplyTo, operation, operation.from))
@@ -265,10 +265,10 @@ namespace Microsoft.AspNet.JsonPatch.Adapters
             // the path must end with "/position" or "/-", which we already determined before.
             if (positionAsInteger > -1)
             {
-                if (IsNonStringArray(patchProperty))
+                if (IsNonStringArray(patchProperty.Property.PropertyType))
                 {
-                    // now, get the generic type of the enumerable
+                    // now, get the generic type of the IList<> from Property type.
-                    var genericTypeOfArray = PropertyHelpers.GetEnumerableType(patchProperty.Property.PropertyType);
+                    var genericTypeOfArray = GetIListType(patchProperty.Property.PropertyType);
 
                     // get value (it can be cast, we just checked that)
                     var array = (IList)patchProperty.Property.ValueProvider.GetValue(patchProperty.Parent);
@@ -345,7 +345,7 @@ namespace Microsoft.AspNet.JsonPatch.Adapters
             }
             else
             {
-                positionAsInteger = PropertyHelpers.GetNumericEnd(path);
+                positionAsInteger = GetNumericEnd(path);
 
                 if (positionAsInteger > -1)
                 {
@@ -354,8 +354,7 @@ namespace Microsoft.AspNet.JsonPatch.Adapters
                 }
             }
 
-            var patchProperty = PropertyHelpers
+            var patchProperty = FindPropertyAndParent(objectToApplyTo, actualPathToProperty);
-                .FindPropertyAndParent(objectToApplyTo, actualPathToProperty, ContractResolver);
 
             // does the target location exist?
             if (!CheckIfPropertyExists(patchProperty, objectToApplyTo, operationToReport, path))
@@ -369,10 +368,10 @@ namespace Microsoft.AspNet.JsonPatch.Adapters
             if (removeFromList || positionAsInteger > -1)
             {
                 // what if it's an array but there's no position??
-                if (IsNonStringArray(patchProperty))
+                if (IsNonStringArray(patchProperty.Property.PropertyType))
                 {
-                    // now, get the generic type of the enumerable
+                    // now, get the generic type of the IList<> from Property type.
-                    var genericTypeOfArray = PropertyHelpers.GetEnumerableType(patchProperty.Property.PropertyType);
+                    var genericTypeOfArray = GetIListType(patchProperty.Property.PropertyType);
 
                     // get value (it can be cast, we just checked that)
                     var array = (IList)patchProperty.Property.ValueProvider.GetValue(patchProperty.Parent);
@@ -480,7 +479,7 @@ namespace Microsoft.AspNet.JsonPatch.Adapters
             var positionInPathAsInteger = -1;
             var actualPathProperty = operation.path;
 
-            positionInPathAsInteger = PropertyHelpers.GetNumericEnd(operation.path);
+            positionInPathAsInteger = GetNumericEnd(operation.path);
 
             if (positionInPathAsInteger > -1)
             {
@@ -488,8 +487,7 @@ namespace Microsoft.AspNet.JsonPatch.Adapters
                     operation.path.IndexOf('/' + positionInPathAsInteger.ToString()));
             }
 
-            var patchProperty = PropertyHelpers
+            var patchProperty = FindPropertyAndParent(objectToApplyTo, actualPathProperty);
-                .FindPropertyAndParent(objectToApplyTo, actualPathProperty, ContractResolver);
 
             // does property at path exist?
             if (!CheckIfPropertyExists(patchProperty, objectToApplyTo, operation, operation.path))
@@ -504,11 +502,10 @@ namespace Microsoft.AspNet.JsonPatch.Adapters
             // the path must end with "/position" or "/-", which we already determined before.
             if (positionInPathAsInteger > -1)
             {
-                if (IsNonStringArray(patchProperty))
+                if (IsNonStringArray(patchProperty.Property.PropertyType))
                 {
-                    // now, get the generic type of the enumerable
+                    // now, get the generic type of the IList<> from Property type.
-                    typeOfFinalPropertyAtPathLocation = PropertyHelpers
+                    typeOfFinalPropertyAtPathLocation = GetIListType(patchProperty.Property.PropertyType);
-                        .GetEnumerableType(patchProperty.Property.PropertyType);
 
                     // get value (it can be cast, we just checked that)
                     var array = (IList)patchProperty.Property.ValueProvider.GetValue(patchProperty.Parent);
@@ -542,9 +539,7 @@ namespace Microsoft.AspNet.JsonPatch.Adapters
                 typeOfFinalPropertyAtPathLocation = patchProperty.Property.PropertyType;
             }
 
-            var conversionResultTuple = PropertyHelpers.ConvertToActualType(
+            var conversionResultTuple = ConvertToActualType(typeOfFinalPropertyAtPathLocation, operation.value);
-                typeOfFinalPropertyAtPathLocation,
-                operation.value);
 
             // Is conversion successful
             if (!CheckIfPropertyCanBeSet(conversionResultTuple, objectToApplyTo, operation, operation.path))
@@ -610,7 +605,7 @@ namespace Microsoft.AspNet.JsonPatch.Adapters
             var positionAsInteger = -1;
             var actualFromProperty = operation.from;
 
-            positionAsInteger = PropertyHelpers.GetNumericEnd(operation.from);
+            positionAsInteger = GetNumericEnd(operation.from);
 
             if (positionAsInteger > -1)
             {
@@ -618,8 +613,7 @@ namespace Microsoft.AspNet.JsonPatch.Adapters
                     operation.from.IndexOf('/' + positionAsInteger.ToString()));
             }
 
-            var patchProperty = PropertyHelpers
+            var patchProperty = FindPropertyAndParent(objectToApplyTo, actualFromProperty);
-                .FindPropertyAndParent(objectToApplyTo, actualFromProperty, ContractResolver);
 
             // does property at from exist?
             if (!CheckIfPropertyExists(patchProperty, objectToApplyTo, operation, operation.from))
@@ -632,10 +626,10 @@ namespace Microsoft.AspNet.JsonPatch.Adapters
             // the path must end with "/position" or "/-", which we already determined before.
             if (positionAsInteger > -1)
             {
-                if (IsNonStringArray(patchProperty))
+                if (IsNonStringArray(patchProperty.Property.PropertyType))
                 {
-                    // now, get the generic type of the enumerable
+                    // now, get the generic type of the IList<> from Property type.
-                    var genericTypeOfArray = PropertyHelpers.GetEnumerableType(patchProperty.Property.PropertyType);
+                    var genericTypeOfArray = GetIListType(patchProperty.Property.PropertyType);
 
                     // get value (it can be cast, we just checked that)
                     var array = (IList)patchProperty.Property.ValueProvider.GetValue(patchProperty.Parent);
@@ -702,11 +696,14 @@ namespace Microsoft.AspNet.JsonPatch.Adapters
             return true;
         }
 
-        private bool IsNonStringArray(JsonPatchProperty patchProperty)
+        private bool IsNonStringArray(Type type)
         {
-            return !(patchProperty.Property.PropertyType == typeof(string))
+            if (GetIListType(type) != null)
-                    && typeof(IList).GetTypeInfo().IsAssignableFrom(
+            {
-                        patchProperty.Property.PropertyType.GetTypeInfo());
+                return true;
+            }
+
+            return (!(type == typeof(string)) && typeof(IList).GetTypeInfo().IsAssignableFrom(type.GetTypeInfo()));
         }
 
         private bool CheckIfPropertyCanBeSet(
@@ -739,5 +736,111 @@ namespace Microsoft.AspNet.JsonPatch.Adapters
                 throw new JsonPatchException<T>(jsonPatchError);
             }
         }
+
+        private JsonPatchProperty FindPropertyAndParent(
+            object targetObject,
+            string propertyPath)
+        {
+            try
+            {
+                var splitPath = propertyPath.Split('/');
+
+                // skip the first one if it's empty
+                var startIndex = (string.IsNullOrWhiteSpace(splitPath[0]) ? 1 : 0);
+
+                for (int i = startIndex; i < splitPath.Length; i++)
+                {
+                    var jsonContract = (JsonObjectContract)ContractResolver.ResolveContract(targetObject.GetType());
+
+                    foreach (var property in jsonContract.Properties)
+                    {
+                        if (string.Equals(property.PropertyName, splitPath[i], StringComparison.OrdinalIgnoreCase))
+                        {
+                            if (i == (splitPath.Length - 1))
+                            {
+                                return new JsonPatchProperty(property, targetObject);
+                            }
+                            else
+                            {
+                                targetObject = property.ValueProvider.GetValue(targetObject);
+
+                                // if property is of IList type then get the array index from splitPath and get the
+                                // object at the indexed position from the list.
+                               if (GetIListType(property.PropertyType) != null)
+                                {
+                                    var index = int.Parse(splitPath[++i]);
+                                    targetObject = ((IList)targetObject)[index];
+                                }
+                            }
+
+                            break;
+                        }
+                    }
+                }
+
+                return null;
+            }
+            catch (Exception)
+            {
+                // will result in JsonPatchException in calling class, as expected
+                return null;
+            }
+        }
+
+        private ConversionResult ConvertToActualType(Type propertyType, object value)
+        {
+            try
+            {
+                var o = JsonConvert.DeserializeObject(JsonConvert.SerializeObject(value), propertyType);
+
+                return new ConversionResult(true, o);
+            }
+            catch (Exception)
+            {
+                return new ConversionResult(false, null);
+            }
+        }
+
+        private Type GetIListType([NotNull] Type type)
+        {
+            if (IsGenericListType(type))
+            {
+                return type.GetGenericArguments()[0];
+            }
+
+            foreach (Type interfaceType in type.GetTypeInfo().ImplementedInterfaces)
+            {
+                if (IsGenericListType(interfaceType))
+                {
+                    return interfaceType.GetGenericArguments()[0];
+                }
+            }
+
+            return null;
+        }
+
+        private bool IsGenericListType([NotNull] Type type)
+        {
+            if (type.GetTypeInfo().IsGenericType &&
+                    type.GetGenericTypeDefinition() == typeof(IList<>))
+            {
+                return true;
+            }
+
+            return false;
+        }
+
+        private int GetNumericEnd(string path)
+        {
+            var possibleIndex = path.Substring(path.LastIndexOf("/") + 1);
+            var castedIndex = -1;
+
+            if (int.TryParse(possibleIndex, out castedIndex))
+            {
+                return castedIndex;
+            }
+
+            return -1;
+        }
     }
 }

src/Microsoft.AspNet.JsonPatch/Helpers/PropertyHelpers.cs

@@ -1,107 +0,0 @@
-// Copyright (c) Microsoft Open Technologies, Inc. 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;
-using System.Collections.Generic;
-using System.Reflection;
-using Newtonsoft.Json;
-using Newtonsoft.Json.Serialization;
-
-namespace Microsoft.AspNet.JsonPatch.Helpers
-{
-    internal static class PropertyHelpers
-    {
-        public static JsonPatchProperty FindPropertyAndParent(
-            object targetObject,
-            string propertyPath,
-            IContractResolver contractResolver)
-        {
-            try
-            {
-                var splitPath = propertyPath.Split('/');
-
-                // skip the first one if it's empty
-                var startIndex = (string.IsNullOrWhiteSpace(splitPath[0]) ? 1 : 0);
-
-                for (int i = startIndex; i < splitPath.Length; i++)
-                {
-                    var jsonContract = (JsonObjectContract)contractResolver.ResolveContract(targetObject.GetType());
-
-                    foreach (var property in jsonContract.Properties)
-                    {
-                        if (string.Equals(property.PropertyName, splitPath[i], StringComparison.OrdinalIgnoreCase))
-                        {
-                            if (i == (splitPath.Length - 1))
-                            {
-                                return new JsonPatchProperty(property, targetObject);
-                            }
-                            else
-                            {
-                                targetObject = property.ValueProvider.GetValue(targetObject);
-
-                                // if property is of IList type then get the array index from splitPath and get the
-                                // object at the indexed position from the list.
-                                if (typeof(IList).GetTypeInfo().IsAssignableFrom(property.PropertyType.GetTypeInfo()))
-                                {
-                                    var index = int.Parse(splitPath[++i]);
-                                    targetObject = ((IList)targetObject)[index];
-                                }
-                            }
-
-                            break;
-                        }
-                    }
-                }
-
-                return null;
-            }
-            catch (Exception)
-            {
-                // will result in JsonPatchException in calling class, as expected
-                return null;
-            }
-        }
-
-        internal static ConversionResult ConvertToActualType(Type propertyType, object value)
-        {
-            try
-            {
-                var o = JsonConvert.DeserializeObject(JsonConvert.SerializeObject(value), propertyType);
-
-                return new ConversionResult(true, o);
-            }
-            catch (Exception)
-            {
-                return new ConversionResult(false, null);
-            }
-        }
-
-        internal static Type GetEnumerableType(Type type)
-        {
-            if (type == null) throw new ArgumentNullException();
-            foreach (Type interfaceType in type.GetInterfaces())
-            {
-                if (interfaceType.GetTypeInfo().IsGenericType &&
-                    interfaceType.GetGenericTypeDefinition() == typeof(IEnumerable<>))
-                {
-                    return interfaceType.GetGenericArguments()[0];
-                }
-            }
-            return null;
-        }
-
-        internal static int GetNumericEnd(string path)
-        {
-            var possibleIndex = path.Substring(path.LastIndexOf("/") + 1);
-            var castedIndex = -1;
-
-            if (int.TryParse(possibleIndex, out castedIndex))
-            {
-                return castedIndex;
-            }
-
-            return -1;
-        }
-    }
-}

test/Microsoft.AspNet.JsonPatch.Test/NestedObjectTests.cs

@@ -201,6 +201,110 @@ namespace Microsoft.AspNet.JsonPatch.Test
             Assert.Equal(new List<int>() { 4, 1, 2, 3 }, doc.SimpleDTO.IntegerList);
         }
 
+        [Fact]
+        public void AddToIntegerIList()
+        {
+            // Arrange
+            var doc = new SimpleDTOWithNestedDTO()
+            {
+                SimpleDTO = new SimpleDTO()
+                {
+                    IntegerIList = new List<int>() { 1, 2, 3 }
+                }
+            };
+
+            // create patch
+            var patchDoc = new JsonPatchDocument<SimpleDTOWithNestedDTO>();
+            patchDoc.Add<int>(o => (List<int>)o.SimpleDTO.IntegerIList, 4, 0);
+
+            // Act
+            patchDoc.ApplyTo(doc);
+
+            // Assert
+            Assert.Equal(new List<int>() { 4, 1, 2, 3 }, doc.SimpleDTO.IntegerIList);
+        }
+
+        [Fact]
+        public void AddToIntegerIListWithSerialization()
+        {
+            // Arrange
+            var doc = new SimpleDTOWithNestedDTO()
+            {
+                SimpleDTO = new SimpleDTO()
+                {
+                    IntegerIList = new List<int>() { 1, 2, 3 }
+                }
+            };
+
+            // create patch
+            var patchDoc = new JsonPatchDocument<SimpleDTOWithNestedDTO>();
+            patchDoc.Add<int>(o => (List<int>)o.SimpleDTO.IntegerIList, 4, 0);
+
+            var serialized = JsonConvert.SerializeObject(patchDoc);
+            var deserialized = JsonConvert.DeserializeObject<JsonPatchDocument<SimpleDTOWithNestedDTO>>(serialized);
+
+            // Act
+            deserialized.ApplyTo(doc);
+
+            // Assert
+            Assert.Equal(new List<int>() { 4, 1, 2, 3 }, doc.SimpleDTO.IntegerIList);
+        }
+
+        [Fact]
+        public void AddToNestedIntegerIList()
+        {
+            // Arrange
+            var doc = new SimpleDTOWithNestedDTO()
+            {
+                SimpleDTOIList = new List<SimpleDTO>
+                {
+                    new SimpleDTO
+                    {
+                        IntegerIList = new List<int>() { 1, 2, 3 }
+                    }
+                }
+            };
+
+            // create patch
+            var patchDoc = new JsonPatchDocument<SimpleDTOWithNestedDTO>();
+            patchDoc.Add<int>(o => (List<int>)o.SimpleDTOIList[0].IntegerIList, 4, 0);
+
+            // Act
+            patchDoc.ApplyTo(doc);
+
+            // Assert
+            Assert.Equal(new List<int>() { 4, 1, 2, 3 }, doc.SimpleDTOIList[0].IntegerIList);
+        }
+
+        [Fact]
+        public void AddToNestedIntegerIListWithSerialization()
+        {
+            // Arrange
+            var doc = new SimpleDTOWithNestedDTO()
+            {
+                SimpleDTOIList = new List<SimpleDTO>
+                {
+                    new SimpleDTO
+                    {
+                        IntegerIList = new List<int>() { 1, 2, 3 }
+                    }
+                }
+            };
+
+            // create patch
+            var patchDoc = new JsonPatchDocument<SimpleDTOWithNestedDTO>();
+            patchDoc.Add<int>(o => (List<int>)o.SimpleDTOIList[0].IntegerIList, 4, 0);
+
+            var serialized = JsonConvert.SerializeObject(patchDoc);
+            var deserialized = JsonConvert.DeserializeObject<JsonPatchDocument<SimpleDTOWithNestedDTO>>(serialized);
+
+            // Act
+            deserialized.ApplyTo(doc);
+
+            // Assert
+            Assert.Equal(new List<int>() { 4, 1, 2, 3 }, doc.SimpleDTOIList[0].IntegerIList);
+        }
+
         [Fact]
         public void AddToComplextTypeListSpecifyIndex()
         {

test/Microsoft.AspNet.JsonPatch.Test/ObjectAdapterTests.cs

@@ -98,6 +98,49 @@ namespace Microsoft.AspNet.JsonPatch.Test
             Assert.Equal(new List<int>() { 4, 1, 2, 3 }, doc.IntegerList);
         }
 
+        [Fact]
+        public void AddToIntegerIList()
+        {
+            // Arrange
+            var doc = new SimpleDTO()
+            {
+                IntegerIList = new List<int>() { 1, 2, 3 }
+            };
+
+            // create patch
+            var patchDoc = new JsonPatchDocument<SimpleDTO>();
+            patchDoc.Add<int>(o => o.IntegerIList, 4, 0);
+
+            // Act
+            patchDoc.ApplyTo(doc);
+
+            // Assert
+            Assert.Equal(new List<int>() { 4, 1, 2, 3 }, doc.IntegerIList);
+        }
+
+        [Fact]
+        public void AddToIntegerIListWithSerialization()
+        {
+            // Arrange
+            var doc = new SimpleDTO()
+            {
+                IntegerIList = new List<int>() { 1, 2, 3 }
+            };
+
+            // create patch
+            var patchDoc = new JsonPatchDocument<SimpleDTO>();
+            patchDoc.Add<int>(o => o.IntegerIList, 4, 0);
+
+            var serialized = JsonConvert.SerializeObject(patchDoc);
+            var deserialized = JsonConvert.DeserializeObject<JsonPatchDocument<SimpleDTO>>(serialized);
+
+            // Act
+            deserialized.ApplyTo(doc);
+
+            // Assert
+            Assert.Equal(new List<int>() { 4, 1, 2, 3 }, doc.IntegerIList);
+        }
+
         [Fact]
         public void AddToListInvalidPositionTooLarge()
         {

test/Microsoft.AspNet.JsonPatch.Test/SimpleDTO.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 System.Collections;
 using System.Collections.Generic;
 
 namespace Microsoft.AspNet.JsonPatch.Test
@@ -9,6 +10,7 @@ namespace Microsoft.AspNet.JsonPatch.Test
     public class SimpleDTO
     {
         public List<int> IntegerList { get; set; }
+        public IList<int> IntegerIList { get; set; }
         public int IntegerValue { get; set; }
         public string StringProperty { get; set; }
         public string AnotherStringProperty { get; set; }

test/Microsoft.AspNet.JsonPatch.Test/SimpleDTOWithNestedDTO.cs

@@ -15,6 +15,8 @@ namespace Microsoft.AspNet.JsonPatch.Test
 
         public List<SimpleDTO> SimpleDTOList { get; set; }
 
+        public IList<SimpleDTO> SimpleDTOIList { get; set; }
+
         public SimpleDTOWithNestedDTO()
         {
             this.NestedDTO = new NestedDTO();