Fix key type mismatch in EntityUniqueKey #1647
Conversation
Fix key type mismatch in EntityUniqueKey EntityUniqueKey was assuming the value to be always semi-resolved (to be the identifier instead of the entity for an entity type). It was always computing the value hashcode with the semi-resolved type, which is the identifier type for an entity. But this has changed with nhibernate#1492, which caches unique keys also for already fully loaded entities. The value hashcode must then be computed with the entity type, which accounts for proxies. Otherwise the proxy loading may get triggered while the code is already loading the proxy, which results in a stack overflow. Add test case adapted from Michael Estermann repro Fix nhibernate#1645 Co-authored-by: Michael Estermann <[email protected]>
fredericDelaporte
force-pushed
the
GH1645
branch
from
9e020e2 to
42a4a40
Compare
fredericDelaporte
changed the title
| @@ -34,7 +34,7 @@ public EntityUniqueKey(string entityName, string uniqueKeyName, object semiResol | |||
| this.entityName = entityName; | |||
| this.uniqueKeyName = uniqueKeyName; | |||
| key = semiResolvedKey; | |||
| this.keyType = keyType.GetSemiResolvedType(factory); | |||
| this.keyType = keyType; | |||
There was a problem hiding this comment.
EntityUniqueKey was assuming the value to be always semi-resolved (to be the identifier instead of the entity for an entity type). It was always computing the value hashcode with the semi-resolved type, which is the identifier type for an entity.
But this has changed with #1492, which caches unique keys also for already fully loaded entities. The value hashcode must then be computed with the entity type, which accounts for proxies.
Otherwise the proxy loading may get triggered while the code is already loading the proxy, which results in a stack overflow.
The type to use is now fully computed before constructing EntityUniqueKey.
It is constructed in only two places: in the loader, see its changes; in the EntityType.LoadByUniqueKey, which uses GetIdentifierOrUniqueKeyType which yields types having themselves as semi-resolved types. (For being extra-safe, a call to GetSemiResolvedType could still be added in LoadByUniqueKey just in case.)
Ideally semiResolvedKey parameter should be renamed simply key (no more assumptions). But renaming a public method parameter is a gray area possible breaking change (if anyone has written a call explicitly naming parameters), and I avoid doing this one in patch releases.
There was a problem hiding this comment.
For being extra-safe, a call to GetSemiResolvedType could still be added in LoadByUniqueKey just in case.
Let's do this.
Ideally semiResolvedKey parameter should be renamed simply key (no more assumptions).
Please add a TODO for 6.0
| // But since removing this call from EntityUniqueKey is done for a patch and that the code path here has | ||
| // no known bugs with this GetSemiResolvedType, moving its call here for avoiding altering this code | ||
| // path. See GH1645. | ||
| .GetSemiResolvedType(factory); |
There was a problem hiding this comment.
Based on the implementation, this actually could happen only if the primary or unique key is entity itself. Is it event possible?
There was a problem hiding this comment.
Indeed, that is the case in #1645, the unique key is an entity. (Property-ref cases.)
Fixes #1645.