Fix version update on dirty collection with another property having optimistic-lock false (#3632)

Co-authored-by: CSharper2010 <[email protected]>

Author: wvopt

src/NHibernate.Test/Async/VersionTest/Db/DbVersionFixture.cs

@@ -52,6 +52,7 @@ public async System.Threading.Tasks.Task CollectionVersionAsync()
 			admin = await (s.GetAsync<Group>(admin.Id));
 			guy.Groups.Add(admin);
 			admin.Users.Add(guy);
+			guy.NoOptimisticLock = "changed";
 			await (t.CommitAsync());
 			s.Close();
 

src/NHibernate.Test/VersionTest/Db/DbVersionFixture.cs

@@ -42,6 +42,7 @@ public void CollectionVersion()
 			admin = s.Get<Group>(admin.Id);
 			guy.Groups.Add(admin);
 			admin.Users.Add(guy);
+			guy.NoOptimisticLock = "changed";
 			t.Commit();
 			s.Close();
 

src/NHibernate.Test/VersionTest/Db/User.cs

@@ -11,8 +11,10 @@ public class User
 
 		public virtual string Username { get; set; }
 
+		public virtual string NoOptimisticLock { get; set; }
+
 		public virtual ISet<Group> Groups { get; set; }
 
 		public virtual ISet<Permission> Permissions { get; set; }
 	}
-}
+}

src/NHibernate.Test/VersionTest/Db/User.hbm.xml

@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8" ?>
+<?xml version="1.0" encoding="utf-8" ?>
 <!--
     Demonstrates how to control the optimistic locking behavior
     of a collection (do changes to the collection result in
@@ -14,6 +14,7 @@
 		</id>
 		<timestamp name="Timestamp" column="ts" source="db"/>
 		<property name="Username" column="user_name" type="string" unique="true"/>
+		<property name="NoOptimisticLock" column="no_optimistic_lock" type="string" optimistic-lock="false"/>
 		<set name="Groups" table="db_vers_user_group" batch-size="9" inverse="true" optimistic-lock="true" lazy="true" cascade="none" >
 			<key column="user_id"/>
 			<many-to-many column="group_id" class="Group" lazy="false" fetch="join" />
@@ -45,4 +46,4 @@
 		<property name="Context" column="ctx" type="string"/>
 		<property name="Access" column="priv" type="string"/>
 	</class>
-</hibernate-mapping>
+</hibernate-mapping>

src/NHibernate/Async/Event/Default/DefaultFlushEntityEventListener.cs

@@ -336,33 +336,35 @@ private async Task<object> GetNextVersionAsync(FlushEntityEvent @event, Cancella
 		/// to synchronize its state to the database. Modifies the event by side-effect!
 		/// Note: this method is quite slow, avoid calling if possible!
 		/// </summary>
-		protected Task<bool> IsUpdateNecessaryAsync(FlushEntityEvent @event, CancellationToken cancellationToken)
+		protected async Task<bool> IsUpdateNecessaryAsync(FlushEntityEvent @event, CancellationToken cancellationToken)
 		{
-			if (cancellationToken.IsCancellationRequested)
-			{
-				return Task.FromCanceled<bool>(cancellationToken);
-			}
+			cancellationToken.ThrowIfCancellationRequested();
 			IEntityPersister persister = @event.EntityEntry.Persister;
 			Status status = @event.EntityEntry.Status;
 
 			if (!@event.DirtyCheckPossible)
 			{
-				return Task.FromResult<bool>(true);
+				return true;
 			}
 			else
 			{
+				// call to HasDirtyCollections must not be optimized away because of its side effect
+				bool hasDirtyCollections = await (HasDirtyCollectionsAsync(@event, persister, status, cancellationToken)).ConfigureAwait(false);
+
 				int[] dirtyProperties = @event.DirtyProperties;
-				if (dirtyProperties != null && dirtyProperties.Length != 0)
-				{
-					return Task.FromResult<bool>(true); //TODO: suck into event class
-				}
-				else
-				{
-					return HasDirtyCollectionsAsync(@event, persister, status, cancellationToken);
-				}
+				return dirtyProperties != null && dirtyProperties.Length != 0 || hasDirtyCollections;
 			}
 		}
 
+		/// <summary>
+		/// Check if there are any dirty collections.
+		/// Has a side effect of setting the HasDirtyCollection property of the event.
+		/// </summary>
+		/// <param name="event"></param>
+		/// <param name="persister"></param>
+		/// <param name="status"></param>
+		/// <param name="cancellationToken">A cancellation token that can be used to cancel the work</param>
+		/// <returns></returns>
 		private async Task<bool> HasDirtyCollectionsAsync(FlushEntityEvent @event, IEntityPersister persister, Status status, CancellationToken cancellationToken)
 		{
 			cancellationToken.ThrowIfCancellationRequested();

src/NHibernate/Event/Default/DefaultFlushEntityEventListener.cs

@@ -389,18 +389,22 @@ protected bool IsUpdateNecessary(FlushEntityEvent @event)
 			}
 			else
 			{
+				// call to HasDirtyCollections must not be optimized away because of its side effect
+				bool hasDirtyCollections = HasDirtyCollections(@event, persister, status);
+
 				int[] dirtyProperties = @event.DirtyProperties;
-				if (dirtyProperties != null && dirtyProperties.Length != 0)
-				{
-					return true; //TODO: suck into event class
-				}
-				else
-				{
-					return HasDirtyCollections(@event, persister, status);
-				}
+				return dirtyProperties != null && dirtyProperties.Length != 0 || hasDirtyCollections;
 			}
 		}
 
+		/// <summary>
+		/// Check if there are any dirty collections.
+		/// Has a side effect of setting the HasDirtyCollection property of the event.
+		/// </summary>
+		/// <param name="event"></param>
+		/// <param name="persister"></param>
+		/// <param name="status"></param>
+		/// <returns></returns>
 		private bool HasDirtyCollections(FlushEntityEvent @event, IEntityPersister persister, Status status)
 		{
 			if (IsCollectionDirtyCheckNecessary(persister, status))