Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jjavierdguezas opened this issue Jul 12, 2023 · 4 comments
Assignees
Milestone

Comments

@jjavierdguezas
Copy link

jjavierdguezas commented Jul 12, 2023

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:

var sqlConstantExpression = _sqlExpressionFactory.ApplyDefaultTypeMapping(
_sqlExpressionFactory.Constant(
GetJsonPropertyName(member) ?? member.Name,
_unquotedStringTypeMapping));

and

public override string GetJsonPropertyName(MemberInfo member)
=> member.GetCustomAttribute<JsonPropertyAttribute>()?.PropertyName;

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
Copy link
Author

any news about this?

@jjavierdguezas
Copy link
Author

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

@trejjam
Copy link

trejjam commented Feb 9, 2024

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

@lauxjpn lauxjpn self-assigned this Feb 14, 2024
@lauxjpn lauxjpn added this to the 9.0.0 milestone Feb 14, 2024
@lauxjpn
Copy link
Collaborator

lauxjpn commented Feb 14, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants