Relationship rework of Version 3 broke support for data classes that use custom implementations of GetHashCode

[TAkkerman]

When I upgraded Dapper.FastCrud to the latest Version from a previous 2.x Version, I encountered a new issue in the context of relations.

Consider the following simplified scenario: I have a data type Customer that has a set of Orders and a set of Payments. As soon as a single Customer has more than one Order and at least one Payment, whenever I query for that Customer and also ask to include both the related Orders and Payments, the result of that query will have duplicate Payments and/or Orders (the exact amount of duplicates depends on the number of unique Payments and Orders).

I could trace back this issue to the fact that I use protobuf messages as data types. The c# code generator for protobuf messages automatically adds a custom implementation of GetHashCode() that will return a hash value that depends on all values that are part of the object, especially including the contents of the collections that are navigation properties to the child sets (in my case Orders and Payments).

This is a problem because the implementation of LocalEntityCollectionKey uses the result of _instance.GetHashCode() in order to generate its own lazy _hashCode - as far as I can tell, _instance is the real object instance and not an EntityInstanceWrapper, therefore the implementation of GetHashCode() of the real data class is used.

Now, whenever the TypedEntityContainer is called to GetOrAddToLocalCollection, a new LocalEntityCollectionKey will be generated for the single Customer and that key will never match any of previous ones because even though all previous ones were constructed using the same Customer object, that Customer object had fewer child Order/Payment attached to it at the time the hash was evaluated. Therefore _localEntityInstances.TryGetValue using that key will never find the already existing entry for that Customer and in consequence all repetitions of child Order/Payment objects are also added to the navigation collections.

I think this problem can be averted if LocalEntityCollectionKey is changed to use System.Runtime.CompilerServices.RuntimeHelpers.GetHashCode(_instance) instead of this._instance.GetHashCode(). Maybe passing the EntityInstanceWrapper around the real data object to the LocalEntityCollectionKey in order to generate the hash value from the instance wrapper would be another solution?

[MoonStorm]

Yikes, that's quite a mess. Don't think that avoiding the call to GetHashCode on the entity instance is the right way to fix this though. I can totally see us forgetting about this edge scenario in the future and breaking it again.

I'm inclined to recommend not using these entities for more than what they are. They should only live in the DAL and mapped to business entities and DTOs for data transmission.

[TAkkerman]

Thanks for the acknowledgement.

I actually think that using the EntityInstanceWrapper instead of the Entity itself could solve that issue, especially since the EntityInstanceWrapper implements its own hashcode function that seems to work upon the property values actually read from database - that means the hashcode of the wrapper looks to be very suitable for questions concerning the identity of the wrapped entity. Also since that class is part of Dapper.Fastcrud, you also have complete control over the hash implementation.

To implement that change, the ctor of LocalEntityCollectionKey (as well as the method GetOrAddToLocalCollection of the class TypedEntityContainer) need to be altered to ask for an EntityInstanceWrapper instead of the object instance (or actualInstance).

However there is one case where an instance of MainEntityResultSetParser<TMainEntity> is passed to the GetOrAddToLocalCollection, which obviously is not an EntityInstanceWrapper and it is probably not feasible to just wrap it at that place. That means there has to be an overload or alternative method for this particular case. (Also note that the parser class does not override the hash function provided by the base object type - that implementation is safe to use in the given context)

[MoonStorm]

Don't think anyone will take a look at this. As I said, overriding GetHashCode for DB entities is a really bad idea and can cause other issues, especially since these guys could only be partially filled with data at various points. For example, you insert an entity without providing a primary key, which is a great candidate for identifying the entity via Equals and GetHashCode, just to come back later with the identifier filled in by the DB. It's the same entity, however the custom GetHashCode could say otherwise.

I strongly recommend creating a proper set of DTOs for data transmissions and have a set of mappers between the DB entities and your DTOs. That way you won't have any kind of problems and the two layers can evolve independently.