Use correct value comparers for collection properties with value conversion

[BalintBanyasz]

My entity has a collection of SmartEnums (simplified in the code example below) which is persisted in the database using a ValueConverter in order to keep the domain model clean (I wanted to avoid introducing a wrapper entity with an extra ID property). It works correctly for retrieving data but unfortunately doesn't seem to pick up changes to the collection automatically. When calling SaveChanges, the changes are not persisted unless the entity state is manually set to EntityState.Modified beforehand.

Steps to reproduce

MySmartEnum

public class MySmartEnum
    {
        public string Value { get; set; }

        public static MySmartEnum FromValue(string value)
        {
            return new MySmartEnum { Value = value };
        }
    }

Entity

public class Entity
    {
        public Entity()
        {
            SmartEnumCollection = new HashSet<MySmartEnum>();
        }

        public int Id { get; set; }
        public virtual ICollection<MySmartEnum> SmartEnumCollection { get; set; }
    }

Context

public class TestContext : DbContext
    {
        public TestContext()
        { }

        public TestContext(DbContextOptions<TestContext> options)
            : base(options)
        { }

        public virtual DbSet<Entity> Entities { get; set; }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            var valueConverter = new ValueConverter<ICollection<MySmartEnum>, string>
            (
                e => string.Join(',', e.Select(c => c.Value)),
                str => new HashSet<MySmartEnum>(
                            str.Split(new char[] { ',' }, StringSplitOptions.RemoveEmptyEntries)
                               .Select(x => MySmartEnum.FromValue(x)))
            );

            modelBuilder.Entity<Entity>()
                .Property(e => e.SmartEnumCollection)
                .HasConversion(valueConverter);
        }
    }

Update method

using (var context = new TestContext(options))
            {
                var entity = context.Entities
                    .First();

                entity.SmartEnumCollection.Add(MySmartEnum.FromValue("Test"));

                // Changes are persisted only if the following line is uncommented:
                //context.Entry(entity).State = EntityState.Modified;

                context.SaveChanges();
            }

Further technical details

EF Core version: 3.0.0-rc1.19427.8
Database Provider: Microsoft.EntityFrameworkCore.SqlServer (Version 3.0.0-rc1.19427.8)
Operating system: Windows 10 (Version 10.0.18362.295)
IDE: Visual Studio 2019 16.2.3

[ajcvickers]

@BalintBanyasz The issue here is that EF Core needs to be able to create a snapshot of the current value and then compare that snapshot with the new value to see if it has changed. This requires special code when using value conversion to map a type with significant structure--in this case the ICollection<>.

The fix for this is to tell EF how to snapshot and compare these collections. For example:

var valueComparer = new ValueComparer<ICollection<MySmartEnum>>(
    (c1, c2) => c1.SequenceEqual(c2), 
    c => c.Aggregate(0, (a, v) => HashCode.Combine(a, v.GetHashCode())),
    c => (ICollection<MySmartEnum>)c.ToHashSet());

modelBuilder.Entity<Entity>()
    .Property(e => e.SmartEnumCollection)
    .HasConversion(valueConverter)
    .Metadata.SetValueComparer(valueComparer);

Notes for triage: we should consider:

• Documenting how to do this--filed Show how to write value conversions for structured types EntityFramework.Docs#1680
• If a simple property is a collection (or maybe just an IEnumerable), then we could automatically use a value comparer like shown above.

[ajcvickers]

Note from triage:

• Warn about this case in 3.1
• Consider shipping well-known specific value comparers
• In future, decide if we should do this automatically and also:
  • Can we detect IEquatable or IComparable and use it, only generating the snapshotting?

[BalintBanyasz]

@BalintBanyasz The issue here is that EF Core needs to be able to create a snapshot of the current value and then compare that snapshot with the new value to see if it has changed. This requires special code when using value conversion to map a type with significant structure--in this case the ICollection<>.

The fix for this is to tell EF how to snapshot and compare these collections. For example:

var valueComparer = new ValueComparer<ICollection<MySmartEnum>>(
    (c1, c2) => c1.SequenceEqual(c2), 
    c => c.Aggregate(0, (a, v) => HashCode.Combine(a, v.GetHashCode())),
    c => (ICollection<MySmartEnum>)c.ToHashSet());

modelBuilder.Entity<Entity>()
    .Property(e => e.SmartEnumCollection)
    .HasConversion(valueConverter)
    .Metadata.SetValueComparer(valueComparer);

Notes for triage: we should consider:

• Documenting how to do this--filed aspnet/EntityFramework.Docs#1680
• If a simple property is a collection (or maybe just an IEnumerable), then we could automatically use a value comparer like shown above.

Thank you @ajcvickers. It works perfectly with the ValueComparer and snapshotExpression configured.

[ajcvickers]

3.1 part of this (warning) split out into #18600

[petrmosna15]

Fox me the ValueComparer was not working, I had to remove ToHashSet() in the snapshotExpression

[coolhome]

+1 for better documentation and +1 for an easier solution.

I have not tested this but I think a string backing field could be used in junction with a getter/setter that will serialize/deserialize to and from the backing field.

Either way both are less than ideal imho.

[samburovkv]

I tried to follow @ajcvickers's advice. But my code doesn't work properly.
My code:

public class Person
{
    public string Id { get; set; }

    public virtual List<int> Numbers { get; set; }

    public Person()
    {
        Id = Guid.NewGuid().ToString();
        Numbers = new List<int>();
    }
}

public class PersonEntityTypeConfiguration : IEntityTypeConfiguration<Person>
    {
        internal static readonly ValueConverter<List<int>, string> Converter
            = new ValueConverter<List<int>, string>(
                v => JsonSerializer.Serialize(v, null),
                v => JsonSerializer.Deserialize<List<int>>(v, null));

        public void Configure(EntityTypeBuilder<Person> builder)
        {
            builder.HasKey(x => x.Id);
            builder
                .Property(x => x.Numbers)
                .HasDefaultValue(new List<int>())
                .IsRequired()
                .HasConversion(Converter)
                .Metadata.SetValueComparer(new ValueComparer<List<int>>(
                    (c1, c2) => c1.SequenceEqual(c2),
                    c => c.Aggregate(0, (a, v) => HashCode.Combine(a, v.GetHashCode())),
                    c => c.ToList()));
        }
    }

var person = dbContext.Persons.FirstOrDefault(x => x.Id == "6c7a5476-0266-459e-8229-544483379108");
person.Numbers.Add(98);
await dbContext.SaveChangesAsync();

I get "person" entity from database then I add new item to "Numbers" list. But SaveChanges doesn't save new values in the collection to database. I have test project with test database https://github.com/samburovkv/EfSQLiteTests

I'd like to know how this feature works.
This code works:

var person = dbContext.Persons.FirstOrDefault(x => x.Id == "6c7a5476-0266-459e-8229-544483379108");

person.Numbers = new List<int>(person.Numbers);
person.Numbers.Add(98); 
await dbContext.SaveChangesAsync();

[ajcvickers]

@samburovkv I moved this to a new issue: #21986

[flamaniac]

@BalintBanyasz The issue here is that EF Core needs to be able to create a snapshot of the current value and then compare that snapshot with the new value to see if it has changed. This requires special code when using value conversion to map a type with significant structure--in this case the ICollection<>.

The fix for this is to tell EF how to snapshot and compare these collections. For example:

var valueComparer = new ValueComparer<ICollection<MySmartEnum>>(
    (c1, c2) => c1.SequenceEqual(c2), 
    c => c.Aggregate(0, (a, v) => HashCode.Combine(a, v.GetHashCode())),
    c => (ICollection<MySmartEnum>)c.ToHashSet());

modelBuilder.Entity<Entity>()
    .Property(e => e.SmartEnumCollection)
    .HasConversion(valueConverter)
    .Metadata.SetValueComparer(valueComparer);

Notes for triage: we should consider:

* Documenting how to do this--filed [Show how to write value conversions for structured types EntityFramework.Docs#1680](https://github.com/dotnet/EntityFramework.Docs/issues/1680)

* If a simple property is a collection (or maybe just an `IEnumerable`), then we could automatically use a value comparer like shown above.

equalsExpression paramters are defined as nullable and this code (c1, c2) => c1.SequenceEqual(c2), will throw an exception if c1 or c2 will be null.

[lonix1]

Consider shipping well-known specific value comparers

Yes! For example, for ICollection<string>, ICollection<int>.

equalsExpression paramters are defined as nullable and this code (c1, c2) => c1.SequenceEqual(c2), will throw an exception if c1 or c2 will be null.

Yes my analysers also caught that bit. And one cannot use null propagation in the expression tree, so I just used null-forgiving (!) to ignore the issue... bad idea, but can't see an alternative.