Pomelo.EntityFrameworkCore.MySql icon indicating copy to clipboard operation
Pomelo.EntityFrameworkCore.MySql copied to clipboard

Pomelo for Newtonsoft.Json is not using serialization settings when generating property paths in MySQL Functions that search JSON values

Open jjavierdguezas opened this issue 2 years ago • 4 comments

Steps to reproduce

using Pomelo and Newtonsoft.Json if you have an entity with a column marked as json when querying for a field in this column, Pomelo generates a query that uses a MySQL function to deal with JSON values but when the path to the property is generated it does not take into account that the properties may be being serialized in a format other than the default. Pomelo does take into account the JsonProperty attribute but with Newtonsoft.Json you can also configure globally how you want to serialize property names (for example in camelCase) and it is not being taken into account

EXAMPLE

Given:

  • the entity:
public class Person
{
    public int Id { get; set; }
    public string Name { get; set; }

    public DataContainer Container { get; set; }
}

public class IntData
{
    public int IntValue { get; set; }
}

public class StringData
{
    public string StringValue { get; set; }
}

public class DataContainer
{
    public IntData IntData { get; set; }
    public StringData StringData { get; set; }
}
  • the entity configuration:
modelBuilder.Entity<Person>().HasKey(x => x.Id);
modelBuilder.Entity<Person>().Property(x => x.Container).HasColumnType("json");
modelBuilder.Entity<Person>().ToTable("People");
  • and this configuration
JsonConvert.DefaultSettings = () =>
{
    var settings = new JsonSerializerSettings();
    settings.Converters.Add(new Newtonsoft.Json.Converters.StringEnumConverter());
    settings.NullValueHandling = NullValueHandling.Include;
    settings.ContractResolver = new CamelCasePropertyNamesContractResolver(); // <-----
    settings.ReferenceLoopHandling = ReferenceLoopHandling.Ignore;
    settings.DateTimeZoneHandling = DateTimeZoneHandling.Local;
    return settings;
};

services.AddDbContext<TestDbContext>(o =>
        o
        .UseMySql(Constants.MySqlDbCnx, ServerVersion.AutoDetect(Constants.MySqlDbCnx), mySqlOpts =>
        {
            mySqlOpts.UseNewtonsoftJson();
            mySqlOpts.MigrationsAssembly(typeof(Program).Assembly.GetName().Name);
        })
        .LogTo(Console.WriteLine, LogLevel.Information)
        .EnableSensitiveDataLogging()
        .EnableDetailedErrors()
);

an entity instance like:

var p1 = new Person
{
    Id = 1,
    Name = "Person 1",
    Container = new DataContainer
    {
        IntData = new IntData { IntValue = 34 },
        StringData = new StringData { StringValue = "hi" }
    }
};

will be saved on the db like:

| Id | Name     | Container                                                          |
|----|----------|--------------------------------------------------------------------|
| 1  | Person 1 | {"intData": {"intValue": 34}, "stringData": {"stringValue": "hi"}} |

(note the camelCase usage in property names)

the query:

var p1 = await db.Set<Person>().FirstAsync(x => x.Container.StringData.StringValue == "h1")

is translated to:

SELECT `p`.`Id`, `p`.`Container`, `p`.`Name`
FROM `People` AS `p`
WHERE JSON_EXTRACT(`p`.`Container`, '$.StringData.StringValue') = 'hi'
LIMIT 1

and will return 0 rows in response, wrongly, due to '$.StringData.StringValue'

if we change the definition of the entities by adding the JsonProperty attribute it works fine, but it is very inconvenient and goes against the idea of defining this behavior globally

given:

public class IntData
{
    [JsonProperty("intValue")]
    public int IntValue { get; set; }
}
public class StringData
{
    [JsonProperty("stringValue")]
    public string StringValue { get; set; }
}

public class DataContainer
{
    [JsonProperty("intData")]
    public IntData IntData { get; set; }

    [JsonProperty("stringData")]
    public StringData StringData { get; set; }
}

the serialization of the entity will be the same and the query is translated to:

SELECT `p`.`Id`, `p`.`Container`, `p`.`Name`
FROM `People` AS `p`
WHERE JSON_EXTRACT(`p`.`Container`, '$.stringData.stringValue') = 'hi'
LIMIT 1

and it will return 1 row

also if we do not use the global configuration

/*
JsonConvert.DefaultSettings = () =>
{
    var settings = new JsonSerializerSettings();
    settings.Converters.Add(new Newtonsoft.Json.Converters.StringEnumConverter());
    settings.NullValueHandling = NullValueHandling.Include;
    settings.ContractResolver = new CamelCasePropertyNamesContractResolver();
    settings.ReferenceLoopHandling = ReferenceLoopHandling.Ignore;
    settings.DateTimeZoneHandling = DateTimeZoneHandling.Local;
    return settings;
};
*/

the entity will be saved like:

| Id | Name     | Container                                                          |
|----|----------|--------------------------------------------------------------------|
| 1  | Person 1 | {"IntData": {"IntValue": 34}, "StringData": {"StringValue": "hi"}} |

and the query is translated to:

SELECT `p`.`Id`, `p`.`Container`, `p`.`Name`
FROM `People` AS `p`
WHERE JSON_EXTRACT(`p`.`Container`, '$.StringData.StringValue') = 'hi'
LIMIT 1

retrieving 1 row as expected

The issue

If Newtonsoft.Json is configured globally, Pomelo does not use the serialization convention for property names defined there when generating the paths for MySQL Json functions, resulting in invalid queries and/or not returning the expected values

Suggestion

I reviewed the Pomelo source code a bit and although I can't assimilate it 100%, I have seen that somewhere, for example, in MySqlJsonPocoTranslator you do:

https://github.com/PomeloFoundation/Pomelo.EntityFrameworkCore.MySql/blob/27f227388a5e1b68292f6c649227bdf0637598d7/src/EFCore.MySql/Query/ExpressionTranslators/Internal/MySqlJsonPocoTranslator.cs#L45-L48

and

https://github.com/PomeloFoundation/Pomelo.EntityFrameworkCore.MySql/blob/27f227388a5e1b68292f6c649227bdf0637598d7/src/EFCore.MySql.Json.Newtonsoft/Query/Internal/MySqlJsonNewtonsoftPocoTranslator.cs#L20-L21

so, maybe you could check with the Newtonsoft.Json global configuration like this:

public override string GetJsonPropertyName(MemberInfo member)
{
    var attributeName = member.GetCustomAttribute<JsonPropertyAttribute>()?.PropertyName;
    if (!string.IsNullOrWhiteSpace(attributeName)) 
        return attributeName;

    if (JsonConvert.DefaultSettings?.Invoke()?.ContractResolver is DefaultContractResolver resolver)
        return resolver.GetResolvedPropertyName(member.Name);

    return new DefaultContractResolver().GetResolvedPropertyName(member.Name);
}

Further technical details

MySQL version: 8.0.32-mysql (docker image) Operating system: windows (host), linux (container) Pomelo.EntityFrameworkCore.MySql version: 7.0.0 Pomelo.EntityFrameworkCore.MySql.Json.Newtonsoft: 7.0.0 Microsoft.AspNetCore.App version: 6.0.18

PS: I attached a code where this error can be reproduced : PomeloTest.zip

jjavierdguezas avatar Jul 12 '23 19:07 jjavierdguezas

any news about this?

jjavierdguezas avatar Oct 31 '23 10:10 jjavierdguezas

Hi @trejjam, do you think this will be solved with #1827 ?

jjavierdguezas avatar Feb 09 '24 11:02 jjavierdguezas

My PR is only extending System.Text.Json support.

But I do not know if it fixes query building. It probably can be fixed using configuration from my PR

trejjam avatar Feb 09 '24 18:02 trejjam

@jjavierdguezas First of all, great issue report!

I think we should allow users to configure default JSON settings via the model, and then allow them to override those for entity properties or using the MySqlJsonNewtonsoftPocoValueConverter<T>.

There might be other areas in an app (or even within a DbContext model) that need to use JSON (de-/)serialization with different settings.

lauxjpn avatar Feb 14 '24 01:02 lauxjpn