dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.77k stars 3.18k forks source link

[Bug]: Attach with `ValueGenerator` composite key not working in case of Relations (FK) #26448

Closed mseada94 closed 2 years ago

mseada94 commented 3 years ago

Setup

Error and notes

Code Sample (Console App) >> attach-demo branch https://github.com/mohamed-seada-1994/EFCore-ValueGenerator-Bug-Sample/tree/attach-demo

Note: this sample use the workaround mentioned in #26332

EF Core version: Database provider: Microsoft.EntityFrameworkCore.SqlServer Target framework: NET 5.0 Operating system: Win10 IDE: Visual Studio 2019 16.11.5

mseada94 commented 3 years ago

I test EF Core 6.0.0-rc.2.21480.5, and both cases wouldn't identify the entities, the state in both cases is Added. I don't think this should be the right behavior. If a key property was a value-generated shadow property, It will be impossible to use Attach or Update.

Code Sample (Console App) >> attach-demo-net6 https://github.com/mohamed-seada-1994/EFCore-ValueGenerator-Bug-Sample/tree/attach-demo-net6

ajcvickers commented 3 years ago

Note for triage: the fundamental question here is whether the generation of a stable value should be enough to consider the entity as new or not. For example, in the following code, the state of the attached entity is Added.

public class Book
{
    public Guid Id { get; set; }
}

public class SomeDbContext : DbContext
{
    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder.UseSqlServer(Your.ConnectionString);

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Book>().Property(e => e.Id).HasValueGenerator<TenantIdGenerator>();
    }
}

public class TenantIdGenerator : ValueGenerator<Guid>
{
    public override Guid Next(EntityEntry entry) => Guid.Parse("98D06A82-C691-4988-EA39-08D98E2C8D8F");
    public override bool GeneratesTemporaryValues => false;
    public override bool GeneratesStableValues => true;
}

public class Program
{
    public static void Main()
    {
        using (var context = new SomeDbContext())
        {
            var book = new Book();

            context.Attach(book);

            Console.WriteLine($"State = {context.Entry(book).State}");
        }
    }
}
mseada94 commented 3 years ago

I consider it a bug, as EF core 5 has different behaviour for two similar situations.

When I test it in EF Core 6 and both the entities had Added state, I am not sure if this was a fix or unintended side effect that needs to be fixed.

If it was intented, is there any way to attach the existing entities with a value generated key?

ajcvickers commented 3 years ago

@mohamed-seada-1994 If the key value is not set, then the entity is considered new and will get the Added state.

mseada94 commented 3 years ago

@ajcvickers For normal property, I could set the value of the key before calling attach or update. But what about auto-generated shadow property as key?

The use case I want to cover involves a shadow property and updating an entity using update. How could I set the value of the shadow property and get the entity attached correctly? I expect:

The problem I have with update is caused because EFCore is not able to attach the entity with unset or Shadow auto-generated value.

For non-shadow property, the entity is considered Added, and then the property value change as required by the value-generator. I think the value should be generated before attempting to detect the entity state.

eation commented 3 years ago

@ajcvickers @mohamed-seada-1994 Maybe it's a bug.

InternalEntityEntry.cs#L1513-L1542 This HasDefaultValue method return true in final, but not everything has a default value.

InternalEntityEntry.cs#L198-L218 And this will return false when newState is Modified. It's false, so on this InternalEntityEntry.cs#L134-L137, it was not generated any value.

ValueGenerationManager.cs#L91-L117 On line 97, not everything has a default value, so it will be skip some ValueGenerator property, skip generated value.

ajcvickers commented 3 years ago

@eation All of that seems by-design, except possibly changing the handling of stable generated values.

@mohamed-seada-1994 If you have a shadow property that is part of a primary key, then it needs to be set explicitly to the correct value before attaching. This is one reason (of many) not to put PKs into shadow state. If you're seeing some behavior that involves a FK that is not also part of a PK, then please attach a small, runnable project or post a small, runnable code listing that reproduces what you are seeing so that we can investigate.

ajcvickers commented 3 years ago

Note from triage: generation of a stable value should not be enough to treat an entity as new.

mseada94 commented 3 years ago

@ajcvickers I Add User, UserClaim Entities where FK is simple and not part of the PK.

EfCore 5: It works fine like Sample with no FK. https://github.com/mohamed-seada-1994/EFCore-ValueGenerator-Bug-Sample/tree/attach-demo

EFCore 6: All 3 samples consider entity new with Added State https://github.com/mohamed-seada-1994/EFCore-ValueGenerator-Bug-Sample/tree/attach-demo-net6

mseada94 commented 3 years ago

@ajcvickers Is there any workaround I could use to set/generate shadow property explicitly before update/attach?

eation commented 3 years ago

@mohamed-seada-1994 I was make a simple fix for #6999 on this fork, it work for this test ConcurrencyValueGeneratorTest current in efcore5,but need more test in efcore6, maybe u can test it. ps: Attach(entity) has a bug, it not tracked any entity, ChangingCount or ChangedCount is 0, so u can replace with Add() or Update(). To set/generate a key property, need to set [DatabaseGenerated(DatabaseGeneratedOption.None)] dataannotations and then use custom ValueGenerator to avoid Identity Insert OFF error at this time.

@ajcvickers

ajcvickers commented 3 years ago

@mohamed-seada-1994 TenantId is part of the PK. It has a generated value. This means the state of the entity should be Added. EF Core 6.0 has the correct behavior here. Other than what has been discussed with stable values above, this is behaving as designed.

I should point out again that using value generators to generate primary key values that are not new (i.e. are already stored in the database) is not something EF Core was designed for. It's not something I have ever seen anyone do before this issue. Stable keys potentially allows for this, again as discussed above. The best workaround is to not used a value generator to set values that already exist in the database.

mseada94 commented 2 years ago

@eation I think you solution is related to generate values on update before saving the changes, It's not going to work in case of autogenerated PK before attach, update.


@ajcvickers ValueGenerator is designed to work on Add not on Update. Entity with Shadow PK Attach and Update behaviors were not considered on design, and should be avoided if possible.

The use case I have requires using Shadow Property as PK.

GeneratesStableValues option is a feature that allows for Shadow property to be used as primary key, this feature should support a stable solution to all possible operations not just adding such as (searching, updating) this added entity. This require a way to set the value of the shadow property with 2 possible situations:

  1. ValueGenerator generate a consistence value based on the current context (could be a scoped option or constant) so it could be used with search and update.
  2. ValueGenerator generate a random value or sequence which require an explicit way to set the Shadow Key before search and update.

I found this workaround to set the value of the shadow property before Attach or Update, which work fine for EFCore 5 and EFCore 6.

var generatedTenantId = Guid.Parse("98D06A82-C691-4988-EA39-08D98E2C8D8F");

var entry = context.Entry(book);
entry.Property("TenantId").OriginalValue = generatedTenantId;
entry.Property("TenantId").CurrentValue = generatedTenantId;
entry.Property("TenantId").IsModified = false;

book.Title = "Modified!";
var attachedEntry = context.Books.Update(book); // attachedEntry.Status >> "Modified"
context.SaveChanges();

With this workaround I have to set the shadow property on before calling Update, Attach. The shadow property value should be resolved from Request Context as a scoped option, Is there a way to enforce this behavior implicitly using ValueGenerator or custom DbContext Property without changing the caller code?