Skip to content

Commit 0309504

Browse files
fredericDelaportefredericDelaporte
authored
Fix extra-lazy collection forgetting changes (#1878)
Fixes #1874
1 parent baee90e commit 0309504

File tree

10 files changed

+212
-69
lines changed

10 files changed

+212
-69
lines changed

‎src/NHibernate.Test/Async/Extralazy/ExtraLazyFixture.cs

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
using System.Collections;
1212
using System.Collections.Generic;
13+
using System.Linq;
1314
using NUnit.Framework;
1415

1516
namespace NHibernate.Test.Extralazy
@@ -33,6 +34,16 @@ protected override string CacheConcurrencyStrategy
3334
get { return null; }
3435
}
3536

37+
protected override void OnTearDown()
38+
{
39+
using (var s = OpenSession())
40+
using (var t = s.BeginTransaction())
41+
{
42+
s.Delete("from System.Object");
43+
t.Commit();
44+
}
45+
}
46+
3647
[Test]
3748
public async Task ExtraLazyWithWhereClauseAsync()
3849
{
@@ -94,6 +105,10 @@ public async Task OrphanDeleteAsync()
94105
gavin = await (s.GetAsync<User>("gavin"));
95106
Assert.AreEqual(2, gavin.Documents.Count);
96107
gavin.Documents.Remove(hia2);
108+
// Do not convert Documents.Contains to Does.Contain/Does.Not.Contain: NUnit constraints will trigger
109+
// initialization of the collection. Moreover, with extra-lazy, collection.Contains works even if
110+
// the entity does not properly overrides Equals and GetHashCode and a detached instance is used,
111+
// provided the collection is not yet initialized, which is the case here.
97112
Assert.IsFalse(gavin.Documents.Contains(hia2));
98113
Assert.IsTrue(gavin.Documents.Contains(hia));
99114
Assert.AreEqual(1, gavin.Documents.Count);
@@ -117,6 +132,47 @@ public async Task OrphanDeleteAsync()
117132
}
118133
}
119134

135+
[Test]
136+
public async Task OrphanDeleteWithEnumerationAsync()
137+
{
138+
using (var s = OpenSession())
139+
using (var t = s.BeginTransaction())
140+
{
141+
var gavin = new User("gavin", "secret");
142+
gavin.Documents.Add(new Document("HiA", "blah blah blah", gavin));
143+
gavin.Documents.Add(new Document("HiA2", "blah blah blah blah", gavin));
144+
await (s.PersistAsync(gavin));
145+
await (t.CommitAsync());
146+
}
147+
148+
using (var s = OpenSession())
149+
using (var t = s.BeginTransaction())
150+
{
151+
var hia2 = await (s.GetAsync<Document>("HiA2"));
152+
var hia = await (s.GetAsync<Document>("HiA"));
153+
var gavin = await (s.GetAsync<User>("gavin"));
154+
Assert.That(gavin.Documents, Has.Count.EqualTo(2));
155+
Assert.That(NHibernateUtil.IsInitialized(gavin.Documents), Is.False, "Expecting non initialized collection");
156+
gavin.Documents.Remove(hia2);
157+
// Force an enumeration
158+
using (var e = gavin.Documents.GetEnumerator())
159+
e.MoveNext();
160+
Assert.That(NHibernateUtil.IsInitialized(gavin.Documents), Is.True, "Expecting initialized collection");
161+
Assert.That(gavin.Documents, Does.Not.Contain(hia2).And.Contain(hia).And.Count.EqualTo(1));
162+
await (t.CommitAsync());
163+
}
164+
165+
using (var s = OpenSession())
166+
using (var t = s.BeginTransaction())
167+
{
168+
var hia = await (s.GetAsync<Document>("HiA"));
169+
var gavin = await (s.GetAsync<User>("gavin"));
170+
Assert.That(gavin.Documents, Has.Count.EqualTo(1).And.Contain(hia));
171+
Assert.That(await (s.GetAsync<Document>("HiA2")), Is.Null);
172+
await (t.CommitAsync());
173+
}
174+
}
175+
120176
[Test]
121177
public async Task GetAsync()
122178
{
@@ -340,4 +396,4 @@ public async Task AddToUninitializedSetWithLaterLazyLoadAsync()
340396
}
341397
}
342398
}
343-
}
399+
}

‎src/NHibernate.Test/Extralazy/ExtraLazyFixture.cs

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
using System.Collections;
22
using System.Collections.Generic;
3+
using System.Linq;
34
using NUnit.Framework;
45

56
namespace NHibernate.Test.Extralazy
@@ -22,6 +23,16 @@ protected override string CacheConcurrencyStrategy
2223
get { return null; }
2324
}
2425

26+
protected override void OnTearDown()
27+
{
28+
using (var s = OpenSession())
29+
using (var t = s.BeginTransaction())
30+
{
31+
s.Delete("from System.Object");
32+
t.Commit();
33+
}
34+
}
35+
2536
[Test]
2637
public void ExtraLazyWithWhereClause()
2738
{
@@ -83,6 +94,10 @@ public void OrphanDelete()
8394
gavin = s.Get<User>("gavin");
8495
Assert.AreEqual(2, gavin.Documents.Count);
8596
gavin.Documents.Remove(hia2);
97+
// Do not convert Documents.Contains to Does.Contain/Does.Not.Contain: NUnit constraints will trigger
98+
// initialization of the collection. Moreover, with extra-lazy, collection.Contains works even if
99+
// the entity does not properly overrides Equals and GetHashCode and a detached instance is used,
100+
// provided the collection is not yet initialized, which is the case here.
86101
Assert.IsFalse(gavin.Documents.Contains(hia2));
87102
Assert.IsTrue(gavin.Documents.Contains(hia));
88103
Assert.AreEqual(1, gavin.Documents.Count);
@@ -106,6 +121,47 @@ public void OrphanDelete()
106121
}
107122
}
108123

124+
[Test]
125+
public void OrphanDeleteWithEnumeration()
126+
{
127+
using (var s = OpenSession())
128+
using (var t = s.BeginTransaction())
129+
{
130+
var gavin = new User("gavin", "secret");
131+
gavin.Documents.Add(new Document("HiA", "blah blah blah", gavin));
132+
gavin.Documents.Add(new Document("HiA2", "blah blah blah blah", gavin));
133+
s.Persist(gavin);
134+
t.Commit();
135+
}
136+
137+
using (var s = OpenSession())
138+
using (var t = s.BeginTransaction())
139+
{
140+
var hia2 = s.Get<Document>("HiA2");
141+
var hia = s.Get<Document>("HiA");
142+
var gavin = s.Get<User>("gavin");
143+
Assert.That(gavin.Documents, Has.Count.EqualTo(2));
144+
Assert.That(NHibernateUtil.IsInitialized(gavin.Documents), Is.False, "Expecting non initialized collection");
145+
gavin.Documents.Remove(hia2);
146+
// Force an enumeration
147+
using (var e = gavin.Documents.GetEnumerator())
148+
e.MoveNext();
149+
Assert.That(NHibernateUtil.IsInitialized(gavin.Documents), Is.True, "Expecting initialized collection");
150+
Assert.That(gavin.Documents, Does.Not.Contain(hia2).And.Contain(hia).And.Count.EqualTo(1));
151+
t.Commit();
152+
}
153+
154+
using (var s = OpenSession())
155+
using (var t = s.BeginTransaction())
156+
{
157+
var hia = s.Get<Document>("HiA");
158+
var gavin = s.Get<User>("gavin");
159+
Assert.That(gavin.Documents, Has.Count.EqualTo(1).And.Contain(hia));
160+
Assert.That(s.Get<Document>("HiA2"), Is.Null);
161+
t.Commit();
162+
}
163+
}
164+
109165
[Test]
110166
public void Get()
111167
{
@@ -329,4 +385,4 @@ public void AddToUninitializedSetWithLaterLazyLoad()
329385
}
330386
}
331387
}
332-
}
388+
}

‎src/NHibernate/Async/Collection/IPersistentCollection.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
//------------------------------------------------------------------------------
99

1010

11+
using System;
1112
using System.Collections;
1213
using System.Data.Common;
1314
using NHibernate.Collection.Generic;

‎src/NHibernate/Async/Engine/Loading/CollectionLoadContext.cs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ private async Task EndLoadingCollectionAsync(LoadingCollectionEntry lce, ICollec
144144
stopWath.Start();
145145
}
146146

147-
bool hasNoQueuedAdds = lce.Collection.EndRead(persister); // warning: can cause a recursive calls! (proxy initialization)
147+
bool hasNoQueuedOperations = lce.Collection.EndRead(persister); // warning: can cause a recursive calls! (proxy initialization)
148148

149149
if (persister.CollectionType.HasHolder())
150150
{
@@ -161,14 +161,17 @@ private async Task EndLoadingCollectionAsync(LoadingCollectionEntry lce, ICollec
161161
ce.PostInitialize(lce.Collection, persistenceContext);
162162
}
163163

164-
bool addToCache = hasNoQueuedAdds && persister.HasCache &&
164+
bool addToCache = hasNoQueuedOperations && persister.HasCache &&
165165
session.CacheMode.HasFlag(CacheMode.Put) && !ce.IsDoremove; // and this is not a forced initialization during flush
166166

167167
if (addToCache)
168168
{
169169
await (AddCollectionToCacheAsync(lce, persister, cacheBatchingHandler, cancellationToken)).ConfigureAwait(false);
170170
}
171171

172+
if (!hasNoQueuedOperations)
173+
lce.Collection.ApplyQueuedOperations();
174+
172175
if (log.IsDebugEnabled())
173176
{
174177
log.Debug("collection fully initialized: {0}", MessageHelper.CollectionInfoString(persister, lce.Collection, lce.Key, session));

‎src/NHibernate/Async/Event/Default/DefaultInitializeCollectionEventListener.cs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,9 @@ private async Task<bool> AssembleAsync(CacheKey ck, object ce, ICollectionPersis
161161
await (cacheEntry.AssembleAsync(collection, persister, persistenceContext.GetCollectionOwner(id, persister), cancellationToken)).ConfigureAwait(false);
162162

163163
persistenceContext.GetCollectionEntry(collection).PostInitialize(collection, persistenceContext);
164+
165+
if (collection.HasQueuedOperations)
166+
collection.ApplyQueuedOperations();
164167
return true;
165168
}
166169
}

‎src/NHibernate/Collection/AbstractPersistentCollection.cs

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -379,10 +379,12 @@ protected virtual void QueueOperation(IDelayedOperation element)
379379
dirty = true; //needed so that we remove this collection from the second-level cache
380380
}
381381

382-
/// <summary>
382+
// Obsolete since v5.2
383+
/// <summary>
383384
/// After reading all existing elements from the database,
384385
/// add the queued elements to the underlying collection.
385386
/// </summary>
387+
[Obsolete("Use or override ApplyQueuedOperations instead")]
386388
protected virtual void PerformQueuedOperations()
387389
{
388390
for (int i = 0; i < operationQueue.Count; i++)
@@ -391,6 +393,22 @@ protected virtual void PerformQueuedOperations()
391393
}
392394
}
393395

396+
/// <summary>
397+
/// After reading all existing elements from the database, do the queued operations
398+
/// (adds or removes) on the underlying collection.
399+
/// </summary>
400+
public virtual void ApplyQueuedOperations()
401+
{
402+
if (operationQueue == null)
403+
throw new InvalidOperationException("There are no operation queue.");
404+
405+
#pragma warning disable 618
406+
PerformQueuedOperations();
407+
#pragma warning restore 618
408+
409+
operationQueue = null;
410+
}
411+
394412
public void SetSnapshot(object key, string role, object snapshot)
395413
{
396414
this.key = key;
@@ -437,18 +455,8 @@ public virtual bool EndRead(ICollectionPersister persister)
437455
public virtual bool AfterInitialize(ICollectionPersister persister)
438456
{
439457
SetInitialized();
440-
//do this bit after setting initialized to true or it will recurse
441-
if (operationQueue != null)
442-
{
443-
PerformQueuedOperations();
444-
operationQueue = null;
445-
cachedSize = -1;
446-
return false;
447-
}
448-
else
449-
{
450-
return true;
451-
}
458+
cachedSize = -1;
459+
return operationQueue == null;
452460
}
453461

454462
/// <summary>

‎src/NHibernate/Collection/Generic/PersistentGenericBag.cs

Lines changed: 23 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ public partial class PersistentGenericBag<T> : AbstractPersistentCollection, ILi
4848
* <one-to-many> <bag>!
4949
*/
5050
private IList<T> _gbag;
51+
private bool _isOneToMany;
5152

5253
public PersistentGenericBag()
5354
{
@@ -255,59 +256,10 @@ public void RemoveAt(int index)
255256
_gbag.RemoveAt(index);
256257
}
257258

258-
public override bool AfterInitialize(ICollectionPersister persister)
259-
{
260-
// NH Different behavior : NH-739
261-
// would be nice to prevent this overhead but the operation is managed where the ICollectionPersister is not available
262-
bool result;
263-
if (persister.IsOneToMany && HasQueuedOperations)
264-
{
265-
var additionStartFrom = _gbag.Count;
266-
IList additionQueue = new List<object>(additionStartFrom);
267-
foreach (var o in QueuedAdditionIterator)
268-
{
269-
if (o != null)
270-
{
271-
for (var i = 0; i < _gbag.Count; i++)
272-
{
273-
// we are using ReferenceEquals to be sure that is exactly the same queued instance
274-
if (ReferenceEquals(o, _gbag[i]))
275-
{
276-
additionQueue.Add(o);
277-
break;
278-
}
279-
}
280-
}
281-
}
282-
283-
result = base.AfterInitialize(persister);
284-
285-
if (!result)
286-
{
287-
// removing duplicated additions
288-
foreach (var o in additionQueue)
289-
{
290-
for (var i = additionStartFrom; i < _gbag.Count; i++)
291-
{
292-
if (ReferenceEquals(o, _gbag[i]))
293-
{
294-
_gbag.RemoveAt(i);
295-
break;
296-
}
297-
}
298-
}
299-
}
300-
}
301-
else
302-
{
303-
result = base.AfterInitialize(persister);
304-
}
305-
return result;
306-
}
307-
308259
public override void BeforeInitialize(ICollectionPersister persister, int anticipatedSize)
309260
{
310261
_gbag = (IList<T>) persister.CollectionType.Instantiate(anticipatedSize);
262+
_isOneToMany = persister.IsOneToMany;
311263
}
312264

313265
public override object Disassemble(ICollectionPersister persister)
@@ -597,6 +549,27 @@ public object Orphan
597549

598550
public void Operate()
599551
{
552+
// NH Different behavior for NH-739. A "bag" mapped as a bidirectional one-to-many of an entity with an
553+
// id generator causing it to be inserted on flush must not replay the addition after initialization,
554+
// if the entity was previously saved. In that case, the entity save has inserted it in database with
555+
// its association to the bag, without causing a full flush. So for the bag, the operation is still
556+
// pending, but in database it is already done. On initialization, the bag thus already receives the
557+
// entity in its loaded list, and it should not be added again.
558+
// Since a one-to-many bag is actually a set, we can simply check if the entity is already in the loaded
559+
// state, and discard it if yes. (It also relies on the bag not having pending removes, which is the
560+
// case, since it only handles delayed additions and clears.)
561+
// Since this condition happens with transient instances added in the bag then saved, ReferenceEquals
562+
// is enough to match them.
563+
// This solution is a workaround, the root cause is not fixed. The root cause is the insertion on save
564+
// done without caring for pending operations of one-to-many collections. This root cause could be fixed
565+
// by triggering a full flush there before the insert (currently it just flushes pending inserts), or
566+
// maybe by flushing just the dirty one-to-many non-initialized collections (but this can be tricky).
567+
// (It is also likely one-to-many lists have a similar issue, but nothing is done yet for them. And
568+
// their case is more complex due to having to care for the indexes and to handle many more delayed
569+
// operation kinds.)
570+
if (_enclosingInstance._isOneToMany && _enclosingInstance._gbag.Any(l => ReferenceEquals(l, _value)))
571+
return;
572+
600573
_enclosingInstance._gbag.Add(_value);
601574
}
602575
}

0 commit comments

Comments
 (0)