Avoid initializing proxies when ordering inserts

src/NHibernate.Test/Async/Insertordering/AnimalModel/Fixture.cs

@@ -102,5 +102,66 @@ public async System.Threading.Tasks.Task ElaboratedModelAsync()
 				Assert.DoesNotThrowAsync(() => { return tran.CommitAsync(); });
 			}
 		}
+
+		// #1338
+		[Test]
+		public async System.Threading.Tasks.Task InsertShouldNotInitializeManyToOneProxyAsync()
+		{
+			var person = new Person {  Name = "AnimalOwner" };
+			using (var s = OpenSession())
+			using (var t = s.BeginTransaction())
+			{
+				await (s.SaveAsync(person));
+				await (t.CommitAsync());
+			}
+			await (Sfi.EvictAsync(typeof(Person)));
+
+			using (var s = OpenSession())
+			using (var t = s.BeginTransaction())
+			{
+				var personProxy = await (s.LoadAsync<Person>(person.Id));
+				Assert.That(NHibernateUtil.IsInitialized(personProxy), Is.False, "Person proxy already initialized after load");
+
+				await (s.SaveAsync(new Cat { Name = "Felix", Owner = personProxy }));
+				await (s.SaveAsync(new Cat { Name = "Loustic", Owner = personProxy }));
+				Assert.That(NHibernateUtil.IsInitialized(personProxy), Is.False, "Person proxy initialized after saves");
+				await (t.CommitAsync());
+				Assert.That(NHibernateUtil.IsInitialized(personProxy), Is.False, "Person proxy initialized after commit");
+			}
+		}
+
+		[Test]
+		public async System.Threading.Tasks.Task InsertShouldNotInitializeOneToManyProxyAsync()
+		{
+			var cat = new Cat {  Name = "Felix" };
+			using (var s = OpenSession())
+			using (var t = s.BeginTransaction())
+			{
+				await (s.SaveAsync(cat));
+				await (t.CommitAsync());
+			}
+			await (Sfi.EvictAsync(typeof(Cat)));
+
+			using (var s = OpenSession())
+			using (var t = s.BeginTransaction())
+			{
+				var catProxy = await (s.LoadAsync<Cat>(cat.Id));
+				Assert.That(NHibernateUtil.IsInitialized(catProxy), Is.False, "Cat proxy already initialized after load");
+
+				var owner = new Person { Name = "AnimalOwner" };
+				owner.AnimalsGeneric.Add(catProxy);
+				// Following assert would fail if the collection was changed for a set.
+				Assert.That(NHibernateUtil.IsInitialized(catProxy), Is.False, "Cat proxy initialized after collection add");
+				await (s.SaveAsync(owner));
+				Assert.That(NHibernateUtil.IsInitialized(catProxy), Is.False, "Cat proxy initialized after save");
+				await (t.CommitAsync());
+				Assert.That(NHibernateUtil.IsInitialized(catProxy), Is.False, "Cat proxy initialized after commit");
+				// The collection being inverse, the cat owner is not actually set in this test, but that is enough
+				// to check the trouble. The ordering logic does not short-circuit on inverse collections. (It could
+				// be an optimization, but it may cause regressions for some edge case mappings, like one having an
+				// inverse one-to-many with no matching many-to-one but a basic type property for the foreign key
+				// instead.)
+			}
+		}
 	}
-}
+}

src/NHibernate.Test/Insertordering/AnimalModel/Animal.cs

@@ -7,5 +7,15 @@ public class Animal
 		public virtual Guid Id { get; set; }
 		public virtual string Name { get; set; }
 		public virtual Person Owner { get; set; }
+
+		public override bool Equals(object obj)
+		{
+			return (obj as Animal)?.Id == Id;
+		}
+
+		public override int GetHashCode()
+		{
+			return Id.GetHashCode();
+		}
 	}
 }

src/NHibernate.Test/Insertordering/AnimalModel/Fixture.cs

@@ -92,5 +92,66 @@ public void ElaboratedModel()
 				Assert.DoesNotThrow(() => { tran.Commit(); });
 			}
 		}
+
+		// #1338
+		[Test]
+		public void InsertShouldNotInitializeManyToOneProxy()
+		{
+			var person = new Person {  Name = "AnimalOwner" };
+			using (var s = OpenSession())
+			using (var t = s.BeginTransaction())
+			{
+				s.Save(person);
+				t.Commit();
+			}
+			Sfi.Evict(typeof(Person));
+
+			using (var s = OpenSession())
+			using (var t = s.BeginTransaction())
+			{
+				var personProxy = s.Load<Person>(person.Id);
+				Assert.That(NHibernateUtil.IsInitialized(personProxy), Is.False, "Person proxy already initialized after load");
+
+				s.Save(new Cat { Name = "Felix", Owner = personProxy });
+				s.Save(new Cat { Name = "Loustic", Owner = personProxy });
+				Assert.That(NHibernateUtil.IsInitialized(personProxy), Is.False, "Person proxy initialized after saves");
+				t.Commit();
+				Assert.That(NHibernateUtil.IsInitialized(personProxy), Is.False, "Person proxy initialized after commit");
+			}
+		}
+
+		[Test]
+		public void InsertShouldNotInitializeOneToManyProxy()
+		{
+			var cat = new Cat {  Name = "Felix" };
+			using (var s = OpenSession())
+			using (var t = s.BeginTransaction())
+			{
+				s.Save(cat);
+				t.Commit();
+			}
+			Sfi.Evict(typeof(Cat));
+
+			using (var s = OpenSession())
+			using (var t = s.BeginTransaction())
+			{
+				var catProxy = s.Load<Cat>(cat.Id);
+				Assert.That(NHibernateUtil.IsInitialized(catProxy), Is.False, "Cat proxy already initialized after load");
+
+				var owner = new Person { Name = "AnimalOwner" };
+				owner.AnimalsGeneric.Add(catProxy);
+				// Following assert would fail if the collection was changed for a set.
+				Assert.That(NHibernateUtil.IsInitialized(catProxy), Is.False, "Cat proxy initialized after collection add");
+				s.Save(owner);
+				Assert.That(NHibernateUtil.IsInitialized(catProxy), Is.False, "Cat proxy initialized after save");
+				t.Commit();
+				Assert.That(NHibernateUtil.IsInitialized(catProxy), Is.False, "Cat proxy initialized after commit");
+				// The collection being inverse, the cat owner is not actually set in this test, but that is enough
+				// to check the trouble. The ordering logic does not short-circuit on inverse collections. (It could
+				// be an optimization, but it may cause regressions for some edge case mappings, like one having an
+				// inverse one-to-many with no matching many-to-one but a basic type property for the foreign key
+				// instead.)
+			}
+		}
 	}
-}
+}

src/NHibernate.Test/Insertordering/AnimalModel/Person.cs

@@ -8,5 +8,15 @@ public class Person
 		public virtual Guid Id { get; set; }
 		public virtual string Name { get; set; }
 		public virtual IList<Animal> AnimalsGeneric { get; set; } = new List<Animal>();
+
+		public override bool Equals(object obj)
+		{
+			return (obj as Person)?.Id == Id;
+		}
+
+		public override int GetHashCode()
+		{
+			return Id.GetHashCode();
+		}
 	}
 }

src/NHibernate/Async/Action/IExecutable.cs

@@ -8,6 +8,7 @@
 //------------------------------------------------------------------------------
 
 
+using System;
 using NHibernate.Engine;
 
 namespace NHibernate.Action

src/NHibernate/Async/Engine/ActionQueue.cs

@@ -240,7 +240,6 @@ public async Task AfterTransactionCompletionAsync(bool success, CancellationToke
 				processes.Clear();
 			}
 		}
-
 		private partial class BeforeTransactionCompletionDelegatedProcess : IBeforeTransactionCompletionProcess
 		{
 
@@ -261,7 +260,6 @@ public Task ExecuteBeforeTransactionCompletionAsync(CancellationToken cancellati
 				}
 			}
 		}
-
 		private partial class AfterTransactionCompletionDelegatedProcess : IAfterTransactionCompletionProcess
 		{
 

src/NHibernate/Engine/ActionQueue.cs

@@ -701,7 +701,12 @@ private bool RequireNewBatch(AbstractEntityInsertAction action, int latestBatchN
 					var value = propertyValues[i];
 					var type = propertyTypes[i];
 
-					if (type.IsEntityType && value != null)
+					if (type.IsEntityType && value != null &&
+						// If the value is not initialized, it is a proxy with pending load from database,
+						// so it can only be an already persisted entity. It can not have its own pending
+						// insertion batch. So there is no need to seek for it, and it avoids initializing
+						// it by searching it in a dictionary. Fixes #1338.
+						NHibernateUtil.IsInitialized(value))
 					{
 						// find the batch number associated with the current association, if any.
 						int associationBatchNumber;
@@ -761,8 +766,16 @@ private void UpdateChildrenDependencies(int batchNumber, AbstractEntityInsertAct
 
 					foreach(var child in children)
 					{
-						if (child == null)
+						if (child == null ||
+							// If the child is not initialized, it is a proxy with pending load from database,
+							// so it can only be an already persisted entity. It can not have its own pending
+							// insertion batch. So we do not need to keep track of the highest other batch on
+							// which it depends. And this avoids initializing the proxy by searching it into
+							// a dictionary.
+							!NHibernateUtil.IsInitialized(child))
+						{
 							continue;
+						}
 
 						int latestDependency;
 						if (_entityBatchDependency.TryGetValue(child, out latestDependency) && latestDependency > batchNumber)