class property setters with null checks makes Typed JsonPatchDocument tests fail.

[wannado]

class property setters with null checks like the following makes tests fail.
Eg. I added a backing field for SimpleDTO.StringProperty with a null check like the following.
These tests appear in NestedObjectsTests and ObjectAdapterTests class.

    public class SimpleDTO
    {
        public List<int> IntegerList { get; set; }
        public IList<int> IntegerIList { get; set; }
        public int IntegerValue { get; set; }
        string stringProperty;

        public string StringProperty
        {
            get
            {
                return stringProperty;
            }

            set
            {
                if (value == null) {
                    throw new ArgumentNullException();
                }
                stringProperty = value;
            }
        }

Test Name:  Microsoft.AspNetCore.JsonPatch.Test.NestedObjectTests.Replace
Test FullName:  Microsoft.AspNetCore.JsonPatch.Test.NestedObjectTests.Replace
Test Source:    D:\0\JsonPatch\test\Microsoft.AspNetCore.JsonPatch.Test\NestedObjectTests.cs : line 886
Test Outcome:   Failed
Test Duration:  0:00:00.036

Result StackTrace:  
at Newtonsoft.Json.Serialization.ExpressionValueProvider.SetValue(Object target, Object value)
   at Microsoft.AspNetCore.JsonPatch.Adapters.ObjectAdapter.Remove(String path, Object objectToApplyTo, Operation operationToReport) in D:\0\JsonPatch\src\Microsoft.AspNetCore.JsonPatch\Adapters\ObjectAdapter.cs:line 687
   at Microsoft.AspNetCore.JsonPatch.Adapters.ObjectAdapter.Replace(Operation operation, Object objectToApplyTo) in D:\0\JsonPatch\src\Microsoft.AspNetCore.JsonPatch\Adapters\ObjectAdapter.cs:line 725
   at Microsoft.AspNetCore.JsonPatch.Operations.Operation`1.Apply(TModel objectToApplyTo, IObjectAdapter adapter) in D:\0\JsonPatch\src\Microsoft.AspNetCore.JsonPatch\Operations\OperationOfT.cs:line 67
   at Microsoft.AspNetCore.JsonPatch.JsonPatchDocument`1.ApplyTo(TModel objectToApplyTo, IObjectAdapter adapter) in D:\0\JsonPatch\src\Microsoft.AspNetCore.JsonPatch\JsonPatchDocumentOfT.cs:line 674
   at Microsoft.AspNetCore.JsonPatch.JsonPatchDocument`1.ApplyTo(TModel objectToApplyTo) in D:\0\JsonPatch\src\Microsoft.AspNetCore.JsonPatch\JsonPatchDocumentOfT.cs:line 636
   at Microsoft.AspNetCore.JsonPatch.Test.NestedObjectTests.Replace() in D:\0\JsonPatch\test\Microsoft.AspNetCore.JsonPatch.Test\NestedObjectTests.cs:line 903
   at Microsoft.AspNetCore.JsonPatch.Test.SimpleDTO.set_StringProperty(String value) in D:\0\JsonPatch\test\Microsoft.AspNetCore.JsonPatch.Test\SimpleDTO.cs:line 28
   at Newtonsoft.Json.Serialization.ExpressionValueProvider.SetValue(Object target, Object value)
Result Message: 
Error setting value to 'StringProperty' on 'Microsoft.AspNetCore.JsonPatch.Test.SimpleDTO'.
Value cannot be null.

-----------------------
Test Name:  Microsoft.AspNetCore.JsonPatch.Test.NestedObjectTests.Move
Test FullName:  Microsoft.AspNetCore.JsonPatch.Test.NestedObjectTests.Move
Test Source:    D:\0\JsonPatch\test\Microsoft.AspNetCore.JsonPatch.Test\NestedObjectTests.cs : line 1683
Test Outcome:   Failed
Test Duration:  0:00:00.005

Result StackTrace:  
at Newtonsoft.Json.Serialization.ExpressionValueProvider.SetValue(Object target, Object value)
   at Microsoft.AspNetCore.JsonPatch.Adapters.ObjectAdapter.Remove(String path, Object objectToApplyTo, Operation operationToReport) in D:\0\JsonPatch\src\Microsoft.AspNetCore.JsonPatch\Adapters\ObjectAdapter.cs:line 687
   at Microsoft.AspNetCore.JsonPatch.Adapters.ObjectAdapter.Move(Operation operation, Object objectToApplyTo) in D:\0\JsonPatch\src\Microsoft.AspNetCore.JsonPatch\Adapters\ObjectAdapter.cs:line 411
   at Microsoft.AspNetCore.JsonPatch.Operations.Operation`1.Apply(TModel objectToApplyTo, IObjectAdapter adapter) in D:\0\JsonPatch\src\Microsoft.AspNetCore.JsonPatch\Operations\OperationOfT.cs:line 70
   at Microsoft.AspNetCore.JsonPatch.JsonPatchDocument`1.ApplyTo(TModel objectToApplyTo, IObjectAdapter adapter) in D:\0\JsonPatch\src\Microsoft.AspNetCore.JsonPatch\JsonPatchDocumentOfT.cs:line 674
   at Microsoft.AspNetCore.JsonPatch.JsonPatchDocument`1.ApplyTo(TModel objectToApplyTo) in D:\0\JsonPatch\src\Microsoft.AspNetCore.JsonPatch\JsonPatchDocumentOfT.cs:line 636
   at Microsoft.AspNetCore.JsonPatch.Test.NestedObjectTests.Move() in D:\0\JsonPatch\test\Microsoft.AspNetCore.JsonPatch.Test\NestedObjectTests.cs:line 1699
   at Microsoft.AspNetCore.JsonPatch.Test.SimpleDTO.set_StringProperty(String value) in D:\0\JsonPatch\test\Microsoft.AspNetCore.JsonPatch.Test\SimpleDTO.cs:line 28
   at Newtonsoft.Json.Serialization.ExpressionValueProvider.SetValue(Object target, Object value)
Result Message: 
Error setting value to 'StringProperty' on 'Microsoft.AspNetCore.JsonPatch.Test.SimpleDTO'.
Value cannot be null.

[Eilon]

@wannado out of curiosity, was the PATCH trying to clear out the value? That is, if the PATCH "document" was indicating that the property needed to be cleared, then I would expect this to happen because the property is indicating that it can't be cleared.

Or is the PATCH document just trying to set a new value (e.g. change property from "foo" to "bar")?

[wannado]

@Eilon Thanks for looking in to it!!! Yes patch document had "op" as "replace", as you said, just trying to set a new value (e.g. change property from "foo" to "bar".

[Eilon]

@wannado ah ok then that indeed sounds like a bug. We'll take a look!

[ryanwalls]

We are hitting this as well when we are trying to convert a string to an enum in a setter, e.g.

        public string Status
        {
            get
            {
                return this.SimulationStatus.ToString();
            }

            set
            {
                SimulationStatus updatedStatus;
                Console.WriteLine($"Trying to set simulation status to value: {value}");
                if (Enum.TryParse(value, out updatedStatus))
                {
                    this.SimulationStatus = updatedStatus;
                }
                else
                {
                    throw new ArgumentException($"Invalid simulation status setting: {value}");
                }
            }
        }

[Eilon]

@jbagga can you take a look?

[kichalla]

@Eilon Thanks for looking in to it!!! Yes patch document had "op" as "replace", as you said, just trying to set a new value (e.g. change property from "foo" to "bar".

I cannot repro this.

@ryanwalls what kind of operation are you trying to do when you hit this issue? for example, if you were trying to do a 'move' operation, the value of source property is copied onto the destination property and the source property would be set to its default value (in case of 'string' since its a reference type, it would be set to null).

[kichalla]

Update: I was able to repro it on 1.0...In this version during a Replace operation, a Remove operation is run first and then an Add and in case of Remove operation the default value for a type is set because of which we are seeing this exception.

[jbagga]

@ryanwalls @wannado Are you able to reproduce this? As @kichalla noted, there have been changes to the Replace operation since 1.0.0. If you can provide more information like the build version (1.0.0 or 1.1.0) and type of operation performed, we can investigate this further!

[ryanwalls]

I'm doing a "replace" operation with version 1.1.0.

[jbagga]

@ryanwalls I have tried to reproduce this issue with 1.1.0 but haven't been able to. Can you send us a sample or repro steps?

@Eilon Also, adding tests to 2.0.0 to ensure Replace does not fail for properties with null checks. See #56.

[ryanwalls]

@jbagga No worries. If you've ensured null checks work in 2.0.0, I'll just wait to update. We've worked around the problem for now.

[jbagga]

Closing this as it cannot be reproduced for 1.1.0. Added tests to ensure property setters with null checks do not throw for Replace operation.

Please feel free to reopen this issue if you have a repro sample app for 1.1.0. We can investigate this further then!