Join-fetching with property-ref results in select n+1

src/NHibernate.Test/Async/NHSpecificTest/GH1226/Fixture.cs

@@ -0,0 +1,141 @@
+//------------------------------------------------------------------------------
+// <auto-generated>
+//     This code was generated by AsyncGenerator.
+//
+//     Changes to this file may cause incorrect behavior and will be lost if
+//     the code is regenerated.
+// </auto-generated>
+//------------------------------------------------------------------------------
+
+
+using System.Linq;
+using NHibernate.Engine;
+using NHibernate.Persister.Entity;
+using NUnit.Framework;
+using NHibernate.Linq;
+
+namespace NHibernate.Test.NHSpecificTest.GH1226
+{
+	using System.Threading.Tasks;
+	[TestFixture]
+	public class FixtureAsync : BugTestCase
+	{
+		protected override void OnSetUp()
+		{
+			base.OnSetUp();
+
+			using (var session = OpenSession())
+			{
+				using (var tx = session.BeginTransaction())
+				{
+					var bank = new Bank { Code = "01234" };
+					session.Save(bank);
+
+					var account = new Account { Bank = bank };
+					session.Save(account);
+
+					var account2 = new Account { Bank = bank };
+					session.Save(account2);
+
+					tx.Commit();
+				}
+			}
+			Sfi.Statistics.IsStatisticsEnabled = true;
+		}
+
+		[Test]
+		public async Task BankShouldBeJoinFetchedAsync()
+		{
+			using (var session = OpenSession())
+			using (var tx = session.BeginTransaction())
+			{
+				// Bug only occurs if the Banks are already in the session cache.
+				await (session.CreateQuery("from Bank").ListAsync<Bank>());
+
+				var countBeforeQuery = Sfi.Statistics.PrepareStatementCount;
+
+				var accounts = await (session.CreateQuery("from Account a left join fetch a.Bank").ListAsync<Account>());
+				var associatedBanks = accounts.Select(x => x.Bank).ToList();
+				Assert.That(associatedBanks, Has.All.Matches<object>(NHibernateUtil.IsInitialized),
+				            "One bank or more was lazily loaded.");
+
+				var countAfterQuery = Sfi.Statistics.PrepareStatementCount;
+				var statementCount = countAfterQuery - countBeforeQuery;
+
+				await (tx.CommitAsync());
+
+				Assert.That(statementCount, Is.EqualTo(1));
+			}
+		}
+
+		[Test]
+		public async Task AlteredBankShouldBeJoinFetchedAsync()
+		{
+			using (var s1 = OpenSession())
+			{
+				using (var tx = s1.BeginTransaction())
+				{
+					// Put them all in s1 cache.
+					await (s1.CreateQuery("from Bank").ListAsync());
+					await (tx.CommitAsync());
+				}
+
+				string oldCode;
+				const string newCode = "12345";
+				// Alter the bank code with another session.
+				using (var s2 = OpenSession())
+				using (var tx2 = s2.BeginTransaction())
+				{
+					var accounts = await (s2.Query<Account>().ToListAsync());
+					foreach (var account in accounts)
+						account.Bank = null;
+					await (s2.FlushAsync());
+					var bank = await (s2.Query<Bank>().SingleAsync());
+					oldCode = bank.Code;
+					bank.Code = newCode;
+					await (s2.FlushAsync());
+					foreach (var account in accounts)
+						account.Bank = bank;
+					await (tx2.CommitAsync());
+				}
+
+				// Check querying them with s1 is still consistent
+				using (var tx = s1.BeginTransaction())
+				{
+					var accounts = await (s1.CreateQuery("from Account a left join fetch a.Bank").ListAsync<Account>());
+					var associatedBanks = accounts.Select(x => x.Bank).ToList();
+					Assert.That(associatedBanks, Has.All.Not.Null,
+					            "One bank or more failed loading.");
+					Assert.That(associatedBanks, Has.All.Matches<object>(NHibernateUtil.IsInitialized),
+					            "One bank or more was lazily loaded.");
+					Assert.That(associatedBanks, Has.All.Property(nameof(Bank.Code)).EqualTo(oldCode),
+					            "One bank or more has no more the old code.");
+
+					await (tx.CommitAsync());
+					// Do not check statements count: we are in a special case defeating the eager fetching, because
+					// we have stale data in session for the bank code.
+					// But check that the new code, supposed to be unknown for the session, is not cached.
+					var persister = Sfi.GetEntityPersister(typeof(Bank).FullName);
+					var index = ((IUniqueKeyLoadable) persister).GetPropertyIndex(nameof(Bank.Code));
+					var type = persister.PropertyTypes[index];
+					var euk = new EntityUniqueKey(persister.EntityName, nameof(Bank.Code), newCode, type, Sfi);
+					Assert.That(s1.GetSessionImplementation().PersistenceContext.GetEntity(euk),
+						Is.Null, "Found a bank associated to the new code in s1");
+				}
+			}
+		}
+
+		protected override void OnTearDown()
+		{
+			base.OnTearDown();
+
+			using (var session = OpenSession())
+			using (var tx = session.BeginTransaction())
+			{
+				session.CreateQuery("delete from Account").ExecuteUpdate();
+				session.CreateQuery("delete from Bank").ExecuteUpdate();
+				tx.Commit();
+			}
+		}
+	}
+}

src/NHibernate.Test/NHSpecificTest/GH1226/Fixture.cs

@@ -0,0 +1,129 @@
+using System.Linq;
+using NHibernate.Engine;
+using NHibernate.Persister.Entity;
+using NUnit.Framework;
+
+namespace NHibernate.Test.NHSpecificTest.GH1226
+{
+	[TestFixture]
+	public class Fixture : BugTestCase
+	{
+		protected override void OnSetUp()
+		{
+			base.OnSetUp();
+
+			using (var session = OpenSession())
+			{
+				using (var tx = session.BeginTransaction())
+				{
+					var bank = new Bank { Code = "01234" };
+					session.Save(bank);
+
+					var account = new Account { Bank = bank };
+					session.Save(account);
+
+					var account2 = new Account { Bank = bank };
+					session.Save(account2);
+
+					tx.Commit();
+				}
+			}
+			Sfi.Statistics.IsStatisticsEnabled = true;
+		}
+
+		[Test]
+		public void BankShouldBeJoinFetched()
+		{
+			using (var session = OpenSession())
+			using (var tx = session.BeginTransaction())
+			{
+				// Bug only occurs if the Banks are already in the session cache.
+				session.CreateQuery("from Bank").List<Bank>();
+
+				var countBeforeQuery = Sfi.Statistics.PrepareStatementCount;
+
+				var accounts = session.CreateQuery("from Account a left join fetch a.Bank").List<Account>();
+				var associatedBanks = accounts.Select(x => x.Bank).ToList();
+				Assert.That(associatedBanks, Has.All.Matches<object>(NHibernateUtil.IsInitialized),
+				            "One bank or more was lazily loaded.");
+
+				var countAfterQuery = Sfi.Statistics.PrepareStatementCount;
+				var statementCount = countAfterQuery - countBeforeQuery;
+
+				tx.Commit();
+
+				Assert.That(statementCount, Is.EqualTo(1));
+			}
+		}
+
+		[Test]
+		public void AlteredBankShouldBeJoinFetched()
+		{
+			using (var s1 = OpenSession())
+			{
+				using (var tx = s1.BeginTransaction())
+				{
+					// Put them all in s1 cache.
+					s1.CreateQuery("from Bank").List();
+					tx.Commit();
+				}
+
+				string oldCode;
+				const string newCode = "12345";
+				// Alter the bank code with another session.
+				using (var s2 = OpenSession())
+				using (var tx2 = s2.BeginTransaction())
+				{
+					var accounts = s2.Query<Account>().ToList();
+					foreach (var account in accounts)
+						account.Bank = null;
+					s2.Flush();
+					var bank = s2.Query<Bank>().Single();
+					oldCode = bank.Code;
+					bank.Code = newCode;
+					s2.Flush();
+					foreach (var account in accounts)
+						account.Bank = bank;
+					tx2.Commit();
+				}
+
+				// Check querying them with s1 is still consistent
+				using (var tx = s1.BeginTransaction())
+				{
+					var accounts = s1.CreateQuery("from Account a left join fetch a.Bank").List<Account>();
+					var associatedBanks = accounts.Select(x => x.Bank).ToList();
+					Assert.That(associatedBanks, Has.All.Not.Null,
+					            "One bank or more failed loading.");
+					Assert.That(associatedBanks, Has.All.Matches<object>(NHibernateUtil.IsInitialized),
+					            "One bank or more was lazily loaded.");
+					Assert.That(associatedBanks, Has.All.Property(nameof(Bank.Code)).EqualTo(oldCode),
+					            "One bank or more has no more the old code.");
+
+					tx.Commit();
+					// Do not check statements count: we are in a special case defeating the eager fetching, because
+					// we have stale data in session for the bank code.
+					// But check that the new code, supposed to be unknown for the session, is not cached.
+					var persister = Sfi.GetEntityPersister(typeof(Bank).FullName);
+					var index = ((IUniqueKeyLoadable) persister).GetPropertyIndex(nameof(Bank.Code));
+					var type = persister.PropertyTypes[index];
+					var euk = new EntityUniqueKey(persister.EntityName, nameof(Bank.Code), newCode, type, Sfi);
+					Assert.That(s1.GetSessionImplementation().PersistenceContext.GetEntity(euk),
+						Is.Null, "Found a bank associated to the new code in s1");
+				}
+			}
+		}
+
+		protected override void OnTearDown()
+		{
+			base.OnTearDown();
+
+			using (var session = OpenSession())
+			using (var tx = session.BeginTransaction())
+			{
+				session.CreateQuery("delete from Account").ExecuteUpdate();
+				session.CreateQuery("delete from Bank").ExecuteUpdate();
+				tx.Commit();
+			}
+		}
+	}
+}

src/NHibernate.Test/NHSpecificTest/GH1226/Mappings.hbm.xml

@@ -0,0 +1,18 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<hibernate-mapping xmlns="urn:nhibernate-mapping-2.2"
+                   assembly="NHibernate.Test"
+                   namespace="NHibernate.Test.NHSpecificTest.GH1226">
+  <class name="Account">
+    <id name="Id">
+      <generator class="guid"/>
+    </id>
+    <many-to-one fetch="join" name="Bank" property-ref="Code"/>
+  </class>
+
+  <class name="Bank">
+    <id name="Id">
+      <generator class="guid"/>
+    </id>
+    <property name="Code" unique="true"/>
+  </class>
+</hibernate-mapping>

src/NHibernate.Test/NHSpecificTest/GH1226/Model.cs

@@ -0,0 +1,16 @@
+using System;
+
+namespace NHibernate.Test.NHSpecificTest.GH1226
+{
+	public class Account
+	{
+		public virtual Guid Id { get; set; }
+		public virtual Bank Bank { get; set; }
+	}
+
+	public class Bank
+	{
+		public virtual Guid Id { get; set; }
+		public virtual string Code { get; set; }
+	}
+}

src/NHibernate/Async/Loader/Loader.cs

@@ -591,7 +591,7 @@ private async Task<object[]> GetRowAsync(DbDataReader rs, ILoadable[] persisters
 				object obj = null;
 				EntityKey key = keys[i];
 
-				if (keys[i] == null)
+				if (key == null)
 				{
 					// do nothing
 					/* TODO NH-1001 : if (persisters[i]...EntityType) is an OneToMany or a ManyToOne and
@@ -603,17 +603,22 @@ private async Task<object[]> GetRowAsync(DbDataReader rs, ILoadable[] persisters
 				{
 					//If the object is already loaded, return the loaded one
 					obj = await (session.GetEntityUsingInterceptorAsync(key, cancellationToken)).ConfigureAwait(false);
-					if (obj != null)
+					var alreadyLoaded = obj != null;
+					var persister = persisters[i];
+					if (alreadyLoaded)
 					{
 						//its already loaded so dont need to hydrate it
-						await (InstanceAlreadyLoadedAsync(rs, i, persisters[i], key, obj, lockModes[i], session, cancellationToken)).ConfigureAwait(false);
+						await (InstanceAlreadyLoadedAsync(rs, i, persister, key, obj, lockModes[i], session, cancellationToken)).ConfigureAwait(false);
 					}
 					else
 					{
 						obj =
-							await (InstanceNotYetLoadedAsync(rs, i, persisters[i], key, lockModes[i], descriptors[i].RowIdAlias, optionalObjectKey,
+							await (InstanceNotYetLoadedAsync(rs, i, persister, key, lockModes[i], descriptors[i].RowIdAlias, optionalObjectKey,
 												 optionalObject, hydratedObjects, session, cancellationToken)).ConfigureAwait(false);
 					}
+					// #1226: Even if it is already loaded, if it can be loaded from an association with a property ref, make
+					// sure it is also cached by its unique key.
+					CacheByUniqueKey(i, persister, obj, session, alreadyLoaded);
 				}
 
 				rowResults[i] = obj;
@@ -724,26 +729,6 @@ private async Task LoadFromResultSetAsync(DbDataReader rs, int i, object obj, st
 
 			object rowId = persister.HasRowId ? rs[rowIdAlias] : null;
 
-			IAssociationType[] ownerAssociationTypes = OwnerAssociationTypes;
-			if (ownerAssociationTypes != null && ownerAssociationTypes[i] != null)
-			{
-				string ukName = ownerAssociationTypes[i].RHSUniqueKeyPropertyName;
-				if (ukName != null)
-				{
-					int index = ((IUniqueKeyLoadable)persister).GetPropertyIndex(ukName);
-					IType type = persister.PropertyTypes[index];
-
-					// polymorphism not really handled completely correctly,
-					// perhaps...well, actually its ok, assuming that the
-					// entity name used in the lookup is the same as the
-					// the one used here, which it will be
-
-					EntityUniqueKey euk =
-						new EntityUniqueKey(rootPersister.EntityName, ukName, await (type.SemiResolveAsync(values[index], session, obj, cancellationToken)).ConfigureAwait(false), type, session.Factory);
-					session.PersistenceContext.AddEntity(euk, obj);
-				}
-			}
-
 			TwoPhaseLoad.PostHydrate(persister, id, values, rowId, obj, lockMode, !eagerPropertyFetch, session);
 		}