Skip to content

Commit 3adcfa7

Browse files
fredericDelaportefredericDelaporte
authored
Fix deletion of component with null property (#1755)
Fixes #1170 - Incorrect query when items removed from a collection of components contain null values
1 parent 9f2997c commit 3adcfa7

File tree

14 files changed

+486
-108
lines changed

14 files changed

+486
-108
lines changed

‎doc/reference/modules/component_mapping.xml

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -138,16 +138,6 @@
138138
model and persistence semantics are still slightly different.
139139
</para>
140140

141-
<para>
142-
A composite element mapping does not support null-able properties if you are using
143-
a <literal>&lt;set&gt;</literal>. There is no separate primary key column in the
144-
composite element table. NHibernate uses each column's value to identify a record
145-
when deleting objects, which is not possible with null values. You have to either
146-
use only not-null properties in a composite-element or choose a
147-
<literal>&lt;list&gt;</literal>, <literal>&lt;map&gt;</literal>,
148-
<literal>&lt;bag&gt;</literal> or <literal>&lt;idbag&gt;</literal>.
149-
</para>
150-
151141
<para>
152142
A special case of a composite element is a composite element with a nested
153143
<literal>&lt;many-to-one&gt;</literal> element. This mapping allows you to map extra

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

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
//------------------------------------------------------------------------------
2+
// <auto-generated>
3+
// This code was generated by AsyncGenerator.
4+
//
5+
// Changes to this file may cause incorrect behavior and will be lost if
6+
// the code is regenerated.
7+
// </auto-generated>
8+
//------------------------------------------------------------------------------
9+
10+
11+
using System.Linq;
12+
using NUnit.Framework;
13+
using NHibernate.Linq;
14+
15+
namespace NHibernate.Test.NHSpecificTest.GH1170
16+
{
17+
using System.Threading.Tasks;
18+
[TestFixture]
19+
public class FixtureAsync : BugTestCase
20+
{
21+
// Only the set case is tested, because other cases were not affected:
22+
// - bags delete everything first.
23+
// - indexed collections use their index, which is currently not mappable as a composite index with nullable
24+
// column. All index columns are forced to not-nullable by mapping implementation. When using a formula in
25+
// index, they use the element, but its columns are also forced to not-nullable.
26+
27+
[Test]
28+
public async Task DeleteComponentWithNullAsync()
29+
{
30+
using (var session = OpenSession())
31+
using (var tx = session.BeginTransaction())
32+
{
33+
var parent = await (session.Query<Parent>().SingleAsync());
34+
Assert.That(
35+
parent.ChildComponents,
36+
Has.Count.EqualTo(2).And.One.Property(nameof(ChildComponent.SomeString)).Null);
37+
parent.ChildComponents.Remove(parent.ChildComponents.Single(c => c.SomeString == null));
38+
await (tx.CommitAsync());
39+
}
40+
41+
using (var session = OpenSession())
42+
using (var tx = session.BeginTransaction())
43+
{
44+
var parent = await (session.Query<Parent>().SingleAsync());
45+
Assert.That(
46+
parent.ChildComponents,
47+
Has.Count.EqualTo(1).And.None.Property(nameof(ChildComponent.SomeString)).Null);
48+
await (tx.CommitAsync());
49+
}
50+
}
51+
52+
[Test]
53+
public async Task UpdateComponentWithNullAsync()
54+
{
55+
// Updates on set are indeed handled as delete/insert, so this test is not really needed.
56+
using (var session = OpenSession())
57+
using (var tx = session.BeginTransaction())
58+
{
59+
var parent = await (session.Query<Parent>().SingleAsync());
60+
Assert.That(
61+
parent.ChildComponents,
62+
Has.Count.EqualTo(2).And.One.Property(nameof(ChildComponent.SomeString)).Null);
63+
parent.ChildComponents.Single(c => c.SomeString == null).SomeString = "no more null";
64+
await (tx.CommitAsync());
65+
}
66+
67+
using (var session = OpenSession())
68+
using (var tx = session.BeginTransaction())
69+
{
70+
var parent = await (session.Query<Parent>().SingleAsync());
71+
Assert.That(
72+
parent.ChildComponents,
73+
Has.Count.EqualTo(2).And.None.Property(nameof(ChildComponent.SomeString)).Null);
74+
await (tx.CommitAsync());
75+
}
76+
}
77+
78+
protected override void OnSetUp()
79+
{
80+
using (var session = OpenSession())
81+
using (var tx = session.BeginTransaction())
82+
{
83+
var parent = new Parent();
84+
parent.ChildComponents.Add(new ChildComponent { SomeBool = true, SomeString = "something" });
85+
parent.ChildComponents.Add(new ChildComponent { SomeBool = false, SomeString = null });
86+
session.Save(parent);
87+
88+
tx.Commit();
89+
}
90+
}
91+
92+
protected override void OnTearDown()
93+
{
94+
using (var session = OpenSession())
95+
using (var tx = session.BeginTransaction())
96+
{
97+
session.Delete("from Parent");
98+
tx.Commit();
99+
}
100+
}
101+
}
102+
}

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

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
using System.Linq;
2+
using NUnit.Framework;
3+
4+
namespace NHibernate.Test.NHSpecificTest.GH1170
5+
{
6+
[TestFixture]
7+
public class Fixture : BugTestCase
8+
{
9+
// Only the set case is tested, because other cases were not affected:
10+
// - bags delete everything first.
11+
// - indexed collections use their index, which is currently not mappable as a composite index with nullable
12+
// column. All index columns are forced to not-nullable by mapping implementation. When using a formula in
13+
// index, they use the element, but its columns are also forced to not-nullable.
14+
15+
[Test]
16+
public void DeleteComponentWithNull()
17+
{
18+
using (var session = OpenSession())
19+
using (var tx = session.BeginTransaction())
20+
{
21+
var parent = session.Query<Parent>().Single();
22+
Assert.That(
23+
parent.ChildComponents,
24+
Has.Count.EqualTo(2).And.One.Property(nameof(ChildComponent.SomeString)).Null);
25+
parent.ChildComponents.Remove(parent.ChildComponents.Single(c => c.SomeString == null));
26+
tx.Commit();
27+
}
28+
29+
using (var session = OpenSession())
30+
using (var tx = session.BeginTransaction())
31+
{
32+
var parent = session.Query<Parent>().Single();
33+
Assert.That(
34+
parent.ChildComponents,
35+
Has.Count.EqualTo(1).And.None.Property(nameof(ChildComponent.SomeString)).Null);
36+
tx.Commit();
37+
}
38+
}
39+
40+
[Test]
41+
public void UpdateComponentWithNull()
42+
{
43+
// Updates on set are indeed handled as delete/insert, so this test is not really needed.
44+
using (var session = OpenSession())
45+
using (var tx = session.BeginTransaction())
46+
{
47+
var parent = session.Query<Parent>().Single();
48+
Assert.That(
49+
parent.ChildComponents,
50+
Has.Count.EqualTo(2).And.One.Property(nameof(ChildComponent.SomeString)).Null);
51+
parent.ChildComponents.Single(c => c.SomeString == null).SomeString = "no more null";
52+
tx.Commit();
53+
}
54+
55+
using (var session = OpenSession())
56+
using (var tx = session.BeginTransaction())
57+
{
58+
var parent = session.Query<Parent>().Single();
59+
Assert.That(
60+
parent.ChildComponents,
61+
Has.Count.EqualTo(2).And.None.Property(nameof(ChildComponent.SomeString)).Null);
62+
tx.Commit();
63+
}
64+
}
65+
66+
protected override void OnSetUp()
67+
{
68+
using (var session = OpenSession())
69+
using (var tx = session.BeginTransaction())
70+
{
71+
var parent = new Parent();
72+
parent.ChildComponents.Add(new ChildComponent { SomeBool = true, SomeString = "something" });
73+
parent.ChildComponents.Add(new ChildComponent { SomeBool = false, SomeString = null });
74+
session.Save(parent);
75+
76+
tx.Commit();
77+
}
78+
}
79+
80+
protected override void OnTearDown()
81+
{
82+
using (var session = OpenSession())
83+
using (var tx = session.BeginTransaction())
84+
{
85+
session.Delete("from Parent");
86+
tx.Commit();
87+
}
88+
}
89+
}
90+
}

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
<?xml version="1.0" encoding="utf-8" ?>
2+
<hibernate-mapping xmlns="urn:nhibernate-mapping-2.2"
3+
assembly="NHibernate.Test"
4+
namespace="NHibernate.Test.NHSpecificTest.GH1170">
5+
<class name="Parent">
6+
<id name="Id" column="id_column_of_parent">
7+
<generator class="guid" />
8+
</id>
9+
<set name="ChildComponents">
10+
<key column="key_column_of_child" />
11+
<composite-element class="ChildComponent">
12+
<property name="SomeBool" />
13+
<property name="SomeString" />
14+
</composite-element>
15+
</set>
16+
</class>
17+
</hibernate-mapping>

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
using System;
2+
using System.Collections.Generic;
3+
4+
namespace NHibernate.Test.NHSpecificTest.GH1170
5+
{
6+
public class Parent
7+
{
8+
public virtual ICollection<ChildComponent> ChildComponents { get; set; } = new List<ChildComponent>();
9+
public virtual Guid Id { get; set; }
10+
}
11+
12+
public class ChildComponent
13+
{
14+
public virtual bool SomeBool { get; set; }
15+
public virtual string SomeString { get; set; }
16+
17+
protected bool Equals(ChildComponent other)
18+
{
19+
return SomeBool == other.SomeBool && string.Equals(SomeString, other.SomeString);
20+
}
21+
22+
public override bool Equals(object obj)
23+
{
24+
if (ReferenceEquals(null, obj)) return false;
25+
if (ReferenceEquals(this, obj)) return true;
26+
return obj is ChildComponent other && Equals(other);
27+
}
28+
29+
public override int GetHashCode()
30+
{
31+
unchecked
32+
{
33+
return (SomeBool.GetHashCode() * 397) ^ (SomeString != null ? SomeString.GetHashCode() : 0);
34+
}
35+
}
36+
}
37+
}

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

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
using System;
1212
using System.Collections;
13+
using System.Collections.Concurrent;
1314
using System.Collections.Generic;
1415
using System.Data;
1516
using System.Data.Common;
@@ -32,7 +33,7 @@
3233
using NHibernate.SqlTypes;
3334
using NHibernate.Type;
3435
using NHibernate.Util;
35-
using Array=NHibernate.Mapping.Array;
36+
using Array = NHibernate.Mapping.Array;
3637

3738
namespace NHibernate.Persister.Collection
3839
{
@@ -138,7 +139,7 @@ protected async Task<int> WriteIndexAsync(DbCommand st, object idx, int i, ISess
138139
return i + ArrayHelper.CountTrue(indexColumnIsSettable);
139140
}
140141

141-
protected Task<int> WriteElementToWhereAsync(DbCommand st, object elt, int i, ISessionImplementor session, CancellationToken cancellationToken)
142+
protected Task<int> WriteElementToWhereAsync(DbCommand st, object elt, bool[] columnNullness, int i, ISessionImplementor session, CancellationToken cancellationToken)
142143
{
143144
if (elementIsPureFormula)
144145
{
@@ -152,11 +153,26 @@ protected Task<int> WriteElementToWhereAsync(DbCommand st, object elt, int i, IS
152153
async Task<int> InternalWriteElementToWhereAsync()
153154
{
154155

155-
await (ElementType.NullSafeSetAsync(st, elt, i, elementColumnIsInPrimaryKey, session, cancellationToken)).ConfigureAwait(false);
156-
return i + elementColumnAliases.Length;
156+
var settable = Combine(elementColumnIsInPrimaryKey, columnNullness);
157+
158+
await (ElementType.NullSafeSetAsync(st, elt, i, settable, session, cancellationToken)).ConfigureAwait(false);
159+
return i + settable.Count(s => s);
160+
}
161+
}
162+
163+
// Since v5.2
164+
[Obsolete("Use overload with columnNullness instead")]
165+
protected Task<int> WriteElementToWhereAsync(DbCommand st, object elt, int i, ISessionImplementor session, CancellationToken cancellationToken)
166+
{
167+
if (cancellationToken.IsCancellationRequested)
168+
{
169+
return Task.FromCanceled<int>(cancellationToken);
157170
}
171+
return WriteElementToWhereAsync(st, elt, null, i, session, cancellationToken);
158172
}
159173

174+
// No column nullness handling here: although a composite index could have null columns, the mapping
175+
// current implementation forbirds this by forcing not-null to true on all columns.
160176
protected Task<int> WriteIndexToWhereAsync(DbCommand st, object index, int i, ISessionImplementor session, CancellationToken cancellationToken)
161177
{
162178
if (indexContainsFormula)
@@ -333,19 +349,18 @@ public async Task DeleteRowsAsync(IPersistentCollection collection, object id, I
333349
DbCommand st;
334350
var expectation = Expectations.AppropriateExpectation(deleteCheckStyle);
335351
//var callable = DeleteCallable;
352+
var commandInfo = GetDeleteCommand(deleteByIndex, entry, out var columnNullness);
336353

337354
var useBatch = expectation.CanBeBatched;
338355
if (useBatch)
339356
{
340-
st =
341-
await (session.Batcher.PrepareBatchCommandAsync(SqlDeleteRowString.CommandType, SqlDeleteRowString.Text,
342-
SqlDeleteRowString.ParameterTypes, cancellationToken)).ConfigureAwait(false);
357+
st = await (session.Batcher.PrepareBatchCommandAsync(
358+
commandInfo.CommandType, commandInfo.Text, commandInfo.ParameterTypes, cancellationToken)).ConfigureAwait(false);
343359
}
344360
else
345361
{
346-
st =
347-
await (session.Batcher.PrepareCommandAsync(SqlDeleteRowString.CommandType, SqlDeleteRowString.Text,
348-
SqlDeleteRowString.ParameterTypes, cancellationToken)).ConfigureAwait(false);
362+
st = await (session.Batcher.PrepareCommandAsync(
363+
commandInfo.CommandType, commandInfo.Text, commandInfo.ParameterTypes, cancellationToken)).ConfigureAwait(false);
349364
}
350365
try
351366
{
@@ -364,7 +379,7 @@ public async Task DeleteRowsAsync(IPersistentCollection collection, object id, I
364379
}
365380
else
366381
{
367-
await (WriteElementToWhereAsync(st, entry, loc, session, cancellationToken)).ConfigureAwait(false);
382+
await (WriteElementToWhereAsync(st, entry, columnNullness, loc, session, cancellationToken)).ConfigureAwait(false);
368383
}
369384
}
370385
if (useBatch)

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,10 @@
2020
using NHibernate.Loader.Collection;
2121
using NHibernate.Persister.Entity;
2222
using NHibernate.SqlCommand;
23-
using NHibernate.SqlTypes;
2423
using NHibernate.Type;
2524
using NHibernate.Util;
2625
using System.Collections.Generic;
26+
using NHibernate.SqlTypes;
2727

2828
namespace NHibernate.Persister.Collection
2929
{
@@ -85,7 +85,10 @@ protected override async Task<int> DoUpdateRowsAsync(object id, IPersistentColle
8585
}
8686
else
8787
{
88-
await (WriteElementToWhereAsync(st, collection.GetSnapshotElement(entry, i), loc, session, cancellationToken)).ConfigureAwait(false);
88+
// No nullness handled on update: updates does not occurs with sets or bags, and
89+
// indexed collections allowing formula (maps) force their element columns to
90+
// not-nullable.
91+
await (WriteElementToWhereAsync(st, collection.GetSnapshotElement(entry, i), null, loc, session, cancellationToken)).ConfigureAwait(false);
8992
}
9093
}
9194

0 commit comments

Comments
 (0)