-
Notifications
You must be signed in to change notification settings - Fork 126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Relationship rework of Version 3 broke support for data classes that use custom implementations of GetHashCode #194
Comments
Yikes, that's quite a mess. Don't think that avoiding the call to 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. |
Thanks for the acknowledgement. I actually think that using the To implement that change, the ctor of However there is one case where an instance of |
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. |
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 anEntityInstanceWrapper
, therefore the implementation ofGetHashCode()
of the real data class is used.Now, whenever the
TypedEntityContainer
is called toGetOrAddToLocalCollection
, a newLocalEntityCollectionKey
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 useSystem.Runtime.CompilerServices.RuntimeHelpers.GetHashCode(_instance)
instead ofthis._instance.GetHashCode()
.Maybe passing the
EntityInstanceWrapper
around the real data object to theLocalEntityCollectionKey
in order to generate the hash value from the instance wrapper would be another solution?The text was updated successfully, but these errors were encountered: