Fix one-to-one with property-ref stack overflow (#1647)

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 <[email protected]>

Author: fredericDelaporte

src/NHibernate.Test/Async/NHSpecificTest/GH1645/Fixture.cs

@@ -0,0 +1,66 @@
+//------------------------------------------------------------------------------
+// <auto-generated>
+//     This code was generated by AsyncGenerator.
+//
+//     Changes to this file may cause incorrect behavior and will be lost if
+//     the code is regenerated.
+// </auto-generated>
+//------------------------------------------------------------------------------
+
+
+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<SuperParent>(_superParentId));
+				Assert.That(() => NHibernateUtil.InitializeAsync(superParent), Throws.Nothing);
+				Assert.That(() => NHibernateUtil.InitializeAsync(superParent.Parent), Throws.Nothing);
+			}
+		}
+	}
+}

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();
+		}
+	}
+}

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<SuperParent>(_superParentId);
+				Assert.That(() => NHibernateUtil.Initialize(superParent), Throws.Nothing);
+				Assert.That(() => NHibernateUtil.Initialize(superParent.Parent), Throws.Nothing);
+			}
+		}
+	}
+}

src/NHibernate.Test/NHSpecificTest/GH1645/Mappings.hbm.xml

@@ -0,0 +1,15 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<hibernate-mapping xmlns="urn:nhibernate-mapping-2.2" assembly="NHibernate.Test"
+                   namespace="NHibernate.Test.NHSpecificTest.GH1645">
+
+  <class name="SuperParent">
+    <id name="Id" generator="guid.comb"/>
+    <many-to-one name="Parent"/>
+  </class>
+
+  <class name="Parent">
+    <id name="Id" generator="guid.comb"/>
+    <one-to-one name="SuperParent" property-ref="Parent"/>
+  </class>
+
+</hibernate-mapping>

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; }
+	}
+}

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; }
+    }
+}

src/NHibernate/Async/Type/EntityType.cs

@@ -218,12 +218,19 @@ public async Task<object> 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;

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)
@@ -34,7 +36,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 +90,4 @@ public override string ToString()
 		}
 
 	}
-}
+}

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);
 		}

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;