How should we really be implenting Equals and GetHashCode for NHibernate entities

Question

There are many questions and answers and articles to this question available but in my opinion there seems to be no real clear/correct answer

For me Ayende has the best generic implementation so far that I've seen : http://ayende.com/blog/2500/generic-entity-equality

....But it is from 2007 ....

Is this the 'best way' to implement these methods especially with regard to NHibernate 3.2 which contains some differences in proxy implementation to earlier versions?

Bigelow

Answer 1

Yes!

You should be overriding Equals and GetHashCode. But, you shouldn't be doing value equality (Name == other.Name && Age == other.Age), you should be doing identity equality!

If you don't, you will most likely run into comparing a proxy of an entity with the real entity and it will be miserable to debug. For example:

public class Blog : EntityBase<Blog>
{
    public virtual string Name { get; set; }

    // This would be configured to lazy-load.
    public virtual IList<Post> Posts { get; protected set; }

    public Blog()
    {
        Posts = new List<Post>();
    }

    public virtual Post AddPost(string title, string body)
    {
        var post = new Post() { Title = title, Body = body, Blog = this };
        Posts.Add(post);
        return post;
    }
}

public class Post : EntityBase<Post>
{
    public virtual string Title { get; set; }
    public virtual string Body { get; set; }
    public virtual Blog Blog { get; set; }

    public virtual bool Remove()
    {
        return Blog.Posts.Remove(this);
    }
}

void Main(string[] args)
{
    var post = session.Load<Post>(postId);

    // If we didn't override Equals, the comparisons for
    // "Blog.Posts.Remove(this)" would all fail because of reference equality. 
    // We'd end up be comparing "this" typeof(Post) with a collection of
    // typeof(PostProxy)!
    post.Remove();

    // If we *didn't* override Equals and *just* did 
    // "post.Blog.Posts.Remove(post)", it'd work because we'd be comparing 
    // typeof(PostProxy) with a collection of typeof(PostProxy) (reference 
    // equality would pass!).
}

Here is an example base class if you're using int as your Id (which could also be abstracted to any identity type):

public abstract class EntityBase<T>
    where T : EntityBase<T>
{
    public virtual int Id { get; protected set; }

    protected bool IsTransient { get { return Id == 0; } }

    public override bool Equals(object obj)
    {
        return EntityEquals(obj as EntityBase<T>);
    }

    protected bool EntityEquals(EntityBase<T> other)
    {
        if (other == null)
        {
            return false;
        }
        // One entity is transient and the other is not.
        else if (IsTransient ^ other.IsTransient)
        {
            return false;
        }
        // Both entities are not saved.
        else if (IsTransient && other.IsTransient)
        {
            return ReferenceEquals(this, other);
        }
        else
        {
            // Compare transient instances.
            return Id == other.Id;
        }
    }

    // The hash code is cached because a requirement of a hash code is that
    // it does not change once calculated. For example, if this entity was
    // added to a hashed collection when transient and then saved, we need
    // the same hash code or else it could get lost because it would no 
    // longer live in the same bin.
    private int? cachedHashCode;

    public override int GetHashCode()
    {
        if (cachedHashCode.HasValue) return cachedHashCode.Value;

        cachedHashCode = IsTransient ? base.GetHashCode() : Id.GetHashCode();
        return cachedHashCode.Value;
    }

    // Maintain equality operator semantics for entities.
    public static bool operator ==(EntityBase<T> x, EntityBase<T> y)
    {
        // By default, == and Equals compares references. In order to 
        // maintain these semantics with entities, we need to compare by 
        // identity value. The Equals(x, y) override is used to guard 
        // against null values; it then calls EntityEquals().
        return Object.Equals(x, y);
    }

    // Maintain inequality operator semantics for entities. 
    public static bool operator !=(EntityBase<T> x, EntityBase<T> y)
    {
        return !(x == y);
    }
}

Gyrostabilizer

If a transient entity has its GetHashCode taken and becomes non-transient, it may compare equal to another entity which had not had its hash code taken while it was transient, but its hash code will not equal that of the other entity. Any time an object becomes equal to something it wasn't before, its hash code must equal that other object's hash code, and any time an object comes into existence which is equal to another whose hash has been taken, its hash must equal that of the other object's. What may be needed is to have a ConcurrentDictionary<int, WeakReference> and... – Baseless

...have each EntityBase<T> hold a reference to an Object associated with that id; if GetHashCode is called before an entity is persisted, it could generate an Object which would then be associated with the ID it receives when the object is persisted. It could then use that object's GetHashCode value as its own, and future entities which have the ID the transient one eventually receives could get the same identity token. Periodically the table would have to be scrubbed of entries whose WeakReference had died. – Baseless

@Baseless Would you mind creating an example of what you're proposing? The implementation I have above is the usual recommendation from NH articles. Could you also provide a quick sample of how it could fail? Thanks! – Gyrostabilizer

I don't think there's a general solution for changeable ID, but NHibernate is a special case because the value of an ID can only change from 'nothing' (unsaved value) to 'something' (real ID) when the object is persisted (at least in regular use - or am I missing something?) Unsaved objects are equal only to themselves: also, when their ID changes, it hopefully changes to a new and yet unused value, so the object cannot suddenly become equal to something it wasn't equal to before. So it seems it is actually safe for the object to cache its original hash code. – Stevenson

A miserable 2 days of debugging indeed! I ended up downloading the NHibernate codebase and referencing that to see that my entity wasn't being cached because it had a composite key and the proxy vs unproxied reference caused the cache key to be different. This EntityBase class solved it. – Platina

@Stevenson The problem is that if you put transient instances in a long-lived dictionary before persisting them, they will have been indexed with a hash code NOT based on the id. For example, if the instance is evicted from the NH session and reloaded, or loaded by a different session, that instance would calculate it's hash code based on the id, and you would not be able to use this instance for lookup in the original dictionary. So using this implementation based on cached hash code safely, does come with certain rules. – Quarto

Answer 2

My personal recommendation is not to implement these methods at all, because doing so forces loading in many cases where it isn't really necessary.

Also, if you don't move entities across sessions, you'll never need this. And even if you do, you can always compare by Id when needed.

Frierson

What about when you have a lazy-loaded collection of proxies and a non-proxy? Comparisons against that collection will fail. – Gyrostabilizer

@thecloudlesssky just compare by id manually. You could use LINQ to objects for a projection if you need it. – Frierson

Sure, but you can't control how collection.Remove(entity) does comparisons. Check out the sample in my post below to see how this could fail. – Gyrostabilizer

@Gyrostabilizer it all depends on your usage patterns. – Frierson

Totally. But I think as a community (and to save debugging headaches!), we should always be recommending to override Equals for identity equality. Otherwise, these issues will creep up without you knowing. For example, up until 2 days ago, I never overrode Equals. However, I hit the situation below when removing an entity from a collection and it was a real pain to figure out why it was happening. Having lazy-loaded collections is pretty common. Plus I'm using your lovely NHibernate.CollectionQuery to put all of my domain logic within the models - it's awesome :). – Gyrostabilizer

(cont'd). If you think about it, as long as Equals is implemented correctly, there is nothing wrong with doing this for every model. I'm in the camp of saving time debugging crap when I just expect it to work. Surprises aren't cool! – Gyrostabilizer

Answer 3

I had multiple issues while implementing solution suggested by @TheCloudlessSky.

First, my IDs are not with consistent datatype; some are int, some are Guid and some are string. Also, some are auto generated while other are manually assigned. Other problem may be in future in case I decide to use composite ids. Hence, I cannot put

public virtual int Id { get; protected set; }

in EntityBase base class. I have to define it in respective concrete Entity classes.

Second, as I cannot have Id in base class, it was getting harder to implement bool IsTransient property.

So, I decided to generate Guid per instance and use it to implement GetHashCode and Equals like below:

public abstract class BaseEntity
{
    Guid objectId = Guid.NewGuid();
    public virtual Guid ObjectId { get { return objectId; } }

    public override int GetHashCode()
    {
        return ObjectId.GetHashCode();
    }

    public override bool Equals(object other)
    {
        if(other == null)
            return false;
        if(ObjectId != (other as BaseEntity).ObjectId)
            return false;
        return ReferenceEquals(this, other);
    }
}

Bluff