JsonPatchDocument.Replace() yields invalid path when [JsonProperty] is used (1.1.0)

[visav]

Title

In 1.1.0, JsonPatchDocument<T>.Replace() creates Operation<T> with invalid path, when [JsonProperty] from Newtonsoft.Json is used.

Functional impact

This is regression from "Microsoft.AspNetCore.JsonPatch": "1.0.0" and breaks existing behavior.

Minimal repro steps

global.json:

  "sdk": {
    "version": "1.0.0-preview2-1-003177"
  }

project.json:

{
  "version": "1.0.0-*",
  "buildOptions": {
    "emitEntryPoint": true
  },

  "dependencies": {
    "Microsoft.AspNetCore.JsonPatch": "1.1.0",
    "Microsoft.NETCore.App": {
      "type": "platform",
      "version": "1.1.0"
    },
    "Newtonsoft.Json": "9.0.1"
  },

  "frameworks": {
    "netcoreapp1.0": {
      "imports": "dnxcore50"
    }
  }
}

Program.cs:

using System.Diagnostics;
using System.Linq;
using Microsoft.AspNetCore.JsonPatch;
using Newtonsoft.Json;

namespace JsonPatchReplaceIssue
{
    public class Model
    {
        [JsonProperty]
        public string[] ArrayOfStringsOrNull { get; set; }
    }

    public class Program
    {
        public static void Main(string[] args)
        {
            var patch = new JsonPatchDocument<Model>()
                .Replace(m => m.ArrayOfStringsOrNull, new[]
                {
                    "yo"
                });

            var operation = patch.Operations.Single();

            Debug.Assert(operation.path == $"/{nameof(Model.ArrayOfStringsOrNull).ToLowerInvariant()}",
                $"No it ain't! It is '{operation.path}'");
        }
    }
}

Expected result

The Operation<T>.path should be derived from the property in the expression. Expected path to be "/arrayofstringsornull" in this case.

Actual result

Actual path is "/", which is incorrect.

Further technical details

Regression from "Microsoft.AspNetCore.JsonPatch": "1.0.0" with .NET Core 1.0.
I have tested:

• Change the JsonPatch dependency in project.json to 1.0.0 and it works as expected.
• Remove [JsonProperty] and it works as expected.

Question

Am I doing it wrong? I'm assuming that Newtonsoft.Json's attributes are supported.

[visav]

Ok, I think I've found the reason for this. I was originally using [JsonProperty(NullValueHandling = NullValueHandling.Ignore)] without propertyName.

See https://github.com/aspnet/JsonPatch/blob/dev/src/Microsoft.AspNetCore.JsonPatch/Internal/ExpressionHelpers.cs#L86.

When JsonPropertyAttribute is found in ExpressionHelpers.GetPropertyNameFromMemberExpression(MemberExpression memberExpression), it assumes that PropertyName is valid and returns it. However, when [JsonProperty] is used without explicit propertyName, castedAttribute.PropertyName will be null and the result invalid.

Workaround: use explicit propertyName when using [JsonProperty].

[Eilon]

@kichalla can you investigate? If this is clearly a bug, would you recommend we patch in 1.1.1?

[Eilon]

Putting in 2.0.0 and marking as a bug. In the meantime the workaround should suffice.

[kichalla]

We should consider using the ContractResolver to figure out the property names rather than doing the following:
https://github.com/aspnet/JsonPatch/blob/dev/src/Microsoft.AspNetCore.JsonPatch/Internal/ExpressionHelpers.cs#L76-L78

[Eilon]

And please check if there are any other places in the codes where we may have gotten that wrong.