Invalid SQL Syntax when adding an entity with a default expression

[not-nugget]

Steps to reproduce

Here is a link to a repository with a solution already configured and ready to replicate with branches, each with minor tweaks to the code (master is equivalent to the contents of the .ZIP file below): Solution Repository Or alternatively a .ZIP file containing the original .csproj, .sln and the Program.cs with the files used to replicate the issue: ReproSln.zip

Configure an entity with a binary(16) Guid PK generated on insert with a default expression of (UUID_TO_BIN(UUID(), 1)). (In the solution provided above, it is wrapped in a concrete ID type. This yields no difference in the outcome of the issue)

Add any number of entities to the context DbSet and call DbContext.SaveChangesAsync().

Observe the invalid queries generated.

NOTE: The no-default-expression branch appears to succeed, and does in fact generate a UUID, ~~however the generated value is not ideal for indexing, unlike the result of (UUID_TO_BIN(UUID(), 1))~~ As mentioned in the following comment, the UUID generated this way is index optimized and should be used instead of trying to force UUID_TO_BIN(UUID(), 1).

The issue

Any attempt to track a new Entity will throw a MySqlException when DbContext.SaveChangesAsync is executed. The SQL query that is generated for inserting is correct, but it appends an additional SELECT query with an incomplete WHERE clause that causes the query to fail.

Generated SQL:

INSERT INTO `Entities` (`UUID`)
VALUES (@p0);
SELECT `Id`
FROM `Entities`
WHERE ROW_COUNT() = 1 AND ;

Observed Outer Exception:

Exception message: An error occurred while saving the entity changes. See the inner exception for details.
Stack trace:    at Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.<ExecuteAsync>d__50.MoveNext()
   at Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.<ExecuteAsync>d__9.MoveNext()
   at Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.<ExecuteAsync>d__9.MoveNext()
   at Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.<ExecuteAsync>d__9.MoveNext()
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.<SaveChangesAsync>d__106.MoveNext()
   at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.<SaveChangesAsync>d__110.MoveNext()
   at Pomelo.EntityFrameworkCore.MySql.Storage.Internal.MySqlExecutionStrategy.<ExecuteAsync>d__7`2.MoveNext()
   at Microsoft.EntityFrameworkCore.DbContext.<SaveChangesAsync>d__64.MoveNext()
   at Microsoft.EntityFrameworkCore.DbContext.<SaveChangesAsync>d__64.MoveNext()
   at Scratch.Program.<Main>d__0.MoveNext() in .\Scratch\Program.cs:line 42

Observed Inner Exception:

Exception message: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ';

INSERT INTO `Entities` (`UUID`)
VALUES (_binary'??J????@?4??r?yP');
SELEC' at line 3
Stack trace:       at MySqlConnector.Core.ResultSet.<ReadResultSetHeaderAsync>d__2.MoveNext() in /_/src/MySqlConnector/Core/ResultSet.cs:line 43
   at MySqlConnector.MySqlDataReader.ActivateResultSet(CancellationToken cancellationToken) in /_/src/MySqlConnector/MySqlDataReader.cs:line 130
   at MySqlConnector.MySqlDataReader.<CreateAsync>d__111.MoveNext() in /_/src/MySqlConnector/MySqlDataReader.cs:line 469
   at MySqlConnector.Core.CommandExecutor.<ExecuteReaderAsync>d__0.MoveNext() in /_/src/MySqlConnector/Core/CommandExecutor.cs:line 56
   at MySqlConnector.MySqlCommand.<ExecuteReaderAsync>d__84.MoveNext() in /_/src/MySqlConnector/MySqlCommand.cs:line 344
   at MySqlConnector.MySqlCommand.<ExecuteDbDataReaderAsync>d__83.MoveNext() in /_/src/MySqlConnector/MySqlCommand.cs:line 337
   at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.<ExecuteReaderAsync>d__19.MoveNext()
   at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.<ExecuteReaderAsync>d__19.MoveNext()
   at Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.<ExecuteAsync>d__50.MoveNext()

Further technical details

MySQL version: 8.0.34 Operating system: Win 10 Pomelo.EntityFrameworkCore.MySql version: 7.0.0 Microsoft.NetCore.App version: 6.0.29

Other details about my project setup: Upgrading to a new version of .NET (and thus a newer version of EFCore, Pomelo.EntityFrameworkCore.MySql, or another package dependent on .NET 7 or greater) is not something that can be done

[lauxjpn]

For EF Core to track your newly inserted entity, it needs to either know the ID of the entity in advance, or it needs a mechanism to read it back from the database, after it was generate by the database (which can be done for regular integer IDs with LAST_INSERT_ID()).

Since there is no way for EF Core or Pomelo to reliably read back the ID value generated by the (UUID_TO_BIN(UUID(), 1)) default value expression, because it generates the ID that EF Core/Pomelo would already need to know to read it back, your code cannot work in the way you intend it to.

What you would want to do instead is to let EF Core/Pomelo generate a Guid for the entity when generating the INSERT INTO statement, which is what we do by default, as long as you don't specify a default value expression or explicitly disable it. The Guid we generate is also index optimized (see MySqlSequentialGuidValueGenerator):

Program.cs

using System;
using System.Diagnostics;
using System.Linq;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;
using MySqlConnector;

namespace IssueConsoleTemplate;

public class IceCream
{
    public Guid IceCreamId { get; set; }
    public string Name { get; set; }
}

public class Context : DbContext
{
    public DbSet<IceCream> IceCreams { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        if (!optionsBuilder.IsConfigured)
        {
            var connectionString = new MySqlConnectionStringBuilder("server=127.0.0.1;port=3306;user=root;password=;Database=Issue1909")
            {
                GuidFormat = MySqlGuidFormat.Binary16
            }.ConnectionString;
            
            var serverVersion = ServerVersion.AutoDetect(connectionString);

            optionsBuilder
                .UseMySql(connectionString, serverVersion)
                .LogTo(Console.WriteLine, LogLevel.Information)
                .EnableSensitiveDataLogging()
                .EnableDetailedErrors();
        }
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<IceCream>(
            entity =>
            {
                // This cannot not work:
                // entity.Property(e => e.IceCreamId)
                //     .HasDefaultValueSql("(UUID_TO_BIN(UUID()))");
            });
    }
}

internal static class Program
{
    private static void Main()
    {
        using var context = new Context();
        
        context.Database.EnsureDeleted();
        context.Database.EnsureCreated();

        context.IceCreams.Add(new IceCream { Name = "Vanilla" });
        context.SaveChanges();

        var result = context.IceCreams.Single();
        
        Trace.Assert(result.IceCreamId != Guid.Empty);
        Trace.Assert(result.Name == "Vanilla");
    }
}

Output (SQL)

warn: 25.04.2024 13:19:54.044 CoreEventId.SensitiveDataLoggingEnabledWarning[10400] (Microsoft.EntityFrameworkCore.Infrastructure) 
      Sensitive data logging is enabled. Log entries and exception messages may include sensitive application data; this mode should only be enabled during development.
info: 25.04.2024 13:19:54.374 RelationalEventId.CommandExecuted[20101] (Microsoft.EntityFrameworkCore.Database.Command) 
      Executed DbCommand (25ms) [Parameters=[], CommandType='Text', CommandTimeout='30']                                
      DROP DATABASE `Issue1909`;                                                                                        
info: 25.04.2024 13:20:01.529 RelationalEventId.CommandExecuted[20101] (Microsoft.EntityFrameworkCore.Database.Command) 
      Executed DbCommand (10ms) [Parameters=[], CommandType='Text', CommandTimeout='30']                                
      CREATE DATABASE `Issue1909`;                                                                                      
info: 25.04.2024 13:20:01.649 RelationalEventId.CommandExecuted[20101] (Microsoft.EntityFrameworkCore.Database.Command) 
      Executed DbCommand (7ms) [Parameters=[], CommandType='Text', CommandTimeout='30']                                 
      ALTER DATABASE CHARACTER SET utf8mb4;                                                                             
info: 25.04.2024 13:20:01.680 RelationalEventId.CommandExecuted[20101] (Microsoft.EntityFrameworkCore.Database.Command) 
      Executed DbCommand (30ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
      CREATE TABLE `IceCreams` (
          `IceCreamId` binary(16) NOT NULL,
          `Name` longtext CHARACTER SET utf8mb4 NULL,
          CONSTRAINT `PK_IceCreams` PRIMARY KEY (`IceCreamId`)
      ) CHARACTER SET=utf8mb4;
info: 25.04.2024 13:20:01.861 RelationalEventId.CommandExecuted[20101] (Microsoft.EntityFrameworkCore.Database.Command) 
      Executed DbCommand (22ms) [Parameters=[@p0='08dc6519-a639-4aa6-8942-389cc7b7666a', @p1='Vanilla' (Size = 4000)], CommandType='Text', CommandTimeout='30']
      SET AUTOCOMMIT = 1;
      INSERT INTO `IceCreams` (`IceCreamId`, `Name`)
      VALUES (@p0, @p1);
info: 25.04.2024 13:20:02.099 RelationalEventId.CommandExecuted[20101] (Microsoft.EntityFrameworkCore.Database.Command) 
      Executed DbCommand (5ms) [Parameters=[], CommandType='Text', CommandTimeout='30']
      SELECT `i`.`IceCreamId`, `i`.`Name`
      FROM `IceCreams` AS `i`
      LIMIT 2

---

@ajcvickers It would probably be better if UpdateAndSelectSqlGenerator.AppendWhereAffectedClause() would explicitly throw in cases where there is no clause generated after the AND keyword has already been appended.

[not-nugget]

Understood. I figured EFCore would have been able to track all entities and their relationships via the temporary IDs, and have them respectively populated once DbContext.SaveChangesAsync() was called and assumed this was a seperate issue with the query generator (you know what they say about assumptions).

The Guid we generate is also index optimized (see MySqlSequentialGuidValueGenerator)

Apologies for my original incorrect conclusion that the generated ID was not index-safe. This is good to know, and will prove a perfect alternative to UUID_TO_BIN(UUID(), 1). As much as I would love my database be the only thing that generates IDs, this is a valid compromise.

I will leave this issue open in case of any further discourse, however I would consider it resolved from my end of things. Thank you!

[roji]

It would probably be better if UpdateAndSelectSqlGenerator.AppendWhereAffectedClause() would explicitly throw in cases where there is no clause generated after the AND keyword has already been appended.

@lauxjpn can you open an issue on this with the problematic case etc.? Of course a PR is always welcome too.