Fix extra-lazy collection forgetting changes

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/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/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/Generic/PersistentGenericBag.cs

@@ -48,6 +48,7 @@ public partial class PersistentGenericBag<T> : AbstractPersistentCollection, ILi
 		 * <one-to-many> <bag>!
 		 */
 		private IList<T> _gbag;
+		private bool _isOneToMany;
 
 		public PersistentGenericBag()
 		{
@@ -255,59 +256,10 @@ public void RemoveAt(int index)
 			_gbag.RemoveAt(index);
 		}
 
-		public override bool AfterInitialize(ICollectionPersister persister)
-		{
-			// NH Different behavior : NH-739
-			// would be nice to prevent this overhead but the operation is managed where the ICollectionPersister is not available
-			bool result;
-			if (persister.IsOneToMany && HasQueuedOperations)
-			{
-				var additionStartFrom = _gbag.Count;
-				IList additionQueue = new List<object>(additionStartFrom);
-				foreach (var o in QueuedAdditionIterator)
-				{
-					if (o != null)
-					{
-						for (var i = 0; i < _gbag.Count; i++)
-						{
-							// we are using ReferenceEquals to be sure that is exactly the same queued instance 
-							if (ReferenceEquals(o, _gbag[i]))
-							{
-								additionQueue.Add(o);
-								break;
-							}
-						}
-					}
-				}
-
-				result = base.AfterInitialize(persister);
-
-				if (!result)
-				{
-					// removing duplicated additions
-					foreach (var o in additionQueue)
-					{
-						for (var i = additionStartFrom; i < _gbag.Count; i++)
-						{
-							if (ReferenceEquals(o, _gbag[i]))
-							{
-								_gbag.RemoveAt(i);
-								break;
-							}
-						}
-					}
-				}
-			}
-			else
-			{
-				result = base.AfterInitialize(persister);
-			}
-			return result;
-		}
-
 		public override void BeforeInitialize(ICollectionPersister persister, int anticipatedSize)
 		{
 			_gbag = (IList<T>) persister.CollectionType.Instantiate(anticipatedSize);
+			_isOneToMany = persister.IsOneToMany;
 		}
 
 		public override object Disassemble(ICollectionPersister persister)
@@ -597,6 +549,27 @@ public object Orphan
 
 			public void Operate()
 			{
+				// NH Different behavior for NH-739. A "bag" mapped as a bidirectional one-to-many of an entity with an
+				// id generator causing it to be inserted on flush must not replay the addition after initialization,
+				// if the entity was previously saved. In that case, the entity save has inserted it in database with
+				// its association to the bag, without causing a full flush. So for the bag, the operation is still
+				// pending, but in database it is already done. On initialization, the bag thus already receives the
+				// entity in its loaded list, and it should not be added again.
+				// Since a one-to-many bag is actually a set, we can simply check if the entity is already in the loaded
+				// state, and discard it if yes. (It also relies on the bag not having pending removes, which is the
+				// case, since it only handles delayed additions and clears.)
+				// Since this condition happens with transient instances added in the bag then saved, ReferenceEquals
+				// is enough to match them.
+				// This solution is a workaround, the root cause is not fixed. The root cause is the insertion on save
+				// done without caring for pending operations of one-to-many collections. This root cause could be fixed
+				// by triggering a full flush there before the insert (currently it just flushes pending inserts), or
+				// maybe by flushing just the dirty one-to-many non-initialized collections (but this can be tricky).
+				// (It is also likely one-to-many lists have a similar issue, but nothing is done yet for them. And
+				// their case is more complex due to having to care for the indexes and to handle many more delayed
+				// operation kinds.)
+				if (_enclosingInstance._isOneToMany && _enclosingInstance._gbag.Any(l => ReferenceEquals(l, _value)))
+					return;
+
 				_enclosingInstance._gbag.Add(_value);
 			}
 		}