andrewlock / StronglyTypedId

A Rosyln-powered generator for strongly-typed IDs
MIT License
1.52k stars 80 forks source link

Unable to get SQL Server Identity working properly #59

Closed MarkFl12 closed 9 months ago

MarkFl12 commented 2 years ago

I'm trying to convert an existing Int32 Id property into a strongly typed Id but I'm having trouble getting EF Core 6.0 to do inserts for me.

It's a standard Int32 Id property with SQL Server generating my values for me but no matter what settings I try in my EF config I keep getting errors such as:

The property 'Tag.Id' does not have a value set and no value generator is available for properties of type 'TagId'. Either set a value for the property before adding the entity or configure a value generator for properties of type 'TagId' in 'OnModelCreating'.

I've gone through the posts on the blog and the documentation here and I can't see anything that would help me?

andrewlock commented 2 years ago

Yeah, I can't find the documentation for this anywhere, but as far as I know, you can't use database generation with strongly typed ids/value generators unfortunately 🙁

space-alien commented 2 years ago

I have found that adding a custom ValueGenerator seems to allow strongly-typed Ids with SQL server.

At least... I'm in the initial stages of testing, and it's looking promising.

Here's what I've got so far:

[StronglyTypedId(
    backingType: StronglyTypedIdBackingType.Long,
    converters: StronglyTypedIdConverter.EfCoreValueConverter)]
public partial struct ProductId { }

Model configuration:

// "Id" would be picked up by convention, but this doesn't hurt.
builder.HasKey(e => e.Id);

// The order of calls here is important: `HasConversion()` must come before `UseIdentityColumn()`.
builder.Property(e => e.Id)

    // Can we do this by convention for all StronglyTypedIds?
    .HasConversion<ProductId.EfCoreValueConverter>()

    // **** SECRET SAUCE!!! ****
    .HasValueGenerator<ProductIdValueGenerator>()

    // At least one of UseIdentityColumn() and ValueGeneratedOnAdd() is required for EF Core
    // to add the SqlServer:Identity annotation.

    // UseIdentityColumn() *might* not be strictly necessary: Now that EF knows it has
    // a numeric primary key type, it seems to add the identity annotation by convention.
    // But I'm not certain, and being explicit can't hurt.
    .UseIdentityColumn()     

    // This might be redundant after the call to UseIdentityColumn(), but it's certainly required without it.
    // Again, being explicit can't hurt.
    .ValueGeneratedOnAdd();

And the ValueGenerator:

internal class ProductIdValueGenerator : ValueGenerator<ProductId>
{
    private long _id;

    public override bool GeneratesTemporaryValues => true;

    public override ProductId Next(EntityEntry entry)
    {
        _id -= 1;
        return new ProductId(_id); 
    }
}

I'll update this as I find out more, but I'd welcome any initial thoughts.

If this works, perhaps source-generating the ValueGenerator would be the final piece of the puzzle.

arex388 commented 2 years ago

Also voting for a source-generated ValueGenerator. I tested it out without UseIdentityColumn() and it worked fine. For the ValueGenerator I used this and it worked fine:

internal sealed class EfCoreValueGenerator :
    ValueGenerator<NoteId> {
    public override NoteId Next(
        EntityEntry entry) => new(-1);

    public override bool GeneratesTemporaryValues => true;
}
space-alien commented 2 years ago

@arex388 I think you need to return unique values, per my decrementing version, otherwise you will run into an error if you try to add more than one entity of the same type.

arex388 commented 2 years ago

@space-alien Yes, you're right. I did run into that issue when I was doing a bulk import and all entities had a -1 for the Id... I fixed it by using Random.Shared.Next() * -1. So, the code changed to this:

internal sealed class EfCoreValueGenerator :
    ValueGenerator<NoteId> {
    public override NoteId Next(
        EntityEntry entry) => new(Random.Shared.Next() * -1);

    public override bool GeneratesTemporaryValues => true;
}

I know people frown on using Random, but the value just needs to exist for a fraction of a second until the database generates the real value, so I'm good with that.

uladz-zubrycki commented 1 year ago

It's now supported for EF Core 7. One needs to mark their identity-backed property with a ValueGeneratedOnAdd and provide a ValueConverter for it.

The feature was tracked by https://github.com/dotnet/efcore/issues/11597 and is now described at https://learn.microsoft.com/en-us/ef/core/what-is-new/ef-core-7.0/whatsnew#improved-value-generation

Just note that approach with ValueConverterSelector described at https://andrewlock.net/strongly-typed-ids-in-ef-core-using-strongly-typed-entity-ids-to-avoid-primitive-obsession-part-4/ might still be failing, which is discussed at https://github.com/dotnet/efcore/issues/30749.

andrewlock commented 9 months ago

As described above, this should be solved (mostly) in EF Core 7 🙂