Value conversions for types that are not mapped by default can result in client-eval for simple equality to parameter or constanrt

[Tarig0]

Describe what is not working as expected.

If I have a dictionary that is stored in sql as a json object I am able to store and retrieve it, but when I try to check the equivilance of two empty dictionaries in a where query it will do so locally

Steps to reproduce

Include a complete code listing (or project/solution) that we can run to reproduce the issue.

Partial code listings, or multiple fragments of code, will slow down our response or cause us to push the issue back to you to provide code to reproduce the issue.

protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<SimpleCounter>().ToTable("TestCounters");
            modelBuilder.Entity<SimpleCounter>().HasKey(c => c.CounterId);
            modelBuilder.Entity<SimpleCounter>().HasAlternateKey(c => new { c.StyleKey, c.IsTest, c.Discriminator });
            modelBuilder.Entity<SimpleCounter>().Property(c => c.Discriminator).HasConversion(
                d => JsonConvert.SerializeObject(d),
                json => JsonConvert.DeserializeObject<Dictionary<string, string>>(json));
}

public class SimpleCounter
    {
        public Guid CounterId { get; set; } = Guid.NewGuid();
        public string StyleKey { get; set; }
        public bool IsTest { get; set; }
        public IDictionary<string, string> Discriminator { get; set; } = new Dictionary<string, string>();
        public long? LastUsed { get; set; }
    }

Tests

[Fact] //fails
        public async Task AbleToRetriveCounter()
        {
            using (var con = new SqliteConnection("DataSource=:memory:"))
            {
                await con.OpenAsync();
                using (var db = new SequanceDB(new DbContextOptionsBuilder<SequanceDB>().UseSqlite(con).Options))
                {
                    await db.Database.EnsureCreatedAsync();
                    db.Add(new SimpleCounter() { StyleKey = "Swag" });
                    await db.SaveChangesAsync();

                }
                using (var db = new SequanceDB(new DbContextOptionsBuilder<SequanceDB>().UseSqlite(con).UseLoggerFactory(new LoggerFactory(new[] {
                    new Microsoft.Extensions.Logging.Debug.DebugLoggerProvider()
                })).Options))
                {
                    Assert.NotNull(await db.Set<SimpleCounter>().
                 Where(c => c.StyleKey == "Swag" &&
                 c.IsTest == false)
                 .FirstOrDefaultAsync());
                    Assert.NotNull(await db.Set<SimpleCounter>().
                 Where(c => c.StyleKey == "Swag" &&
                 c.IsTest == false &&
                 c.Discriminator == new Dictionary<string,string>())
                 .FirstOrDefaultAsync());
                }
            }
        }

        [Fact] //passes
        public async Task AbleToRetriveCounterIgnoreDiscrim()
        {
            using (var con = new SqliteConnection("DataSource=:memory:"))
            {
                await con.OpenAsync();
                using (var db = new SequanceDB(new DbContextOptionsBuilder<SequanceDB>().UseSqlite(con).Options))
                {
                    await db.Database.EnsureCreatedAsync();
                    db.Add(new SimpleCounter() { StyleKey = "Swag" });
                    await db.SaveChangesAsync();

                }
                using (var db = new SequanceDB(new DbContextOptionsBuilder<SequanceDB>().UseSqlite(con).UseLoggerFactory(new LoggerFactory(new[] {
                    new Microsoft.Extensions.Logging.Debug.DebugLoggerProvider()
                })).Options))
                {
                    Assert.NotNull(await db.Set<SimpleCounter>().
                 Where(c => c.StyleKey == "Swag" &&
                 c.IsTest == false)
                 .FirstOrDefaultAsync());
                }
            }
        }

Further technical details

EF Core version: 2.1.0-rc1-final
Database Provider: Microsoft.EntityFrameworkCore.Sqlite)
Operating system: Win 10
IDE: Visual Studio 2017 15.7.1

[Tarig0]

The reason why I would expect this is because I can add a "store" only property to the counter class to get the desired server query

modelBuilder.Entity<SimpleCounter>().ToTable("TestCounters");
            modelBuilder.Entity<SimpleCounter>().HasKey(c => c.CounterId);
            modelBuilder.Entity<SimpleCounter>().HasAlternateKey(c => new { c.StyleKey, c.IsTest, c.Discriminator });
            modelBuilder.Entity<SimpleCounter>().Ignore(c => c.Discriminator);
            modelBuilder.Entity<SimpleCounter>().Property("DiscriminatorJson").HasColumnName(nameof(SimpleCounter.Discriminator));

public class SimpleCounter
    {
        public Guid CounterId { get; set; } = Guid.NewGuid();
        public string StyleKey { get; set; }
        public bool IsTest { get; set; }
        public IDictionary<string, string> Discriminator { get; set; } = new Dictionary<string, string>();
        private string DiscriminatorJson
        {
            get => JsonConvert.SerializeObject(Discriminator);
            set => Discriminator = JsonConvert.DeserializeObject<Dictionary<string, string>>(value);
        }
        public long? LastUsed { get; set; }
    }

[Fact]
        public async Task AbleToRetriveCounterBacking()
        {
            using (var con = new SqliteConnection("DataSource=:memory:"))
            {
                await con.OpenAsync();
                using (var db = new SequanceDB(new DbContextOptionsBuilder<SequanceDB>().UseSqlite(con).Options))
                {
                    await db.Database.EnsureCreatedAsync();
                    db.Add(new SimpleCounter() { StyleKey = "Swag" });
                    await db.SaveChangesAsync();

                }
                using (var db = new SequanceDB(new DbContextOptionsBuilder<SequanceDB>().UseSqlite(con).UseLoggerFactory(new LoggerFactory(new[] {
                    new Microsoft.Extensions.Logging.Debug.DebugLoggerProvider()
                })).Options))
                {
                    Assert.NotNull(await db.Set<SimpleCounter>().
                 Where(c => c.StyleKey == "Swag" &&
                 c.IsTest == false &&
                 EF.Property<string>(c,"DiscriminatorJson") == "{}")
                 .FirstOrDefaultAsync());
                }
            }
        }

[ajcvickers]

@ajcvickers @smitpatel to investigate the feasibility of a patch.

[Tarig0]

I think this may have to introduced as a new extension function, to avoid having == change its function in certain contexts

c.Discriminator.StoreEquals(new Dictionary<string,string>())

StoreEquals<T>(T this a, T b)

Edit: 5/22
I just realized that strings == operators change depending on the backing database (case insensitive by default for mssql). So... probably not a good idea.

[smitpatel]

Dependent on #4978

[smitpatel]

blocked on #13192

[RamunasAdamonis]

Any workaround for this? Besides not using value conversions.

[badcommandorfilename]

I have an interesting case and hopefully a workaround for some people. Using netcore2.2 and EntityFrameworkCore v2.2.0.

I have a "Text" class, which is simply a non-nullable string type (see https://andrewlock.net/using-strongly-typed-entity-ids-to-avoid-primitive-obsession-part-3/):

    public readonly struct Text
    {
        public string Value { get; }

        public Text(string value)
        {
            Value = value ?? ""; //Impose some constraint on entity types with this property
        }

        public static implicit operator Text(string value) => new Text(value);

        public static implicit operator string(Text value) => value.Value;
    }

So we use this in an Entity type and add a ValueConverter:

    public class User
    {
        [Key]
        [DatabaseGenerated(DatabaseGeneratedOption.Identity)]
        public Int64 Id { get; set; }
        public string Email { get; set; } //stored as a string
        public Text UserName { get; set; } //stored as a string, but mapped to Text with a ValueConverter
   }

   protected override void OnModelCreating(ModelBuilder modelBuilder)
   {
        modelBuilder.Entity<User>()
        .Property<Text>(x => x.UserName)
        .HasConversion(
                new ValueConverter<Text, string>(
                    t => t.Value,
                    s => new Text(s)
                ));
        );
   }

I want this to be evaluated server-side without having to load all the UserName text values from the DB just to convert them into the CLR type. 'Text' has user-defined implicit operators so casts to and from System.String work transparently. The following query complies no-problem:

            string username = "myuser@test.com";
            var user = users.Where(x =>
                x.UserName == username
                || x.Email == username)
                .FirstOrDefault();

However, when I execute the query, I get the exception "Invalid cast from 'System.String' to 'Text'." (the same problem occurs if I use an explicit conversion too):

      System.InvalidCastException: Invalid cast from 'System.String' to 'Text'.
         at System.Convert.DefaultToType(IConvertible value, Type targetType, IF
ormatProvider provider)
         at Microsoft.EntityFrameworkCore.Storage.ValueConversion.ValueConverter
`2.Sanitize[T](Object value)
         at Microsoft.EntityFrameworkCore.Storage.ValueConversion.ValueConverter
`2.<>c__DisplayClass3_0`2.<SanitizeConverter>b__0(Object v)
         at Microsoft.EntityFrameworkCore.Storage.RelationalTypeMapping.CreatePa
rameter(DbCommand command, String name, Object value, Nullable`1 nullable)
         at Microsoft.EntityFrameworkCore.Storage.Internal.TypeMappedRelationalP
arameter.AddDbParameter(DbCommand command, Object value)
         at Microsoft.EntityFrameworkCore.Storage.Internal.RelationalParameterBa
se.AddDbParameter(DbCommand command, IReadOnlyDictionary`2 parameterValues)
         at Microsoft.EntityFrameworkCore.Storage.Internal.RelationalCommand.Cre
ateCommand(IRelationalConnection connection, IReadOnlyDictionary`2 parameterValu
es)
         at Microsoft.EntityFrameworkCore.Storage.Internal.RelationalCommand.Exe
cute(IRelationalConnection connection, DbCommandMethod executeMethod, IReadOnlyD
ictionary`2 parameterValues)
         at Microsoft.EntityFrameworkCore.Storage.Internal.RelationalCommand.Exe
cuteReader(IRelationalConnection connection, IReadOnlyDictionary`2 parameterValu
es)
         at Microsoft.EntityFrameworkCore.Query.Internal.QueryingEnumerable`1.En
umerator.BufferlessMoveNext(DbContext _, Boolean buffer)
         at Microsoft.EntityFrameworkCore.SqlServer.Storage.Internal.SqlServerEx
ecutionStrategy.Execute[TState,TResult](TState state, Func`3 operation, Func`3 v
erifySucceeded)
         at Microsoft.EntityFrameworkCore.Query.Internal.QueryingEnumerable`1.En
umerator.MoveNext()

After a few days of hair-pulling and confusion, the problem line seems to be:
https://github.com/aspnet/EntityFrameworkCore/blob/master/src/EFCore/Storage/ValueConversion/ValueConverter%60.cs#L53

The call to Convert.ChangeType(value, unwrappedType) does NOT use the user defined conversion operators, but instead blows up when there's no builtin primitive or reference conversion between the types. Since you can't inherit from the sealed string type, Convert.ChangeType can't handle this situation.

That seems like a big limitation to the current ValueConverter implementation...

BUT, after a few more days of hair-pulling and confusion I found a workaround! Override the non-generic version of ValueConverter:

        public class TextConverter : ValueConverter<Text, string>
        {
            static Expression<Func<Text, string>> to = v => v.Value;
            static Expression<Func<string, Text>> from => v => new Text(v);

            public TextConverter() : base(to, from) { }

            public override Func<dynamic, object> ConvertFromProvider => v => new Text(v);
            public override Func<dynamic, object> ConvertToProvider => v => v.ToString(); //Important: this should be v => v.Value, but sometimes a <string> is passed instead of <Text>

            public override Type ModelClrType => typeof(Text);
            public override Type ProviderClrType => typeof(string);

            public override Expression<Func<string, Text>> ConvertFromProviderExpression => from;
            public override Expression<Func<Text, string>> ConvertToProviderExpression => to;
        }

Thankfully, this class has lots of override hooks so we can force the query provider to a plain, user-defined conversion expression! This works AND the conversion is entirely server-side (at least in my simple case of null handling)!

There is one oddity, which is that "ConvertToProvider" seems to be called both for "To" and "From" operations (i.e. when querying a string is passed in and when saving a Text is passed in)... This seems like it could be a bug to me, but I was able to get around it with some sneaky dynamic use and overriding .ToString().

Hope this helps someone!

[ilmax]

Hey @badcommandorfilename I gave it a try but still getting the same exception when filtering a query with a custom type because I've disabled client evaluation of queries.

I'm using a type that wraps an int, and implementing a value converter I'm able to do:

• Insert
• Delete
• Read (read a list or filter by other properties)

But the filter using the custom type is still not accepted by the query pipeline and an exception is thrown. Not sure about update but I suppose it works.

I've also noted something strange, using the Generic HasConversion<MyValueConvertType>() doesn't work and throws exception about the incorrect mapping during the call to Database.EnsureCreated(), while passing an instance to the non generic version of the method works i.e. HasConversion(new MyValueConvertType()).

So unfortunately it's not a complete workaround, looking forward to have this feature implemented for the very same reason you do (given the primitive obsession post link)

[thijsalofs]

If you can't wait till the next major EF Core release, maybe this approach might work for you:
https://andrewlock.net/strongly-typed-ids-in-ef-core-using-strongly-typed-entity-ids-to-avoid-primitive-obsession-part-4/

[ilmax]

@thijsalofs Thankl you for the link, will give this a try!

[warappa]

@thijsalofs Indeed, it works!

I took Andrew Lock's approach and aligned it to the API conventions of EF Core.
With this I was able to even remove all builder.Property(x => x.SomeCustomId).HasConversion(...) calls.

Here's the gist with the code and how to use it:
https://gist.github.com/warappa/f1fa1a46857eb9c144ced518f250dcbd

1. Register the new ValueConverterSelector while registering a DbContext:

dbContextOptionBuilder.UseGlobalValueConverterSelector();

2. Register a conversion for a custom value object:

modelBuilder.Owned<CustomId>()
.HasConversion(x => x.Value, x => new CustomId(x));

[Leon99]

@warappa looks great, kudos for the solution! I noticed that the comments to the original article mention issues with using it with ValueGeneratedOnAdd(). Did you manage to help it with your modification by any chance?

[warappa]

@Leon99 I just tested it. Out-of-the-box (incl. the custom ValueConverterSelector) it doesn't work: EF doesn't know about CustomId. But EF supports custom ValueGenerators.
With a custom ValueGenerator it works:

Entity configuration:

builder.Property(x => x.Id)
    .HasConversion(x => x.Value, x => CustomId.Create(x)).ValueGeneratedOnAdd().HasValueGenerator<CustomIdValueGenerator>();

Custom ValueGenerator:

internal class CustomIdValueGenerator : ValueGenerator<CustomId>
{
    public override CustomId Next(EntityEntry entry) => CustomId.Create(Guid.NewGuid()); // or however it is generated
    public override bool GeneratesTemporaryValues => false;
}

I think a generic solution is possible, but as smitpatel suspects the underlying problem is fixed, I didn't went this far ;)

EDIT:
@Leon99
Arg, whatever, I couldn't resist making it work with the converters. So no HasValueGenerator is required anymore.
This solution reuses the converter registered to GlobalValueConverterSelector and utilizes the fact we're dealing with Guids behind the scenes.

public class ConverterValueGeneratorSelector : ValueGeneratorSelector
{
    // The dictionary in the base type is private, so we need our own one here.
    private static readonly ConcurrentDictionary<Type, ValueGenerator> _generators
        = new ConcurrentDictionary<Type, ValueGenerator>();

    public ConverterValueGeneratorSelector(ValueGeneratorSelectorDependencies dependencies)
        : base(dependencies)
    {
    }

    public override ValueGenerator Select(IProperty property, IEntityType entityType)
    {
        if (!_generators.TryGetValue(property.ClrType, out var generator))
        {
            if (GlobalValueConverterSelector.TryGetConverter(property.ClrType, out var converter))
            {
                generator = new ConverterValueGenerator(converter);
                _generators.TryAdd(property.ClrType, generator);
                return generator;
            }
        }
        else
        {
            generator = base.Select(property, entityType);
            _generators.TryAdd(property.ClrType, generator);
        }

        return generator;
    }
}
public class ConverterValueGenerator : ValueGenerator<object>
{
    private ValueConverter converter;

    public ConverterValueGenerator(ValueConverter converter)
    {
        this.converter = converter;
    }
       
    public override object Next(EntityEntry entry)
    {
        return converter.ConvertFromProvider(Guid.NewGuid());
    }

    public override bool GeneratesTemporaryValues => false;
}

Addition to GlobalValueConverterSelector:

public static bool TryGetConverter(Type modelClrType, out ValueConverter converter)
{
    if (!_converters.TryGetValue((modelClrType, typeof(Guid)), out var dummy))
    {
        converter = null;
        return false;
    }

    converter = dummy.Create();

    return true;
}

And the register extension method:

public static class DbContextOptionsBuilderValueConverterExtensions
{
    public static DbContextOptionsBuilder UseGlobalValueConverterSelector(this DbContextOptionsBuilder builder)
    {
        builder.ReplaceService<IValueConverterSelector, GlobalValueConverterSelector>();
        builder.ReplaceService<IValueGeneratorSelector, ConverterValueGeneratorSelector>();
        return builder;
    }
}

[davisnw]

I'm probably just missing something, but it's not clear to me what has actually been resolved here. The pull request that closed this issue only adds tests, it doesn't show any changes to core code.

There is a claim that this is a dupe of #10861, but that is still open.

What is actually supported now with the close of this issue?

[smitpatel]

@davisnw - The bug report filed in first post has been fixed in 3.0 release. The attached PR only added test for regression. The fix has been part of larger query pipeline rewrite.

Nowhere this issue has been claimed as duplicate of #10861 in this issue. So that issue being open is irrelevant.