ValueConverter not applied to operands on ExecuteUpdateAsync

[Danielku15]

When upgrading from EFCore7 to EFCore8 the code below doesn't work anymore. In EFCore7 the ValueConverter configured on the property was applied to the TimeSpan operand making the update successful.

In EFCore8 the ValueConverter is not applied anymore to the expression and the SQL translates to a string which leads to an exception.

Include your code

using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Logging;

var services = new ServiceCollection()
    .AddLogging(l =>
    {
        l.SetMinimumLevel(LogLevel.Trace);
        l.AddConsole();
    })
    .AddDbContext<MyDbContext>(o=>
    {
        o.EnableSensitiveDataLogging();
        o.UseSqlServer("Data Source=(local);Integrated Security=SSPI;Database=EFCore8TimeSpanModify;MultipleActiveResultSets=True;Trust Server Certificate=true;");
    }, ServiceLifetime.Singleton)
    .BuildServiceProvider();

var context = services.GetRequiredService<MyDbContext>();
await context.Database.EnsureCreatedAsync();

var entity = new WithTimeSpan { TimeSpan = TimeSpan.FromTicks(100) };
await context.AddAsync(entity);
await context.SaveChangesAsync();

var interval = TimeSpan.FromTicks(10);
await context.WithTimeSpans.ExecuteUpdateAsync(x =>
    x.SetProperty(e => e.TimeSpan, e => e.TimeSpan + interval)
);

class WithTimeSpan
{
    public int Id { get; set; }
    public TimeSpan TimeSpan { get; set; }
}

class MyDbContext : DbContext
{
    public DbSet<WithTimeSpan> WithTimeSpans => Set<WithTimeSpan>();

    public MyDbContext(DbContextOptions<MyDbContext> o) : base(o)
    {
    }
    
    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<WithTimeSpan>(e =>
        {
            e.HasKey(x => x.Id);
            e.Property(x => x.Id).ValueGeneratedOnAdd();
            e.Property(x => x.TimeSpan)
                .HasColumnType("bigint")
                .HasConversion(v => v.Ticks,
                    v => TimeSpan.FromTicks(v));
        });
    }
}

Include stack traces

Microsoft.Data.SqlClient.SqlException (0x80131904): Operand type clash: time is incompatible with bigint
   at Microsoft.Data.SqlClient.SqlConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at Microsoft.Data.SqlClient.SqlInternalConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at Microsoft.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, Boolean callerHasConnectionLock, Boolean asyncClose)
   at Microsoft.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, Boolean& dataReady)
   at Microsoft.Data.SqlClient.SqlCommand.FinishExecuteReader(SqlDataReader ds, RunBehavior runBehavior, String resetOptionsString, Boolean isInternal, Boolean forDescribeParameterEncryption, Boolean shouldCacheForAlwaysEncrypted)
   at Microsoft.Data.SqlClient.SqlCommand.CompleteAsyncExecuteReader(Boolean isInternal, Boolean forDescribeParameterEncryption)
   at Microsoft.Data.SqlClient.SqlCommand.InternalEndExecuteNonQuery(IAsyncResult asyncResult, Boolean isInternal, String endMethod)
   at Microsoft.Data.SqlClient.SqlCommand.EndExecuteNonQueryInternal(IAsyncResult asyncResult)
   at Microsoft.Data.SqlClient.SqlCommand.EndExecuteNonQueryAsync(IAsyncResult asyncResult)
   at Microsoft.Data.SqlClient.SqlCommand.<>c.<InternalExecuteNonQueryAsync>b__210_1(IAsyncResult result)
   at System.Threading.Tasks.TaskFactory`1.FromAsyncCoreLogic(IAsyncResult iar, Func`2 endFunction, Action`1 endAction, Task`1 promise, Boolean requiresSynchronization)
--- End of stack trace from previous location ---
   at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteNonQueryAsync(RelationalCommandParameterObject parameterObject, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteNonQueryAsync(RelationalCommandParameterObject parameterObject, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteNonQueryAsync(RelationalCommandParameterObject parameterObject, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal.SqlServerExecutionStrategy.ExecuteAsync[TState,TResult](TState state, Func`4 operation, Func`4 verifySucceeded, CancellationToken cancellationToken)
   at Program.<Main>$(String[] args) in D:\Dev\Git\EFCore8TimeSpanModify\EFCore8TimeSpanModify\EFCore8TimeSpanModify\Program.cs:line 26
   at Program.<Main>(String[] args)

Include provider and version information

EF Core version: 8.0.3
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET 8.0
Operating system: Windows 11
IDE: Rider 2023.3.4

Entry point of difference

In v7 a special trick was used to translate a setter expression and unwrap it. The translated SQL expression has the ValueConverter set:

efcore/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

Line 1372 in c9d1c2d

var setter = Infrastructure.ExpressionExtensions.CreateEqualsExpression(left, right);

In v8 this type mapping is not applied at the respective location leading to an invalid SQL:

efcore/src/EFCore.Relational/Query/RelationalQueryableMethodTranslatingExpressionVisitor.cs

Lines 1555 to 1564 in c2f9969

	if (visitor.TranslateExpression(remappedValueSelector, applyDefaultTypeMapping: false)
	is not SqlExpression translatedValueSelector)
	{
	visitor.AddTranslationErrorDetails(RelationalStrings.InvalidValueInSetProperty(valueSelector.Print()));
	return null;
	}
	// Apply the type mapping of the column (translated from the property selector above) to the value,
	// and apply alias uniquification to it.
	translatedValueSelector = visitor._sqlExpressionFactory.ApplyTypeMapping(translatedValueSelector, column.TypeMapping);

[Danielku15]

I tried a local hack in our project where we luckily have a custom SqlServerSqlTranslatingExpressionVisitor registered. Its far from clean but applying the type mapping of the left expression to the right expression ensures the correct SQL:

I wouldn't call this a feasable workaround but rather a proof that something with the type mappings is either missing or not supported well.

internal class CustomSqlServerSqlTranslatingExpressionVisitor : SqlServerSqlTranslatingExpressionVisitor
{
    public CustomSqlServerSqlTranslatingExpressionVisitor(
        RelationalSqlTranslatingExpressionVisitorDependencies dependencies,
        SqlServerQueryCompilationContext queryCompilationContext,
        QueryableMethodTranslatingExpressionVisitor queryableMethodTranslatingExpressionVisitor)
        : base(dependencies,
            queryCompilationContext,
            queryableMethodTranslatingExpressionVisitor)
    {
    }

    protected override Expression VisitBinary(BinaryExpression binaryExpression)
    {
        // https://github.com/dotnet/efcore/issues/33330
        // x => x.TimeSpanProp + timeSpanVariable
        // x => x.TimeSpanProp + TimeSpan.FromSeconds(1)
        // x => x.TimeSpanProp - timeSpanVariable
        // x => x.TimeSpanProp - TimeSpan.FromSeconds(1)
        if (binaryExpression.NodeType is ExpressionType.Add or ExpressionType.Subtract &&
            binaryExpression.Left.Type == typeof(TimeSpan) &&
            binaryExpression.Left.NodeType == ExpressionType.MemberAccess &&
            binaryExpression.Right.Type == typeof(TimeSpan) &&
            binaryExpression.Right.NodeType is ExpressionType.Parameter or ExpressionType.Constant)
        {
            var original = base.VisitBinary(binaryExpression);
            if (original is SqlBinaryExpression { Left.TypeMapping.Converter: not null } sqlBinaryExpression
               )
            {
                return sqlBinaryExpression.Right switch
                {
                    SqlParameterExpression r => sqlBinaryExpression.Update(sqlBinaryExpression.Left,
                        r.ApplyTypeMapping(sqlBinaryExpression.Left.TypeMapping)),
                    SqlConstantExpression r => sqlBinaryExpression.Update(sqlBinaryExpression.Left,
                        r.ApplyTypeMapping(sqlBinaryExpression.Left.TypeMapping)),
                    _ => original
                };
            }
        }

        return base.VisitBinary(binaryExpression);
    }
}

[roji]

In v8 this type mapping is not applied at the respective location leading to an invalid SQL [...]

In v8 the application of the type mapping has been cleaned up and no longer happens via a temporary equality node as before; this happens here:

SqlExpression? TranslateSqlSetterValueSelector(
    ShapedQueryExpression source,
    Expression valueSelector,
    ColumnExpression column)
{
    if (TranslateSetterValueSelector(source, valueSelector, column.Type) is SqlExpression translatedSelector)
    {
        // Apply the type mapping of the column (translated from the property selector above) to the value
        translatedSelector = _sqlExpressionFactory.ApplyTypeMapping(translatedSelector, column.TypeMapping);
        return translatedSelector;
    }

    AddTranslationErrorDetails(RelationalStrings.InvalidValueInSetProperty(valueSelector.Print()));
    return null;
}

In any case, the bug is reproducible outside of ExecuteUpdate, wherever a value-converted column is concatenated (Add operator) with a parameter/constant:

await context.WithTimeSpans.Where(w => w.TimeSpan + interval == TimeSpan.Zero).ToListAsync();

Full minimal repro

await using var context = new BlogContext();
await context.Database.EnsureDeletedAsync();
await context.Database.EnsureCreatedAsync();

var entity = new WithTimeSpan { TimeSpan = TimeSpan.FromTicks(100) };
await context.AddAsync(entity);
await context.SaveChangesAsync();

var interval = TimeSpan.FromTicks(10);

// Case 1:
// await context.WithTimeSpans.ExecuteUpdateAsync(x =>
//     x.SetProperty(e => e.TimeSpan, e => e.TimeSpan + interval)
// );

// Case 2:
await context.WithTimeSpans.Where(w => w.TimeSpan + interval == TimeSpan.Zero).ToListAsync();

public class BlogContext : DbContext
{
    public DbSet<WithTimeSpan> WithTimeSpans => Set<WithTimeSpan>();

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseSqlServer("Server=localhost;Database=test;User=SA;Password=Abcd5678;Connect Timeout=60;ConnectRetryCount=0;Encrypt=false")
            .LogTo(Console.WriteLine, LogLevel.Information)
            .EnableSensitiveDataLogging();

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<WithTimeSpan>(e =>
        {
            e.HasKey(x => x.Id);
            e.Property(x => x.Id).ValueGeneratedOnAdd();
            e.Property(x => x.TimeSpan)
                .HasColumnType("bigint")
                .HasConversion(v => v.Ticks,
                    v => TimeSpan.FromTicks(v));
        });
    }
}

public class WithTimeSpan
{
    public int Id { get; set; }
    public TimeSpan TimeSpan { get; set; }
}

@ajcvickers I strongly suspect the source of the bug is #32510 - that code was written with strings in mind, and when other things are concatenated, it seems to do the wrong thing... Assigning to you for investigation for now (but let me know if you'd rather not).

[Danielku15]

I can confirm that this code seems to be the root cause. I enabled the hidden switch for enabling the old behavior and it generates the right SQL afterwards.

efcore/src/EFCore.Relational/Query/SqlExpressionFactory.cs

Lines 210 to 213 in c2f9969

	if (UseOldBehavior32325)
	{
	inferredTypeMapping ??= ExpressionExtensions.InferTypeMapping(left, right);
	}

[roji]

Thanks for confirming @Danielku15!