Feature/translate json functions #29306

[joelmandell]

Fixes #29306

• I've read the guidelines for contributing and seen the walkthrough
• I've posted a comment on an issue with a detailed description of how I am planning to contribute and got approval from a member of the team
• The code builds and tests pass locally (also verified by our automated build checks)
• Commit messages follow this format: (This I totally forgot)

        Summary of the changes
        - Detail 1
        - Detail 2

        Fixes #bugnumber

• Tests for the changes have been added (for bug fixes / features)
• Code follows the same patterns and style as existing code in this repo (Kinda... little bit different with unit-tests. But want to get your feedback if that's okay).

[roji]

Thanks for this PR - it's a good start, see comments below.

Note that there are some other JSON functions supported by SQL Server (ISJSON, JSON_ARRAY, JSON_OBJECT...). Ideally we'd have translations for all of them (and the relevant SQLite ones as well); of course you don't have to do that in this PR, but if you feel like working on those that would be great.

[joelmandell]

.

[joelmandell]

I made some changes now. And think I got them all covered. But little bit uncertain about the propagation of null.

[roji]

@joelmandell sorry, it's taking long for me to get around to this, here are some comments. Please rebase this PR on the latest main and check out any test failures after that.

[roji]

Any reason to add this rather than simply using JsonEntityBasic?

[joelmandell]

JsonEntityBasic has mappings of the columns to classes, and this works only for strings If I understand it correctly.

[celsowm]

json_value in differents rdbms:

mysql: https://dbfiddle.uk/?rdbms=mysql_8.0&fiddle=591207ceddca6fe4f11a3ad8589e7c5b
sqlserver: https://dbfiddle.uk/?rdbms=sqlserver_2017&fiddle=5c70245f5bb7c96d4adcc6b10ebed195
postgres: https://dbfiddle.uk/o46x7dyL
sqlite: https://www.db-fiddle.com/f/tMcVDVmjxb5wSjemg34dy8/2

[joelmandell]

@roji I know I was late to the party and missed to tag you in to this before feature freeze for 8.0... Do you have any advice or comments on how I can move forward with this one to perhaps get it ready for 9.0? I can as well look into the other requests for json functions if possible.

[roji]

@joelmandell sure thing - I'd also definitely like us to get this in for 9.0.

Here's my advice... Give us around a month, maybe a bit more to deal with the 8.0 release and the subsequent patching and other work - and then ping me here again. Assuming it's a good time to iterate on it, you can then rebase this PR on the latest main, and then we can get this to completed state and merge.

[Abdurahmon0412]

You must resolve conflict

[joelmandell]

Ping @roji. I do not know if it is to early to continue on this one now? :)

[roji]

@joelmandell I'm looking at this PR again (sorry for all the breaks and pauses). Since there was previously some problematic merge commit which messed up the entire history, I've squashed everything cleanly rebased on top of the latest main. In the interest of finally getting moving quickly and finally getting this merge, I'll do whatever fixup is still necessary and push commits on this PR.

[joelmandell]

@roji Sounds good. Thx

[mseada94]

@roji @joelmandell
Having JsonValue extension method on DbFunctions on sql server provider may cause conflict when other provider wants to extend the same function,
For example if oracle provider want to map this function which has the same name and parameters, it should use different name otherwise it will conflict with SqlServer provider extension if user reference both providers.

The extension should be added on the relational level and the mapping is left to each provider to define.

---

I think json expression should be tested before translation to allows only

• String expressions (column, constant, complex)
• Owned JSON column expression
• Column with string value converter expression

For SqlServer

arguments[0].TypeMapping is SqlServerOwnedJsonTypeMapping or StringTypeMapping

For Sqlite

arguments[0].TypeMapping is SqliteJsonTypeMapping or StringTypeMapping

[roji]

Having JsonValue extension method on DbFunctions on sql server provider may cause conflict when other provider wants to extend the same function,
For example if oracle provider want to map this function which has the same name and parameters, it should use different name otherwise it will conflict with SqlServer provider extension if user reference both providers.
The extension should be added on the relational level and the mapping is left to each provider to define.

You're probably right for this case.

Conflict between same-named extension methods across different providers is something we already have in various other places where multiple providers implement the same extension method (especially in model building). We generally tell users to simply use static method syntax instead of extension method - it seems much better for the few users who actually target mutiple databases to do this, rather than the vast majority of users who target a single database to have weird naming (FYI early versions of EF prefixed all extension methods with ForSqlServer and similar).

Putting something on the relational level usually has a high bar, since even when the function exists with the same name across different databases, there are typically slight differences in the signature etc.

However, for JSON_VALUE specifically (and probably some of the other JSON functions), it's part of the SQL standard and seems to be pretty closely followed across most databases which do support it:

• SQL Server (no support for the RETURNING, ON EMPTY and ON ERROR clauses, but that's supposed to come)
• PostgreSQL
• Oracle
• MySQL
• MariaDB (no support for the RETURNING, ON EMPTY and ON ERROR clauses)
• SQLite - not supported (but they'll likely follow suit at some point, as they've done with the other PG JSON functions).

So I think this all makes sense.

Note the extra clauses RETURNING, ON EMPTY and ON ERROR which are widely supported (and probably in the standard). These have special syntax beyond the regular SQL function syntax, but we already internally have a special JsonScalarExpression which represents JSON_VALUE, and which could be augmented to support these clauses.

For the EF.Functions side, we could support these clauses via extra optional parameters - I'll see when implementing if that makes sense.

[roji]

I think json expression should be tested before translation to allows only

In general, users using functions on EF.Functions take responsibility for what they do - it's a relatively direct way to specify a SQL function invocation, and there's not much point for us adding validation in EF if the database will error anyway. It's also possible for users to do some convoluted things, e.g. do JSON_VALUE over a non-column (parameter/literal) JSON document, with the JSON path coming from a database column; if we restricted the translation to only support the JSON type mapping, this scenario would be blocked (and again, for no good reason).

[mseada94]

It's also possible for users to do some convoluted things, e.g. do JSON_VALUE over a non-column (parameter/literal) JSON document, with the JSON path coming from a database column

non-column (parameter/literal) JSON document should be of type string and the mapping should be of type StringTypeMapping

[roji]

@mseada94 right, so what's there to test exactly? What's needed beyond just typing the arguments as string, for now (until we make it possible to also invoke these functions with arbitrary objects mapped as complex types/owned entity types)?

[mseada94]

The test here to allow translation and calling for expression with valid type mapping (having first argument as object or generic type)

arguments[0].TypeMapping is SqlServerOwnedJsonTypeMapping or StringTypeMapping

• StringTypeMapping
  • string literal
  • string column
  • string parameter
  • string complex expression
  • arbitrary object configured to be converted as string when storing on the database (value converter should be supported not just json owned and complex types)
• SqlServerOwnedJsonTypeMapping
  • owned entity configured to be stored as json
• Future: complex type configured to be stored as json

It should prevent translation otherwise for example

• entity which is mapped to table
• owned entity which is mapped to table
• other datatypes (int, byte, Guid, Datetime, ....etc)

We need to validate type mapping not the type itself

[mseada94]

Introducing complex type json support will reduce the need for using ValueConverter to store JSON, but it will not eliminate the need for it, so it should be supported as well
Dictionary could be converted and might not be supported by Complex Type, and some users might need to have custom json serialization and deserialization and can't use complex type.

---

please consider the following example

• using JsonValue function with Address is valid, the value is serialized and converted to json (That's the reason to have first argument as object or generic not only string)
• However, Department here is a table not a json and the translation should not be allowed here (That's the reason for having validation)

using Microsoft.EntityFrameworkCore;
using System.Text.Json;

using var context = new SomeDbContext();

var validUsage = context.Users.Where(u => EF.Functions.JsonValue(u.Address, "$.Country") == "Egypt");
var inValidUsage = context.Users.Where(u => EF.Functions.JsonValue(u.Department, "$.Name") == "Engineering");

public class User
{
    public Guid Id { get; set; }
    public Address Address { get; set; }
    public Department Department { get; set; }

}

public class Department
{
    public Guid Id { get; set; }
    public string Name { get; set; }
}


public class Address
{
    public string Country { get; set; }
    public string City { get; set; }
}


public class SomeDbContext : DbContext
{
    public DbSet<User> Users { get; set; }
    public DbSet<Department> Departments { get; set; }
    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder.UseSqlServer(Your.ConnectionString);

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<User>().HasOne(d => d.Department).WithMany();

        modelBuilder.Entity<User>().Property(x => x.Address).HasConversion(
            address => JsonSerializer.Serialize(address, DefaultJsonSerializerOptions),
            json => JsonSerializer.Deserialize<Address>(json, DefaultJsonSerializerOptions)!);
    }

    private static JsonSerializerOptions DefaultJsonSerializerOptions = new JsonSerializerOptions
    {
        Encoder = System.Text.Encodings.Web.JavaScriptEncoder.UnsafeRelaxedJsonEscaping,
    };
}

[ljani]

Thanks for the great feature. I tested this in my project where path is a(n interpolated) variable and got the following exception:

System.InvalidCastException: 'Unable to cast object of type 'Microsoft.EntityFrameworkCore.Query.SqlExpressions.SqlParameterExpression' to type 'Microsoft.EntityFrameworkCore.Query.SqlExpressions.SqlConstantExpression'.'

(I forgot to copy the stacktrace, but it was from SqliteJsonFunctionsTranslator.cs#60 with the SqlConstantExpression cast.)

This was fixed by tagging path with [NotParameterized] here. I'm not 100 % sure if this is the correct fix, but something to consider.