Changes according to second review

Author: maca88

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

@@ -2049,7 +2049,7 @@ public async Task MapSetAsync(bool initialize)
 				Assert.That(Sfi.Statistics.PrepareStatementCount, Is.EqualTo(1), "Statements count after load");
 				Assert.That(NHibernateUtil.IsInitialized(gavin.Settings), Is.False, "User settings initialization status after load");
 
-				// Set a key that does not exist in db and it is not in the queue
+				// Set a key that does not exist in db and is not in the queue
 				Sfi.Statistics.Clear();
 				for (var i = 0; i < 5; i++)
 				{
@@ -2075,7 +2075,7 @@ public async Task MapSetAsync(bool initialize)
 				Assert.That(Sfi.Statistics.PrepareStatementCount, Is.EqualTo(0), "Statements count after re-adding existing keys");
 				Assert.That(NHibernateUtil.IsInitialized(gavin.Settings), Is.False, "User settings initialization status after re-adding existing keys");
 
-				// Set a key that exists in db and it is not in the queue
+				// Set a key that exists in db and is not in the queue
 				Sfi.Statistics.Clear();
 				gavin.Settings["s0"] = new UserSetting("s0", "s0", gavin);
 
@@ -2172,7 +2172,7 @@ public async Task MapRemoveAsync(bool initialize)
 				Assert.That(Sfi.Statistics.PrepareStatementCount, Is.EqualTo(5), "Statements count after adding");
 				Assert.That(NHibernateUtil.IsInitialized(gavin.Settings), Is.False, "User settings initialization status after adding");
 
-				// Remove a key that exists in db and it is not in the queue and removal queue
+				// Remove a key that exists in db and is not in the queue and removal queue
 				Sfi.Statistics.Clear();
 				Assert.That(gavin.Settings.Remove("s0"), Is.True, "Removing an existing element");
 
@@ -2193,7 +2193,7 @@ public async Task MapRemoveAsync(bool initialize)
 				Assert.That(Sfi.Statistics.PrepareStatementCount, Is.EqualTo(0), "Statements count after removing a re-added element");
 				Assert.That(NHibernateUtil.IsInitialized(gavin.Settings), Is.False, "User settings initialization status after removing a re-added element");
 
-				// Remove a key that does not exist in db and it is not in the queue
+				// Remove a key that does not exist in db and is not in the queue
 				Sfi.Statistics.Clear();
 				Assert.That(gavin.Settings.Remove("test"), Is.False, "Removing not existing element");
 

src/NHibernate.Test/Extralazy/ExtraLazyFixture.cs

@@ -2038,7 +2038,7 @@ public void MapSet(bool initialize)
 				Assert.That(Sfi.Statistics.PrepareStatementCount, Is.EqualTo(1), "Statements count after load");
 				Assert.That(NHibernateUtil.IsInitialized(gavin.Settings), Is.False, "User settings initialization status after load");
 
-				// Set a key that does not exist in db and it is not in the queue
+				// Set a key that does not exist in db and is not in the queue
 				Sfi.Statistics.Clear();
 				for (var i = 0; i < 5; i++)
 				{
@@ -2064,7 +2064,7 @@ public void MapSet(bool initialize)
 				Assert.That(Sfi.Statistics.PrepareStatementCount, Is.EqualTo(0), "Statements count after re-adding existing keys");
 				Assert.That(NHibernateUtil.IsInitialized(gavin.Settings), Is.False, "User settings initialization status after re-adding existing keys");
 
-				// Set a key that exists in db and it is not in the queue
+				// Set a key that exists in db and is not in the queue
 				Sfi.Statistics.Clear();
 				gavin.Settings["s0"] = new UserSetting("s0", "s0", gavin);
 
@@ -2161,7 +2161,7 @@ public void MapRemove(bool initialize)
 				Assert.That(Sfi.Statistics.PrepareStatementCount, Is.EqualTo(5), "Statements count after adding");
 				Assert.That(NHibernateUtil.IsInitialized(gavin.Settings), Is.False, "User settings initialization status after adding");
 
-				// Remove a key that exists in db and it is not in the queue and removal queue
+				// Remove a key that exists in db and is not in the queue and removal queue
 				Sfi.Statistics.Clear();
 				Assert.That(gavin.Settings.Remove("s0"), Is.True, "Removing an existing element");
 
@@ -2182,7 +2182,7 @@ public void MapRemove(bool initialize)
 				Assert.That(Sfi.Statistics.PrepareStatementCount, Is.EqualTo(0), "Statements count after removing a re-added element");
 				Assert.That(NHibernateUtil.IsInitialized(gavin.Settings), Is.False, "User settings initialization status after removing a re-added element");
 
-				// Remove a key that does not exist in db and it is not in the queue
+				// Remove a key that does not exist in db and is not in the queue
 				Sfi.Statistics.Clear();
 				Assert.That(gavin.Settings.Remove("test"), Is.False, "Removing not existing element");
 

src/NHibernate/Collection/AbstractPersistentCollection.cs

@@ -307,7 +307,7 @@ protected virtual bool ReadSize()
 						return true;
 					}
 
-					if (!queueOperationTracker.Cleared && queueOperationTracker.DatabaseCollectionSize < 0)
+					if (!queueOperationTracker.Cleared && !queueOperationTracker.DatabaseCollectionSize.HasValue)
 					{
 						queueOperationTracker.DatabaseCollectionSize = persister.GetSize(entry.LoadedKey, session);
 					}
@@ -517,12 +517,6 @@ protected virtual object ReadElementByIndex(object index)
 							existsInDb = null;
 							return false;
 						}
-
-						if (queueOperationTracker.TryGetDatabaseElementByKey(elementKey, out element))
-						{
-							existsInDb = true;
-							return true;
-						}
 					}
 
 					var elementByKey = persister.GetElementByIndex(entry.LoadedKey, elementKey, session, owner);
@@ -570,7 +564,7 @@ protected virtual object ReadElementByIndex(object index)
 						}
 						else
 						{
-							if (queueOperationTracker.DatabaseCollectionSize < 0 && 
+							if (!queueOperationTracker.DatabaseCollectionSize.HasValue && 
 								queueOperationTracker.RequiresDatabaseCollectionSize(nameof(queueOperationTracker.TryGetElementAtIndex)) &&
 								!ReadSize())
 							{
@@ -589,12 +583,14 @@ protected virtual object ReadElementByIndex(object index)
 							}
 
 							// We have to calculate the database index based on the changes in the queue
-							index = queueOperationTracker.CalculateDatabaseElementIndex(index);
-							if (index < 0)
+							var dbIndex = queueOperationTracker.GetDatabaseElementIndex(index);
+							if (!dbIndex.HasValue)
 							{
 								element = default(T);
 								return false;
 							}
+
+							index = dbIndex.Value;
 						}
 					}
 
@@ -746,7 +742,7 @@ private AbstractQueueOperationTracker TryFlushAndGetQueueOperationTracker(string
 			}
 
 			queueOperationTracker.BeforeOperation(operationName);
-			if (queueOperationTracker.DatabaseCollectionSize < 0 && queueOperationTracker.RequiresDatabaseCollectionSize(operationName) && !ReadSize())
+			if (!queueOperationTracker.DatabaseCollectionSize.HasValue && queueOperationTracker.RequiresDatabaseCollectionSize(operationName) && !ReadSize())
 			{
 				throw new InvalidOperationException($"The collection role {Role} must be mapped as extra lazy.");
 			}

src/NHibernate/Collection/Generic/PersistentGenericBag.cs

@@ -140,17 +140,7 @@ int IList.Add(object value)
 			}
 
 			var val = (T) value;
-			var queueOperationTracker = GetOrCreateQueueOperationTracker();
-			if (queueOperationTracker != null)
-			{
-				QueueAddElement(val);
-			}
-			else
-			{
-#pragma warning disable 618
-				QueueOperation(new SimpleAddDelayedOperation(this, val));
-#pragma warning restore 618
-			}
+			QueueAddElement(val);
 
 			return CachedSize;
 		}
@@ -206,35 +196,15 @@ public void Add(T item)
 			}
 			else
 			{
-				var queueOperationTracker = GetOrCreateQueueOperationTracker();
-				if (queueOperationTracker != null)
-				{
-					QueueAddElement(item);
-				}
-				else
-				{
-#pragma warning disable 618
-					QueueOperation(new SimpleAddDelayedOperation(this, item));
-#pragma warning restore 618
-				}
+				QueueAddElement(item);
 			}
 		}
 
 		public void Clear()
 		{
 			if (ClearQueueEnabled)
 			{
-				var queueOperationTracker = GetOrCreateQueueOperationTracker();
-				if (queueOperationTracker != null)
-				{
-					QueueClearCollection();
-				}
-				else
-				{
-#pragma warning disable 618
-					QueueOperation(new ClearDelayedOperation(this));
-#pragma warning restore 618
-				}
+				QueueClearCollection();
 			}
 			else
 			{
@@ -545,82 +515,5 @@ private static int CountOccurrences(object element, IEnumerable list, IType elem
 
 			return result;
 		}
-
-		// Since v5.3
-		[Obsolete("This class has no more usages in NHibernate and will be removed in a future version.")]
-		private sealed class ClearDelayedOperation : IDelayedOperation
-		{
-			private readonly PersistentGenericBag<T> _enclosingInstance;
-
-			public ClearDelayedOperation(PersistentGenericBag<T> enclosingInstance)
-			{
-				_enclosingInstance = enclosingInstance;
-			}
-
-			public object AddedInstance
-			{
-				get { return null; }
-			}
-
-			public object Orphan
-			{
-				get { throw new NotSupportedException("queued clear cannot be used with orphan delete"); }
-			}
-
-			public void Operate()
-			{
-				_enclosingInstance._gbag.Clear();
-			}
-		}
-
-		// Since v5.3
-		[Obsolete("This class has no more usages in NHibernate and will be removed in a future version.")]
-		private sealed class SimpleAddDelayedOperation : IDelayedOperation
-		{
-			private readonly PersistentGenericBag<T> _enclosingInstance;
-			private readonly T _value;
-
-			public SimpleAddDelayedOperation(PersistentGenericBag<T> enclosingInstance, T value)
-			{
-				_enclosingInstance = enclosingInstance;
-				_value = value;
-			}
-
-			public object AddedInstance
-			{
-				get { return _value; }
-			}
-
-			public object Orphan
-			{
-				get { return null; }
-			}
-
-			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);
-			}
-		}
 	}
 }