Issue with composite owned type primary key and non-reference equality

[ajcvickers]

Originally posted by @voroninp here: #14675

---

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

namespace EfCoreOwnedEntitiesBug
{
    class Program
    {
        static void Main(string[] args)
        {
            var options = new DbContextOptionsBuilder<TestContext>()
                .EnableSensitiveDataLogging()
                .UseSqlServer("Server=(localdb)\\MSSQLLocalDB;Database=OwnedEntitiesBug;Integrated Security=True;MultipleActiveResultSets=True")
                .Options;

            var context = new TestContext(options);
            context.Database.EnsureCreated();

            var owned1 = new Owned { Id = Guid.NewGuid()};
            var owned2 = new Owned { Id = Guid.NewGuid()};
            var owner = new Owner()
            {
                Owned = { owned1, owned2 }
            };
            context.Add(owner);
            context.SaveChanges();
            var ownerId = owner.Id;

            // recreate context
            context = new TestContext(options);
            // retrieve owner
            owner = context.Find<Owner>(ownerId);
            Console.WriteLine($"{owner.Owned.Count} owned entities.");
            owner.Owned.Clear();
            owner.Owned.Add(new Owned { Id = owned1.Id});
            owner.Owned.Add(new Owned { Id = owned2.Id});
            context.SaveChanges();
            Console.WriteLine($"{owner.Owned.Count} owned entities. - WTF moment, isn't it?");
            Console.ReadKey();
        }
    }

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

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<Owner>(b =>
            {
                b.OwnsMany(_ => _.Owned, c =>
                {
                    c.HasForeignKey("OwnerId");
                    c.HasKey("OwnerId", "Id");
                });
            });
        }
    }

    public sealed class Owner
    {
        public Guid Id { get; set; }

        public HashSet<Owned> Owned { get; } = new HashSet<Owned>();
    }

    public sealed class Owned
    {
        public Guid Id { get; set; }

        public int Property { get; set; }

        public override bool Equals(object obj)
        {
            if (obj is null || GetType() != obj.GetType())
            {
                return false;
            }

            var that = (Owned)obj;

            return Id == that.Id && Property == that.Property;
        }

        public override int GetHashCode()
        {
            return Id.GetHashCode() ^ Property.GetHashCode();
        }
    }
}

[ajcvickers]

Note for triage: my initial impression here is that the non-reference equality here is incompatible with EF Core tracking; removing it resolves the error. However, putting it here for others to look at in triage before closing.

[voroninp]

@ajcvickers ,

You in #14675

note that EF always uses reference equality to determine entity instance identity, and so if you are expecting EF to use some different equality then this won't work.

non-reference equality here is incompatible with EF Core tracking

What?!

...for others to look at in triage before closing.

Closing?! =D

[ajcvickers]

@voroninp I was terse because I was just making a note for triage, not a resolution of the bug.

To be a bit clearer, EF uses reference equality for everything it does when it can. However, in your code you are explicitly creating a navigation property that does not use reference equality:

public HashSet<Owned> Owned { get; } = new HashSet<Owned>();

We know this has issues, and we're considering #14675 to make this better for lists, but this would not be something we can do for HashSet since it would largely defeat the purpose of using a HashSet in the first place. Therefore, the correct thing to do here from an EF perspective is for your code to create the HashSet with reference equality. Hence this issue would be closed as by-design.

However, as I said, I would like others on the team to take a look first, hence my note for triage.

[voroninp]

@ajcvickers Hm... But why is type of navigation property important here?
Doesn't tracker hold the sequence of references navigation property contained on read?
On SaveChanges() I'd expect it to compare that sequence with the current state of the navigation property which is just an enumerable. Does it mean that somewhere in deep internals Contains() is called? ;-) which miserably breaks things for HashSet?

[ajcvickers]

@voroninp Yes, Contains is called because the reason for using a HashSet is to get fast look-ups, so using a linear search instead would hurt perf in many cases.

[ajcvickers]

Triage concurs with analysis. The HashSet must use reference equality.

[voroninp]

@ajcvickers Ok, but this behaviour is not obvious at all. Can EF detect this situation and at least emit a warning or even throw an exception by default?

If HashSet<T> is used as navigation property and T overrides Equals, throw or require explicit configuration to allow value equality.

[ajcvickers]

@voroninp I've investigated detecting this in the past but was not able to detect this situation in a robust and performant way.

[voroninp]

@ajcvickers Will value equality work for List<T> then? Won't same problem occur?

[ajcvickers]

@voroninp Yes, but #14675 is a potential improvement in this for lists.

But as a general rule you should avoid changing from reference equality for entity types, since EF can only track a single instance of a given entity identity. Doing otherwise results in lots of complications around merging graphs with potential conflicts.

It's also worth noting that while owned types are sometimes considered "value objects", my personal view is that this is an anti-pattern. Value objects don't have keys and should have value semantics. This isn't a great fit for any type of entity type since it will never have value semantics and must have a key to be tracked, even if it is owned.

[voroninp]

@ajcvickers How do you offer to implement a collection of value objects? In my case key of principal entity is a shadow property, it's not even the part of value object, and all other properties just comprise the value itself. Composite key is just an implementation detail here.

[ajcvickers]

@voroninp I don't think there is a good way to do that currently. Ultimately the combination of #13947 and #4179 should give a decent experience.

[voroninp]

@ajcvickers I am still pondering over the problem.
Honestly, I don't get how these two issues help. I will end up with entity containing a collection of value objects with multiple properties and overridden equality.

[Timovzl]

Hm, comparing the two hashsets correctly would require iterating over the left one and confirming that each entry is present in the right, and then also doing the reverse. (If you imagine a hashset that uses StringComparer.OrdinalIgnoreCase and put differently-cased but otherwise identical strings in each set, you might see why this is necessary. It gets even more fascinating when one hashset uses OrdinalIgnoreCase while the other uses Ordinal! They can still contain values considered equal... but we need the two-way equality checks.)

I suppose the performance would be on the same order as that of a custom implementation for lists. @ajcvickers Should you be interested, I have correct implementations for all common collection variants (luckily, not that many) lying around from an earlier project related to structural equality.

@voroninp As for workarounds... Do you truly need a hashset, or is it just the most correct type to use? If there will not be many child entities, most solutions would work: replacing the set instead of appending to it, or using a list instead. There is ImmutableList<T>, a collection you replace, but that bases itself on the previous "version" to be more performant than, say, duplicating a list.

If you truly need fast lookup with many elements, you could consider implementing your own ISet<T> implementation something like this:

• Use a HashSet<T> or SortedDictionary<TKey, TValue> under the hood.
• On each modification, return a new instance of your collection (which you will assign to the entity's property), while using the same underlying collection.