Skip to content

Fix #2608 - persisting a one-to-one with delayed insert fails #2609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jan 26, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions src/NHibernate.Test/Async/NHSpecificTest/GH2608/Fixture.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
//------------------------------------------------------------------------------
// <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 NUnit.Framework;

namespace NHibernate.Test.NHSpecificTest.GH2608
{
using System.Threading.Tasks;
[TestFixture]
public class FixtureAsync : BugTestCase
{
protected override void OnTearDown()
{
using (var session = OpenSession())
using (var transaction = session.BeginTransaction())
{
session.CreateQuery("delete from PersonalDetails").ExecuteUpdate();
session.CreateQuery("delete from System.Object").ExecuteUpdate();

transaction.Commit();
}
}

[Test]
public async Task MergeBidiPrimaryKeyOneToOneAsync()
{
using (var s = OpenSession())
using (var tx = s.BeginTransaction())
{
var p = new Person { Name = "steve" };
p.Details = new PersonalDetails { SomePersonalDetail = "I have big feet", Person = p };
await (s.MergeAsync(p));
await (tx.CommitAsync());
}
}

[Test]
public async Task PersistBidiPrimaryKeyOneToOneAsync()
{
using (var s = OpenSession())
using (var tx = s.BeginTransaction())
{
var p = new Person { Name = "steve" };
p.Details = new PersonalDetails { SomePersonalDetail = "I have big feet", Person = p };
await (s.PersistAsync(p));
await (tx.CommitAsync());
}
}
}
}
39 changes: 39 additions & 0 deletions src/NHibernate.Test/Async/Operations/MergeFixture.cs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ protected override void OnTearDown()
await (s.DeleteAsync("from Competition", cancellationToken));

await (s.DeleteAsync("from Employer", cancellationToken));
await (s.DeleteAsync("from Address", cancellationToken));
await (s.DeleteAsync("from Person", cancellationToken));

await (tx.CommitAsync(cancellationToken));
}
Expand All @@ -76,6 +78,8 @@ private void Cleanup()
s.Delete("from Competition");

s.Delete("from Employer");
s.Delete("from Address");
s.Delete("from Person");

tx.Commit();
}
Expand Down Expand Up @@ -156,6 +160,41 @@ public async Task MergeBidiForeignKeyOneToOneAsync()
}
}

[Test]
public async Task MergeBidiPrimayKeyOneToOneAsync()
{
Person p;
using (ISession s = OpenSession())
using (ITransaction tx = s.BeginTransaction())
{
p = new Person {Name = "steve"};
new PersonalDetails {SomePersonalDetail = "I have big feet", Person = p};
await (s.PersistAsync(p));
await (tx.CommitAsync());
}

ClearCounts();

p.Details.SomePersonalDetail = p.Details.SomePersonalDetail + " and big hands too";
using (ISession s = OpenSession())
using (ITransaction tx = s.BeginTransaction())
{
p = (Person) await (s.MergeAsync(p));
await (tx.CommitAsync());
}

AssertInsertCount(0);
AssertUpdateCount(1);
AssertDeleteCount(0);

using (ISession s = OpenSession())
using (ITransaction tx = s.BeginTransaction())
{
await (s.DeleteAsync(p));
await (tx.CommitAsync());
}
}

[Test]
public async Task MergeDeepTreeAsync()
{
Expand Down
46 changes: 46 additions & 0 deletions src/NHibernate.Test/NHSpecificTest/GH2608/Fixture.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
using NUnit.Framework;

namespace NHibernate.Test.NHSpecificTest.GH2608
{
[TestFixture]
public class Fixture : BugTestCase
{
protected override void OnTearDown()
{
using (var session = OpenSession())
using (var transaction = session.BeginTransaction())
{
session.CreateQuery("delete from PersonalDetails").ExecuteUpdate();
session.CreateQuery("delete from System.Object").ExecuteUpdate();

transaction.Commit();
}
}

[Test]
public void MergeBidiPrimaryKeyOneToOne()
{
using (var s = OpenSession())
using (var tx = s.BeginTransaction())
{
var p = new Person { Name = "steve" };
p.Details = new PersonalDetails { SomePersonalDetail = "I have big feet", Person = p };
s.Merge(p);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the same exception should be thrown on Persist too. Am I right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, persist does not fail, it least with my tests on a modified MergeFixture.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange... I thought it's directly related to #1754 in which case Persist should fail the same (at least for this test case). I added test for Persist - let's see...

Copy link
Member

@bahusoid bahusoid Nov 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And Persist fails the same...

it least with my tests on a modified MergeFixture.

But in current state it also doesn't fail on Merge.... The only failed tests are newly added GH2608

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The MergeFixture one fails if we change its id generator from increment to identity. But as this would mean changing from a app-side generated id to a db-side one, I have not done that change, it would change too much the test.

I have re-checked these GH2608 tests on 5.2.x just in case, both succeed on 5.2.x.

tx.Commit();
}
}

[Test]
public void PersistBidiPrimaryKeyOneToOne()
{
using (var s = OpenSession())
using (var tx = s.BeginTransaction())
{
var p = new Person { Name = "steve" };
p.Details = new PersonalDetails { SomePersonalDetail = "I have big feet", Person = p };
s.Persist(p);
tx.Commit();
}
}
}
}
24 changes: 24 additions & 0 deletions src/NHibernate.Test/NHSpecificTest/GH2608/Mappings.hbm.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?xml version="1.0" encoding="utf-8" ?>
<hibernate-mapping xmlns="urn:nhibernate-mapping-2.2"
assembly="NHibernate.Test"
namespace="NHibernate.Test.NHSpecificTest.GH2608">

<class name="Person">
<id name="Id">
<generator class="native"/>
</id>
<property name="Name"/>
<one-to-one name="Details" class="PersonalDetails" cascade="all"/>
</class>

<class name="PersonalDetails">
<id name="Id">
<generator class="foreign">
<param name="property">Person</param>
</generator>
</id>
<property name="SomePersonalDetail"/>
<one-to-one name="Person" class="Person" constrained="true"/>
</class>

</hibernate-mapping>
9 changes: 9 additions & 0 deletions src/NHibernate.Test/NHSpecificTest/GH2608/Person.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
namespace NHibernate.Test.NHSpecificTest.GH2608
{
public class Person
{
public virtual long Id { get; set; }
public virtual string Name { get; set; }
public virtual PersonalDetails Details { get; set; }
}
}
10 changes: 10 additions & 0 deletions src/NHibernate.Test/NHSpecificTest/GH2608/PersonalDetails.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
namespace NHibernate.Test.NHSpecificTest.GH2608
{
public class PersonalDetails
{
public virtual long Id { get; set; }
public virtual string SomePersonalDetail { get; set; }

public virtual Person Person { get; set; }
}
}
4 changes: 3 additions & 1 deletion src/NHibernate.Test/Operations/MergeFixture.cs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ private void Cleanup()
s.Delete("from Competition");

s.Delete("from Employer");
s.Delete("from Address");
s.Delete("from Person");

tx.Commit();
}
Expand Down Expand Up @@ -118,7 +120,7 @@ public void MergeBidiForeignKeyOneToOne()
}
}

[Test, Ignore("Need some more investigation about id sync.")]
[Test]
public void MergeBidiPrimayKeyOneToOne()
{
Person p;
Expand Down
6 changes: 4 additions & 2 deletions src/NHibernate.Test/Operations/OneToOne.hbm.xml
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,12 @@

<class name="PersonalDetails" table="OPS_PERS_DETAIL">
<id name="Id" column="ID" type="long">
<generator class="increment"/>
<generator class="foreign">
<param name="property">Person</param>
</generator>
Comment on lines -35 to +37
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already had a test really near what is required for reproducing #2608, unfortunately ignored and anyway, not using an id strategy on its parent which would be adequate for triggering the bug. As switching the parent id strategy would impact other tests, I have just fix that other test.
Strictly speaking it should go in a dedicated PR, but let it be here.

</id>
<property name="SomePersonalDetail" column="SOME_DETAIL" type="string"/>
<one-to-one name="Person" class="Person" constrained="true" />
</class>

</hibernate-mapping>
</hibernate-mapping>
5 changes: 5 additions & 0 deletions src/NHibernate/Action/DelayedPostInsertIdentifier.cs
Original file line number Diff line number Diff line change
Expand Up @@ -45,5 +45,10 @@ public override string ToString()
{
return string.Format("<delayed:{0}>", sequence);
}

/// <summary>
/// The actual identifier value that has been generated.
/// </summary>
public object ActualId { get; set; }
}
}
13 changes: 13 additions & 0 deletions src/NHibernate/Action/EntityInsertAction.cs
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,19 @@ public override void Execute()

bool veto = PreInsert();

var wasDelayed = false;
// Don't need to lock the cache here, since if someone
// else inserted the same pk first, the insert would fail
if (!veto)
{
// The identifier may be a foreign delayed identifier, which at this point should have been resolved.
if (id is DelayedPostInsertIdentifier delayed)
{
wasDelayed = true;
id = delayed.ActualId ??
throw new InvalidOperationException(
$"The delayed foreign identifier {delayed} has not been resolved before insertion of a {instance}");
}
persister.Insert(id, State, instance, Session);

EntityEntry entry = Session.PersistenceContext.GetEntry(instance);
Expand All @@ -57,6 +66,10 @@ public override void Execute()
}

entry.PostInsert();
if (wasDelayed)
{
Session.PersistenceContext.ReplaceDelayedEntityIdentityInsertKeys(entry.EntityKey, id);
}

if (persister.HasInsertGeneratedProperties)
{
Expand Down
13 changes: 13 additions & 0 deletions src/NHibernate/Async/Action/EntityInsertAction.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,19 @@ public override async Task ExecuteAsync(CancellationToken cancellationToken)

bool veto = await (PreInsertAsync(cancellationToken)).ConfigureAwait(false);

var wasDelayed = false;
// Don't need to lock the cache here, since if someone
// else inserted the same pk first, the insert would fail
if (!veto)
{
// The identifier may be a foreign delayed identifier, which at this point should have been resolved.
if (id is DelayedPostInsertIdentifier delayed)
{
wasDelayed = true;
id = delayed.ActualId ??
throw new InvalidOperationException(
$"The delayed foreign identifier {delayed} has not been resolved before insertion of a {instance}");
}
await (persister.InsertAsync(id, State, instance, Session, cancellationToken)).ConfigureAwait(false);

EntityEntry entry = Session.PersistenceContext.GetEntry(instance);
Expand All @@ -53,6 +62,10 @@ public override async Task ExecuteAsync(CancellationToken cancellationToken)
}

entry.PostInsert();
if (wasDelayed)
{
Session.PersistenceContext.ReplaceDelayedEntityIdentityInsertKeys(entry.EntityKey, id);
}

if (persister.HasInsertGeneratedProperties)
{
Expand Down
2 changes: 1 addition & 1 deletion src/NHibernate/Async/Engine/StatefulPersistenceContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@
using System.Collections.Generic;
using System.Runtime.Serialization;
using System.Security;
using System.Security.Permissions;
using System.Text;
using NHibernate.Action;
using NHibernate.Collection;
using NHibernate.Engine.Loading;
using NHibernate.Impl;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,8 @@ protected virtual async Task<object> PerformSaveAsync(object entity, object id,
throw new NonUniqueObjectException(id, persister.EntityName);
}
}
persister.SetIdentifier(entity, id);
if (!(id is DelayedPostInsertIdentifier))
persister.SetIdentifier(entity, id);
}
else
{
Expand Down
4 changes: 3 additions & 1 deletion src/NHibernate/Engine/StatefulPersistenceContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
using System.Collections.Generic;
using System.Runtime.Serialization;
using System.Security;
using System.Security.Permissions;
using System.Text;
using NHibernate.Action;
using NHibernate.Collection;
using NHibernate.Engine.Loading;
using NHibernate.Impl;
Expand Down Expand Up @@ -1392,6 +1392,8 @@ public void ReplaceDelayedEntityIdentityInsertKeys(EntityKey oldKey, object gene
AddEntity(newKey, entity);
AddEntry(entity, oldEntry.Status, oldEntry.LoadedState, oldEntry.RowId, generatedId, oldEntry.Version,
oldEntry.LockMode, oldEntry.ExistsInDatabase, oldEntry.Persister, oldEntry.IsBeingReplicated);
if (oldKey.Identifier is DelayedPostInsertIdentifier delayed)
delayed.ActualId = generatedId;
}

public bool IsLoadFinished
Expand Down
3 changes: 2 additions & 1 deletion src/NHibernate/Event/Default/AbstractSaveEventListener.cs
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,8 @@ protected virtual object PerformSave(object entity, object id, IEntityPersister
throw new NonUniqueObjectException(id, persister.EntityName);
}
}
persister.SetIdentifier(entity, id);
if (!(id is DelayedPostInsertIdentifier))
persister.SetIdentifier(entity, id);
}
else
{
Expand Down