Skip to content

Commit 897474c

Browse files
fredericDelaportefredericDelaporte
committed
Fix deletion of component with null property
Fixes #1170
1 parent e7a6d6b commit 897474c

File tree

12 files changed

+344
-106
lines changed

12 files changed

+344
-106
lines changed

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

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,13 @@ namespace NHibernate.Test.NHSpecificTest.GH1170
1818
[TestFixture]
1919
public class FixtureAsync : BugTestCase
2020
{
21-
[Test, KnownBug("GH1170")]
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]
2228
public async Task DeleteComponentWithNullAsync()
2329
{
2430
using (var session = OpenSession())
@@ -43,6 +49,32 @@ public async Task DeleteComponentWithNullAsync()
4349
}
4450
}
4551

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+
4678
protected override void OnSetUp()
4779
{
4880
using (var session = OpenSession())

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

Lines changed: 33 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,13 @@ namespace NHibernate.Test.NHSpecificTest.GH1170
66
[TestFixture]
77
public class Fixture : BugTestCase
88
{
9-
[Test, KnownBug("GH1170")]
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]
1016
public void DeleteComponentWithNull()
1117
{
1218
using (var session = OpenSession())
@@ -31,6 +37,32 @@ public void DeleteComponentWithNull()
3137
}
3238
}
3339

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+
3466
protected override void OnSetUp()
3567
{
3668
using (var session = OpenSession())

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

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,18 +5,33 @@ namespace NHibernate.Test.NHSpecificTest.GH1170
55
{
66
public class Parent
77
{
8-
public Parent()
9-
{
10-
ChildComponents = new List<ChildComponent>();
11-
}
12-
13-
public virtual ICollection<ChildComponent> ChildComponents { get; set; }
8+
public virtual ICollection<ChildComponent> ChildComponents { get; set; } = new List<ChildComponent>();
149
public virtual Guid Id { get; set; }
1510
}
1611

1712
public class ChildComponent
1813
{
1914
public virtual bool SomeBool { get; set; }
2015
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+
}
2136
}
2237
}

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

Lines changed: 25 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,25 @@ 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+
[Obsolete("Use overload with columnNullness instead")]
164+
protected Task<int> WriteElementToWhereAsync(DbCommand st, object elt, int i, ISessionImplementor session, CancellationToken cancellationToken)
165+
{
166+
if (cancellationToken.IsCancellationRequested)
167+
{
168+
return Task.FromCanceled<int>(cancellationToken);
157169
}
170+
return WriteElementToWhereAsync(st, elt, null, i, session, cancellationToken);
158171
}
159172

173+
// No column nullness handling here: although a composite index could have null columns, the mapping
174+
// current implementation forbirds this by forcing not-null to true on all columns.
160175
protected Task<int> WriteIndexToWhereAsync(DbCommand st, object index, int i, ISessionImplementor session, CancellationToken cancellationToken)
161176
{
162177
if (indexContainsFormula)
@@ -333,19 +348,18 @@ public async Task DeleteRowsAsync(IPersistentCollection collection, object id, I
333348
DbCommand st;
334349
var expectation = Expectations.AppropriateExpectation(deleteCheckStyle);
335350
//var callable = DeleteCallable;
351+
var commandInfo = GetDeleteCommand(deleteByIndex, entry, out var columnNullness);
336352

337353
var useBatch = expectation.CanBeBatched;
338354
if (useBatch)
339355
{
340-
st =
341-
await (session.Batcher.PrepareBatchCommandAsync(SqlDeleteRowString.CommandType, SqlDeleteRowString.Text,
342-
SqlDeleteRowString.ParameterTypes, cancellationToken)).ConfigureAwait(false);
356+
st = await (session.Batcher.PrepareBatchCommandAsync(
357+
commandInfo.CommandType, commandInfo.Text, commandInfo.ParameterTypes, cancellationToken)).ConfigureAwait(false);
343358
}
344359
else
345360
{
346-
st =
347-
await (session.Batcher.PrepareCommandAsync(SqlDeleteRowString.CommandType, SqlDeleteRowString.Text,
348-
SqlDeleteRowString.ParameterTypes, cancellationToken)).ConfigureAwait(false);
361+
st = await (session.Batcher.PrepareCommandAsync(
362+
commandInfo.CommandType, commandInfo.Text, commandInfo.ParameterTypes, cancellationToken)).ConfigureAwait(false);
349363
}
350364
try
351365
{
@@ -364,7 +378,7 @@ public async Task DeleteRowsAsync(IPersistentCollection collection, object id, I
364378
}
365379
else
366380
{
367-
await (WriteElementToWhereAsync(st, entry, loc, session, cancellationToken)).ConfigureAwait(false);
381+
await (WriteElementToWhereAsync(st, entry, columnNullness, loc, session, cancellationToken)).ConfigureAwait(false);
368382
}
369383
}
370384
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

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ protected override async Task<int> DoUpdateRowsAsync(object id, IPersistentColle
5555
{
5656
if (await (collection.NeedsUpdatingAsync(entry, i, ElementType, cancellationToken)).ConfigureAwait(false))
5757
{
58-
DbCommand st = null;
58+
DbCommand st;
5959
// will still be issued when it used to be null
6060
if (useBatch)
6161
{
@@ -71,7 +71,9 @@ protected override async Task<int> DoUpdateRowsAsync(object id, IPersistentColle
7171
try
7272
{
7373
int loc = await (WriteKeyAsync(st, id, offset, session, cancellationToken)).ConfigureAwait(false);
74-
await (WriteElementToWhereAsync(st, collection.GetSnapshotElement(entry, i), loc, session, cancellationToken)).ConfigureAwait(false);
74+
// No columnNullness handling: the element is the entity key and should not contain null
75+
// values.
76+
await (WriteElementToWhereAsync(st, collection.GetSnapshotElement(entry, i), null, loc, session, cancellationToken)).ConfigureAwait(false);
7577
if (useBatch)
7678
{
7779
await (session.Batcher.AddToBatchAsync(deleteExpectation, cancellationToken)).ConfigureAwait(false);
@@ -117,7 +119,7 @@ protected override async Task<int> DoUpdateRowsAsync(object id, IPersistentColle
117119
{
118120
if (await (collection.NeedsUpdatingAsync(entry, i, ElementType, cancellationToken)).ConfigureAwait(false))
119121
{
120-
DbCommand st = null;
122+
DbCommand st;
121123
if (useBatch)
122124
{
123125
st = await (session.Batcher.PrepareBatchCommandAsync(SqlInsertRowString.CommandType, sql.Text,
@@ -137,7 +139,9 @@ protected override async Task<int> DoUpdateRowsAsync(object id, IPersistentColle
137139
{
138140
loc = await (WriteIndexToWhereAsync(st, collection.GetIndex(entry, i, this), loc, session, cancellationToken)).ConfigureAwait(false);
139141
}
140-
await (WriteElementToWhereAsync(st, collection.GetElement(entry), loc, session, cancellationToken)).ConfigureAwait(false);
142+
// No columnNullness handling: the element is the entity key and should not contain null
143+
// values.
144+
await (WriteElementToWhereAsync(st, collection.GetElement(entry), null, loc, session, cancellationToken)).ConfigureAwait(false);
141145
if (useBatch)
142146
{
143147
await (session.Batcher.AddToBatchAsync(insertExpectation, cancellationToken)).ConfigureAwait(false);

‎src/NHibernate/Async/Type/IType.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -191,4 +191,4 @@ public partial interface IType : ICacheAssembler
191191
/// <returns>The value to be merged.</returns>
192192
Task<object> ReplaceAsync(object original, object target, ISessionImplementor session, object owner, IDictionary copyCache, ForeignKeyDirection foreignKeyDirection, CancellationToken cancellationToken);
193193
}
194-
}
194+
}

0 commit comments

Comments
 (0)