From 73bba295f09d195ce83bda189996af4a4a84cee1 Mon Sep 17 00:00:00 2001 From: Roger Kratz Date: Thu, 6 Dec 2018 11:24:27 +0100 Subject: [PATCH 1/4] Failing test for GH1920 --- .../GH1920/EntityWithBatchSize.cs | 10 ++++ .../NHSpecificTest/GH1920/Fixture.cs | 51 +++++++++++++++++++ .../NHSpecificTest/GH1920/Mappings.hbm.xml | 9 ++++ 3 files changed, 70 insertions(+) create mode 100644 src/NHibernate.Test/NHSpecificTest/GH1920/EntityWithBatchSize.cs create mode 100644 src/NHibernate.Test/NHSpecificTest/GH1920/Fixture.cs create mode 100644 src/NHibernate.Test/NHSpecificTest/GH1920/Mappings.hbm.xml diff --git a/src/NHibernate.Test/NHSpecificTest/GH1920/EntityWithBatchSize.cs b/src/NHibernate.Test/NHSpecificTest/GH1920/EntityWithBatchSize.cs new file mode 100644 index 00000000000..8cb470eaf51 --- /dev/null +++ b/src/NHibernate.Test/NHSpecificTest/GH1920/EntityWithBatchSize.cs @@ -0,0 +1,10 @@ +using System; + +namespace NHibernate.Test.NHSpecificTest.GH1920 +{ + public class EntityWithBatchSize + { + public virtual Guid Id { get; set; } + public virtual string Name { get; set; } + } +} diff --git a/src/NHibernate.Test/NHSpecificTest/GH1920/Fixture.cs b/src/NHibernate.Test/NHSpecificTest/GH1920/Fixture.cs new file mode 100644 index 00000000000..d40b2b8cf51 --- /dev/null +++ b/src/NHibernate.Test/NHSpecificTest/GH1920/Fixture.cs @@ -0,0 +1,51 @@ +using System; +using NUnit.Framework; + +namespace NHibernate.Test.NHSpecificTest.GH1920 +{ + [TestFixture] + public class Fixture : BugTestCase + { + private Guid entityId; + private Guid someOtherEntityId; + + protected override void OnSetUp() + { + using (var session = OpenSession()) + using (var transaction = session.BeginTransaction()) + { + entityId = (Guid) session.Save(new EntityWithBatchSize {Name = "some name"}); + someOtherEntityId = (Guid) session.Save(new EntityWithBatchSize()); + + transaction.Commit(); + } + } + + [TestCase(true)] + [TestCase(false)] + public void CanLoadEntity(bool loadProxyOfOtherEntity) + { + using (var session = OpenSession()) + using (session.BeginTransaction()) + { + if(loadProxyOfOtherEntity) + session.Load(someOtherEntityId); + + var result = session.Get(entityId); + + Assert.That(result.Name, Is.Not.Null); + } + } + + + protected override void OnTearDown() + { + using (var session = OpenSession()) + using (var transaction = session.BeginTransaction()) + { + session.CreateQuery("delete from EntityWithBatchSize").ExecuteUpdate(); + transaction.Commit(); + } + } + } +} diff --git a/src/NHibernate.Test/NHSpecificTest/GH1920/Mappings.hbm.xml b/src/NHibernate.Test/NHSpecificTest/GH1920/Mappings.hbm.xml new file mode 100644 index 00000000000..602b6c8cad5 --- /dev/null +++ b/src/NHibernate.Test/NHSpecificTest/GH1920/Mappings.hbm.xml @@ -0,0 +1,9 @@ + + + + + + + + From 1bef5439fc093ba9f0023f7ac3334269ff013834 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= <12201973+fredericdelaporte@users.noreply.github.com> Date: Thu, 6 Dec 2018 13:04:31 +0100 Subject: [PATCH 2/4] Fix Get failing with a null exception --- .../Async/NHSpecificTest/GH1920/Fixture.cs | 62 +++++++++++++++++++ .../NHSpecificTest/GH1920/Fixture.cs | 11 ++-- .../Async/Engine/BatchFetchQueue.cs | 5 +- src/NHibernate/Engine/BatchFetchQueue.cs | 9 ++- 4 files changed, 77 insertions(+), 10 deletions(-) create mode 100644 src/NHibernate.Test/Async/NHSpecificTest/GH1920/Fixture.cs diff --git a/src/NHibernate.Test/Async/NHSpecificTest/GH1920/Fixture.cs b/src/NHibernate.Test/Async/NHSpecificTest/GH1920/Fixture.cs new file mode 100644 index 00000000000..75693b26430 --- /dev/null +++ b/src/NHibernate.Test/Async/NHSpecificTest/GH1920/Fixture.cs @@ -0,0 +1,62 @@ +//------------------------------------------------------------------------------ +// +// This code was generated by AsyncGenerator. +// +// Changes to this file may cause incorrect behavior and will be lost if +// the code is regenerated. +// +//------------------------------------------------------------------------------ + + +using System; +using NUnit.Framework; + +namespace NHibernate.Test.NHSpecificTest.GH1920 +{ + using System.Threading.Tasks; + using System.Threading; + [TestFixture] + public class FixtureAsync : BugTestCase + { + private Guid entityId; + private Guid someOtherEntityId; + + protected override void OnSetUp() + { + using (var session = OpenSession()) + using (var transaction = session.BeginTransaction()) + { + entityId = (Guid) session.Save(new EntityWithBatchSize { Name = "some name" }); + someOtherEntityId = (Guid) session.Save(new EntityWithBatchSize()); + + transaction.Commit(); + } + } + + [TestCase(true)] + [TestCase(false)] + public async Task CanLoadEntityAsync(bool loadProxyOfOtherEntity, CancellationToken cancellationToken = default(CancellationToken)) + { + using (var session = OpenSession()) + using (session.BeginTransaction()) + { + if (loadProxyOfOtherEntity) + await (session.LoadAsync(someOtherEntityId, cancellationToken)); + + var result = await (session.GetAsync(entityId, cancellationToken)); + + Assert.That(result.Name, Is.Not.Null); + } + } + + protected override void OnTearDown() + { + using (var session = OpenSession()) + using (var transaction = session.BeginTransaction()) + { + session.CreateQuery("delete from EntityWithBatchSize").ExecuteUpdate(); + transaction.Commit(); + } + } + } +} diff --git a/src/NHibernate.Test/NHSpecificTest/GH1920/Fixture.cs b/src/NHibernate.Test/NHSpecificTest/GH1920/Fixture.cs index d40b2b8cf51..e8f21e9e436 100644 --- a/src/NHibernate.Test/NHSpecificTest/GH1920/Fixture.cs +++ b/src/NHibernate.Test/NHSpecificTest/GH1920/Fixture.cs @@ -8,13 +8,13 @@ public class Fixture : BugTestCase { private Guid entityId; private Guid someOtherEntityId; - + protected override void OnSetUp() { using (var session = OpenSession()) using (var transaction = session.BeginTransaction()) { - entityId = (Guid) session.Save(new EntityWithBatchSize {Name = "some name"}); + entityId = (Guid) session.Save(new EntityWithBatchSize { Name = "some name" }); someOtherEntityId = (Guid) session.Save(new EntityWithBatchSize()); transaction.Commit(); @@ -28,16 +28,15 @@ public void CanLoadEntity(bool loadProxyOfOtherEntity) using (var session = OpenSession()) using (session.BeginTransaction()) { - if(loadProxyOfOtherEntity) + if (loadProxyOfOtherEntity) session.Load(someOtherEntityId); - + var result = session.Get(entityId); Assert.That(result.Name, Is.Not.Null); } } - - + protected override void OnTearDown() { using (var session = OpenSession()) diff --git a/src/NHibernate/Async/Engine/BatchFetchQueue.cs b/src/NHibernate/Async/Engine/BatchFetchQueue.cs index d8a1807a043..200bcdc79e2 100644 --- a/src/NHibernate/Async/Engine/BatchFetchQueue.cs +++ b/src/NHibernate/Async/Engine/BatchFetchQueue.cs @@ -295,7 +295,10 @@ async Task CheckCacheAndProcessResultAsync() ? entityKeys.Count - Math.Min(batchSize, entityKeys.Count) : 0; var toIndex = entityKeys.Count - 1; - var indexes = GetSortedKeyIndexes(entityKeys, idIndex.Value, fromIndex, toIndex); + // In case of an ISession.Get on an entity not already loaded as an uninitialized proxy, + // it will not be found in entities awaiting a load, and idIndex will be null. Providing + // -1 then allows to take the entities most recently loaded as uninitialized proxies. + var indexes = GetSortedKeyIndexes(entityKeys, idIndex ?? -1, fromIndex, toIndex); if (batchableCache == null) { for (var j = 0; j < entityKeys.Count; j++) diff --git a/src/NHibernate/Engine/BatchFetchQueue.cs b/src/NHibernate/Engine/BatchFetchQueue.cs index 05426554fb5..b2d8daff434 100644 --- a/src/NHibernate/Engine/BatchFetchQueue.cs +++ b/src/NHibernate/Engine/BatchFetchQueue.cs @@ -455,7 +455,10 @@ bool CheckCacheAndProcessResult() ? entityKeys.Count - Math.Min(batchSize, entityKeys.Count) : 0; var toIndex = entityKeys.Count - 1; - var indexes = GetSortedKeyIndexes(entityKeys, idIndex.Value, fromIndex, toIndex); + // In case of an ISession.Get on an entity not already loaded as an uninitialized proxy, + // it will not be found in entities awaiting a load, and idIndex will be null. Providing + // -1 then allows to take the entities most recently loaded as uninitialized proxies. + var indexes = GetSortedKeyIndexes(entityKeys, idIndex ?? -1, fromIndex, toIndex); if (batchableCache == null) { for (var j = 0; j < entityKeys.Count; j++) @@ -612,11 +615,11 @@ private bool[] AreCached(List - /// Sorts the given keys by thier indexes, where the keys that are after the demanded key will be located + /// Sorts the given keys by their indexes, where the keys that are after the demanded key will be located /// at the start and the remaining indexes at the end of the returned array. /// /// The type of the key - /// The list of pairs of keys and thier indexes. + /// The list of pairs of keys and their indexes. /// The index of the demanded key /// The index where the sorting will begin. /// The index where the sorting will end. From d2d49b5dc6a3c5f4c7b7991ab53ea60ce8673fea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= <12201973+fredericdelaporte@users.noreply.github.com> Date: Thu, 6 Dec 2018 14:29:38 +0100 Subject: [PATCH 3/4] Ensure the trouble will not occur for collections --- src/NHibernate/Async/Engine/BatchFetchQueue.cs | 4 +++- src/NHibernate/Engine/BatchFetchQueue.cs | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/NHibernate/Async/Engine/BatchFetchQueue.cs b/src/NHibernate/Async/Engine/BatchFetchQueue.cs index 200bcdc79e2..9f8dc3abb2a 100644 --- a/src/NHibernate/Async/Engine/BatchFetchQueue.cs +++ b/src/NHibernate/Async/Engine/BatchFetchQueue.cs @@ -102,7 +102,9 @@ async Task CheckCacheAndProcessResultAsync() ? collectionKeys.Count - Math.Min(batchSize, collectionKeys.Count) : 0; var toIndex = collectionKeys.Count - 1; - var indexes = GetSortedKeyIndexes(collectionKeys, keyIndex.Value, fromIndex, toIndex); + // In case the collection having triggered the load was not registered for batching, load + // most recently registered collections. + var indexes = GetSortedKeyIndexes(collectionKeys, keyIndex ?? -1, fromIndex, toIndex); if (batchableCache == null) { for (var j = 0; j < collectionKeys.Count; j++) diff --git a/src/NHibernate/Engine/BatchFetchQueue.cs b/src/NHibernate/Engine/BatchFetchQueue.cs index b2d8daff434..ad23b80e528 100644 --- a/src/NHibernate/Engine/BatchFetchQueue.cs +++ b/src/NHibernate/Engine/BatchFetchQueue.cs @@ -271,7 +271,9 @@ bool CheckCacheAndProcessResult() ? collectionKeys.Count - Math.Min(batchSize, collectionKeys.Count) : 0; var toIndex = collectionKeys.Count - 1; - var indexes = GetSortedKeyIndexes(collectionKeys, keyIndex.Value, fromIndex, toIndex); + // In case the collection having triggered the load was not registered for batching, load + // most recently registered collections. + var indexes = GetSortedKeyIndexes(collectionKeys, keyIndex ?? -1, fromIndex, toIndex); if (batchableCache == null) { for (var j = 0; j < collectionKeys.Count; j++) From 41fad6b809501462f9b0a77f0b82681495e220f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= <12201973+fredericdelaporte@users.noreply.github.com> Date: Thu, 6 Dec 2018 15:43:06 +0100 Subject: [PATCH 4/4] Improve the fix --- src/NHibernate/Async/Engine/BatchFetchQueue.cs | 9 ++------- src/NHibernate/Engine/BatchFetchQueue.cs | 13 +++++-------- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/src/NHibernate/Async/Engine/BatchFetchQueue.cs b/src/NHibernate/Async/Engine/BatchFetchQueue.cs index 9f8dc3abb2a..074c823c9b0 100644 --- a/src/NHibernate/Async/Engine/BatchFetchQueue.cs +++ b/src/NHibernate/Async/Engine/BatchFetchQueue.cs @@ -102,9 +102,7 @@ async Task CheckCacheAndProcessResultAsync() ? collectionKeys.Count - Math.Min(batchSize, collectionKeys.Count) : 0; var toIndex = collectionKeys.Count - 1; - // In case the collection having triggered the load was not registered for batching, load - // most recently registered collections. - var indexes = GetSortedKeyIndexes(collectionKeys, keyIndex ?? -1, fromIndex, toIndex); + var indexes = GetSortedKeyIndexes(collectionKeys, keyIndex, fromIndex, toIndex); if (batchableCache == null) { for (var j = 0; j < collectionKeys.Count; j++) @@ -297,10 +295,7 @@ async Task CheckCacheAndProcessResultAsync() ? entityKeys.Count - Math.Min(batchSize, entityKeys.Count) : 0; var toIndex = entityKeys.Count - 1; - // In case of an ISession.Get on an entity not already loaded as an uninitialized proxy, - // it will not be found in entities awaiting a load, and idIndex will be null. Providing - // -1 then allows to take the entities most recently loaded as uninitialized proxies. - var indexes = GetSortedKeyIndexes(entityKeys, idIndex ?? -1, fromIndex, toIndex); + var indexes = GetSortedKeyIndexes(entityKeys, idIndex, fromIndex, toIndex); if (batchableCache == null) { for (var j = 0; j < entityKeys.Count; j++) diff --git a/src/NHibernate/Engine/BatchFetchQueue.cs b/src/NHibernate/Engine/BatchFetchQueue.cs index ad23b80e528..0202507ad7c 100644 --- a/src/NHibernate/Engine/BatchFetchQueue.cs +++ b/src/NHibernate/Engine/BatchFetchQueue.cs @@ -271,9 +271,7 @@ bool CheckCacheAndProcessResult() ? collectionKeys.Count - Math.Min(batchSize, collectionKeys.Count) : 0; var toIndex = collectionKeys.Count - 1; - // In case the collection having triggered the load was not registered for batching, load - // most recently registered collections. - var indexes = GetSortedKeyIndexes(collectionKeys, keyIndex ?? -1, fromIndex, toIndex); + var indexes = GetSortedKeyIndexes(collectionKeys, keyIndex, fromIndex, toIndex); if (batchableCache == null) { for (var j = 0; j < collectionKeys.Count; j++) @@ -457,10 +455,7 @@ bool CheckCacheAndProcessResult() ? entityKeys.Count - Math.Min(batchSize, entityKeys.Count) : 0; var toIndex = entityKeys.Count - 1; - // In case of an ISession.Get on an entity not already loaded as an uninitialized proxy, - // it will not be found in entities awaiting a load, and idIndex will be null. Providing - // -1 then allows to take the entities most recently loaded as uninitialized proxies. - var indexes = GetSortedKeyIndexes(entityKeys, idIndex ?? -1, fromIndex, toIndex); + var indexes = GetSortedKeyIndexes(entityKeys, idIndex, fromIndex, toIndex); if (batchableCache == null) { for (var j = 0; j < entityKeys.Count; j++) @@ -626,13 +621,15 @@ private bool[] AreCached(ListThe index where the sorting will begin. /// The index where the sorting will end. /// An array of sorted key indexes. - private static int[] GetSortedKeyIndexes(List> keys, int keyIndex, int fromIndex, int toIndex) + private static int[] GetSortedKeyIndexes(List> keys, int? keyIndex, int fromIndex, int toIndex) { var result = new int[Math.Abs(toIndex - fromIndex) + 1]; var lowerIndexes = new List(); var i = 0; for (var j = fromIndex; j <= toIndex; j++) { + // If the index was not found (null), this test will be falsy and it will take the most recently + // registered entities or collections. if (keys[j].Value < keyIndex) { lowerIndexes.Add(j);