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

SQL Server UseIdentityColumn() (and others) are incompatible with value generation #33413

Closed
roji opened this issue Mar 27, 2024 · 0 comments · Fixed by #33457
Closed

SQL Server UseIdentityColumn() (and others) are incompatible with value generation #33413

roji opened this issue Mar 27, 2024 · 0 comments · Fixed by #33457
Labels
area-model-building area-sqlserver closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Milestone

Comments

@roji
Copy link
Member

roji commented Mar 27, 2024

Try to call UseIdentityColumn() on a property that has a value converter configured fails with: "Identity value generation cannot be used for the property 'Id' on entity type 'Blog' because the property type is 'BlogId'. Identity value generation can only be used with signed integer properties."

Removing the explicit UseIdentityColumn() (but leaving ValueGeneratedOnAdd()) succeeds, creating an identity column as expected - it seems like our check for UseIdentityColumn() compatibility (and other similar value generation strategies) is too strict.

On a related note, leaving out ValueGeneratedOnAdd() doesn't create an identity column at all, although my expectation here is for that to happen, as the property is still called Id and has a provider type of int (but my expectation here may be wrong).

Originally opened by @grzegorzkarolewski against PostgreSQL in npgsql/efcore.pg#3141.

Repro
await using var ctx = new BlogContext();
await ctx.Database.EnsureDeletedAsync();
await ctx.Database.EnsureCreatedAsync();

public class BlogContext : DbContext
{
    public DbSet<Blog> Blogs { get; set; }

    protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        => optionsBuilder
            .UseSqlServer(@"Server=localhost;Database=test;User=SA;Password=Abcd5678;Connect Timeout=60;ConnectRetryCount=0;Trust Server Certificate=true")
            .LogTo(Console.WriteLine, LogLevel.Information)
            .EnableSensitiveDataLogging();

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        modelBuilder.Entity<Blog>()
            .Property(b => b.Id)
            .ValueGeneratedOnAdd()
            .UseIdentityColumn()
            .HasConversion(i => i.Value, v => new BlogId { Value = v });
    }
}

public class Blog
{
    public BlogId Id { get; set; }
    public string? Name { get; set; }
}

public class BlogId
{
    public int Value;
}

,/details>

@ajcvickers ajcvickers added this to the Backlog milestone Mar 27, 2024
ajcvickers added a commit that referenced this issue Apr 2, 2024
Fixes #33413

Because value converters are now supported for generated properties, but the converter may not be configured yet.
@ajcvickers ajcvickers modified the milestones: Backlog, 9.0.0 Apr 2, 2024
@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 Apr 2, 2024
ajcvickers added a commit that referenced this issue Apr 3, 2024
Fixes #33413

Because value converters are now supported for generated properties, but the converter may not be configured yet.
@ajcvickers ajcvickers modified the milestones: 9.0.0, 9.0.0-preview4 Apr 3, 2024
@ajcvickers ajcvickers removed their assignment Aug 31, 2024
@roji roji modified the milestones: 9.0.0-preview4, 9.0.0 Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-model-building area-sqlserver 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.

3 participants