Skip to content

Commit 0a207d4

Browse files
fredericDelaportefredericDelaporte
committed
Resolve collection owner correctly in case of property-ref
Root cause of NH-3480
1 parent baf8329 commit 0a207d4

File tree

15 files changed

+291
-33
lines changed

15 files changed

+291
-33
lines changed

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

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,15 @@ protected override void OnSetUp()
2626
{
2727
using (ITransaction transaction = session.BeginTransaction())
2828
{
29-
var parent1 = new Entity { Id = new Entity.Key() { Id = Guid.NewGuid() }, Name = "Bob", OtherId = 20 };
29+
var parent1 = new Entity
30+
{
31+
Id = new Entity.Key { Id = Guid.NewGuid() },
32+
Name = "Bob",
33+
OtherId = 20,
34+
YetAnotherOtherId = 21
35+
};
36+
parent1.Elements.Add(1);
37+
parent1.Elements.Add(2);
3038
session.Save(parent1);
3139

3240
var child1 = new Child { Name = "Bob1", Parent = parent1 };
@@ -72,5 +80,19 @@ public async Task TestAsync()
7280
}
7381
}
7482
}
83+
84+
[Test]
85+
public async Task TestOwnerAsync()
86+
{
87+
using (var session = OpenSession())
88+
using (var t = session.BeginTransaction())
89+
{
90+
var entity = await (session.Query<Entity>().SingleAsync(e => e.Name == "Bob"));
91+
92+
await (NHibernateUtil.InitializeAsync(entity.Elements));
93+
Assert.That(entity.Elements, Has.Count.GreaterThan(0));
94+
await (t.CommitAsync());
95+
}
96+
}
7597
}
7698
}

‎src/NHibernate.Test/NHSpecificTest/NH3480/Entity.cs

Lines changed: 81 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,10 @@
11
using System;
22
using System.Collections.Generic;
3+
using System.Data.Common;
4+
using NHibernate.Engine;
5+
using NHibernate.SqlTypes;
6+
using NHibernate.UserTypes;
7+
using NUnit.Framework;
38

49
namespace NHibernate.Test.NHSpecificTest.NH3480
510
{
@@ -13,7 +18,9 @@ public Entity()
1318
public virtual Key Id { get; set; }
1419
public virtual string Name { get; set; }
1520
public virtual int OtherId { get; set; }
21+
public virtual int YetAnotherOtherId { get; set; }
1622
public virtual ISet<Child> Children { get; set; }
23+
public virtual ISet<int> Elements { get; set; } = new HashSet<int>();
1724

1825
public override bool Equals(object obj)
1926
{
@@ -58,4 +65,77 @@ class Child
5865
public virtual string Name { get; set; }
5966
public virtual Entity Parent { get; set; }
6067
}
61-
}
68+
69+
// This user type is the Elements collection element type
70+
public class SimpleCustomType : IUserType
71+
{
72+
private static readonly SqlType[] ReturnSqlTypes = { SqlTypeFactory.Int32 };
73+
74+
#region IUserType Members
75+
76+
public new bool Equals(object x, object y)
77+
{
78+
if (ReferenceEquals(x, y))
79+
{
80+
return true;
81+
}
82+
if (x == null || y == null)
83+
{
84+
return false;
85+
}
86+
87+
return x.Equals(y);
88+
}
89+
90+
public int GetHashCode(object x)
91+
{
92+
return x?.GetHashCode() ?? 0;
93+
}
94+
95+
public SqlType[] SqlTypes => ReturnSqlTypes;
96+
97+
public object DeepCopy(object value)
98+
{
99+
return value;
100+
}
101+
102+
public void NullSafeSet(DbCommand cmd, object value, int index, ISessionImplementor session)
103+
{
104+
cmd.Parameters[index].Value = value ?? DBNull.Value;
105+
}
106+
107+
public System.Type ReturnedType => typeof(int);
108+
109+
public object NullSafeGet(DbDataReader rs, string[] names, ISessionImplementor session, object owner)
110+
{
111+
// Detect if the root cause of the bug is still there: if yes, the owner will be null.
112+
Assert.That(owner, Is.Not.Null);
113+
114+
var index = rs.GetOrdinal(names[0]);
115+
if (rs.IsDBNull(index))
116+
{
117+
return null;
118+
}
119+
return rs.GetInt32(index);
120+
}
121+
122+
public bool IsMutable => false;
123+
124+
public object Replace(object original, object target, object owner)
125+
{
126+
return original;
127+
}
128+
129+
public object Assemble(object cached, object owner)
130+
{
131+
return cached;
132+
}
133+
134+
public object Disassemble(object value)
135+
{
136+
return value;
137+
}
138+
139+
#endregion
140+
}
141+
}

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

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,15 @@ protected override void OnSetUp()
1515
{
1616
using (ITransaction transaction = session.BeginTransaction())
1717
{
18-
var parent1 = new Entity { Id = new Entity.Key() { Id = Guid.NewGuid() }, Name = "Bob", OtherId = 20 };
18+
var parent1 = new Entity
19+
{
20+
Id = new Entity.Key { Id = Guid.NewGuid() },
21+
Name = "Bob",
22+
OtherId = 20,
23+
YetAnotherOtherId = 21
24+
};
25+
parent1.Elements.Add(1);
26+
parent1.Elements.Add(2);
1927
session.Save(parent1);
2028

2129
var child1 = new Child { Name = "Bob1", Parent = parent1 };
@@ -61,5 +69,19 @@ public void Test()
6169
}
6270
}
6371
}
72+
73+
[Test]
74+
public void TestOwner()
75+
{
76+
using (var session = OpenSession())
77+
using (var t = session.BeginTransaction())
78+
{
79+
var entity = session.Query<Entity>().Single(e => e.Name == "Bob");
80+
81+
NHibernateUtil.Initialize(entity.Elements);
82+
Assert.That(entity.Elements, Has.Count.GreaterThan(0));
83+
t.Commit();
84+
}
85+
}
6486
}
6587
}

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

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,21 @@
11
<?xml version="1.0" encoding="utf-8" ?>
22
<hibernate-mapping xmlns="urn:nhibernate-mapping-2.2" assembly="NHibernate.Test" namespace="NHibernate.Test.NHSpecificTest.NH3480">
33

4-
<class name="Entity">
4+
<class name="Entity">
55
<composite-id name="Id">
66
<key-property name="Id" />
77
</composite-id>
8-
<property name="Name" not-null="true" />
8+
<property name="Name" not-null="true" />
99
<property name="OtherId" unique="true" not-null="true" />
10+
<property name="YetAnotherOtherId" unique="true" not-null="true" />
1011
<set name="Children" cascade="all-delete-orphan" inverse="true">
1112
<key property-ref="OtherId" column="Parent" />
1213
<one-to-many class="Child" />
1314
</set>
15+
<set name="Elements">
16+
<key property-ref="YetAnotherOtherId" column="Parent" />
17+
<element type="NHibernate.Test.NHSpecificTest.NH3480.SimpleCustomType, NHibernate.Test"/>
18+
</set>
1419
</class>
1520

1621
<class name="Child">
@@ -19,4 +24,4 @@
1924
<many-to-one name="Parent" property-ref="OtherId" not-null="true" />
2025
</class>
2126

22-
</hibernate-mapping>
27+
</hibernate-mapping>

‎src/NHibernate/Async/Loader/Loader.cs

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -646,9 +646,18 @@ private async Task<object[]> GetRowAsync(DbDataReader rs, ILoadable[] persisters
646646
obj =
647647
await (InstanceNotYetLoadedAsync(rs, i, persister, key, lockModes[i], optionalObjectKey,
648648
optionalObject, hydratedObjects, session, cancellationToken)).ConfigureAwait(false);
649+
650+
// IUniqueKeyLoadable.CacheByUniqueKeys caches all unique keys of the entity, regardless of
651+
// associations loaded by the query. So if the entity is already loaded, it has forcibly already
652+
// been cached too for all its unique keys, provided its persister implement it. With this new
653+
// way of caching unique keys, it is no more needed to handle caching for alreadyLoaded path
654+
// too.
655+
await ((persister as IUniqueKeyLoadable).CacheByUniqueKeysAsync(obj, session, cancellationToken)).ConfigureAwait(false);
649656
}
650-
// #1226: Even if it is already loaded, if it can be loaded from an association with a property ref, make
651-
// sure it is also cached by its unique key.
657+
// 6.0 TODO: this call is nor more needed for up-to-date persisters, remove once CacheByUniqueKeys
658+
// is merged in IUniqueKeyLoadable interface instead of being an extension method
659+
// #1226 old fix: Even if it is already loaded, if it can be loaded from an association with a property ref,
660+
// make sure it is also cached by its unique key.
652661
CacheByUniqueKey(i, persister, obj, session, alreadyLoaded);
653662
}
654663

‎src/NHibernate/Async/Persister/Entity/AbstractEntityPersister.cs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,25 @@ public Task<object> LoadByUniqueKeyAsync(string propertyName, object uniqueKey,
259259
}
260260
}
261261

262+
public async Task CacheByUniqueKeysAsync(object entity, ISessionImplementor session, CancellationToken cancellationToken)
263+
{
264+
cancellationToken.ThrowIfCancellationRequested();
265+
for (var i = 0; i < PropertySpan; i++)
266+
{
267+
if (!propertyUniqueness[i])
268+
continue;
269+
270+
// The caching is done by semi-resolved values.
271+
var propertyValue = session.PersistenceContext.GetEntry(entity).LoadedState[i];
272+
if (propertyValue == null)
273+
continue;
274+
var type = PropertyTypes[i].GetSemiResolvedType(session.Factory);
275+
propertyValue = await (type.SemiResolveAsync(propertyValue, session, entity, cancellationToken)).ConfigureAwait(false);
276+
var euk = new EntityUniqueKey(EntityName, PropertyNames[i], propertyValue, type, session.Factory);
277+
session.PersistenceContext.AddEntity(euk, entity);
278+
}
279+
}
280+
262281
protected Task<int> DehydrateAsync(object id, object[] fields, bool[] includeProperty, bool[][] includeColumns, int j, DbCommand st, ISessionImplementor session, CancellationToken cancellationToken)
263282
{
264283
if (cancellationToken.IsCancellationRequested)

‎src/NHibernate/Async/Persister/Entity/IUniqueKeyLoadable.cs

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,4 +21,38 @@ public partial interface IUniqueKeyLoadable
2121
/// </summary>
2222
Task<object> LoadByUniqueKeyAsync(string propertyName, object uniqueKey, ISessionImplementor session, CancellationToken cancellationToken);
2323
}
24-
}
24+
25+
public static partial class UniqueKeyLoadableExtensions
26+
{
27+
28+
// 6.0 TODO: merge in IUniqueKeyLoadable
29+
public static async Task CacheByUniqueKeysAsync(
30+
this IUniqueKeyLoadable ukLoadable,
31+
object entity,
32+
ISessionImplementor session, CancellationToken cancellationToken)
33+
{
34+
cancellationToken.ThrowIfCancellationRequested();
35+
if (ukLoadable is AbstractEntityPersister persister)
36+
{
37+
await (persister.CacheByUniqueKeysAsync(entity, session, cancellationToken)).ConfigureAwait(false);
38+
return;
39+
}
40+
41+
// Use reflection for supporting custom persisters.
42+
var ukLoadableType = ukLoadable.GetType();
43+
var registerMethod = ukLoadableType.GetMethod(
44+
nameof(AbstractEntityPersister.CacheByUniqueKeysAsync),
45+
new[] { typeof(object), typeof(ISessionImplementor) });
46+
if (registerMethod != null)
47+
{
48+
registerMethod.Invoke(ukLoadable, new[] { entity, session });
49+
return;
50+
}
51+
52+
Logger.Warn(
53+
"{0} does not implement 'void CacheByUniqueKeys(object, ISessionImplementor)', " +
54+
"some caching may be lacking",
55+
ukLoadableType);
56+
}
57+
}
58+
}

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

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -219,11 +219,6 @@ public async Task<object> LoadByUniqueKeyAsync(string entityName, string uniqueK
219219
//TODO: implement caching?! proxies?!
220220

221221
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.
227222
.GetSemiResolvedType(factory);
228223
EntityUniqueKey euk =
229224
new EntityUniqueKey(

‎src/NHibernate/Cfg/XmlHbmBinding/CollectionBinder.cs

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -821,13 +821,14 @@ private void BindManyToMany(HbmManyToMany manyToManyMapping, Mapping.Collection
821821
var restrictedLaziness = manyToManyMapping.lazySpecified ? manyToManyMapping.lazy : (HbmRestrictedLaziness?) null;
822822
InitLaziness(restrictedLaziness, manyToMany, true);
823823

824-
if(!string.IsNullOrEmpty(manyToManyMapping.propertyref))
824+
manyToMany.ReferencedEntityName = GetEntityName(manyToManyMapping, mappings);
825+
826+
if (!string.IsNullOrEmpty(manyToManyMapping.propertyref))
825827
{
826828
manyToMany.ReferencedPropertyName = manyToManyMapping.propertyref;
829+
mappings.AddUniquePropertyReference(manyToMany.ReferencedEntityName, manyToMany.ReferencedPropertyName);
827830
}
828831

829-
manyToMany.ReferencedEntityName = GetEntityName(manyToManyMapping, mappings);
830-
831832
manyToMany.IsIgnoreNotFound = manyToManyMapping.NotFoundMode == HbmNotFoundMode.Ignore;
832833

833834
if(!string.IsNullOrEmpty(manyToManyMapping.foreignkey))
@@ -904,6 +905,7 @@ private void BindKey(HbmKey keyMapping, Mapping.Collection model)
904905
else
905906
{
906907
keyValue = (IKeyValue) model.Owner.GetProperty(propRef).Value;
908+
mappings.AddUniquePropertyReference(model.OwnerEntityName, propRef);
907909
}
908910
var key = new DependantValue(model.CollectionTable, keyValue)
909911
{IsCascadeDeleteEnabled = keyMapping.ondelete == HbmOndelete.Cascade};

‎src/NHibernate/Engine/EntityUniqueKey.cs

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,8 @@ 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.
23+
// 6.0 TODO: rename keyType as semiResolvedKeyType. That is not the responsibility of this class to make any
24+
// assumption on the key type being semi-resolved or not, that is the responsibility of its callers.
2525
public EntityUniqueKey(string entityName, string uniqueKeyName, object semiResolvedKey, IType keyType, ISessionFactoryImplementor factory)
2626
{
2727
if (string.IsNullOrEmpty(entityName))
@@ -80,8 +80,15 @@ public override bool Equals(object obj)
8080

8181
public bool Equals(EntityUniqueKey that)
8282
{
83-
return that == null ? false :
84-
that.EntityName.Equals(entityName) && that.UniqueKeyName.Equals(uniqueKeyName) && keyType.IsEqual(that.key, key);
83+
return that != null && that.EntityName.Equals(entityName) && that.UniqueKeyName.Equals(uniqueKeyName) &&
84+
// Normally entities are cached by semi-resolved type only, but the initial fix of #1226 cause it to be
85+
// cached by type too. This may then cause issues (including Stack Overflow Exception) when this happens
86+
// with the that.keyType being an entity type while its value is an uninitialized proxy: if this.keyType
87+
// is not an entity type too, its IsEqual will trigger the proxy loading.
88+
// So we need to short-circuit on keyType inequality, at least till Loader.CacheByUniqueKey is removed.
89+
// 6.0 TODO: consider removing the keyType.Equals(that.keyType) check, see above comment.
90+
keyType.Equals(that.keyType) &&
91+
keyType.IsEqual(that.key, key);
8592
}
8693

8794
public override string ToString()

‎src/NHibernate/Engine/StatefulPersistenceContext.cs

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -782,7 +782,15 @@ public object ProxyFor(object impl)
782782
/// <summary> Get the entity that owns this persistent collection</summary>
783783
public object GetCollectionOwner(object key, ICollectionPersister collectionPersister)
784784
{
785-
return GetEntity(session.GenerateEntityKey(key, collectionPersister.OwnerEntityPersister));
785+
if (collectionPersister.CollectionType.UseLHSPrimaryKey)
786+
return GetEntity(session.GenerateEntityKey(key, collectionPersister.OwnerEntityPersister));
787+
788+
return GetEntity(
789+
new EntityUniqueKey(
790+
collectionPersister.OwnerEntityPersister.EntityName,
791+
collectionPersister.CollectionType.LHSPropertyName,
792+
key, collectionPersister.KeyType, session.Factory
793+
));
786794
}
787795

788796
/// <summary> Get the entity that owned this persistent collection when it was loaded </summary>
@@ -801,10 +809,9 @@ public virtual object GetLoadedCollectionOwnerOrNull(IPersistentCollection colle
801809
object loadedOwner = null;
802810
// TODO: an alternative is to check if the owner has changed; if it hasn't then
803811
// return collection.getOwner()
804-
object entityId = GetLoadedCollectionOwnerIdOrNull(ce);
805-
if (entityId != null)
812+
if (ce.LoadedKey != null)
806813
{
807-
loadedOwner = GetCollectionOwner(entityId, ce.LoadedPersister);
814+
loadedOwner = GetCollectionOwner(ce.LoadedKey, ce.LoadedPersister);
808815
}
809816
return loadedOwner;
810817
}

0 commit comments

Comments
 (0)