From bf9fd0d10667e605d1df45f89e198be7f3bd1138 Mon Sep 17 00:00:00 2001 From: Alexander Shabunevich Date: Fri, 9 Nov 2018 01:29:34 +0300 Subject: [PATCH] Prevent null refs when copying a property with a null value * Fix aspnet/AspNetCore#3559 Json Patch: System.NullReferenceException while trying to use copy operation from property with null value. * Fix aspnet/AspNetCore#3559: Missing tests added. --- .../Adapters/ObjectAdapter.cs | 2 +- .../Internal/ConversionResultProvider.cs | 2 +- .../JsonPatchDocumentOfT.cs | 2 +- .../ExpandoObjectIntegrationTest.cs | 19 ++++++++++++++ .../NestedObjectIntegrationTest.cs | 25 ++++++++++++++++++- .../SimpleObjectIntegrationTest.cs | 20 +++++++++++++++ 6 files changed, 66 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.AspNetCore.JsonPatch/Adapters/ObjectAdapter.cs b/src/Microsoft.AspNetCore.JsonPatch/Adapters/ObjectAdapter.cs index d625176376..75cc513312 100644 --- a/src/Microsoft.AspNetCore.JsonPatch/Adapters/ObjectAdapter.cs +++ b/src/Microsoft.AspNetCore.JsonPatch/Adapters/ObjectAdapter.cs @@ -229,7 +229,7 @@ namespace Microsoft.AspNetCore.JsonPatch.Adapters if (TryGetValue(operation.from, objectToApplyTo, operation, out var propertyValue)) { // Create deep copy - var copyResult = ConversionResultProvider.CopyTo(propertyValue, propertyValue.GetType()); + var copyResult = ConversionResultProvider.CopyTo(propertyValue, propertyValue?.GetType()); if (copyResult.CanBeConverted) { Add(operation.path, diff --git a/src/Microsoft.AspNetCore.JsonPatch/Internal/ConversionResultProvider.cs b/src/Microsoft.AspNetCore.JsonPatch/Internal/ConversionResultProvider.cs index 71af0d27fc..3d0b6cc979 100644 --- a/src/Microsoft.AspNetCore.JsonPatch/Internal/ConversionResultProvider.cs +++ b/src/Microsoft.AspNetCore.JsonPatch/Internal/ConversionResultProvider.cs @@ -39,7 +39,7 @@ namespace Microsoft.AspNetCore.JsonPatch.Internal var targetType = typeToConvertTo; if (value == null) { - return new ConversionResult(IsNullableType(typeToConvertTo), null); + return new ConversionResult(canBeConverted: true, convertedInstance: null); } else if (typeToConvertTo.IsAssignableFrom(value.GetType())) { diff --git a/src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocumentOfT.cs b/src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocumentOfT.cs index 3db0aac458..e0b4fca240 100644 --- a/src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocumentOfT.cs +++ b/src/Microsoft.AspNetCore.JsonPatch/JsonPatchDocumentOfT.cs @@ -503,7 +503,7 @@ namespace Microsoft.AspNetCore.JsonPatch } /// - /// Copy the value at specified location to the target location. Willr esult in, for example: + /// Copy the value at specified location to the target location. Will result in, for example: /// { "op": "copy", "from": "/a/b/c", "path": "/a/b/e" } /// /// source location diff --git a/test/Microsoft.AspNetCore.JsonPatch.Test/IntegrationTests/ExpandoObjectIntegrationTest.cs b/test/Microsoft.AspNetCore.JsonPatch.Test/IntegrationTests/ExpandoObjectIntegrationTest.cs index b1b58556f0..29fa5fc731 100644 --- a/test/Microsoft.AspNetCore.JsonPatch.Test/IntegrationTests/ExpandoObjectIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.JsonPatch.Test/IntegrationTests/ExpandoObjectIntegrationTest.cs @@ -159,6 +159,25 @@ namespace Microsoft.AspNetCore.JsonPatch.IntegrationTests Assert.Equal("A", targetObject.AnotherStringProperty); } + [Fact] + public void CopyNullStringProperty_ToAnotherStringProperty() + { + // Arrange + dynamic targetObject = new ExpandoObject(); + + targetObject.StringProperty = null; + targetObject.AnotherStringProperty = "B"; + + var patchDocument = new JsonPatchDocument(); + patchDocument.Copy("StringProperty", "AnotherStringProperty"); + + // Act + patchDocument.ApplyTo(targetObject); + + // Assert + Assert.Null(targetObject.AnotherStringProperty); + } + [Fact] public void MoveIntegerValue_ToAnotherIntegerProperty() { diff --git a/test/Microsoft.AspNetCore.JsonPatch.Test/IntegrationTests/NestedObjectIntegrationTest.cs b/test/Microsoft.AspNetCore.JsonPatch.Test/IntegrationTests/NestedObjectIntegrationTest.cs index 92c0e7fb2d..2a7d6f7cd2 100644 --- a/test/Microsoft.AspNetCore.JsonPatch.Test/IntegrationTests/NestedObjectIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.JsonPatch.Test/IntegrationTests/NestedObjectIntegrationTest.cs @@ -175,7 +175,30 @@ namespace Microsoft.AspNetCore.JsonPatch.IntegrationTests // Assert Assert.Equal("A", targetObject.SimpleObject.AnotherStringProperty); - } + } + + [Fact] + public void CopyNullStringProperty_ToAnotherStringProperty() + { + // Arrange + var targetObject = new SimpleObjectWithNestedObject() + { + SimpleObject = new SimpleObject() + { + StringProperty = null, + AnotherStringProperty = "B" + } + }; + + var patchDocument = new JsonPatchDocument(); + patchDocument.Copy(o => o.SimpleObject.StringProperty, o => o.SimpleObject.AnotherStringProperty); + + // Act + patchDocument.ApplyTo(targetObject); + + // Assert + Assert.Null(targetObject.SimpleObject.AnotherStringProperty); + } [Fact] public void Copy_DeepClonesObject() diff --git a/test/Microsoft.AspNetCore.JsonPatch.Test/IntegrationTests/SimpleObjectIntegrationTest.cs b/test/Microsoft.AspNetCore.JsonPatch.Test/IntegrationTests/SimpleObjectIntegrationTest.cs index 4672d9c97b..731e181697 100644 --- a/test/Microsoft.AspNetCore.JsonPatch.Test/IntegrationTests/SimpleObjectIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.JsonPatch.Test/IntegrationTests/SimpleObjectIntegrationTest.cs @@ -46,6 +46,26 @@ namespace Microsoft.AspNetCore.JsonPatch.IntegrationTests Assert.Equal("A", targetObject.AnotherStringProperty); } + [Fact] + public void CopyNullStringProperty_ToAnotherStringProperty() + { + // Arrange + var targetObject = new SimpleObject() + { + StringProperty = null, + AnotherStringProperty = "B" + }; + + var patchDocument = new JsonPatchDocument(); + patchDocument.Copy("StringProperty", "AnotherStringProperty"); + + // Act + patchDocument.ApplyTo(targetObject); + + // Assert + Assert.Null(targetObject.AnotherStringProperty); + } + [Fact] public void MoveIntegerProperty_ToAnotherIntegerProperty() {