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

[Regression] Replacing PK to PK TPH dependent with different type fails #29789

Closed
Jejuni opened this issue Dec 7, 2022 · 1 comment · Fixed by #29876
Closed

[Regression] Replacing PK to PK TPH dependent with different type fails #29789

Jejuni opened this issue Dec 7, 2022 · 1 comment · Fixed by #29876
Labels
area-save-changes closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported Servicing-approved type-bug
Milestone

Comments

@Jejuni
Copy link

Jejuni commented Dec 7, 2022

Description

Assume we have 3 concrete and 1 abstract entity:

  • MainEntity (concrete)
  • EntityWithInheritance (abstract)
    • EntityWithInheritanceOne (concrete, derives from EntityWithInheritance)
    • EntityWithInheritanceTwo (concrete, derives from EntityWithInheritance)

The hierarchy between them is: MainEntity -> EntityWithInheritance configured as 1:1

Assume we have an instance as follows: MainEntity -> EntityWithInheritanceOne saved in the database.
The discriminator column correctly denotes EntityWithInheritanceOne.

We now want to update MainEntity by replacing the existing property with one of type EntityWithInheritanceTwo.
After calling SaveChanges the discriminator column still incorrectly has the OLD value of EntityWithInheritanceOne instead of the correct, new one.
All other (potential) properties seem to have updated correctly.

This did NOT happen in any EfCore version prior to version 7.

Repro

The following code in an excerpt from the linked repo
You will need to replace the connection string in MyContext class to point to a valid SqlServer instance.

Database and config:

internal class MyContext : DbContext
{
    public DbSet<MainEntity> MainEntities => Set<MainEntity>();

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
    {
        optionsBuilder.UseSqlServer(
            @"Server=localhost,5433;Initial Catalog=EFCoreDatabaseTest2;User Id=sa;Password=Pass@word;TrustServerCertificate=true");
    }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        base.OnModelCreating(modelBuilder);

        modelBuilder.Entity<EntityWithInheritance>()
            .HasDiscriminator<string>(EntityWithInheritanceConfig.DiscriminatorColumnName)
            .HasValue<EntityWithInheritanceOne>(EntityWithInheritanceConfig.EntityWithInheritanceOne)
            .HasValue<EntityWithInheritanceTwo>(EntityWithInheritanceConfig.EntityWithInheritanceTwo);

        modelBuilder.Entity<EntityWithInheritance>().HasOne<MainEntity>()
            .WithOne(x => x.EntityWithInheritance)
            .HasForeignKey<EntityWithInheritance>();
    }
}

public class MainEntity
{
    public Guid Id { get; set; }
    public virtual EntityWithInheritance? EntityWithInheritance { get; set; }
}

public static class EntityWithInheritanceConfig
{
    public const string EntityWithInheritanceOne = "EntityWithInheritanceOne";
    public const string EntityWithInheritanceTwo = "EntityWithInheritanceTwo";
    public const string DiscriminatorColumnName = "Discriminator";
}

public abstract class EntityWithInheritance
{
    public Guid Id { get; set; }
    public int MyNumber { get; set; }
}

public class EntityWithInheritanceOne : EntityWithInheritance
{
}

public class EntityWithInheritanceTwo : EntityWithInheritance
{
    public int MyOtherNumber { get; set; }
}

And a program to test the problem:

using EFCoreDatabase.Contexts;
using Microsoft.EntityFrameworkCore;

await using (var ctx = new MyContext())
{
    await ctx.Database.EnsureDeletedAsync();
    await ctx.Database.EnsureCreatedAsync();
}

await using (var ctx2 = new MyContext())
{
    var mainEntity = new MainEntity
    {
        EntityWithInheritance = new EntityWithInheritanceOne
        {
            MyNumber = 1
        }
    };

    await ctx2.MainEntities.AddAsync(mainEntity);
    await ctx2.SaveChangesAsync();
}

await using (var ctx3 = new MyContext())
{
    var mainEntity = await ctx3
        .MainEntities
        .Include(x => x.EntityWithInheritance)
        .FirstAsync();

    mainEntity.EntityWithInheritance = new EntityWithInheritanceTwo
    {
        MyNumber = 11,
        MyOtherNumber = 5
    };
    await ctx3.SaveChangesAsync();
}

await using (var ctx4 = new MyContext())
{
    var mainEntity = await ctx4
        .MainEntities
        .Include(x => x!.EntityWithInheritance)
        .FirstAsync();

    if (mainEntity.EntityWithInheritance.GetType() != typeof(EntityWithInheritanceTwo))
        throw new Exception("Invalid behavior");
}

Notice the exception being thrown when the incorrect type is returned.
Switch <PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="7.0.0" /> to <PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="6.0.10" /> or any other version prior to 7 and the code above works,

NOTE: The configuration above is the most default setup we could think of. Adding a MainEntityId property to EntityWithInheritance and then configuring that property as the foreign key to the MainEntity table the code above also works. Probably because the FK and PK are now different. This is obviously undesirable, however, as it requires additional configuration and the PK is essentially useless.

Include provider and version information

EF Core version:
Database provider: Microsoft.EntityFrameworkCore.SqlServer 7.0.0
Target framework: (e.g. .NET 7.0)
Operating system: Windows
IDE: (e.g. Visual Studio 2022 17.4)

@Jejuni Jejuni changed the title [Regression] Derived Entites not updated correctly [Regression] Derived Entites not updated correctly since v7 Dec 7, 2022
@ajcvickers
Copy link
Member

ajcvickers commented Dec 7, 2022

Note for triage: this is a consequence of making the discriminator read-only. The model is in the following state before SaveChanges:

EntityWithInheritanceOne (Shared) {Id: 8180f6aa-7111-4d6a-d8d9-08dad8888fee} Deleted
  Id: '8180f6aa-7111-4d6a-d8d9-08dad8888fee' PK FK
  Discriminator: 'EntityWithInheritanceOne'
  MyNumber: 1
EntityWithInheritanceTwo (Shared) {Id: 8180f6aa-7111-4d6a-d8d9-08dad8888fee} Added
  Id: '8180f6aa-7111-4d6a-d8d9-08dad8888fee' PK FK
  Discriminator: 'EntityWithInheritanceTwo'
  MyNumber: 11
  MyOtherNumber: 5
MainEntity {Id: 8180f6aa-7111-4d6a-d8d9-08dad8888fee} Unchanged
  Id: '8180f6aa-7111-4d6a-d8d9-08dad8888fee' PK
  EntityWithInheritance: {Id: 8180f6aa-7111-4d6a-d8d9-08dad8888fee}

The update pipeline collapses the Delete and Add into an Update, but the discriminator value is not changed because it is marked as "AfterSave:Throw".

@Jejuni Workaround is to configure the discriminator such that it is allowed to be modified:

modelBuilder.Entity<EntityWithInheritance>()
    .Property(EntityWithInheritanceConfig.DiscriminatorColumnName)
    .Metadata
    .SetAfterSaveBehavior(PropertySaveBehavior.Save);

@ajcvickers ajcvickers changed the title [Regression] Derived Entites not updated correctly since v7 [Regression] Replacing PK to PK TPH dependent with different type fails Dec 16, 2022
@ajcvickers ajcvickers self-assigned this Dec 16, 2022
@ajcvickers ajcvickers reopened this Dec 20, 2022
@ajcvickers ajcvickers added this to the 7.0.x milestone Dec 22, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.x, 7.0.3 Jan 4, 2023
@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 Jan 5, 2023
@ajcvickers ajcvickers removed their assignment Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-save-changes closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported Servicing-approved type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants