Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Throw a when two objects represent the same document in a session #601

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Throw a when two objects represent the same document in a session #601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 9 additions & 0 deletions src/YesSql.Core/Session.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,15 @@ public async Task SaveAsync(object entity, bool checkConcurrency = false, string

if (id > 0)
{
// If we got an object from a different identity map, without change tracking, the object reference could be different than
// the one in the identity map, and the previous state.IdentityMap.TryGetDocumentId would have returned false.
// In this case we need to assume it's an updated object and not try to "Add" it to the identity map.

if (state.IdentityMap.TryGetEntityById(id, out var _))
{
throw new InvalidOperationException("An object with the same identity is already part of this transaction. Reload it before doing any changes on it.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

How would this code fix the problem we noticed today? Wouldn't this throw a different spin exception but still fails?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes, this is not fixing the problem "in OC", there is a PR in OC for that. It's just showing a better message such that if someone missuses yessql they will know.

}

state.IdentityMap.AddEntity(id, entity);
state.Updated.Add(entity);

Expand Down
30 changes: 30 additions & 0 deletions test/YesSql.Tests/CoreTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1014,6 +1014,36 @@ public async Task DetachAllRemoveAllEntitiesFromIdentityMap()
Assert.NotEqual(p3a, p3d);
}

[Fact]
public async Task SavingRefreshedDocument()
{
await using var session = _store.CreateSession();
var bill = new Person
{
Firstname = "Bill",
Lastname = "Gates"
};

await session.SaveAsync(bill);
await session.SaveChangesAsync();

var newBill1 = await session.GetAsync<Person>(bill.Id);
await session.SaveAsync(newBill1);
await session.SaveChangesAsync();

// IdentityMap is cleared after SaveChangesAsync so we should get a new instance
var newBill2 = await session.GetAsync<Person>(bill.Id);

// This should be no-op because newBill1 is tracked
await session.SaveAsync(newBill2);

// Saving an object whose id is already in the identity map should throw an exception
// since it shows that two objects representing the same document, with potentially
// different changes, would be stored, so some changes would be lost.

await Assert.ThrowsAsync<InvalidOperationException>(() => session.SaveAsync(newBill1));
}

[Fact]
public async Task ShouldUpdateAutoFlushedIndex()
{
Expand Down