I'm already quite sensitive to missing Equals and GetHashCode implementations for NHibernate entities or value types that caused issues like re-inserting or duplicating items within collections hundred times.
A good rule for Equals and GetHashCode (that is clearly stated in the documentation) is to make it do comparisons based on the set of fields that create "business" (real-life) identification of an object whenever possible - and not on database-level identifiers. It works well also when comparing objects from different sessions (detached) or not yet persisted (transient) - without any "unproxying" magic.
Today, my personal counter of hours devoted into cursing and fighting NHibernate-related corner cases or strange issues increased once again. I have an interesting (two hours later - frustrating) case of entities being mixed up in spite of correct SQL queries issued. And I was quite confident that my ReSharper-generated Equals and GetHashCode methods were correct and the root cause was somewhere else. Just look how simple was the code of my entity:
public class City { public virtual int Id { get; set; } public virtual string Name { get; set; } public virtual bool Equals(City other) { if (ReferenceEquals(null , other)) return false ; if (ReferenceEquals(this , other)) return true ; return Equals(other.Name, Name); } public override bool Equals(object obj) { if (ReferenceEquals(null , obj)) return false ; if (ReferenceEquals(this , obj)) return true ; if (obj.GetType() != typeof (City)) return false ; return Equals((City ) obj); } public override int GetHashCode() { return (Name != null ? Name.GetHashCode() : 0); } }
I am comparing City instances using a natural key - its name. In the faulty code I was querying the database for different entities that reference City and later the referenced cities were compared for equality. Here is the simplified sketch of the test case (written in Machine.Specifications) with initialization that creates the object graph and two fetching scenarios as a separate tests.
public class EqualsTest : DatabaseTests { Establish context = () => sut.WithinSessionAndTransaction(sess => { var city = new City() { Name = "Llanfairpwllgwyngyll" }; sess.Persist(city); sess.Persist( new Address () { City = city }); sess.Persist( new District () { City = city }); }); It should_have_equal_cities = () => sut.WithinSessionAndTransaction(sess => { var address = sess.Query<Address>().Single(); var district = sess.Query<District >().Single(); district.City.ShouldEqual(address.City); }); It should_correctly_use_city_in_lookup = () => sut.WithinSessionAndTransaction(sess => { var address = sess.Query<Address >().Single(); var districts = sess.Query<District >().ToLookup(x => x.City); districts[address.City].ShouldNotBeEmpty(); }); }
In the first test I'm doing the direct comparison of cities, in the second one I'm creating a lookup table with City instance as a key. In both cases lazy loading takes place so I'm working with NHibernate-generated City proxies. But it should not be a problem, as NHibernate guarantees object identity within a single session, right?
Well, uhm, the first test passes as expected, but the second test fails! It turned out that the City instance used as the key in districts (proxy instance) does not maintain identity with the proxy instance fetched for Address instance, even if they are both pointing at the same (single) city and are used within single session!
I'm not sure why this happens and I'm pretty confident it shouldn't, but fortunately the workaround is quite easy. As the proxies instances used in Address and District instances are now different references, they are compared using the Equals method we've provided. When Equals (or, more precisely, GetHashCode) is called on one of the objects to compare it with the second one, lazy fetch from the database is performed and it becomes the "real", unproxied object. But the second one doesn't - it is still CityProxy instance. And Equals offered by ReSharper, when checking objects types, unfortunately expects the type to exactly match:
if (obj.GetType() != typeof (City)) return false ;
But obj.GetType() is a CityProxy and we're exiting the comparison with the negative result here. The workaround is just to replace that exact check with more semantic one, checking only whether obj can be treated as a City instance:
if (!(obj is City)) return false ;
In this case, nor City neither CityProxy are eliminated, NHibernate can continue to compare city names and see that the proxy points to the same objects. This simple change done - and voilà - we have two tests passed!
Mutable fields should never, ever be used for calculating hash codes. If someone changes the value of Name, then the hash code will change. This will in turn break any dictionary/hash table that contains an instance of the class.
ReplyDeleteEither you are misunderstanding the design patterns used by NHibernate or NHibernate itself is royally screwed up.
I know I should not use mutable objects as keys, but it's not the case here - I'm using Cities as predefined dictionary, not mutating it, so I don't expect the hash code to change.
DeleteR# has three different options for equality member generation:
ReplyDelete- The same type as this (x.GetType() == this.GetType())
- Exactly type as 'your type' (x.GetType() == typeof(TypeOfX))
- Equal or subtype of 'your type' (x as TypeOfX)
With NHibernate you have to use 3rd option. Please always pay attention on options. It is not R# or NH fault, it is your inadvertence.
I hoped I didn't sound like blaming NHibernate again, if I had, it's probably my natural and unintended style of writing :)
DeleteAnyway, using default ReSharper setting (what probably is what majority of us do) caused me a headache and I believe this is worth sharing.
Hi. Currently I'm the owner of Alt+Ins functionality in ReSharper team.
ReplyDeleteIn version 7.0 option "Comparand type check" was introduced exactly to solve any issues with ORMs and partial equality scenarios — http://www.jetbrains.com/resharper/webhelp/Code_Generation__Equality_Members.html
We can't use "is" check as a default option because in most of the user scenarios the definition of "equality" doesn't allow different derived types with equal subclasses parts to be equal.
Hi. Than the most of user scenerios is breaking LSP (Liskov Substitution Principle) and that's bad news :(
DeleteStop it, Milan.
Delete