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.

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

Differnce in behavior between TPH and TPT/TPC with new entity added through navigation property #32084

Closed
Euphoric opened this issue Oct 18, 2023 · 8 comments · Fixed by #32497
Assignees
Labels
area-change-tracking closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug

Comments

@Euphoric
Copy link

File a bug

I'm having an issue where TPH works but TPT/TPC does not. When I'm creating a new entity and adding it by assigning it to navigation property.

Include your code

Full code to reproduce : https://github.com/Euphoric/EfCore.InheritanceIssue/blob/main/EfCore.InheritanceIssue/EfCore.InheritanceIssue/Program.cs

The relevant pieces are :

public class ParentEntity
{
    public Guid Id { get; set; }
    public ChildBaseEntity? Child { get; set; }
}

public abstract class ChildBaseEntity
{
    public Guid Id { get; set; }
    public Guid ParentId { get; set; }
}

public class ChildEntity : ChildBaseEntity
{
    public String? ChildValue { get; set; }
}

public class TestDbContext : DbContext
{
    public TestDbContext(DbContextOptions options)
        : base(options)
    {
    }

    public DbSet<ParentEntity> Parents { get; private set; } = null!;

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        var parent = modelBuilder.Entity<ParentEntity>();
        parent.HasOne(x => x.Child)
            .WithOne()
            .HasForeignKey<ChildBaseEntity>(x=>x.ParentId)
            ;
        var childBase = modelBuilder.Entity<ChildBaseEntity>();
        //childBase.UseTphMappingStrategy(); // Case 1 : Works correctly
        //childBase.UseTptMappingStrategy(); // Case 2 : Doesn't work
        childBase.UseTpcMappingStrategy(); // Case 3 : Doesn't work
        
        var child = modelBuilder.Entity<ChildEntity>();
        child.HasBaseType<ChildBaseEntity>();
    }
}

// And when executed:

var parentId = Guid.NewGuid();
context.Parents.Add(new ParentEntity {Id = parentId});

var parent = await context.Parents.FindAsync(parentId);
var child = new ChildEntity {Id = Guid.NewGuid(), ParentId = parent!.Id, ChildValue = "test value"};
//await context.Set<ChildBaseEntity>().AddAsync(child); // fixes
parent.Child = child;
await context.SaveChangesAsync();

When using TPH, this code works as expected.
But when using TPT or TPC this code fails when trying to save changes:

     An exception occurred in the database while saving changes for context type 'EfCore.InheritanceIssue.TestDbContext'.
      Microsoft.EntityFrameworkCore.DbUpdateConcurrencyException: The database operation was expected to affect 1 row(s), but actually affected 0 row(s); data may have been modified or deleted since entities were loaded. See http://go.microsoft.com/fwlink/?LinkId=527962 for information on understandi
         at Npgsql.EntityFrameworkCore.PostgreSQL.Update.Internal.NpgsqlModificationCommandBatch.ThrowAggregateUpdateConcurrencyExceptionAsync(RelationalDataReader reader, Int32 commandIndex, Int32 expectedRowsAffected, Int32 rowsAffected, CancellationToken cancellationToken)
         at Npgsql.EntityFrameworkCore.PostgreSQL.Update.Internal.NpgsqlModificationCommandBatch.Consume(RelationalDataReader reader, Boolean async, CancellationToken cancellationToken)
         at Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.ExecuteAsync(IRelationalConnection connection, CancellationToken cancellationToken)
         at Microsoft.EntityFrameworkCore.Update.ReaderModificationCommandBatch.ExecuteAsync(IRelationalConnection connection, CancellationToken cancellationToken)
         at Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.ExecuteAsync(IEnumerable`1 commandBatches, IRelationalConnection connection, CancellationToken cancellationToken)
         at Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.ExecuteAsync(IEnumerable`1 commandBatches, IRelationalConnection connection, CancellationToken cancellationToken)
         at Microsoft.EntityFrameworkCore.Update.Internal.BatchExecutor.ExecuteAsync(IEnumerable`1 commandBatches, IRelationalConnection connection, CancellationToken cancellationToken)
         at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChangesAsync(IList`1 entriesToSave, CancellationToken cancellationToken)
         at Microsoft.EntityFrameworkCore.ChangeTracking.Internal.StateManager.SaveChangesAsync(StateManager stateManager, Boolean acceptAllChangesOnSuccess, CancellationToken cancellationToken)
         at Npgsql.EntityFrameworkCore.PostgreSQL.Storage.Internal.NpgsqlExecutionStrategy.ExecuteAsync[TState,TResult](TState state, Func`4 operation, Func`4 verifySucceeded, CancellationToken cancellationToken)
         at Microsoft.EntityFrameworkCore.DbContext.SaveChangesAsync(Boolean acceptAllChangesOnSuccess, CancellationToken cancellationToken)

Looking at log, the difference is that for TPH, EF generates INSERT:

      Executed DbCommand (6ms) [Parameters=[@p0='?' (DbType = Guid), @p1='?', @p2='?', @p3='?' (DbType = Guid)], CommandType='Text', CommandTimeout='30']
      INSERT INTO "ChildBaseEntity" ("Id", "ChildValue", "Discriminator", "ParentId")
      VALUES (@p0, @p1, @p2, @p3);

But for TPC, EF generates UPDATE, even though the row was never inserted before:

      Executed DbCommand (8ms) [Parameters=[@p2='?' (DbType = Guid), @p0='?', @p1='?' (DbType = Guid)], CommandType='Text', CommandTimeout='30']
      UPDATE "ChildEntity" SET "ChildValue" = @p0, "ParentId" = @p1
      WHERE "Id" = @p2;

I have also verified this is issue on both PosgreSql and SqlServer.

Workaround

Adding the child entity explicity fixes the issue and makes EF insert it as new entity.

Question

Is this expected behavior?
If yes, is it possible to configure EF so it maps to table-per-concretetype and works correctly even without adding the new referenced entity explicitly?

Include provider and version information

EF Core version: 7.0.12
Database provider: PostgreSql, SqlServer
Target framework: .NET 7
Operating system: Windows 10
IDE: Rider 2023.2.2

@ajcvickers
Copy link
Member

ajcvickers commented Oct 18, 2023

@Euphoric I am not able to reproduce this--my code is below. However, it's likely because you are assigning explicit values to Guid keys even though they are configured by default to be auto-generated. Consider either not providing explicit values, or configuring the key properties as not-generated--for example, modelBuilder.Entity<ParentEntity>().Property(e => e.Id).ValueGeneratedNever();.

using (var context = new SomeDbContext())
{
    await context.Database.EnsureDeletedAsync();
    await context.Database.EnsureCreatedAsync();

    var parentId = Guid.NewGuid();
    context.Add(new ParentEntity {Id = parentId});

    var parent = await context.Parents.FindAsync(parentId);
    
    var child = new ChildEntity {Id = Guid.NewGuid(), ParentId = parent!.Id, ChildValue = "test value"};

    //await context.Set<ChildBaseEntity>().AddAsync(child); // fixes
    parent.Child = child;
    await context.SaveChangesAsync();
}

public class SomeDbContext : DbContext
{
    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseSqlServer(@"Data Source=(LocalDb)\MSSQLLocalDB;Database=AllTogetherNow")
            .LogTo(Console.WriteLine, LogLevel.Information)
            .EnableSensitiveDataLogging();

    public DbSet<ParentEntity> Parents => Set<ParentEntity>();

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        var parent = modelBuilder.Entity<ParentEntity>();
        parent.HasOne(x => x.Child)
            .WithOne()
            .HasForeignKey<ChildBaseEntity>(x=>x.ParentId)
            ;
        
        var childBase = modelBuilder.Entity<ChildBaseEntity>();
        //childBase.UseTphMappingStrategy(); // Case 1 : Works correctly
        //childBase.UseTptMappingStrategy(); // Case 2 : Doesn't work
        childBase.UseTpcMappingStrategy(); // Case 3 : Doesn't work
        
        var child = modelBuilder.Entity<ChildEntity>();
        child.HasBaseType<ChildBaseEntity>();    }
}

public class ParentEntity
{
    public Guid Id { get; set; }
    public ChildBaseEntity? Child { get; set; }
}

public abstract class ChildBaseEntity
{
    public Guid Id { get; set; }
    public Guid ParentId { get; set; }
}

public class ChildEntity : ChildBaseEntity
{
    public String? ChildValue { get; set; }
}

@Euphoric
Copy link
Author

@ajcvickers Can you try storing the parent in one dbContext scope and loading it+assigning child in second scope?

@ajcvickers
Copy link
Member

@Euphoric In that case it should fail for all inheritance strategies.

@Euphoric
Copy link
Author

@ajcvickers So. If I'm understanding it correctly. Correct lifetime tracking for entities added as part of navigation property is supported if no inheritance is involved. But is unsuported or undefined if the entity is part part of inheritance tree?

@ajcvickers
Copy link
Member

@Euphoric No, it should not be affected by the mapping strategy. That's why we need a repro.

@Euphoric
Copy link
Author

@ajcvickers I'm sorry, I'm confused here. Above, I said that to reproduce the issue, the insert of new parent and assignment of child must happen in separate DbContext scopes. I can reproduce it when i use:

ar context = new SomeDbContext())
{
    await context.Database.EnsureDeletedAsync();
    await context.Database.EnsureCreatedAsync();
}

var parentId = Guid.NewGuid();
using (var context = new SomeDbContext())
{
    context.Add(new ParentEntity {Id = parentId});
    await context.SaveChangesAsync();
}

using (var context = new SomeDbContext())
{
    var parent = await context.Parents.FindAsync(parentId);

    var child = new ChildEntity {Id = Guid.NewGuid(), ParentId = parent!.Id, ChildValue = "test value"};

    //await context.Set<ChildBaseEntity>().AddAsync(child); // fixes
    parent.Child = child;
    await context.SaveChangesAsync();
}

My assumptions are :

  • Using different scopes is valid use case, as it simulates different web requests
  • Adding new entity via navigation property is valid use case and EF core should track that entity as newly added one, even if it is not added as new one into it's DbSet

Then, the question is how does inheritance strategies involve into this. Either it is expected to work for all of the inheritance strategies, in which case TPT and TPC are buggy. Or it is not expected to work and TPH working is just a incidental to work.

@ajcvickers
Copy link
Member

@Euphoric Thanks for the repro code; looks like this is a bug that happens when generated key values have been explicitly set on an entity mapped via TPH. The correct thing to do is, as stated above, to configure your keys as not-generated, or to stop setting explicit values and let EF generate them.

Note for triage: bug was introduced by the fix for #26448. The dependent gets marked as Added because the discriminator value generator generates a stable value. But the presence of a non-stable generator should stop this from happening even if that generator was not used because the value has been explicitly set.

@Euphoric
Copy link
Author

@ajcvickers Thank you for verifying.

I can confirm that setting up childBase.Property(e => e.Id).ValueGeneratedNever(); does fix the issue. Which is acceptable workaround for me, as my code uses code-generated guids.

@ajcvickers ajcvickers self-assigned this Oct 23, 2023
@ajcvickers ajcvickers added this to the Backlog milestone Oct 27, 2023
ajcvickers added a commit that referenced this issue Dec 10, 2023
…y set (#32497)

* Don't consider a generated value stable just because it was explicitly set

Fixes #32084

* Updated based on feedback
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Dec 10, 2023
@ajcvickers ajcvickers modified the milestones: Backlog, 9.0.0 Dec 10, 2023
@ajcvickers ajcvickers modified the milestones: 9.0.0, 9.0.0-preview1 Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-change-tracking closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants