Unexpected behavior of value-converted expressions in queries

[ranma42]

Bug description

Using value converters seems to be very error prone, as they can be freely used within LINQ expressions, but in several cases the resulting SQL query does not take value conversion into account.

Apparently (under the assumption that the value conversion is a pure function and that the two directions are indeed inverse), equality comparison between a value-converted column against a constant or parameter matches the "obvious" behavior (the one you would get through client-side evaluation).

Most other expressions seem to be prone to mismatching translations, which can lead to exceptions and/or silently return "wrong" results.

This is likely related to

• Support different SQL translation when store representation has changed due to value conversion #10434
• Support server side value conversions #10861
• Some values in queries are not going through value converter #11347

IIUC the plan was to add a warning (see the comment #11347 (comment)) but I was unable to trigger it when running the code and I did not it in the documentation.

If there is still a desire to work towards emitting warnings in these cases, this code example might be a good starting point for further testing/bug hunting (I did not find a PR or an issue with this specific goal).

NOTE: I am not sure whether the code I have written is valid or not 😅
I might be misunderstanding the explanation of the limitations of value converters. The closest one I see is "It isn't possible to query into value-converted properties, e.g. reference members on the value-converted .NET type in your LINQ queries.", but it seems to be aimed specifically at member access, while the issue I am hitting involves simple scalar values (or, rather, expressions on simple scalar values).

If this is not tackled as a bug in EFCore, I think it might be a good idea to extend the documentation to explicitly mention the pitfalls around queries that rely on value-converted fields.

To reproduce, the Sqlite version of the code runs on https://dotnetfiddle.net/ just fine; the SqlServer version needs a DB to connect to.

Your code

// @nuget: Microsoft.EntityFrameworkCore.Sqlite -Version 9.0.1

using System;
using System.Collections.Generic;
using System.Data;
using System.Linq;
using Microsoft.EntityFrameworkCore;

using var db = new BloggingContext();

db.Database.EnsureDeleted();
db.Database.EnsureCreated();

var clientSideEntities = db.MyEntities.ToList().AsQueryable();

// constants work as expected
LogResults(q => q.Where(x => x.MyHex == 16)); // 1(16, 10), 2(16, 16)
LogResults(q => q.Where(x => x.MyDec == 16)); // 2(16, 16)

var myInt = 16;
// parameters work as expected
LogResults(q => q.Where(x => x.MyHex == myInt)); // 1(16, 10), 2(16, 16)
LogResults(q => q.Where(x => x.MyDec == myInt)); // 2(16, 16)

// column-column comparison does not work as expected
LogResults(q => q.Where(x => x.MyHex == x.MyDec));
// expected: 2(16, 16)
// got: 1(16, 10)

// expressions can succeed with unexpected results
LogResults(q => q.Select(x => x.MyHex + 0));
// expected: 16, 16
// got(SqlServer): 256, 256
// got(Sqlite): 10, 10

// expressions can fail on SQL server
LogResults(q => q.Select(x => x.MyHex - 1));
// expected: 15, 15
// got(SqlServer): The data types nvarchar(max) and nvarchar are incompatible in the subtract operator.
// got(Sqlite): 9, 9

void LogResults<T>(Func<IQueryable<MyEntity>, IEnumerable<T>> list)
{
    var expected = list(clientSideEntities).ToList();
    var actual = list(db.MyEntities).ToList();
    PrintList("expected: ", expected);
    PrintList("got: ", actual);
}

void PrintList<T>(string prefix, IEnumerable<T> list)
{
    try
    {
        Console.WriteLine(prefix + string.Join(", ", list.Select(x => x?.ToString())));
    }
    catch (Exception ex)
    {
        Console.WriteLine(ex.Message);
    }
}

public class BloggingContext : DbContext
{
    public DbSet<MyEntity> MyEntities { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder options)
        => options
            .LogTo(Console.WriteLine, Microsoft.Extensions.Logging.LogLevel.Information)
            .EnableSensitiveDataLogging()
            //.UseSqlServer("use a valid connection string here");
            .UseSqlite("Data Source=test.db");

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<MyEntity>()
            .Property(x => x.MyHex)
            .HasConversion(
                v => v.ToString("x"),
                v => int.Parse(v, System.Globalization.NumberStyles.HexNumber)
            );

        modelBuilder.Entity<MyEntity>()
            .Property(x => x.MyDec)
            .HasConversion(
                v => v.ToString(),
                v => int.Parse(v)
            );

        modelBuilder.Entity<MyEntity>().HasData(new MyEntity { Id = 1, MyHex = 16, MyDec = 10 });
        modelBuilder.Entity<MyEntity>().HasData(new MyEntity { Id = 2, MyHex = 16, MyDec = 16 });
    }
}

public class MyEntity
{
    public int Id { get; set; }
    public int MyHex { get; set; }
    public int MyDec { get; set; }

    public override string ToString() => $"{Id}({MyHex}, {MyDec})";
}

Stack traces

Verbose output

EF Core version

9.0.1

Database provider

Microsoft.EntityFrameworkCore.Sqlite

Target framework

.NET 9.0

Operating system

Kali Linux

IDE

Visual Studio Code 1.96.4

EDIT: added one of the relevant issues

[ranma42]

The limitations documentation used to contain a relevant bullet point:

• Use of value conversions may impact the ability of EF Core to translate expressions to SQL. A warning will be logged for such cases.

but it has been removed in dotnet/EntityFramework.Docs@1a9ef8a (maybe because no warning is logged anymore 🤔 )

[roji]

Hey @ranma42 👋 Sorry it took so long to respond, we're in a particularly intensive work period (which for me will likely continue at least until May - expect delays unfortunately...).

Yeah, value converters are indeed incompatible with many (probably most) querying patterns that involve them. Interestingly, I think we see less people struggling with this than I'd expect - people seem to kind of get it, or maybe aren't attempting to involve value-converted properties in non-trivial queries (by which I mean, going beyond comparison with a constant/parameter).

I can see us implemented some targeted warnings: your case above of comparing two columns with different value converters seems like a good candidate; though that may end up warning for legitimate scenarios as well (I'd have to think if I can come up with some) - that's also OK, as we have warnings for high-probability (but not 100%) issues as well. I wouldn't see this as high priority simply because again, I don't remember anyone bringing up this paritcular kind of problem.

I absolutely agree that our docs here can and should be extended, to make sure people are fully aware of the limitations; basically something like "if you go beyond the basics of comparing to constant/parameter, be very careful and confirm, and be aware that most cases won't work as expected".

The two last cases - projecting with arithmetic operators - look like we may want to treat them as low-priority bugs. Just like regular method/member translations, I believe we should consider not allowing operators in these cases. However, this is made a bit more complex by the important distinction between value converters which change store types, and those which don't. In other words, a value converter that just e.g. appends a suffix on a string or divides a number by two is harmless; it shouldn't interfere with any translations in general. Properties which are value-converted to another store type, on the other hand, should be blocked from participating in most translations (operators, methods, members...).

There's obviously a lot to think about and discuss here. I'd start by improving the docs; given that we don't see people failing here too much, i wouldn't prioritize the rest too highly right away - but we can of course open issues to track various possibilities.

Let me know what you think.