From 42a4a4097a8488530a2d8ddadde729a3b3368c25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= Date: Mon, 9 Apr 2018 13:23:10 +0200 Subject: [PATCH 1/2] Fix one-to-one with property-ref stack overflow 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 #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 #1645 Co-authored-by: Michael Estermann --- .../Async/NHSpecificTest/GH1645/Fixture.cs | 66 ++++++++++++++++++ .../NHSpecificTest/GH1645/EntityBase.cs | 67 +++++++++++++++++++ .../NHSpecificTest/GH1645/Fixture.cs | 55 +++++++++++++++ .../NHSpecificTest/GH1645/Mappings.hbm.xml | 15 +++++ .../NHSpecificTest/GH1645/Parent.cs | 7 ++ .../NHSpecificTest/GH1645/SuperParent.cs | 7 ++ src/NHibernate/Engine/EntityUniqueKey.cs | 4 +- src/NHibernate/Loader/Loader.cs | 2 + 8 files changed, 221 insertions(+), 2 deletions(-) create mode 100644 src/NHibernate.Test/Async/NHSpecificTest/GH1645/Fixture.cs create mode 100644 src/NHibernate.Test/NHSpecificTest/GH1645/EntityBase.cs create mode 100644 src/NHibernate.Test/NHSpecificTest/GH1645/Fixture.cs create mode 100644 src/NHibernate.Test/NHSpecificTest/GH1645/Mappings.hbm.xml create mode 100644 src/NHibernate.Test/NHSpecificTest/GH1645/Parent.cs create mode 100644 src/NHibernate.Test/NHSpecificTest/GH1645/SuperParent.cs diff --git a/src/NHibernate.Test/Async/NHSpecificTest/GH1645/Fixture.cs b/src/NHibernate.Test/Async/NHSpecificTest/GH1645/Fixture.cs new file mode 100644 index 00000000000..017658be42a --- /dev/null +++ b/src/NHibernate.Test/Async/NHSpecificTest/GH1645/Fixture.cs @@ -0,0 +1,66 @@ +//------------------------------------------------------------------------------ +// +// This code was generated by AsyncGenerator. +// +// Changes to this file may cause incorrect behavior and will be lost if +// the code is regenerated. +// +//------------------------------------------------------------------------------ + + +using System; +using NUnit.Framework; + +namespace NHibernate.Test.NHSpecificTest.GH1645 +{ + using System.Threading.Tasks; + [TestFixture] + public class FixtureAsync : BugTestCase + { + private Guid _superParentId; + private Guid _parentId; + + protected override void OnSetUp() + { + using (var session = OpenSession()) + using (var transaction = session.BeginTransaction()) + { + var p = new Parent(); + session.Save(p); + _parentId = p.Id; + + _superParentId = (Guid) session.Save(new SuperParent { Parent = p }); + + transaction.Commit(); + } + } + + protected override void OnTearDown() + { + using (var session = OpenSession()) + using (var transaction = session.BeginTransaction()) + { + // The HQL delete does all the job inside the database without loading the entities, but it does + // not handle delete order for avoiding violating constraints if any. Use + // session.Delete("from System.Object"); + // instead if in need of having NHbernate ordering the deletes, but this will cause + // loading the entities in the session. + session.CreateQuery("delete from System.Object").ExecuteUpdate(); + + transaction.Commit(); + } + } + + [Test] + public async Task SOEOnLoadAsync() + { + using (var session = OpenSession()) + using (session.BeginTransaction()) + { + var superParent = await (session.LoadAsync(_superParentId)); + Assert.That(() => NHibernateUtil.InitializeAsync(superParent), Throws.Nothing); + Assert.That(() => NHibernateUtil.InitializeAsync(superParent.Parent), Throws.Nothing); + } + } + } +} diff --git a/src/NHibernate.Test/NHSpecificTest/GH1645/EntityBase.cs b/src/NHibernate.Test/NHSpecificTest/GH1645/EntityBase.cs new file mode 100644 index 00000000000..a855e6b58de --- /dev/null +++ b/src/NHibernate.Test/NHSpecificTest/GH1645/EntityBase.cs @@ -0,0 +1,67 @@ +using System; + +namespace NHibernate.Test.NHSpecificTest.GH1645 +{ + public abstract class EntityBase + { + public virtual Guid Id { get; protected set; } + + public static bool operator ==(EntityBase left, EntityBase right) + { + return Equals(left, right); + } + + public static bool operator !=(EntityBase left, EntityBase right) + { + return !Equals(left, right); + } + + public override bool Equals(object obj) + { + return Equals(obj as EntityBase); + } + + public virtual bool Equals(EntityBase other) + { + if (other == null) + { + return false; + } + + if (ReferenceEquals(this, other)) + { + return true; + } + + if (!IsTransient(this) && !IsTransient(other) && Equals(Id, other.Id)) + { + var otherType = other.GetUnproxiedType(); + var thisType = GetUnproxiedType(); + return thisType.IsAssignableFrom(otherType) || + otherType.IsAssignableFrom(thisType); + } + + return false; + } + + public override int GetHashCode() + { + if (Equals(Id, default(Guid))) + { + return base.GetHashCode(); + } + + return Id.GetHashCode(); + } + + private static bool IsTransient(EntityBase obj) + { + return obj != null && Equals(obj.Id, default(Guid)); + } + + private System.Type GetUnproxiedType() + { + return GetType(); + } + } +} diff --git a/src/NHibernate.Test/NHSpecificTest/GH1645/Fixture.cs b/src/NHibernate.Test/NHSpecificTest/GH1645/Fixture.cs new file mode 100644 index 00000000000..64a27ef63d6 --- /dev/null +++ b/src/NHibernate.Test/NHSpecificTest/GH1645/Fixture.cs @@ -0,0 +1,55 @@ +using System; +using NUnit.Framework; + +namespace NHibernate.Test.NHSpecificTest.GH1645 +{ + [TestFixture] + public class Fixture : BugTestCase + { + private Guid _superParentId; + private Guid _parentId; + + protected override void OnSetUp() + { + using (var session = OpenSession()) + using (var transaction = session.BeginTransaction()) + { + var p = new Parent(); + session.Save(p); + _parentId = p.Id; + + _superParentId = (Guid) session.Save(new SuperParent { Parent = p }); + + transaction.Commit(); + } + } + + protected override void OnTearDown() + { + using (var session = OpenSession()) + using (var transaction = session.BeginTransaction()) + { + // The HQL delete does all the job inside the database without loading the entities, but it does + // not handle delete order for avoiding violating constraints if any. Use + // session.Delete("from System.Object"); + // instead if in need of having NHbernate ordering the deletes, but this will cause + // loading the entities in the session. + session.CreateQuery("delete from System.Object").ExecuteUpdate(); + + transaction.Commit(); + } + } + + [Test] + public void SOEOnLoad() + { + using (var session = OpenSession()) + using (session.BeginTransaction()) + { + var superParent = session.Load(_superParentId); + Assert.That(() => NHibernateUtil.Initialize(superParent), Throws.Nothing); + Assert.That(() => NHibernateUtil.Initialize(superParent.Parent), Throws.Nothing); + } + } + } +} diff --git a/src/NHibernate.Test/NHSpecificTest/GH1645/Mappings.hbm.xml b/src/NHibernate.Test/NHSpecificTest/GH1645/Mappings.hbm.xml new file mode 100644 index 00000000000..83fc1ab6f10 --- /dev/null +++ b/src/NHibernate.Test/NHSpecificTest/GH1645/Mappings.hbm.xml @@ -0,0 +1,15 @@ + + + + + + + + + + + + + + diff --git a/src/NHibernate.Test/NHSpecificTest/GH1645/Parent.cs b/src/NHibernate.Test/NHSpecificTest/GH1645/Parent.cs new file mode 100644 index 00000000000..c9dc2342d28 --- /dev/null +++ b/src/NHibernate.Test/NHSpecificTest/GH1645/Parent.cs @@ -0,0 +1,7 @@ +namespace NHibernate.Test.NHSpecificTest.GH1645 +{ + public class Parent : EntityBase + { + public virtual SuperParent SuperParent { get; set; } + } +} diff --git a/src/NHibernate.Test/NHSpecificTest/GH1645/SuperParent.cs b/src/NHibernate.Test/NHSpecificTest/GH1645/SuperParent.cs new file mode 100644 index 00000000000..ceeb4651785 --- /dev/null +++ b/src/NHibernate.Test/NHSpecificTest/GH1645/SuperParent.cs @@ -0,0 +1,7 @@ +namespace NHibernate.Test.NHSpecificTest.GH1645 +{ + public class SuperParent : EntityBase + { + public virtual Parent Parent { get; set; } + } +} diff --git a/src/NHibernate/Engine/EntityUniqueKey.cs b/src/NHibernate/Engine/EntityUniqueKey.cs index 8646295e02d..285e3a8fd13 100644 --- a/src/NHibernate/Engine/EntityUniqueKey.cs +++ b/src/NHibernate/Engine/EntityUniqueKey.cs @@ -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; hashCode = GenerateHashCode(factory); } @@ -88,4 +88,4 @@ public override string ToString() } } -} \ No newline at end of file +} diff --git a/src/NHibernate/Loader/Loader.cs b/src/NHibernate/Loader/Loader.cs index fb069e6eb98..bfe3cc92346 100644 --- a/src/NHibernate/Loader/Loader.cs +++ b/src/NHibernate/Loader/Loader.cs @@ -1014,6 +1014,8 @@ private void CacheByUniqueKey(int i, IEntityPersister persister, object obj, ISe if (ukValue == null) return; var type = persister.PropertyTypes[index]; + if (!alreadyLoaded) + type = type.GetSemiResolvedType(session.Factory); var euk = new EntityUniqueKey(persister.EntityName, ukName, ukValue, type, session.Factory); session.PersistenceContext.AddEntity(euk, obj); } From 630910b305442f70c2b2e08ee782606e6a7c5666 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= Date: Tue, 10 Apr 2018 00:50:33 +0200 Subject: [PATCH 2/2] Adjust code according to review To be squashed. --- src/NHibernate/Async/Type/EntityType.cs | 9 ++++++++- src/NHibernate/Engine/EntityUniqueKey.cs | 4 +++- src/NHibernate/Type/EntityType.cs | 9 ++++++++- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/src/NHibernate/Async/Type/EntityType.cs b/src/NHibernate/Async/Type/EntityType.cs index 6e0e345b1bc..adef950ecdd 100644 --- a/src/NHibernate/Async/Type/EntityType.cs +++ b/src/NHibernate/Async/Type/EntityType.cs @@ -218,12 +218,19 @@ public async Task LoadByUniqueKeyAsync(string entityName, string uniqueK //TODO: implement caching?! proxies?! + var keyType = GetIdentifierOrUniqueKeyType(factory) + // EntityUniqueKey was doing this on the type. I suspect this was needed only for its usage in Loader, + // which can work with entities as keys not yet instanciated and just represented by their identifiers. + // 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); EntityUniqueKey euk = new EntityUniqueKey( entityName, uniqueKeyPropertyName, key, - GetIdentifierOrUniqueKeyType(factory), + keyType, session.Factory); IPersistenceContext persistenceContext = session.PersistenceContext; diff --git a/src/NHibernate/Engine/EntityUniqueKey.cs b/src/NHibernate/Engine/EntityUniqueKey.cs index 285e3a8fd13..9affd5a664e 100644 --- a/src/NHibernate/Engine/EntityUniqueKey.cs +++ b/src/NHibernate/Engine/EntityUniqueKey.cs @@ -20,12 +20,14 @@ public class EntityUniqueKey private readonly IType keyType; private readonly int hashCode; + // 6.0 TODO: rename semiResolvedKey as simply key. That is not the responsibility of this class to make any + // assumption on the key being semi-resolved or not, that is the responsibility of its callers. public EntityUniqueKey(string entityName, string uniqueKeyName, object semiResolvedKey, IType keyType, ISessionFactoryImplementor factory) { if (string.IsNullOrEmpty(entityName)) throw new ArgumentNullException("entityName"); if (string.IsNullOrEmpty(uniqueKeyName)) - throw new ArgumentNullException("entityName"); + throw new ArgumentNullException("uniqueKeyName"); if (semiResolvedKey == null) throw new ArgumentNullException("semiResolvedKey"); if (keyType == null) diff --git a/src/NHibernate/Type/EntityType.cs b/src/NHibernate/Type/EntityType.cs index 5b1e94f1307..fd603e2acb4 100644 --- a/src/NHibernate/Type/EntityType.cs +++ b/src/NHibernate/Type/EntityType.cs @@ -508,12 +508,19 @@ public object LoadByUniqueKey(string entityName, string uniqueKeyPropertyName, o //TODO: implement caching?! proxies?! + var keyType = GetIdentifierOrUniqueKeyType(factory) + // EntityUniqueKey was doing this on the type. I suspect this was needed only for its usage in Loader, + // which can work with entities as keys not yet instanciated and just represented by their identifiers. + // 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); EntityUniqueKey euk = new EntityUniqueKey( entityName, uniqueKeyPropertyName, key, - GetIdentifierOrUniqueKeyType(factory), + keyType, session.Factory); IPersistenceContext persistenceContext = session.PersistenceContext;