Does this solve Nhibernate identity problem and GetHashCode issues?

Question

The solution I propose involves quite a bit of code, but you can just copy it all and past it in a VS test solution assuming you have SqLite installed, and you should be able to run the tests yourself.

As I have been struggling with the object identity versus object equality and database identity problem using Nhibernate, I have read various posts. However, I could not get a clear picture of how to properly set up object identity in conjunction with collections. Basically, the big problem, as I got, is that once an object is added to a collection it's identity (as derived by the GetHashCode) method cannot change. The preferred way to implement the GetHasHCode is to use a business key. But what if the business key was not the right one? I would like to have that entity updated with it's new business key. But then my collections are out of sync as I violated the immutability of the identity of that object.

The below code is a proposal to solve this problem. However, as I am certainly not a NHibernate expert and also not a very experienced developer, I would gladly receive comments from more senior developers whether this is a viable approach.

using System;
using System.Collections.Generic;
using FluentNHibernate.Cfg;
using FluentNHibernate.Cfg.Db;
using FluentNHibernate.Mapping;
using Iesi.Collections.Generic;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using NHibernate;
using NHibernate.Cfg;
using NHibernate.Tool.hbm2ddl;
using NHibernate.Util;

namespace NHibernateTests
{
    public class InMemoryDatabase : IDisposable
    {
        private static Configuration _configuration;
        private static ISessionFactory _sessionFactory;

        private ISession _session;

        public ISession Session { get { return _session ?? (_session = _sessionFactory.OpenSession()); } }

        public InMemoryDatabase()
        {
// Uncomment this line if you do not use NHProfiler
            HibernatingRhinos.Profiler.Appender.NHibernate.NHibernateProfiler.Initialize();
            _sessionFactory = CreateSessionFactory();
            BuildSchema(Session);
        }

        private static ISessionFactory CreateSessionFactory()
        {
            return Fluently.Configure()
              .Database(SQLiteConfiguration.Standard.InMemory().Raw("hbm2ddl.keywords", "none").ShowSql())
              .Mappings(m => m.FluentMappings.AddFromAssemblyOf<Brand>())
              .ExposeConfiguration(cfg => _configuration = cfg)
              .BuildSessionFactory();
        }

        private static void BuildSchema(ISession Session)
        {
            SchemaExport export = new SchemaExport(_configuration);
            export.Execute(true, true, false, Session.Connection, null);
        }

        public void Dispose()
        {
            Session.Dispose();
        }

    }


    public abstract class Entity<T>
        where T: Entity<T>
    {
        private readonly IEqualityComparer<T> _comparer;

        protected Entity(IEqualityComparer<T> comparer)
        {
            _comparer = comparer;
        } 

        public virtual Guid Id { get; protected set; }

        public virtual bool IsTransient()
        {
            return Id == Guid.Empty;
        }

        public override bool Equals(object obj)
        {
            if (obj == null) return false;
            return _comparer.Equals((T)this, (T)obj);
        }

        public override int GetHashCode()
        {
            return  _comparer.GetHashCode((T)this);
        }

    }

    public class Brand: Entity<Brand>
    {
        protected Brand() : base(new BrandComparer()) {}

        public Brand(String name) : base (new BrandComparer())
        {
            SetName(name);
        }

        private void SetName(string name)
        {
            Name = name;
        }

        public virtual String Name { get; protected set; }

        public virtual Manufactor Manufactor { get; set; }

        public virtual void ChangeName(string name)
        {
            Name = name;
        }
    }

    public class BrandComparer : IEqualityComparer<Brand>
    {
        public bool Equals(Brand x, Brand y)
        {
            return x.Name == y.Name;
        }

        public int GetHashCode(Brand obj)
        {
            return obj.Name.GetHashCode();
        }
    }

    public class BrandMap : ClassMap<Brand>
    {
        public BrandMap()
        {
            Id(x => x.Id).GeneratedBy.GuidComb();
            Map(x => x.Name).Not.Nullable().Unique();
            References(x => x.Manufactor)
                .Cascade.SaveUpdate();
        }
    }

    public class Manufactor : Entity<Manufactor>
    {
        private Iesi.Collections.Generic.ISet<Brand> _brands = new HashedSet<Brand>();

        protected Manufactor() : base(new ManufactorComparer()) {}

        public Manufactor(String name) : base(new ManufactorComparer())
        {
            SetName(name);
        }

        private void SetName(string name)
        {
            Name = name;
        }

        public virtual String Name { get; protected set; }

        public virtual Iesi.Collections.Generic.ISet<Brand> Brands
        {
            get { return _brands; }
            protected set { _brands = value; }
        }

        public virtual void AddBrand(Brand brand)
        {
            if (_brands.Contains(brand)) return;

            _brands.Add(brand);
            brand.Manufactor = this;
        }
    }

    public class ManufactorMap : ClassMap<Manufactor>
    {
        public ManufactorMap()
        {
            Id(x => x.Id);
            Map(x => x.Name);
            HasMany(x => x.Brands)
                .AsSet()
                .Cascade.AllDeleteOrphan().Inverse();
        }
    }

    public class ManufactorComparer : IEqualityComparer<Manufactor>
    {
        public bool Equals(Manufactor x, Manufactor y)
        {
            return x.Name == y.Name;
        }

        public int GetHashCode(Manufactor obj)
        {
            return obj.Name.GetHashCode();
        }
    }

    public static class IdentityChanger
    {
        public static void ChangeIdentity<T>(Action<T> changeIdentity, T newIdentity, ISession session)
        {
            changeIdentity.Invoke(newIdentity);
            session.Flush();
            session.Clear();
        }
    }

    [TestClass]
    public class BusinessIdentityTest
    {
        private InMemoryDatabase _db;

        [TestInitialize]
        public void SetUpInMemoryDb()
        {
            _db = new InMemoryDatabase();
        }

        [TestCleanup]
        public void DisposeInMemoryDb()
        {
            _db.Dispose();
        }

        [TestMethod]
        public void ThatBrandIsIdentifiedByBrandComparer()
        {
            var brand = new Brand("Dynatra");

            Assert.AreEqual("Dynatra".GetHashCode(), new BrandComparer().GetHashCode(brand));
        }

        [TestMethod]
        public void ThatSetOfBrandIsHashedByBrandComparer()
        {
            var brand = new Brand("Dynatra");
            var manufactor = new Manufactor("Lily");
            manufactor.AddBrand(brand);

            Assert.IsTrue(manufactor.Brands.Contains(brand));
        }

        [TestMethod]
        public void ThatHashOfBrandInSetIsThatOfComparer()
        {
            var brand = new Brand("Dynatra");
            var manufactor = new Manufactor("Lily");
            manufactor.AddBrand(brand);

            Assert.AreEqual(manufactor.Brands.First().GetHashCode(), "Dynatra".GetHashCode());
        }

        [TestMethod]
        public void ThatSameBrandCannotBeAddedTwice()
        {
            var brand = new Brand("Dynatra");
            var duplicate = new Brand("Dynatra");
            var manufactor = new Manufactor("Lily");
            manufactor.AddBrand(brand);
            manufactor.AddBrand(duplicate);

            Assert.AreEqual(1, manufactor.Brands.Count);
        }

        [TestMethod]
        public void ThatPersistedBrandIsSameAsLoadedBrandWithSameId()
        {
            var brand = new Brand("Dynatra");
            var manufactor = new Manufactor("Lily");
            manufactor.AddBrand(brand);

            _db.Session.Transaction.Begin();
            _db.Session.Save(brand);

            var copy = _db.Session.Load<Brand>(brand.Id);
            _db.Session.Transaction.Commit();

            Assert.AreSame(brand, copy);
        }

        [TestMethod]
        public void ThatLoadedBrandIsContainedByManufactor()
        {
            var brand = new Brand("Dynatra");
            var manufactor = new Manufactor("Lily");
            manufactor.AddBrand(brand);

            _db.Session.Transaction.Begin();
            _db.Session.Save(brand);

            var copy = _db.Session.Load<Brand>(brand.Id);
            _db.Session.Transaction.Commit();

            Assert.IsTrue(brand.Manufactor.Brands.Contains(copy));
        }

        [TestMethod]
        public void ThatAbrandThatIsLoadedUsesTheSameHash()
        {
            var brand = new Brand("Dynatra");
            var manufactor = new Manufactor("Lily");
            manufactor.AddBrand(brand);

            _db.Session.Transaction.Begin();
            _db.Session.Save(brand);
            var id = brand.Id;

            brand = _db.Session.Load<Brand>(brand.Id);

            Assert.IsTrue(brand.Manufactor.Brands.Contains(new Brand("Dynatra")));

        }

        [TestMethod]
        public void ThatBrandCannotBeFoundIfIdentityChanges()
        {
            var brand = new Brand("Dynatra");
            var manufactor = new Manufactor("Lily");
            manufactor.AddBrand(brand);

            _db.Session.Transaction.Begin();
            _db.Session.Save(brand);

            Assert.IsTrue(brand.Manufactor.Brands.Contains(new Brand("Dynatra")));
            brand.ChangeName("Dynatra_");

            Assert.AreEqual("Dynatra_", brand.Name);
            Assert.AreEqual("Dynatra_".GetHashCode(), brand.Manufactor.Brands.First().GetHashCode());
            Assert.IsFalse(brand.Manufactor.Brands.Contains(brand));
            // ToDo: I don't understand why this test fails
            Assert.IsTrue(brand.Manufactor.Brands.Contains(new Brand("Dynatra")));
        }

        [TestMethod]
        public void ThatSessionNeedsToBeClearedAfterIdentityChange()
        {
            var brand = new Brand("Dynatra");
            var manufactor = new Manufactor("Lily");
            manufactor.AddBrand(brand);

            _db.Session.Transaction.Begin();
            _db.Session.Save(brand);
            var id = brand.Id;

            brand = _db.Session.Load<Brand>(brand.Id);

            // This makes the test pass
            IdentityChanger.ChangeIdentity(brand.ChangeName, "Dynatra_", _db.Session);

            brand = _db.Session.Load<Brand>(id);

            Assert.IsFalse(brand.Manufactor.Brands.Contains(new Brand("Dynatra")));
            Assert.IsTrue(brand.Manufactor.Brands.Contains(new Brand("Dynatra_")));

        }
    }
}

Important edit! I now consider the approach I was suggesting, as has been pointed out as not the right approach. I have provided a different answer to the dilemma I was facing.

Answer 1

That's an interesting approach but instead of taking the time to understand and critique I will just offer my solution to this problem.

I don't like the idea of a generic entity base class, so my solution only supports int, Guid and string identities. Some of the code below, such as using a Func<int> to get the hash code, only exists to support case-insensitive string comparisons. If I ignored string identifiers (and I wish I could), the code would be more compact.

This code passes the unit tests I have for it and hasn't let me down in our applications but I'm sure there are edge cases. The only one I've thought of is: If I new and save an entity it will keep its original hash code, but if after the save I retrieve an instance of the same entity from the database in another session it will have a different hash code.

Feedback welcome.

Base class:

[Serializable]
public abstract class Entity
{
    protected int? _cachedHashCode;

    public abstract bool IsTransient { get; }

    // Check equality by comparing transient state or id.
    protected bool EntityEquals(Entity other, Func<bool> idEquals)
    {
        if (other == null)
        {
            return false;
        }
        if (IsTransient ^ other.IsTransient)
        {
            return false;
        }
        if (IsTransient && other.IsTransient)
        {
            return ReferenceEquals(this, other);
        }
        return idEquals.Invoke();
    }

    // Use cached hash code to ensure that hash code does not change when id is assigned.
    protected int GetHashCode(Func<int> idHashCode)
    {
        if (!_cachedHashCode.HasValue)
        {
            _cachedHashCode = IsTransient ? base.GetHashCode() : idHashCode.Invoke();
        }
        return _cachedHashCode.Value;
    }
}

int identity:

[Serializable]
public abstract class EntityIdentifiedByInt : Entity
{
    public abstract int Id { get; }

    public override bool IsTransient
    {
        get { return Id == 0; }
    }

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

    public virtual bool Equals(EntityIdentifiedByInt other)
    {
        return EntityEquals(other, () => Id == other.Id);
    }

    public override int GetHashCode()
    {
        return GetHashCode(() => Id);
    }
}

Guid identity:

[Serializable]
public abstract class EntityIdentifiedByGuid : Entity
{
    public abstract Guid Id { get; }

    public override bool IsTransient
    {
        get { return Id == Guid.Empty; }
    }

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

    public virtual bool Equals(EntityIdentifiedByGuid other)
    {
        return EntityEquals(other, () => Id == other.Id);
    }

    public override int GetHashCode()
    {
        return GetHashCode(() => Id.GetHashCode());
    }
}

string identity:

[Serializable]
public abstract class EntityIdentifiedByString : Entity
{
    public abstract string Id { get; }

    public override bool IsTransient
    {
        get { return Id == null; }
    }

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

    public virtual bool Equals(EntityIdentifiedByString other)
    {
        Func<bool> idEquals = () => string.Equals(Id, other.Id, StringComparison.OrdinalIgnoreCase);
        return EntityEquals(other, idEquals);
    }

    public override int GetHashCode()
    {
        return GetHashCode(() => Id.ToUpperInvariant().GetHashCode());
    }
}

Answer 2

I think the basic misconception here is that you implement Equals and GetHashCode based on business data. I don't know why you prefer that, I can't see any advantage in it. Except - of course - when dealing with a value object which doesn't have an Id.

There is a great post on nhforge.org about Identity Field, Equality and Hash Code

Edit: This part of your code will cause problems:

    public static class IdentityChanger
    {
        public static void ChangeIdentity<T>(Action<T> changeIdentity, T newIdentity, ISession session)
        {
            changeIdentity.Invoke(newIdentity);
            session.Flush();
            session.Clear();
        }
    }

1. Flushing is expensive
2. Clearing the session makes NH loading the same entities again.
   1. It may produce much too many db queries because the entities aren't found in the session anymore.
   2. It may produce confusion when an entity that had been read from the db is linked to another and NH complains that it is transient
   3. It may produce memory leaks, for instance when it happens in a loop

You should implement Equals and GetHashCode based on immutable data. Changing the hash is not possible in a reasonable way.

Answer 3

It took me quite a while to get it, but I think the answer to my problem is actually deceptively simple. The best approach, as has been long advocated by the Hibernate team, is just to not override equals and gethashcode. What I did not get was that when I call Contains on a set of business objects, obviously I want to know whether that set contains an object with a specific business value. But that was something I did not get from the Nhibernate persistence set. But Stefan Steinegger put it right in a comment on a different question on this subject I was asking: 'the persistence set is not a business collection'! I completely failed to understand his remark the first time.

The key issue was that I should not try to make the persistence set to behave as a business collection. Instead I should make use of a persistence set wrapped in a business collection. Then things get much easier. So, in my code I created a wrapper:

internal abstract class EntityCollection<TEnt, TParent> : IEnumerable<TEnt>
{
    private readonly Iesi.Collections.Generic.ISet<TEnt> _set;
    private readonly TParent _parent;
    private readonly IEqualityComparer<TEnt> _comparer;

    protected EntityCollection(Iesi.Collections.Generic.ISet<TEnt> set, TParent parent, IEqualityComparer<TEnt> comparer)
    {
        _set = set;
        _parent = parent;
        _comparer = comparer;
    } 

    public IEnumerator<TEnt> GetEnumerator()
    {
        return _set.GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }

    public bool Contains(TEnt entity)
    {
        return _set.Any(x => _comparer.Equals(x, entity));
    }

    internal Iesi.Collections.Generic.ISet<TEnt> GetEntitySet()
    {
        return _set;
    }

    internal protected virtual void Add(TEnt entity, Action<TParent> addParent)
    {
        if (_set.Contains(entity)) return;

        if (Contains(entity)) throw new CannotAddItemException<TEnt>(entity);

        _set.Add(entity);
        addParent.Invoke(_parent);
    }

    internal protected virtual void Remove(TEnt entity, Action<TParent> removeParent)
    {
        if (_set.Contains(entity)) return;

        _set.Remove(entity);
        removeParent.Invoke(_parent);
    }
}

This is a generic wrapper that implements the business meaning of a set. It knows when two business objects are equal by value through a IEqualityComparer, it presents itself as a true business collection exposing the entity as an enumerable of entity interfaces (much cleaner than exposing the persistence set) and it even knows how to handle bidirectional relations with the parent.

The parent entity that owns this business collection has the following code:

    public virtual IEnumerable<IProduct> Products
    {
        get { return _products; }
    }

    public virtual Iesi.Collections.Generic.ISet<Product> ProductSet
    {
        get { return _products.GetEntitySet(); }
        protected set { _products = new ProductCollection<Brand>(value, this); }
    }

    public virtual void AddProduct(IProduct product)
    {
        _products.Add((Product)product, ((Product)product).SetBrand);
    }

    public virtual void RemoveProduct(IProduct product)
    {
        _products.Remove((Product)product, ((Product)product).RemoveFromBrand);
    }

So, the entity has in fact two interfaces, a business interface exposing the business collection and a entity interface that is exposed to Nhibernate to deal with persistency of the collection. Note that the same persistence set is returned to Nhibernate as is passed in using the ProductSet property.

It basically all boils down to separation of concerns:

• the persistence set is not my concern but is handled by nhibernate to persist my collecion
• the business meaning of equality by value is handled by equality comparers
• the business meaning of a set, i.e. when a set already contains an entity with the same business value, I should not be able to pass in a second different object with the same business value, is handled by the business collection object.

Only, when I want to intermix entities between sessions I would have to resort to other solutions as mentioned above. But I think that if you can avoid that situation, you should.