Fix extra-lazy collection forgetting changes

Fixes #1874

Author: fredericDelaporte

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

@@ -10,6 +10,7 @@
 
 using System.Collections;
 using System.Collections.Generic;
+using System.Linq;
 using NUnit.Framework;
 
 namespace NHibernate.Test.Extralazy
@@ -33,6 +34,16 @@ protected override string CacheConcurrencyStrategy
 			get { return null; }
 		}
 
+		protected override void OnTearDown()
+		{
+			using (var s = OpenSession())
+			using (var t = s.BeginTransaction())
+			{
+				s.Delete("from System.Object");
+				t.Commit();
+			}
+		}
+
 		[Test]
 		public async Task ExtraLazyWithWhereClauseAsync()
 		{
@@ -94,6 +105,10 @@ public async Task OrphanDeleteAsync()
 				gavin = await (s.GetAsync<User>("gavin"));
 				Assert.AreEqual(2, gavin.Documents.Count);
 				gavin.Documents.Remove(hia2);
+				// Do not convert Documents.Contains to Does.Contain/Does.Not.Contain: NUnit constraints will trigger
+				// initialization of the collection. Moreover, with extra-lazy, collection.Contains works even if
+				// the entity does not properly overrides Equals and GetHashCode and a detached instance is used,
+				// provided the collection is not yet initialized, which is the case here.
 				Assert.IsFalse(gavin.Documents.Contains(hia2));
 				Assert.IsTrue(gavin.Documents.Contains(hia));
 				Assert.AreEqual(1, gavin.Documents.Count);
@@ -117,6 +132,47 @@ public async Task OrphanDeleteAsync()
 			}
 		}
 
+		[Test]
+		public async Task OrphanDeleteWithEnumerationAsync()
+		{
+			using (var s = OpenSession())
+			using (var t = s.BeginTransaction())
+			{
+				var gavin = new User("gavin", "secret");
+				gavin.Documents.Add(new Document("HiA", "blah blah blah", gavin));
+				gavin.Documents.Add(new Document("HiA2", "blah blah blah blah", gavin));
+				await (s.PersistAsync(gavin));
+				await (t.CommitAsync());
+			}
+
+			using (var s = OpenSession())
+			using (var t = s.BeginTransaction())
+			{
+				var hia2 = await (s.GetAsync<Document>("HiA2"));
+				var hia = await (s.GetAsync<Document>("HiA"));
+				var gavin = await (s.GetAsync<User>("gavin"));
+				Assert.That(gavin.Documents, Has.Count.EqualTo(2));
+				Assert.That(NHibernateUtil.IsInitialized(gavin.Documents), Is.False, "Expecting non initialized collection");
+				gavin.Documents.Remove(hia2);
+				// Force an enumeration
+				using (var e = gavin.Documents.GetEnumerator())
+					e.MoveNext();
+				Assert.That(NHibernateUtil.IsInitialized(gavin.Documents), Is.True, "Expecting initialized collection");
+				Assert.That(gavin.Documents, Does.Not.Contain(hia2).And.Contain(hia).And.Count.EqualTo(1));
+				await (t.CommitAsync());
+			}
+
+			using (var s = OpenSession())
+			using (var t = s.BeginTransaction())
+			{
+				var hia = await (s.GetAsync<Document>("HiA"));
+				var gavin = await (s.GetAsync<User>("gavin"));
+				Assert.That(gavin.Documents, Has.Count.EqualTo(1).And.Contain(hia));
+				Assert.That(await (s.GetAsync<Document>("HiA2")), Is.Null);
+				await (t.CommitAsync());
+			}
+		}
+
 		[Test]
 		public async Task GetAsync()
 		{
@@ -340,4 +396,4 @@ public async Task AddToUninitializedSetWithLaterLazyLoadAsync()
 			}
 		}
 	}
-}
+}

src/NHibernate.Test/Extralazy/ExtraLazyFixture.cs

@@ -1,5 +1,6 @@
 using System.Collections;
 using System.Collections.Generic;
+using System.Linq;
 using NUnit.Framework;
 
 namespace NHibernate.Test.Extralazy
@@ -22,6 +23,16 @@ protected override string CacheConcurrencyStrategy
 			get { return null; }
 		}
 
+		protected override void OnTearDown()
+		{
+			using (var s = OpenSession())
+			using (var t = s.BeginTransaction())
+			{
+				s.Delete("from System.Object");
+				t.Commit();
+			}
+		}
+
 		[Test]
 		public void ExtraLazyWithWhereClause()
 		{
@@ -83,6 +94,10 @@ public void OrphanDelete()
 				gavin = s.Get<User>("gavin");
 				Assert.AreEqual(2, gavin.Documents.Count);
 				gavin.Documents.Remove(hia2);
+				// Do not convert Documents.Contains to Does.Contain/Does.Not.Contain: NUnit constraints will trigger
+				// initialization of the collection. Moreover, with extra-lazy, collection.Contains works even if
+				// the entity does not properly overrides Equals and GetHashCode and a detached instance is used,
+				// provided the collection is not yet initialized, which is the case here.
 				Assert.IsFalse(gavin.Documents.Contains(hia2));
 				Assert.IsTrue(gavin.Documents.Contains(hia));
 				Assert.AreEqual(1, gavin.Documents.Count);
@@ -106,6 +121,47 @@ public void OrphanDelete()
 			}
 		}
 
+		[Test]
+		public void OrphanDeleteWithEnumeration()
+		{
+			using (var s = OpenSession())
+			using (var t = s.BeginTransaction())
+			{
+				var gavin = new User("gavin", "secret");
+				gavin.Documents.Add(new Document("HiA", "blah blah blah", gavin));
+				gavin.Documents.Add(new Document("HiA2", "blah blah blah blah", gavin));
+				s.Persist(gavin);
+				t.Commit();
+			}
+
+			using (var s = OpenSession())
+			using (var t = s.BeginTransaction())
+			{
+				var hia2 = s.Get<Document>("HiA2");
+				var hia = s.Get<Document>("HiA");
+				var gavin = s.Get<User>("gavin");
+				Assert.That(gavin.Documents, Has.Count.EqualTo(2));
+				Assert.That(NHibernateUtil.IsInitialized(gavin.Documents), Is.False, "Expecting non initialized collection");
+				gavin.Documents.Remove(hia2);
+				// Force an enumeration
+				using (var e = gavin.Documents.GetEnumerator())
+					e.MoveNext();
+				Assert.That(NHibernateUtil.IsInitialized(gavin.Documents), Is.True, "Expecting initialized collection");
+				Assert.That(gavin.Documents, Does.Not.Contain(hia2).And.Contain(hia).And.Count.EqualTo(1));
+				t.Commit();
+			}
+
+			using (var s = OpenSession())
+			using (var t = s.BeginTransaction())
+			{
+				var hia = s.Get<Document>("HiA");
+				var gavin = s.Get<User>("gavin");
+				Assert.That(gavin.Documents, Has.Count.EqualTo(1).And.Contain(hia));
+				Assert.That(s.Get<Document>("HiA2"), Is.Null);
+				t.Commit();
+			}
+		}
+
 		[Test]
 		public void Get()
 		{
@@ -329,4 +385,4 @@ public void AddToUninitializedSetWithLaterLazyLoad()
 			}
 		}
 	}
-}
+}

src/NHibernate/Async/Cache/Entry/CollectionCacheEntry.cs

@@ -34,7 +34,7 @@ public virtual async Task AssembleAsync(IPersistentCollection collection, IColle
 		{
 			cancellationToken.ThrowIfCancellationRequested();
 			await (collection.InitializeFromCacheAsync(persister, state, owner, cancellationToken)).ConfigureAwait(false);
-			collection.AfterInitialize(persister);
+			var hasNoQueuedOperations = collection.AfterInitialize(persister);
 		}
 	}
 }

src/NHibernate/Async/Collection/IPersistentCollection.cs

@@ -8,6 +8,7 @@
 //------------------------------------------------------------------------------
 
 
+using System;
 using System.Collections;
 using System.Data.Common;
 using NHibernate.Collection.Generic;

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

@@ -144,7 +144,7 @@ private async Task EndLoadingCollectionAsync(LoadingCollectionEntry lce, ICollec
 				stopWath.Start();
 			}
 
-			bool hasNoQueuedAdds = lce.Collection.EndRead(persister); // warning: can cause a recursive calls! (proxy initialization)
+			bool hasNoQueuedOperations = lce.Collection.EndRead(persister); // warning: can cause a recursive calls! (proxy initialization)
 
 			if (persister.CollectionType.HasHolder())
 			{
@@ -161,14 +161,17 @@ private async Task EndLoadingCollectionAsync(LoadingCollectionEntry lce, ICollec
 				ce.PostInitialize(lce.Collection, persistenceContext);
 			}
 
-			bool addToCache = hasNoQueuedAdds && persister.HasCache && 
+			bool addToCache = hasNoQueuedOperations && persister.HasCache && 
 				session.CacheMode.HasFlag(CacheMode.Put) && !ce.IsDoremove; // and this is not a forced initialization during flush
 
 			if (addToCache)
 			{
 				await (AddCollectionToCacheAsync(lce, persister, cacheBatchingHandler, cancellationToken)).ConfigureAwait(false);
 			}
 
+			if (!hasNoQueuedOperations)
+				lce.Collection.ApplyQueuedOperations();
+
 			if (log.IsDebugEnabled())
 			{
 				log.Debug("collection fully initialized: {0}", MessageHelper.CollectionInfoString(persister, lce.Collection, lce.Key, session));

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

@@ -161,6 +161,9 @@ private async Task<bool> AssembleAsync(CacheKey ck, object ce, ICollectionPersis
 				await (cacheEntry.AssembleAsync(collection, persister, persistenceContext.GetCollectionOwner(id, persister), cancellationToken)).ConfigureAwait(false);
 
 				persistenceContext.GetCollectionEntry(collection).PostInitialize(collection, persistenceContext);
+
+				if (collection.HasQueuedOperations)
+					collection.ApplyQueuedOperations();
 				return true;
 			}
 		}

src/NHibernate/Cache/Entry/CollectionCacheEntry.cs

@@ -42,7 +42,7 @@ public virtual object[] State
 		public virtual void Assemble(IPersistentCollection collection, ICollectionPersister persister, object owner)
 		{
 			collection.InitializeFromCache(persister, state, owner);
-			collection.AfterInitialize(persister);
+			var hasNoQueuedOperations = collection.AfterInitialize(persister);
 		}
 
 		public override string ToString()

src/NHibernate/Collection/AbstractPersistentCollection.cs

@@ -379,10 +379,12 @@ protected virtual void QueueOperation(IDelayedOperation element)
 			dirty = true; //needed so that we remove this collection from the second-level cache
 		}
 
-		/// <summary> 
+		// Obsolete since v5.2
+		/// <summary>
 		/// After reading all existing elements from the database,
 		/// add the queued elements to the underlying collection.
 		/// </summary>
+		[Obsolete("Use or override ApplyQueuedOperations instead")]
 		protected virtual void PerformQueuedOperations()
 		{
 			for (int i = 0; i < operationQueue.Count; i++)
@@ -391,6 +393,22 @@ protected virtual void PerformQueuedOperations()
 			}
 		}
 
+		/// <summary>
+		/// After reading all existing elements from the database, do the queued operations
+		/// (adds or removes) on the underlying collection.
+		/// </summary>
+		public virtual void ApplyQueuedOperations()
+		{
+			if (operationQueue == null)
+				throw new InvalidOperationException("There are no operation queue.");
+
+#pragma warning disable 618
+			PerformQueuedOperations();
+#pragma warning restore 618
+
+			operationQueue = null;
+		}
+
 		public void SetSnapshot(object key, string role, object snapshot)
 		{
 			this.key = key;
@@ -437,18 +455,8 @@ public virtual bool EndRead(ICollectionPersister persister)
 		public virtual bool AfterInitialize(ICollectionPersister persister)
 		{
 			SetInitialized();
-			//do this bit after setting initialized to true or it will recurse
-			if (operationQueue != null)
-			{
-				PerformQueuedOperations();
-				operationQueue = null;
-				cachedSize = -1;
-				return false;
-			}
-			else
-			{
-				return true;
-			}
+			cachedSize = -1;
+			return operationQueue == null;
 		}
 
 		/// <summary>

src/NHibernate/Collection/IPersistentCollection.cs

@@ -1,3 +1,4 @@
+using System;
 using System.Collections;
 using System.Data.Common;
 using NHibernate.Collection.Generic;
@@ -337,4 +338,40 @@ public partial interface IPersistentCollection
 		/// </returns>
 		ICollection GetOrphans(object snapshot, string entityName);
 	}
+
+	// 6.0 TODO: merge into IPersistentCollection
+	public static class PersistentCollectionExtensions
+	{
+		private static readonly INHibernateLogger Logger = NHibernateLogger.For(typeof(PersistentCollectionExtensions));
+
+		/// <summary>
+		/// After reading all existing elements from the database, do the queued operations
+		/// (adds or removes) on the underlying collection.
+		/// </summary>
+		/// <param name="collection">The collection.</param>
+		public static void ApplyQueuedOperations(this IPersistentCollection collection)
+		{
+			if (collection is AbstractPersistentCollection baseImpl)
+			{
+				baseImpl.ApplyQueuedOperations();
+				return;
+			}
+
+			// Fallback on reflection for custom implementations
+			var collectionType = collection.GetType();
+			var applyQueuedOperationsMethod = collectionType.GetMethod(
+				nameof(AbstractPersistentCollection.ApplyQueuedOperations),
+				Array.Empty<System.Type>());
+			if (applyQueuedOperationsMethod != null)
+			{
+				applyQueuedOperationsMethod.Invoke(collection, Array.Empty<object>());
+				return;
+			}
+
+			Logger.Warn(
+				"{0} does not implement 'void ApplyQueuedOperations()'. It should move any queued operations" +
+				"processing out of 'AfterInitialize' and put it in a 'public void ApplyQueuedOperations()'.",
+				collectionType);
+		}
+	}
 }

src/NHibernate/Engine/Loading/CollectionLoadContext.cs

@@ -252,7 +252,7 @@ private void EndLoadingCollection(LoadingCollectionEntry lce, ICollectionPersist
 				stopWath.Start();
 			}
 
-			bool hasNoQueuedAdds = lce.Collection.EndRead(persister); // warning: can cause a recursive calls! (proxy initialization)
+			bool hasNoQueuedOperations = lce.Collection.EndRead(persister); // warning: can cause a recursive calls! (proxy initialization)
 
 			if (persister.CollectionType.HasHolder())
 			{
@@ -269,14 +269,17 @@ private void EndLoadingCollection(LoadingCollectionEntry lce, ICollectionPersist
 				ce.PostInitialize(lce.Collection, persistenceContext);
 			}
 
-			bool addToCache = hasNoQueuedAdds && persister.HasCache && 
+			bool addToCache = hasNoQueuedOperations && persister.HasCache && 
 				session.CacheMode.HasFlag(CacheMode.Put) && !ce.IsDoremove; // and this is not a forced initialization during flush
 
 			if (addToCache)
 			{
 				AddCollectionToCache(lce, persister, cacheBatchingHandler);
 			}
 
+			if (!hasNoQueuedOperations)
+				lce.Collection.ApplyQueuedOperations();
+
 			if (log.IsDebugEnabled())
 			{
 				log.Debug("collection fully initialized: {0}", MessageHelper.CollectionInfoString(persister, lce.Collection, lce.Key, session));

src/NHibernate/Event/Default/DefaultInitializeCollectionEventListener.cs

@@ -148,6 +148,9 @@ private bool Assemble(CacheKey ck, object ce, ICollectionPersister persister,  I
 				cacheEntry.Assemble(collection, persister, persistenceContext.GetCollectionOwner(id, persister));
 
 				persistenceContext.GetCollectionEntry(collection).PostInitialize(collection, persistenceContext);
+
+				if (collection.HasQueuedOperations)
+					collection.ApplyQueuedOperations();
 				return true;
 			}
 		}