Pomelo.EntityFrameworkCore.MySql: JsonObject property setters do not track changes in array elements

Steps to reproduce

Copy paste the IceCreames example. Add and change Program.Main() to below.

using (var context = new Context())
{
    context.Database.EnsureDeleted();
    context.Database.EnsureCreated();

    context.IceCreams.AddRange(
        new IceCream
        {
            Name = "Vanilla",
            Energy = new Energy
            {
                Kilojoules = 866.0,
                Kilocalories = 207.0,
            },
            Comments = new[]
            {
                    "First!",
                    "Delicios!"
            },
        },
        new IceCream
        {
            Name = "Chocolate",
            Energy = new Energy
            {
                Kilojoules = 904.0,
                Kilocalories = 216.0,
            },
            Comments = new[]
            {
                    "My husband linkes this one a lot."
            },
        });

    context.SaveChanges();
}

using (var context = new Context())
{
    IceCream iceCream = context.IceCreams.Where(e => e.IceCreamId == 1).First();
    //iceCream.Comments.Object[0] = "Test update.";
    //iceCream.Comments.Object = new string[] { "Test update.", "Second line." };
    //iceCream.Comments.Json = "[ 'Test update.', 'Second line.' ]";
    iceCream.Comments = new string[] { "Test update.", "Second line." };
    iceCream.Comments = "[ 'Test update.', 'Second line.' ]";
    context.SaveChanges();
}

using (var context = new Context())
{
    var result = context.IceCreams
        .OrderBy(e => e.IceCreamId)
        .ToList();

    Debug.Assert(result.Count == 2);

    Debug.Assert(result[0].Name == "Vanilla");
    Debug.Assert(result[0].Energy.Object.Kilojoules == 866.0);
    Debug.Assert(result[0].Comments.Object.Length == 2);
    Debug.Assert(result[0].Comments.Object[0] == "Test update.");
}

The issue

Assertion failed with below commented cases.

//iceCream.Comments.Object[0] = "Test update.";
//iceCream.Comments.Object = new string[] { "Test update.", "Second line." };
//iceCream.Comments.Json = "[ 'Test update.', 'Second line.' ]";
iceCream.Comments = new string[] { "Test update.", "Second line." };
iceCream.Comments = "[ 'Test update.', 'Second line.' ]";

Only assertion succeed with implicit operators(last two ones).

I can understand first one fails. It is almost impossible to track changes and other systems also do not consider such like things.

But it is confusing second and third one fail which are accessing property setters. I think, removing the setters or implement as same with the implicit operators is better.

Further technical details

MySQL version: MariaDB 10.3 Operating system: Windows 10 Pomelo.EntityFrameworkCore.MySql version: 3.12 Microsoft.AspNetCore.App version: .NET Core 3.1 Console Application

About this issue

Original URL
State: closed
Created 4 years ago
Comments: 22

Most upvoted comments

Nothing to add here, but wanted to thank everyone working to resolve the complexities involved w/ JSON support.

mguinness on Sep 9, 2020

Yeah, the JsonObject<T> implementation isn’t clever enough to handle changes inside of arrays. You need to replace the whole JsonObject<T>. Because of operator overloading, you can also just replace the whole array:

iceCream.Comments.Object[0] = "Test update.";
iceCream.Comments = (string[]) iceCream.Comments.Object.Clone();

Alternatively, you could introduce some extension method:

public static class JsonObjectExtensions
{
    public static JsonObject<T[]> SetAt<T>(this JsonObject<T[]> source, int index, T value)
    {
        var clone = source.Object.ToArray();
        clone[index] = value;
        return new JsonObject<T[]>(clone);
    }
}

That could be used like this:

iceCream.Comments = iceCream.Comments.SetAt(0, "Test update.");

I don’t think there is a lot of merit for us to officially extend JsonObject<T>, because I basically finished implementing the full JSON support, so JsonObject<T> will become obsolete shortly.

lauxjpn on Aug 15, 2020

Yeah, you’re right that since JsonDocument/JsonElement don’t override Equals, the default implementation should work fine. In fact, unless I’m mistaken, the default implementation is what we get when no comparer is explicitly passed, so without doing anything we already get the above.

However, while this all works great for JsonDocument, JsonElement is a struct - and so things become quite a bit more complicated, because there’s no identity/reference equality. Of course, it’s still possible to do a recursive structural comparison, but we’re basically back to the deep snapshot/comparison perf issues of POCOs. So unless I’ve missed something, mapping directly to JsonElement should probably be discouraged; in fact, it was probably a mistake to allow mapping it in the first place (I’d consider removing that support from your provider). The main advantage mapping JsonElement provides is not having to call RootElement on JsonDocument, which really is a tiny bit of sugar.

roji on Sep 12, 2020