Difference in query translation compared to Pomelo 8.0

[crjc]

Describe the bug
In Pomelo's 8.x, the following query would be translated differently. Upgrading to Pomelo's 9.x release or the latest release of this fork produces a different query which can provide unexpected results.

// User.GroupId is of type `Guid?`
string? value = "71c9eacb-468b-41a9-81c5-579d4119264e";
context.Users.Where(x => x.GroupId.ToString() == value).First();

(EXPECTED BEHAVIOUR) In 8.0, the translated query was:

SELECT `u`.`Id`, `u`.`GroupId`, `u`.`Name`
FROM `Users` AS `u`
WHERE CAST(`u`.`GroupId` AS char) = '71c9eacb-468b-41a9-81c5-579d4119264e'

In 9.0+, the translation is:

SELECT `u`.`Id`, `u`.`GroupId`, `u`.`Name`
FROM `Users` AS `u`
WHERE COALESCE(CAST(`u`.`GroupId` AS char), '') = '6507e45c-9468-4154-87ff-9a4544a242f0'

However, if we set string? value = null, the queries become:

(EXPECTED BEHAVIOUR) 8.0:

SELECT `u`.`Id`, `u`.`GroupId`, `u`.`Name`
FROM `Users` AS `u`
WHERE `u`.`GroupId` IS NULL

9.0+

SELECT `u`.`Id`, `u`.`GroupId`, `u`.`Name`
FROM `Users` AS `u`
WHERE FALSE

To Reproduce

This test code will log an error, but not on older 8.0 versions:

using Microsoft.EntityFrameworkCore;

var connectionString = "...";

var options = new DbContextOptionsBuilder<ApplicationDbContext>()
    .UseMySql(connectionString, ServerVersion.AutoDetect(connectionString))
    .Options;

try
{
    using var context = new ApplicationDbContext(options);

    await context.Database.EnsureDeletedAsync();
    await context.Database.EnsureCreatedAsync();

    // create entities we can query
    var groupId = Guid.NewGuid();
    context.Users.Add(new User()
    {
        Id = Guid.NewGuid(),
        GroupId = groupId,
        Name = "John"
    });
    context.Users.Add(new User()
    {
        Id = Guid.NewGuid(),
        GroupId = null,
        Name = "Bob"
    });
    await context.SaveChangesAsync();

    // `FirstAsync` will throw an exception as no entities match the new translated query
    string? groupIdAsString = null;
    await context.Users.Where(x => x.GroupId.ToString() == groupIdAsString).FirstAsync();
    Console.WriteLine("   ✓ Query successful!");
}
catch (Exception ex)
{ 
    Console.WriteLine($"Message: {ex.Message}");
    if (ex.InnerException != null)
    {
        Console.WriteLine($"Inner Exception: {ex.InnerException.Message}");
    }
    Console.WriteLine($"\nStack Trace:\n{ex.StackTrace}");
    return 1;
}

return 0;

public class User
{
    public Guid Id { get; set; }
    public Guid? GroupId { get; set; }
    public required string Name { get; set; }
}

public class ApplicationDbContext : DbContext
{
    public ApplicationDbContext(DbContextOptions<ApplicationDbContext> options)
        : base(options)
    {
    }

    protected override void OnModelCreating(ModelBuilder builder)
    {
        builder.Entity<User>().HasKey(x => x.Id);
        base.OnModelCreating(builder);
    }

    public DbSet<User> Users { get; set; }
}

Additional context

The COALESCE method originates from this line of code:

Pomelo.EntityFrameworkCore.MySql/src/EFCore.MySql/Query/Internal/MySqlObjectToStringTranslator.cs

Line 94 in 13312b2

? _sqlExpressionFactory.Coalesce(

In v8.0, this method is not present:
https://github.com/PomeloFoundation/Pomelo.EntityFrameworkCore.MySql/blob/401acc215fe7db320447c22a6e4337f09008e619/src/EFCore.MySql/Query/Internal/MySqlObjectToStringTranslator.cs#L87

I have confirmed removing the COALESCE expression brings back the old expected behaviour, but I have not looked into the rationale of the original change yet.

[taoyouh]

I don't think that's an error. x.GroupId.ToString() == null can never be true. If x.GroupId is null, that function will throw an exception instead of returning false.

You should use x.GroupId?.ToString() == null in your code.

[crjc]

You should use x.GroupId?.ToString() == null in your code.

This is true for ordinary C# code, but here the expression tree is translated rather than being executed directly, so no exception is thrown. This is very much an edge case that I came across.

Unfortunately, using a null propagating operator isn't an option due to CS8072.

[taoyouh]

Well, although it's translated, it is translated from C# which means the original expression should be a valid C# expression. No matter you map a throwing exception to a true or a false, someone will get surprised.

The most acceptable result to me is: either treat it as "UB", or throw an error declaring it cannot be translated properly.

As for the CS8072, we have many ways to work-around it. You can treat null separately or use x.GroupId == null ? null : x.GroupId.ToString()

[crjc]

I agree it makes sense to throw an exception OR restore previous behaviour from older versions of Pomelo.