SQLite: Make AUTOINCREMENT more first-class

[smitpatel]

public partial class Preference
{
    public int PreferenceId { get; set; }
    public string Name { get; set; }
    public int? Value { get; set; }
    public string ValueString { get; set; }
}
modelBuilder.Entity<Preference>(entity =>
{
    entity.HasKey(e => e.PreferenceId);

    entity.Property(e => e.PreferenceId).HasColumnName("PreferenceID");

    entity.Property(e => e.Name).HasMaxLength(50);

    entity.Property(e => e.Value).HasDefaultValueSql(@"((0))");

    entity.Property(e => e.ValueString)
        .HasMaxLength(50)
        .HasDefaultValueSql(@"('')");
});

Generates following migration

migrationBuilder.CreateTable(
    name: "Preference",
    columns: table => new
    {
        PreferenceID = table.Column<int>(nullable: false)
            .Annotation("Sqlite:Autoincrement", true),
        Name = table.Column<string>(maxLength: 50, nullable: true),
        Value = table.Column<int>(nullable: true, defaultValueSql: "((0))")
            .Annotation("Sqlite:Autoincrement", true),
        ValueString = table.Column<string>(maxLength: 50, nullable: true, defaultValueSql: "('')")
    },
    constraints: table =>
    {
        table.PrimaryKey("PK_Preference", x => x.PreferenceID);
    });

This is because of ad-hoc logic here https://github.com/aspnet/EntityFrameworkCore/blob/b86eb8548a0deedc1199c3b4bc6b8632bd7824e3/src/EFCore.Sqlite.Core/Migrations/Internal/SqliteMigrationsAnnotationProvider.cs#L34-L38

And due to other hacks, later all annotations which are not on PK gets ignored. We should make autoincrement a first class for provider just like how SqlServer deals with identity.

@ErikEJ - SqlCE faces the same issue due to similar code and in SQL CE it tries to create multiple Identity columns failing at Update-Database command. You would also need to update SQL CE provider. (I found this after talking to customer on slack who hit issue on SQL CE)

[ajcvickers]

Assigning to @AndriySvyryd to give SQLite the same treatment (flag conventions, etc.) that SQL Server uses for this. @bricelam can help out on the Migrations part, if needed.

[ajcvickers]

@AndriySvyryd I added "propose-close" to make you feel at home with this one. :trollface:

[verstarr]

Is there a workaround for this? I want to test against an inmemory sqlite database since I have to use a relational database provider to run db commands for my asserts. (can't use inmemory option because of this requirement) unless there are alternatives

[ajcvickers]

@verstarr What issue are you having, specifically?

[bricelam]

Related: (mentioned in #15497)

There should be a way of configuring AUTOINCREMENT on one column of a composite primary key.

Scratch that, it's not possible in SQLite

[smitpatel]

SqlServer:Identity is computed same way now. So only thing here to do is not generate auto increment for properties which are not autoincrement. Marking this as bug.

[bricelam]

Design proposal

• Add model building API to configure AUTOINCREMENT. Parallel to UseIdentityColumn on SQL Server

modelBuilder.Entity<MyEntity>().Property(e => e.Id).UseAutoincrement();
modelBuilder.Entity<MyOtherEntity>().Property(e => e.Id).UseAutoincrement(false);

• Add a convention to configure this by default on non-composite primary key properties with store type INTEGER

• Add validation to make sure it's not configured on non-INTEGER, composite, or non-primary-key properties

• Honor the configuration in Migrations

• Ensure RevEng does the right thing

[bricelam]

SqlServer:Identity is computed same way

Does it honor the SqlServer:ValueGenerationStrategy annotation on the model? Or is there a bug...

[smitpatel]

It utilizes SqlServerValueGenerationStrategy.

[voroninp]

Also not in v 8? =)

[roji]

@voroninp as always, we prioritize features based on user interest (among other things), and this issue has received only 2 votes.

[voroninp]

@roji , I keep questioning myself how come I always bump into edge cases while others are happy with what they have =D

[bricelam]

To be fair, at least four other people have ran into issues because of the current design. Unfortunately they, like @voroninp, haven't upvoted this issue. Remember to vote if you want to see things fixed. With 1.9k issues it really can make years of difference.

[voroninp]

@bricelam, I'd 100% upvote, if I faced the issue before, but it happened on the 2nd day after I used sqlite provider for the first time. =)

[HalinSky]

4th Anniversary coming in two weeks or so and I just stumbled upon this while using SQLite for in-memory db for unit testing sheesh.

Upvoted, maybe one day.

[bricelam]

+1 This one is important. The current implementation is causing a lot of issues with value converters.