diff --git a/src/NHibernate.Test/Async/Extralazy/ExtraLazyFixture.cs b/src/NHibernate.Test/Async/Extralazy/ExtraLazyFixture.cs index 49cec3ba33c..4dd7e85fced 100644 --- a/src/NHibernate.Test/Async/Extralazy/ExtraLazyFixture.cs +++ b/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("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("HiA2")); + var hia = await (s.GetAsync("HiA")); + var gavin = await (s.GetAsync("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("HiA")); + var gavin = await (s.GetAsync("gavin")); + Assert.That(gavin.Documents, Has.Count.EqualTo(1).And.Contain(hia)); + Assert.That(await (s.GetAsync("HiA2")), Is.Null); + await (t.CommitAsync()); + } + } + [Test] public async Task GetAsync() { @@ -340,4 +396,4 @@ public async Task AddToUninitializedSetWithLaterLazyLoadAsync() } } } -} \ No newline at end of file +} diff --git a/src/NHibernate.Test/Extralazy/ExtraLazyFixture.cs b/src/NHibernate.Test/Extralazy/ExtraLazyFixture.cs index b6652c499ef..248abdf63e8 100644 --- a/src/NHibernate.Test/Extralazy/ExtraLazyFixture.cs +++ b/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("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("HiA2"); + var hia = s.Get("HiA"); + var gavin = s.Get("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("HiA"); + var gavin = s.Get("gavin"); + Assert.That(gavin.Documents, Has.Count.EqualTo(1).And.Contain(hia)); + Assert.That(s.Get("HiA2"), Is.Null); + t.Commit(); + } + } + [Test] public void Get() { @@ -329,4 +385,4 @@ public void AddToUninitializedSetWithLaterLazyLoad() } } } -} \ No newline at end of file +} diff --git a/src/NHibernate/Async/Collection/IPersistentCollection.cs b/src/NHibernate/Async/Collection/IPersistentCollection.cs index 19508b99376..14bdcec49fe 100644 --- a/src/NHibernate/Async/Collection/IPersistentCollection.cs +++ b/src/NHibernate/Async/Collection/IPersistentCollection.cs @@ -8,6 +8,7 @@ //------------------------------------------------------------------------------ +using System; using System.Collections; using System.Data.Common; using NHibernate.Collection.Generic; diff --git a/src/NHibernate/Async/Engine/Loading/CollectionLoadContext.cs b/src/NHibernate/Async/Engine/Loading/CollectionLoadContext.cs index 4a9899649ae..35f9d356193 100644 --- a/src/NHibernate/Async/Engine/Loading/CollectionLoadContext.cs +++ b/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,7 +161,7 @@ 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) @@ -169,6 +169,9 @@ private async Task EndLoadingCollectionAsync(LoadingCollectionEntry lce, ICollec 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)); diff --git a/src/NHibernate/Async/Event/Default/DefaultInitializeCollectionEventListener.cs b/src/NHibernate/Async/Event/Default/DefaultInitializeCollectionEventListener.cs index 4e71de9ca1d..29df2ebbd33 100644 --- a/src/NHibernate/Async/Event/Default/DefaultInitializeCollectionEventListener.cs +++ b/src/NHibernate/Async/Event/Default/DefaultInitializeCollectionEventListener.cs @@ -161,6 +161,9 @@ private async Task 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; } } diff --git a/src/NHibernate/Collection/AbstractPersistentCollection.cs b/src/NHibernate/Collection/AbstractPersistentCollection.cs index 78ed8a90a92..9387b9f8ff0 100644 --- a/src/NHibernate/Collection/AbstractPersistentCollection.cs +++ b/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 } - /// + // Obsolete since v5.2 + /// /// After reading all existing elements from the database, /// add the queued elements to the underlying collection. /// + [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() } } + /// + /// After reading all existing elements from the database, do the queued operations + /// (adds or removes) on the underlying collection. + /// + 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; } /// diff --git a/src/NHibernate/Collection/Generic/PersistentGenericBag.cs b/src/NHibernate/Collection/Generic/PersistentGenericBag.cs index 14cd9f7688b..67564a5de3e 100644 --- a/src/NHibernate/Collection/Generic/PersistentGenericBag.cs +++ b/src/NHibernate/Collection/Generic/PersistentGenericBag.cs @@ -48,6 +48,7 @@ public partial class PersistentGenericBag : AbstractPersistentCollection, ILi * ! */ private IList _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(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) 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); } } diff --git a/src/NHibernate/Collection/IPersistentCollection.cs b/src/NHibernate/Collection/IPersistentCollection.cs index c16f4bef857..2021eceff12 100644 --- a/src/NHibernate/Collection/IPersistentCollection.cs +++ b/src/NHibernate/Collection/IPersistentCollection.cs @@ -1,3 +1,4 @@ +using System; using System.Collections; using System.Data.Common; using NHibernate.Collection.Generic; @@ -337,4 +338,40 @@ public partial interface IPersistentCollection /// ICollection GetOrphans(object snapshot, string entityName); } + + // 6.0 TODO: merge into IPersistentCollection + public static class PersistentCollectionExtensions + { + private static readonly INHibernateLogger Logger = NHibernateLogger.For(typeof(PersistentCollectionExtensions)); + + /// + /// After reading all existing elements from the database, do the queued operations + /// (adds or removes) on the underlying collection. + /// + /// The collection. + public static void ApplyQueuedOperations(this IPersistentCollection collection) + { + if (collection is AbstractPersistentCollection baseImpl) + { + baseImpl.ApplyQueuedOperations(); + return; + } + + // Fallback on reflection for custom implementations + var collectionType = collection.GetType(); + var applyQueuedOperationsMethod = collectionType.GetMethod( + nameof(AbstractPersistentCollection.ApplyQueuedOperations), + Array.Empty()); + if (applyQueuedOperationsMethod != null) + { + applyQueuedOperationsMethod.Invoke(collection, Array.Empty()); + return; + } + + Logger.Warn( + "{0} does not implement 'void ApplyQueuedOperations()'. It should move any queued operations" + + "processing out of 'AfterInitialize' and put it in a 'public void ApplyQueuedOperations()'.", + collectionType); + } + } } diff --git a/src/NHibernate/Engine/Loading/CollectionLoadContext.cs b/src/NHibernate/Engine/Loading/CollectionLoadContext.cs index 588f17f1ad2..d74ac7abcc8 100644 --- a/src/NHibernate/Engine/Loading/CollectionLoadContext.cs +++ b/src/NHibernate/Engine/Loading/CollectionLoadContext.cs @@ -252,7 +252,7 @@ private void EndLoadingCollection(LoadingCollectionEntry lce, ICollectionPersist 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()) { @@ -269,7 +269,7 @@ private void EndLoadingCollection(LoadingCollectionEntry lce, ICollectionPersist 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) @@ -277,6 +277,9 @@ private void EndLoadingCollection(LoadingCollectionEntry lce, ICollectionPersist AddCollectionToCache(lce, persister, cacheBatchingHandler); } + if (!hasNoQueuedOperations) + lce.Collection.ApplyQueuedOperations(); + if (log.IsDebugEnabled()) { log.Debug("collection fully initialized: {0}", MessageHelper.CollectionInfoString(persister, lce.Collection, lce.Key, session)); diff --git a/src/NHibernate/Event/Default/DefaultInitializeCollectionEventListener.cs b/src/NHibernate/Event/Default/DefaultInitializeCollectionEventListener.cs index f1bdf215f8b..a93c3e9642a 100644 --- a/src/NHibernate/Event/Default/DefaultInitializeCollectionEventListener.cs +++ b/src/NHibernate/Event/Default/DefaultInitializeCollectionEventListener.cs @@ -148,6 +148,9 @@ private bool Assemble(CacheKey ck, object ce, ICollectionPersister persister, I cacheEntry.Assemble(collection, persister, persistenceContext.GetCollectionOwner(id, persister)); persistenceContext.GetCollectionEntry(collection).PostInitialize(collection, persistenceContext); + + if (collection.HasQueuedOperations) + collection.ApplyQueuedOperations(); return true; } }