Fix deletion of component with null property

doc/reference/modules/component_mapping.xml

@@ -138,16 +138,6 @@
             model and persistence semantics are still slightly different.
         </para>
 
-        <para>
-            A composite element mapping does not support null-able properties if you are using
-            a <literal>&lt;set&gt;</literal>. There is no separate primary key column in the
-            composite element table. NHibernate uses each column's value to identify a record
-            when deleting objects, which is not possible with null values. You have to either
-            use only not-null properties in a composite-element or choose a
-            <literal>&lt;list&gt;</literal>, <literal>&lt;map&gt;</literal>,
-            <literal>&lt;bag&gt;</literal> or <literal>&lt;idbag&gt;</literal>.
-        </para>
-
         <para>
             A special case of a composite element is a composite element with a nested
             <literal>&lt;many-to-one&gt;</literal> element. This mapping allows you to map extra

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

@@ -0,0 +1,102 @@
+//------------------------------------------------------------------------------
+// <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 NUnit.Framework;
+using NHibernate.Linq;
+
+namespace NHibernate.Test.NHSpecificTest.GH1170
+{
+	using System.Threading.Tasks;
+	[TestFixture]
+	public class FixtureAsync : BugTestCase
+	{
+		// Only the set case is tested, because other cases were not affected:
+		// - bags delete everything first.
+		// - indexed collections use their index, which is currently not mappable as a composite index with nullable
+		//   column. All index columns are forced to not-nullable by mapping implementation. When using a formula in
+		//   index, they use the element, but its columns are also forced to not-nullable.
+
+		[Test]
+		public async Task DeleteComponentWithNullAsync()
+		{
+			using (var session = OpenSession())
+			using (var tx = session.BeginTransaction())
+			{
+				var parent = await (session.Query<Parent>().SingleAsync());
+				Assert.That(
+					parent.ChildComponents,
+					Has.Count.EqualTo(2).And.One.Property(nameof(ChildComponent.SomeString)).Null);
+				parent.ChildComponents.Remove(parent.ChildComponents.Single(c => c.SomeString == null));
+				await (tx.CommitAsync());
+			}
+
+			using (var session = OpenSession())
+			using (var tx = session.BeginTransaction())
+			{
+				var parent = await (session.Query<Parent>().SingleAsync());
+				Assert.That(
+					parent.ChildComponents,
+					Has.Count.EqualTo(1).And.None.Property(nameof(ChildComponent.SomeString)).Null);
+				await (tx.CommitAsync());
+			}
+		}
+
+		[Test]
+		public async Task UpdateComponentWithNullAsync()
+		{
+			// Updates on set are indeed handled as delete/insert, so this test is not really needed.
+			using (var session = OpenSession())
+			using (var tx = session.BeginTransaction())
+			{
+				var parent = await (session.Query<Parent>().SingleAsync());
+				Assert.That(
+					parent.ChildComponents,
+					Has.Count.EqualTo(2).And.One.Property(nameof(ChildComponent.SomeString)).Null);
+				parent.ChildComponents.Single(c => c.SomeString == null).SomeString = "no more null";
+				await (tx.CommitAsync());
+			}
+
+			using (var session = OpenSession())
+			using (var tx = session.BeginTransaction())
+			{
+				var parent = await (session.Query<Parent>().SingleAsync());
+				Assert.That(
+					parent.ChildComponents,
+					Has.Count.EqualTo(2).And.None.Property(nameof(ChildComponent.SomeString)).Null);
+				await (tx.CommitAsync());
+			}
+		}
+
+		protected override void OnSetUp()
+		{
+			using (var session = OpenSession())
+			using (var tx = session.BeginTransaction())
+			{
+				var parent = new Parent();
+				parent.ChildComponents.Add(new ChildComponent { SomeBool = true, SomeString = "something" });
+				parent.ChildComponents.Add(new ChildComponent { SomeBool = false, SomeString = null });
+				session.Save(parent);
+
+				tx.Commit();
+			}
+		}
+
+		protected override void OnTearDown()
+		{
+			using (var session = OpenSession())
+			using (var tx = session.BeginTransaction())
+			{
+				session.Delete("from Parent");
+				tx.Commit();
+			}
+		}
+	}
+}

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

@@ -0,0 +1,90 @@
+using System.Linq;
+using NUnit.Framework;
+
+namespace NHibernate.Test.NHSpecificTest.GH1170
+{
+	[TestFixture]
+	public class Fixture : BugTestCase
+	{
+		// Only the set case is tested, because other cases were not affected:
+		// - bags delete everything first.
+		// - indexed collections use their index, which is currently not mappable as a composite index with nullable
+		//   column. All index columns are forced to not-nullable by mapping implementation. When using a formula in
+		//   index, they use the element, but its columns are also forced to not-nullable.
+
+		[Test]
+		public void DeleteComponentWithNull()
+		{
+			using (var session = OpenSession())
+			using (var tx = session.BeginTransaction())
+			{
+				var parent = session.Query<Parent>().Single();
+				Assert.That(
+					parent.ChildComponents,
+					Has.Count.EqualTo(2).And.One.Property(nameof(ChildComponent.SomeString)).Null);
+				parent.ChildComponents.Remove(parent.ChildComponents.Single(c => c.SomeString == null));
+				tx.Commit();
+			}
+
+			using (var session = OpenSession())
+			using (var tx = session.BeginTransaction())
+			{
+				var parent = session.Query<Parent>().Single();
+				Assert.That(
+					parent.ChildComponents,
+					Has.Count.EqualTo(1).And.None.Property(nameof(ChildComponent.SomeString)).Null);
+				tx.Commit();
+			}
+		}
+
+		[Test]
+		public void UpdateComponentWithNull()
+		{
+			// Updates on set are indeed handled as delete/insert, so this test is not really needed.
+			using (var session = OpenSession())
+			using (var tx = session.BeginTransaction())
+			{
+				var parent = session.Query<Parent>().Single();
+				Assert.That(
+					parent.ChildComponents,
+					Has.Count.EqualTo(2).And.One.Property(nameof(ChildComponent.SomeString)).Null);
+				parent.ChildComponents.Single(c => c.SomeString == null).SomeString = "no more null";
+				tx.Commit();
+			}
+
+			using (var session = OpenSession())
+			using (var tx = session.BeginTransaction())
+			{
+				var parent = session.Query<Parent>().Single();
+				Assert.That(
+					parent.ChildComponents,
+					Has.Count.EqualTo(2).And.None.Property(nameof(ChildComponent.SomeString)).Null);
+				tx.Commit();
+			}
+		}
+
+		protected override void OnSetUp()
+		{
+			using (var session = OpenSession())
+			using (var tx = session.BeginTransaction())
+			{
+				var parent = new Parent();
+				parent.ChildComponents.Add(new ChildComponent { SomeBool = true, SomeString = "something" });
+				parent.ChildComponents.Add(new ChildComponent { SomeBool = false, SomeString = null });
+				session.Save(parent);
+
+				tx.Commit();
+			}
+		}
+
+		protected override void OnTearDown()
+		{
+			using (var session = OpenSession())
+			using (var tx = session.BeginTransaction())
+			{
+				session.Delete("from Parent");
+				tx.Commit();
+			}
+		}
+	}
+}

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

@@ -0,0 +1,17 @@
+<?xml version="1.0" encoding="utf-8" ?>
+<hibernate-mapping xmlns="urn:nhibernate-mapping-2.2"
+                   assembly="NHibernate.Test"
+                   namespace="NHibernate.Test.NHSpecificTest.GH1170">
+  <class name="Parent">
+    <id name="Id" column="id_column_of_parent">
+      <generator class="guid" />
+    </id>
+    <set name="ChildComponents">
+      <key column="key_column_of_child" />
+      <composite-element class="ChildComponent">
+        <property name="SomeBool" />
+        <property name="SomeString" />
+      </composite-element>
+    </set>
+  </class>
+</hibernate-mapping>

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

@@ -0,0 +1,37 @@
+using System;
+using System.Collections.Generic;
+
+namespace NHibernate.Test.NHSpecificTest.GH1170
+{
+	public class Parent
+	{
+		public virtual ICollection<ChildComponent> ChildComponents { get; set; } = new List<ChildComponent>();
+		public virtual Guid Id { get; set; }
+	}
+
+	public class ChildComponent
+	{
+		public virtual bool SomeBool { get; set; }
+		public virtual string SomeString { get; set; }
+
+		protected bool Equals(ChildComponent other)
+		{
+			return SomeBool == other.SomeBool && string.Equals(SomeString, other.SomeString);
+		}
+
+		public override bool Equals(object obj)
+		{
+			if (ReferenceEquals(null, obj)) return false;
+			if (ReferenceEquals(this, obj)) return true;
+			return obj is ChildComponent other && Equals(other);
+		}
+
+		public override int GetHashCode()
+		{
+			unchecked
+			{
+				return (SomeBool.GetHashCode() * 397) ^ (SomeString != null ? SomeString.GetHashCode() : 0);
+			}
+		}
+	}
+}

src/NHibernate/Async/Persister/Collection/AbstractCollectionPersister.cs

@@ -10,6 +10,7 @@
 
 using System;
 using System.Collections;
+using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Data;
 using System.Data.Common;
@@ -32,7 +33,7 @@
 using NHibernate.SqlTypes;
 using NHibernate.Type;
 using NHibernate.Util;
-using Array=NHibernate.Mapping.Array;
+using Array = NHibernate.Mapping.Array;
 
 namespace NHibernate.Persister.Collection
 {
@@ -138,7 +139,7 @@ protected async Task<int> WriteIndexAsync(DbCommand st, object idx, int i, ISess
 			return i + ArrayHelper.CountTrue(indexColumnIsSettable);
 		}
 
-		protected Task<int> WriteElementToWhereAsync(DbCommand st, object elt, int i, ISessionImplementor session, CancellationToken cancellationToken)
+		protected Task<int> WriteElementToWhereAsync(DbCommand st, object elt, bool[] columnNullness, int i, ISessionImplementor session, CancellationToken cancellationToken)
 		{
 			if (elementIsPureFormula)
 			{
@@ -152,11 +153,26 @@ protected Task<int> WriteElementToWhereAsync(DbCommand st, object elt, int i, IS
 			async Task<int> InternalWriteElementToWhereAsync()
 			{
 
-				await (ElementType.NullSafeSetAsync(st, elt, i, elementColumnIsInPrimaryKey, session, cancellationToken)).ConfigureAwait(false);
-				return i + elementColumnAliases.Length;
+				var settable = Combine(elementColumnIsInPrimaryKey, columnNullness);
+
+				await (ElementType.NullSafeSetAsync(st, elt, i, settable, session, cancellationToken)).ConfigureAwait(false);
+				return i + settable.Count(s => s);
+			}
+		}
+
+		// Since v5.2
+		[Obsolete("Use overload with columnNullness instead")]
+		protected Task<int> WriteElementToWhereAsync(DbCommand st, object elt, int i, ISessionImplementor session, CancellationToken cancellationToken)
+		{
+			if (cancellationToken.IsCancellationRequested)
+			{
+				return Task.FromCanceled<int>(cancellationToken);
 			}
+			return WriteElementToWhereAsync(st, elt, null, i, session, cancellationToken);
 		}
 
+		// No column nullness handling here: although a composite index could have null columns, the mapping
+		// current implementation forbirds this by forcing not-null to true on all columns.
 		protected Task<int> WriteIndexToWhereAsync(DbCommand st, object index, int i, ISessionImplementor session, CancellationToken cancellationToken)
 		{
 			if (indexContainsFormula)
@@ -333,19 +349,18 @@ public async Task DeleteRowsAsync(IPersistentCollection collection, object id, I
 						DbCommand st;
 						var expectation = Expectations.AppropriateExpectation(deleteCheckStyle);
 						//var callable = DeleteCallable;
+						var commandInfo = GetDeleteCommand(deleteByIndex, entry, out var columnNullness);
 
 						var useBatch = expectation.CanBeBatched;
 						if (useBatch)
 						{
-							st =
-								await (session.Batcher.PrepareBatchCommandAsync(SqlDeleteRowString.CommandType, SqlDeleteRowString.Text,
-																	SqlDeleteRowString.ParameterTypes, cancellationToken)).ConfigureAwait(false);
+							st = await (session.Batcher.PrepareBatchCommandAsync(
+								commandInfo.CommandType, commandInfo.Text, commandInfo.ParameterTypes, cancellationToken)).ConfigureAwait(false);
 						}
 						else
 						{
-							st =
-								await (session.Batcher.PrepareCommandAsync(SqlDeleteRowString.CommandType, SqlDeleteRowString.Text,
-																SqlDeleteRowString.ParameterTypes, cancellationToken)).ConfigureAwait(false);
+							st = await (session.Batcher.PrepareCommandAsync(
+								commandInfo.CommandType, commandInfo.Text, commandInfo.ParameterTypes, cancellationToken)).ConfigureAwait(false);
 						}
 						try
 						{
@@ -364,7 +379,7 @@ public async Task DeleteRowsAsync(IPersistentCollection collection, object id, I
 								}
 								else
 								{
-									await (WriteElementToWhereAsync(st, entry, loc, session, cancellationToken)).ConfigureAwait(false);
+									await (WriteElementToWhereAsync(st, entry, columnNullness, loc, session, cancellationToken)).ConfigureAwait(false);
 								}
 							}
 							if (useBatch)

src/NHibernate/Async/Persister/Collection/BasicCollectionPersister.cs

@@ -20,10 +20,10 @@
 using NHibernate.Loader.Collection;
 using NHibernate.Persister.Entity;
 using NHibernate.SqlCommand;
-using NHibernate.SqlTypes;
 using NHibernate.Type;
 using NHibernate.Util;
 using System.Collections.Generic;
+using NHibernate.SqlTypes;
 
 namespace NHibernate.Persister.Collection
 {
@@ -85,7 +85,10 @@ protected override async Task<int> DoUpdateRowsAsync(object id, IPersistentColle
 								}
 								else
 								{
-									await (WriteElementToWhereAsync(st, collection.GetSnapshotElement(entry, i), loc, session, cancellationToken)).ConfigureAwait(false);
+									// No nullness handled on update: updates does not occurs with sets or bags, and
+									// indexed collections allowing formula (maps) force their element columns to
+									// not-nullable.
+									await (WriteElementToWhereAsync(st, collection.GetSnapshotElement(entry, i), null, loc, session, cancellationToken)).ConfigureAwait(false);
 								}
 							}