Skip to content

Commit 381f1dd

Browse files
fredericDelaportebbv-michaelestermann
fredericDelaporte
and
bbv-michaelestermann
authored
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]>
1 parent 7e54b12 commit 381f1dd

File tree

10 files changed

+240
-5
lines changed

10 files changed

+240
-5
lines changed

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

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
//------------------------------------------------------------------------------
2+
// <auto-generated>
3+
// This code was generated by AsyncGenerator.
4+
//
5+
// Changes to this file may cause incorrect behavior and will be lost if
6+
// the code is regenerated.
7+
// </auto-generated>
8+
//------------------------------------------------------------------------------
9+
10+
11+
using System;
12+
using NUnit.Framework;
13+
14+
namespace NHibernate.Test.NHSpecificTest.GH1645
15+
{
16+
using System.Threading.Tasks;
17+
[TestFixture]
18+
public class FixtureAsync : BugTestCase
19+
{
20+
private Guid _superParentId;
21+
private Guid _parentId;
22+
23+
protected override void OnSetUp()
24+
{
25+
using (var session = OpenSession())
26+
using (var transaction = session.BeginTransaction())
27+
{
28+
var p = new Parent();
29+
session.Save(p);
30+
_parentId = p.Id;
31+
32+
_superParentId = (Guid) session.Save(new SuperParent { Parent = p });
33+
34+
transaction.Commit();
35+
}
36+
}
37+
38+
protected override void OnTearDown()
39+
{
40+
using (var session = OpenSession())
41+
using (var transaction = session.BeginTransaction())
42+
{
43+
// The HQL delete does all the job inside the database without loading the entities, but it does
44+
// not handle delete order for avoiding violating constraints if any. Use
45+
// session.Delete("from System.Object");
46+
// instead if in need of having NHbernate ordering the deletes, but this will cause
47+
// loading the entities in the session.
48+
session.CreateQuery("delete from System.Object").ExecuteUpdate();
49+
50+
transaction.Commit();
51+
}
52+
}
53+
54+
[Test]
55+
public async Task SOEOnLoadAsync()
56+
{
57+
using (var session = OpenSession())
58+
using (session.BeginTransaction())
59+
{
60+
var superParent = await (session.LoadAsync<SuperParent>(_superParentId));
61+
Assert.That(() => NHibernateUtil.InitializeAsync(superParent), Throws.Nothing);
62+
Assert.That(() => NHibernateUtil.InitializeAsync(superParent.Parent), Throws.Nothing);
63+
}
64+
}
65+
}
66+
}

‎src/NHibernate.Test/NHSpecificTest/GH1645/EntityBase.cs‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
using System;
2+
3+
namespace NHibernate.Test.NHSpecificTest.GH1645
4+
{
5+
public abstract class EntityBase
6+
{
7+
public virtual Guid Id { get; protected set; }
8+
9+
public static bool operator ==(EntityBase left, EntityBase right)
10+
{
11+
return Equals(left, right);
12+
}
13+
14+
public static bool operator !=(EntityBase left, EntityBase right)
15+
{
16+
return !Equals(left, right);
17+
}
18+
19+
public override bool Equals(object obj)
20+
{
21+
return Equals(obj as EntityBase);
22+
}
23+
24+
public virtual bool Equals(EntityBase other)
25+
{
26+
if (other == null)
27+
{
28+
return false;
29+
}
30+
31+
if (ReferenceEquals(this, other))
32+
{
33+
return true;
34+
}
35+
36+
if (!IsTransient(this) && !IsTransient(other) && Equals(Id, other.Id))
37+
{
38+
var otherType = other.GetUnproxiedType();
39+
var thisType = GetUnproxiedType();
40+
return thisType.IsAssignableFrom(otherType) ||
41+
otherType.IsAssignableFrom(thisType);
42+
}
43+
44+
return false;
45+
}
46+
47+
public override int GetHashCode()
48+
{
49+
if (Equals(Id, default(Guid)))
50+
{
51+
return base.GetHashCode();
52+
}
53+
54+
return Id.GetHashCode();
55+
}
56+
57+
private static bool IsTransient(EntityBase obj)
58+
{
59+
return obj != null && Equals(obj.Id, default(Guid));
60+
}
61+
62+
private System.Type GetUnproxiedType()
63+
{
64+
return GetType();
65+
}
66+
}
67+
}

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

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
using System;
2+
using NUnit.Framework;
3+
4+
namespace NHibernate.Test.NHSpecificTest.GH1645
5+
{
6+
[TestFixture]
7+
public class Fixture : BugTestCase
8+
{
9+
private Guid _superParentId;
10+
private Guid _parentId;
11+
12+
protected override void OnSetUp()
13+
{
14+
using (var session = OpenSession())
15+
using (var transaction = session.BeginTransaction())
16+
{
17+
var p = new Parent();
18+
session.Save(p);
19+
_parentId = p.Id;
20+
21+
_superParentId = (Guid) session.Save(new SuperParent { Parent = p });
22+
23+
transaction.Commit();
24+
}
25+
}
26+
27+
protected override void OnTearDown()
28+
{
29+
using (var session = OpenSession())
30+
using (var transaction = session.BeginTransaction())
31+
{
32+
// The HQL delete does all the job inside the database without loading the entities, but it does
33+
// not handle delete order for avoiding violating constraints if any. Use
34+
// session.Delete("from System.Object");
35+
// instead if in need of having NHbernate ordering the deletes, but this will cause
36+
// loading the entities in the session.
37+
session.CreateQuery("delete from System.Object").ExecuteUpdate();
38+
39+
transaction.Commit();
40+
}
41+
}
42+
43+
[Test]
44+
public void SOEOnLoad()
45+
{
46+
using (var session = OpenSession())
47+
using (session.BeginTransaction())
48+
{
49+
var superParent = session.Load<SuperParent>(_superParentId);
50+
Assert.That(() => NHibernateUtil.Initialize(superParent), Throws.Nothing);
51+
Assert.That(() => NHibernateUtil.Initialize(superParent.Parent), Throws.Nothing);
52+
}
53+
}
54+
}
55+
}

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
<?xml version="1.0" encoding="utf-8" ?>
2+
<hibernate-mapping xmlns="urn:nhibernate-mapping-2.2" assembly="NHibernate.Test"
3+
namespace="NHibernate.Test.NHSpecificTest.GH1645">
4+
5+
<class name="SuperParent">
6+
<id name="Id" generator="guid.comb"/>
7+
<many-to-one name="Parent"/>
8+
</class>
9+
10+
<class name="Parent">
11+
<id name="Id" generator="guid.comb"/>
12+
<one-to-one name="SuperParent" property-ref="Parent"/>
13+
</class>
14+
15+
</hibernate-mapping>

‎src/NHibernate.Test/NHSpecificTest/GH1645/Parent.cs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
namespace NHibernate.Test.NHSpecificTest.GH1645
2+
{
3+
public class Parent : EntityBase
4+
{
5+
public virtual SuperParent SuperParent { get; set; }
6+
}
7+
}

‎src/NHibernate.Test/NHSpecificTest/GH1645/SuperParent.cs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
namespace NHibernate.Test.NHSpecificTest.GH1645
2+
{
3+
public class SuperParent : EntityBase
4+
{
5+
public virtual Parent Parent { get; set; }
6+
}
7+
}

‎src/NHibernate/Async/Type/EntityType.cs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -218,12 +218,19 @@ public async Task<object> LoadByUniqueKeyAsync(string entityName, string uniqueK
218218

219219
//TODO: implement caching?! proxies?!
220220

221+
var keyType = GetIdentifierOrUniqueKeyType(factory)
222+
// EntityUniqueKey was doing this on the type. I suspect this was needed only for its usage in Loader,
223+
// which can work with entities as keys not yet instanciated and just represented by their identifiers.
224+
// But since removing this call from EntityUniqueKey is done for a patch and that the code path here has
225+
// no known bugs with this GetSemiResolvedType, moving its call here for avoiding altering this code
226+
// path. See GH1645.
227+
.GetSemiResolvedType(factory);
221228
EntityUniqueKey euk =
222229
new EntityUniqueKey(
223230
entityName,
224231
uniqueKeyPropertyName,
225232
key,
226-
GetIdentifierOrUniqueKeyType(factory),
233+
keyType,
227234
session.Factory);
228235

229236
IPersistenceContext persistenceContext = session.PersistenceContext;

‎src/NHibernate/Engine/EntityUniqueKey.cs‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,14 @@ public class EntityUniqueKey
2020
private readonly IType keyType;
2121
private readonly int hashCode;
2222

23+
// 6.0 TODO: rename semiResolvedKey as simply key. That is not the responsibility of this class to make any
24+
// assumption on the key being semi-resolved or not, that is the responsibility of its callers.
2325
public EntityUniqueKey(string entityName, string uniqueKeyName, object semiResolvedKey, IType keyType, ISessionFactoryImplementor factory)
2426
{
2527
if (string.IsNullOrEmpty(entityName))
2628
throw new ArgumentNullException("entityName");
2729
if (string.IsNullOrEmpty(uniqueKeyName))
28-
throw new ArgumentNullException("entityName");
30+
throw new ArgumentNullException("uniqueKeyName");
2931
if (semiResolvedKey == null)
3032
throw new ArgumentNullException("semiResolvedKey");
3133
if (keyType == null)
@@ -34,7 +36,7 @@ public EntityUniqueKey(string entityName, string uniqueKeyName, object semiResol
3436
this.entityName = entityName;
3537
this.uniqueKeyName = uniqueKeyName;
3638
key = semiResolvedKey;
37-
this.keyType = keyType.GetSemiResolvedType(factory);
39+
this.keyType = keyType;
3840
hashCode = GenerateHashCode(factory);
3941
}
4042

@@ -88,4 +90,4 @@ public override string ToString()
8890
}
8991

9092
}
91-
}
93+
}

‎src/NHibernate/Loader/Loader.cs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1014,6 +1014,8 @@ private void CacheByUniqueKey(int i, IEntityPersister persister, object obj, ISe
10141014
if (ukValue == null)
10151015
return;
10161016
var type = persister.PropertyTypes[index];
1017+
if (!alreadyLoaded)
1018+
type = type.GetSemiResolvedType(session.Factory);
10171019
var euk = new EntityUniqueKey(persister.EntityName, ukName, ukValue, type, session.Factory);
10181020
session.PersistenceContext.AddEntity(euk, obj);
10191021
}

‎src/NHibernate/Type/EntityType.cs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -508,12 +508,19 @@ public object LoadByUniqueKey(string entityName, string uniqueKeyPropertyName, o
508508

509509
//TODO: implement caching?! proxies?!
510510

511+
var keyType = GetIdentifierOrUniqueKeyType(factory)
512+
// EntityUniqueKey was doing this on the type. I suspect this was needed only for its usage in Loader,
513+
// which can work with entities as keys not yet instanciated and just represented by their identifiers.
514+
// But since removing this call from EntityUniqueKey is done for a patch and that the code path here has
515+
// no known bugs with this GetSemiResolvedType, moving its call here for avoiding altering this code
516+
// path. See GH1645.
517+
.GetSemiResolvedType(factory);
511518
EntityUniqueKey euk =
512519
new EntityUniqueKey(
513520
entityName,
514521
uniqueKeyPropertyName,
515522
key,
516-
GetIdentifierOrUniqueKeyType(factory),
523+
keyType,
517524
session.Factory);
518525

519526
IPersistenceContext persistenceContext = session.PersistenceContext;

0 commit comments

Comments
 (0)