0
public class Foo
{
    public int Id { get; set; }
    public ICollection<Bar> Bars{get;set;}
}

public class Bar
{
    public int Id { get; set; }
    public int FooId { get; set; }

    public override bool Equals(object obj)
    {
        var bar= obj as Bar;
        return bar != null &&
               Id == bar.Id;
    }

    public override int GetHashCode()
    {
        return 2108858624 + Id.GetHashCode();
    }
}

public class SomeClass
{
    DbContext _context = ...; // injected via DI

    public void AddNewBarToFoo(int fooId)
    {
        // Option 1
        // result: foo.Bars contains two items
        Foo foo = _context.Foos
               .Where(f => f.id == fooId)
               .Include(f => f.Bars)
               .FirstOrDefault();
        foo.Bars.Add(new Bar());
        _context.SaveChanges();

        // Option 2
        // result: foo.Bars contains one item
        Bar bar = new Bar();
        bar.FooId = FooId;
        _context.Bars.Add(bar);
        _context.SaveChanges();

        // Option 3:
        // Remove _context.SaveChanges() from Option 1

        // Option 4:
        // Remove the Id comparison from Bar.Equals()
    }
}

Today I noticed something weird using Entity Framework Core. In the AddNewBarToFoo method of SomeClass I want to add a new Bar to a collection of Foos. Originally I used Option 1, but I noticed that, after the call to SaveChanges, foo.Bars would contain the new Bar twice.
I noticed that removing the call to SaveChanges would not add the second Bar and still persist the Bar correctly. Using Option 2 works as expected too.

I found this quite strange behavior and upon investigating a little I found that the reason for this is that I overrode the Equals method of Bar and that I used the Id to check for equality.
Removing the Id comparison from the Equals method and otherwise using Option 1 works as expected: Foo.Bars contains the new Bar only once.

I actually have two questions now:

  1. What is the reason Entity Framework adds the same Bar twice?
    I suppose it adds the new Bar to the context once, giving it an Id and then encounters it again (somehow?) but this instance still has Id 0 and thus is considered different than the first per my Equals method.

  2. Was I wrong to use the Id in my overridden Equals method? Or in the code for adding a new Bar to a Foo? Is it bad practice to override Equals?
    In short: what is the 'correct' way to do what I tried to do?

Arnaud VdP
  • 227
  • 1
  • 9
  • 1
    `GetHashCode` must be overridden as well if `Equals` is overridden https://stackoverflow.com/q/371328/1132334 – Cee McSharpface Jun 26 '18 at 13:24
  • I did override `GetHashCode` too, but forgot it in this explanation. Added it now. – Arnaud VdP Jun 26 '18 at 13:35
  • How are you instantiating `Bars`? Normally, you don't need to override `Equals` nor `GetHashCode` and just instantiate the `ICollection` as a `HashSet` and EF solves it on its own – Camilo Terevinto Jun 26 '18 at 13:38
  • 1
    In this case `Bars` is instantiated by EF via the `Include` statement (that I forgot to add in this description, now fixed). – Arnaud VdP Jun 26 '18 at 13:45
  • Try making it explicit: `public Foo() { Bars = new HashSet(); } ` and see what happens – Camilo Terevinto Jun 26 '18 at 13:50
  • But that would overwrite the existing collection if there is one already? – Arnaud VdP Jun 26 '18 at 13:52
  • No, Entity Framework calls the default constructor before attempting to do anything with the object. If the collection is initialized in the constructor, EF uses that instance to add the items – Camilo Terevinto Jun 26 '18 at 13:54
  • Sounds like EF Core bug to me. Although you are breaking the contract that `GetHashCode` should not use mutable state. Still IMO EF Core is not supposed to use `GetHashCode` and `Equals` overrides. – Ivan Stoev Jun 26 '18 at 14:06
  • What exact EFC version are we talking about? – Ivan Stoev Jun 26 '18 at 14:17
  • The Id is only changed, or set on adding the Bar to the context. I didn't take that into account. Is there anywhere I could read more about why EF Core is not supposed to use `GetHashCode` or `Equals`? I am using version 2.1.0 – Arnaud VdP Jun 26 '18 at 14:19

1 Answers1

0

This is a side effect of breaking HashSet<T>'s contract with the hash code.

If you're going to put an object into a HashSet (or any other hash-based container, including Dictionary keys), you must guarantee that from the time the object is added to the container until the time it is removed, you do not alter the value returned by GetHashCode (neither the one on object nor the one in IEquatable<T>).

Here, you are adding the object with an as-yet-unknown Id (i.e. zero) and then EF is modifying the Id once it actually gets added to the database -- but this occurs while it's still inside the HashSet, breaking its contract.

When EF checks the collection to see if it's there, because the hash is now different it will fail to find it and will add it again.

You either need to pick another property for your hash that you can more readily guarantee will be at least effectively immutable (sadly EF won't let you declare it in a way that the compiler will enforce this), use a constant hash, or use a plain collection (e.g. List<T>) instead of a HashSet<T>.

Miral
  • 12,637
  • 4
  • 53
  • 93