From bf712263fcf51187fe970c7e177633251b6179c3 Mon Sep 17 00:00:00 2001 From: Kirthi Krishnamraju Date: Thu, 9 Apr 2015 15:42:25 -0700 Subject: [PATCH] Fix for #2280 - Cannot do an "add" patch to IList Added unit tests --- .../Adapters/ObjectAdapter.cs | 187 ++++++++++++++---- .../Helpers/PropertyHelpers.cs | 107 ---------- .../NestedObjectTests.cs | 104 ++++++++++ .../ObjectAdapterTests.cs | 43 ++++ .../SimpleDTO.cs | 2 + .../SimpleDTOWithNestedDTO.cs | 2 + 6 files changed, 296 insertions(+), 149 deletions(-) delete mode 100644 src/Microsoft.AspNet.JsonPatch/Helpers/PropertyHelpers.cs diff --git a/src/Microsoft.AspNet.JsonPatch/Adapters/ObjectAdapter.cs b/src/Microsoft.AspNet.JsonPatch/Adapters/ObjectAdapter.cs index 94da0289b5..25075d47dc 100644 --- a/src/Microsoft.AspNet.JsonPatch/Adapters/ObjectAdapter.cs +++ b/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 - .FindPropertyAndParent(objectToApplyTo, actualPathToProperty, ContractResolver); + var patchProperty = FindPropertyAndParent(objectToApplyTo, actualPathToProperty); // 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 - var genericTypeOfArray = PropertyHelpers.GetEnumerableType( - patchProperty.Property.PropertyType); + // now, get the generic type of the IList<> from Property type. + var genericTypeOfArray = GetIListType(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 - .FindPropertyAndParent(objectToApplyTo, actualFromProperty, ContractResolver); + var patchProperty = FindPropertyAndParent(objectToApplyTo, actualFromProperty); // 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 - var genericTypeOfArray = PropertyHelpers.GetEnumerableType(patchProperty.Property.PropertyType); + // now, get the generic type of the IList<> from Property type. + 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 - .FindPropertyAndParent(objectToApplyTo, actualPathToProperty, ContractResolver); + var patchProperty = FindPropertyAndParent(objectToApplyTo, actualPathToProperty); // 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 - var genericTypeOfArray = PropertyHelpers.GetEnumerableType(patchProperty.Property.PropertyType); + // now, get the generic type of the IList<> from Property type. + 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 - .FindPropertyAndParent(objectToApplyTo, actualPathProperty, ContractResolver); + var patchProperty = FindPropertyAndParent(objectToApplyTo, actualPathProperty); // 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 - typeOfFinalPropertyAtPathLocation = PropertyHelpers - .GetEnumerableType(patchProperty.Property.PropertyType); + // now, get the generic type of the IList<> from Property type. + typeOfFinalPropertyAtPathLocation = GetIListType(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( - typeOfFinalPropertyAtPathLocation, - operation.value); + var conversionResultTuple = ConvertToActualType(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 - .FindPropertyAndParent(objectToApplyTo, actualFromProperty, ContractResolver); + var patchProperty = FindPropertyAndParent(objectToApplyTo, actualFromProperty); // 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 - var genericTypeOfArray = PropertyHelpers.GetEnumerableType(patchProperty.Property.PropertyType); + // now, get the generic type of the IList<> from Property type. + 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)) - && typeof(IList).GetTypeInfo().IsAssignableFrom( - patchProperty.Property.PropertyType.GetTypeInfo()); + if (GetIListType(type) != null) + { + 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(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; + } } } \ No newline at end of file diff --git a/src/Microsoft.AspNet.JsonPatch/Helpers/PropertyHelpers.cs b/src/Microsoft.AspNet.JsonPatch/Helpers/PropertyHelpers.cs deleted file mode 100644 index 35e1247b72..0000000000 --- a/src/Microsoft.AspNet.JsonPatch/Helpers/PropertyHelpers.cs +++ /dev/null @@ -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; - } - } -} \ No newline at end of file diff --git a/test/Microsoft.AspNet.JsonPatch.Test/NestedObjectTests.cs b/test/Microsoft.AspNet.JsonPatch.Test/NestedObjectTests.cs index eb398df775..f3e602423c 100644 --- a/test/Microsoft.AspNet.JsonPatch.Test/NestedObjectTests.cs +++ b/test/Microsoft.AspNet.JsonPatch.Test/NestedObjectTests.cs @@ -201,6 +201,110 @@ namespace Microsoft.AspNet.JsonPatch.Test Assert.Equal(new List() { 4, 1, 2, 3 }, doc.SimpleDTO.IntegerList); } + [Fact] + public void AddToIntegerIList() + { + // Arrange + var doc = new SimpleDTOWithNestedDTO() + { + SimpleDTO = new SimpleDTO() + { + IntegerIList = new List() { 1, 2, 3 } + } + }; + + // create patch + var patchDoc = new JsonPatchDocument(); + patchDoc.Add(o => (List)o.SimpleDTO.IntegerIList, 4, 0); + + // Act + patchDoc.ApplyTo(doc); + + // Assert + Assert.Equal(new List() { 4, 1, 2, 3 }, doc.SimpleDTO.IntegerIList); + } + + [Fact] + public void AddToIntegerIListWithSerialization() + { + // Arrange + var doc = new SimpleDTOWithNestedDTO() + { + SimpleDTO = new SimpleDTO() + { + IntegerIList = new List() { 1, 2, 3 } + } + }; + + // create patch + var patchDoc = new JsonPatchDocument(); + patchDoc.Add(o => (List)o.SimpleDTO.IntegerIList, 4, 0); + + var serialized = JsonConvert.SerializeObject(patchDoc); + var deserialized = JsonConvert.DeserializeObject>(serialized); + + // Act + deserialized.ApplyTo(doc); + + // Assert + Assert.Equal(new List() { 4, 1, 2, 3 }, doc.SimpleDTO.IntegerIList); + } + + [Fact] + public void AddToNestedIntegerIList() + { + // Arrange + var doc = new SimpleDTOWithNestedDTO() + { + SimpleDTOIList = new List + { + new SimpleDTO + { + IntegerIList = new List() { 1, 2, 3 } + } + } + }; + + // create patch + var patchDoc = new JsonPatchDocument(); + patchDoc.Add(o => (List)o.SimpleDTOIList[0].IntegerIList, 4, 0); + + // Act + patchDoc.ApplyTo(doc); + + // Assert + Assert.Equal(new List() { 4, 1, 2, 3 }, doc.SimpleDTOIList[0].IntegerIList); + } + + [Fact] + public void AddToNestedIntegerIListWithSerialization() + { + // Arrange + var doc = new SimpleDTOWithNestedDTO() + { + SimpleDTOIList = new List + { + new SimpleDTO + { + IntegerIList = new List() { 1, 2, 3 } + } + } + }; + + // create patch + var patchDoc = new JsonPatchDocument(); + patchDoc.Add(o => (List)o.SimpleDTOIList[0].IntegerIList, 4, 0); + + var serialized = JsonConvert.SerializeObject(patchDoc); + var deserialized = JsonConvert.DeserializeObject>(serialized); + + // Act + deserialized.ApplyTo(doc); + + // Assert + Assert.Equal(new List() { 4, 1, 2, 3 }, doc.SimpleDTOIList[0].IntegerIList); + } + [Fact] public void AddToComplextTypeListSpecifyIndex() { diff --git a/test/Microsoft.AspNet.JsonPatch.Test/ObjectAdapterTests.cs b/test/Microsoft.AspNet.JsonPatch.Test/ObjectAdapterTests.cs index d7a16b0cff..99789a294d 100644 --- a/test/Microsoft.AspNet.JsonPatch.Test/ObjectAdapterTests.cs +++ b/test/Microsoft.AspNet.JsonPatch.Test/ObjectAdapterTests.cs @@ -98,6 +98,49 @@ namespace Microsoft.AspNet.JsonPatch.Test Assert.Equal(new List() { 4, 1, 2, 3 }, doc.IntegerList); } + [Fact] + public void AddToIntegerIList() + { + // Arrange + var doc = new SimpleDTO() + { + IntegerIList = new List() { 1, 2, 3 } + }; + + // create patch + var patchDoc = new JsonPatchDocument(); + patchDoc.Add(o => o.IntegerIList, 4, 0); + + // Act + patchDoc.ApplyTo(doc); + + // Assert + Assert.Equal(new List() { 4, 1, 2, 3 }, doc.IntegerIList); + } + + [Fact] + public void AddToIntegerIListWithSerialization() + { + // Arrange + var doc = new SimpleDTO() + { + IntegerIList = new List() { 1, 2, 3 } + }; + + // create patch + var patchDoc = new JsonPatchDocument(); + patchDoc.Add(o => o.IntegerIList, 4, 0); + + var serialized = JsonConvert.SerializeObject(patchDoc); + var deserialized = JsonConvert.DeserializeObject>(serialized); + + // Act + deserialized.ApplyTo(doc); + + // Assert + Assert.Equal(new List() { 4, 1, 2, 3 }, doc.IntegerIList); + } + [Fact] public void AddToListInvalidPositionTooLarge() { diff --git a/test/Microsoft.AspNet.JsonPatch.Test/SimpleDTO.cs b/test/Microsoft.AspNet.JsonPatch.Test/SimpleDTO.cs index 7af388099b..b7802b5269 100644 --- a/test/Microsoft.AspNet.JsonPatch.Test/SimpleDTO.cs +++ b/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 IntegerList { get; set; } + public IList IntegerIList { get; set; } public int IntegerValue { get; set; } public string StringProperty { get; set; } public string AnotherStringProperty { get; set; } diff --git a/test/Microsoft.AspNet.JsonPatch.Test/SimpleDTOWithNestedDTO.cs b/test/Microsoft.AspNet.JsonPatch.Test/SimpleDTOWithNestedDTO.cs index fbc9f71fcd..64cbdcf251 100644 --- a/test/Microsoft.AspNet.JsonPatch.Test/SimpleDTOWithNestedDTO.cs +++ b/test/Microsoft.AspNet.JsonPatch.Test/SimpleDTOWithNestedDTO.cs @@ -15,6 +15,8 @@ namespace Microsoft.AspNet.JsonPatch.Test public List SimpleDTOList { get; set; } + public IList SimpleDTOIList { get; set; } + public SimpleDTOWithNestedDTO() { this.NestedDTO = new NestedDTO();