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

ValueGenerator doesn't use NextAsync for the added object by the entity relation after SaveChangesAsync #30936

Closed
don-flamingo opened this issue May 19, 2023 · 5 comments

Comments

@don-flamingo
Copy link

don-flamingo commented May 19, 2023

File a bug

When I have added an entity by the inner relation, without invoking the AddAsync method. After SaveChangesAsync my attached ValueGenerator doesn't use the override NextAsync method to generate async value, instead of it uses sync method.

Include your code

public class Relation
{
    public string Key { get; set; }
    public Guid AggregateId { get; set; }
    public int Number { get; set; }
}

public class Aggregate {
    public Guid Id { get; private set; }
    public ICollection<Relation> Relations { get; private set; }
    
    // ef required
    private Aggregate()
    {
        
    }
    
    public Aggregate(Guid id)
    {
        Id = id;
        Relations = new List<Relation>();
    }
    
    public void AddRelation(string key)
    {
        Relations.Add(new Relation
        {
            AggregateId = Id,
            Key = key,
        });
    }
}

public class RelationNumberValueGenerator : ValueGenerator<int>
{
    public override int Next(EntityEntry entry)
    {
        // Will be used, but should not be
        return 0;
    }

    public override ValueTask<int> NextAsync(EntityEntry entry, CancellationToken cancellationToken = new CancellationToken())
    {
        // Will not be used, but should do
        return new(1);
    }

    public override bool GeneratesTemporaryValues => false;
}

public class RelationEntityTypeConfiguration : IEntityTypeConfiguration<Relation>
{
    public void Configure(EntityTypeBuilder<Relation> builder)
    {
        builder.HasKey(x => x.Key);
        
        builder.Property(x => x.Number)
            .HasValueGenerator<RelationNumberValueGenerator>();
        
        builder.HasOne<Aggregate>()
            .WithMany(x => x.Relations)
            .HasForeignKey(x => x.AggregateId);
    }
}

public class AggregateEntityTypeConfiguration : IEntityTypeConfiguration<Aggregate>
{
    public void Configure(EntityTypeBuilder<Aggregate> builder)
    {
        builder.HasKey(x => x.Id);
    }
}

public class AggDbContext : DbContext
{
    public DbSet<Aggregate> Aggregates => Set<Aggregate>();
    public DbSet<Relation> Relations => Set<Relation>();
    
    public AggDbContext(DbContextOptions<AggDbContext> options) : base(options)
    {
    }
    
    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.ApplyConfigurationsFromAssembly(typeof(AggDbContext).Assembly);
    }
}

var context = new AggDbContext(
    new DbContextOptionsBuilder<AggDbContext>()
    .UseNpgsql("Host=localhost;Database=agg;Username=postgres;Password=postgres")
    .Options);

var agg = context.Aggregates
    .AsTracking()
    .First();

agg.AddRelation("test");

await context.SaveChangesAsync();

Include stack traces

NONE

Include verbose output

it's caused by:

        public virtual async Task<int> SaveChangesAsync(
            bool acceptAllChangesOnSuccess,
            CancellationToken cancellationToken = default)
        {
            CheckDisposed();

            SavingChanges?.Invoke(this, new SavingChangesEventArgs(acceptAllChangesOnSuccess));

            var interceptionResult = await DbContextDependencies.UpdateLogger
                .SaveChangesStartingAsync(this, cancellationToken).ConfigureAwait(acceptAllChangesOnSuccess);
            
            // This method should be async
            TryDetectChanges();
}

Include provider and version information

EF Core version:
Database provider: Npgsql.EntityFrameworkCore.PostgreSQL
Target framework: .NET 6.0 / EF 6.0.6
Operating system: Mac os

@ajcvickers
Copy link
Member

This is currently by-design. If async value generation is needed, then AddAsync must be used to track the entity. This is because, in the general case, an entity can be tracked in many non-async contexts, most notably when DetectChanges is called implicitly by a non-async method.

Note for triage: with HiLo becoming more useful since EF Core 7, async value gen is likely to get more use. While the general case can't be changed, we could re-introduce DetectChangesAsync, which could in turn be called from SaveChangesAsync.

@roji
Copy link
Member

roji commented May 22, 2023

One additional radical/spicy thought: we could also move value generation to SaveChanges-time, rather than doing it at Add-time. The main advantage here is that we'd be able to obsolete AddAsync, which seems to cause quite a bit of confusion (and is indeed rarely needed). This would also align client-side value generation to happen at the same time as database-generated, which would make our behavior a bit more consistent.

Of course, this would mean users need to wait until SaveChanges before they can copy out the generated property (again, just like database-generated). This is related to #30753, where we may want to switch to server-generated GUIDs by default (it represents the same minor breaking change).

@ajcvickers
Copy link
Member

@roji One of the original motivations for Hi/Lo was that it generated real IDs as soon as the entity is tracked--same reason for client-side GUIDs. EF6 could never do this, and it was a frequent complaint. We could create a HiLo generator which has the characteristics that you like while not generating IDs until SaveChanges. (i.e. one extra round trip ever x number of calls to SaveChanges.) That would be much simpler in lots of places, but wouldn't match one of the main reasons for doing it in the first place.

@roji
Copy link
Member

roji commented May 22, 2023

You're saying that many people complained about not being able to get the real ID before calling SaveChanges? That strikes me as a bit odd, but in that case yeah, the current behavior makes more sense.

@roji
Copy link
Member

roji commented May 23, 2023

Duplicate of #30957

@roji roji marked this as a duplicate of #30957 May 23, 2023
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants