IsRowVersion and HasConversion<byte[]> produce incorrect migration

[tx8821]

Problem description

I have a property (named RecordVersion) of ulong type, on which I do .IsRowVersion()and use .HasConversion<byte[]>(). This is recommended way afaik to have a numeric type in model, but rowversion in database. When I create migration with add-migration in package manager console, invalid migration is generated.

In migration file (20201011132803_Initial.cs):

        migrationBuilder.CreateTable(
            name: "ProductCategories",
            schema: "Master",
            columns: table => new
            {
                Id = table.Column<int>(nullable: false)
                    .Annotation("SqlServer:Identity", "1, 1"),
                RecordVersion = table.Column<byte[]>(nullable: false),
                // Other properties follow.....
            },

this is the line that is causing problems:

                RecordVersion = table.Column<byte[]>(nullable: false),

While if I use byte[] type in model class and not use .HasConversion<byte[]>(), I get slightly different line:

                RecordVersion = table.Column<byte[]>(rowVersion: true, nullable: false),

It is interesting, that in MyContextModelSnapshot.cs, the property is described correctly:

                b.Property<byte[]>("RecordVersion")
                    .IsConcurrencyToken()
                    .IsRequired()
                    .ValueGeneratedOnAddOrUpdate()
                    .HasColumnType("rowversion");

What this bug causes

It's also very interesting where this bug manifests itself. Due to missing rowVersion: true argument, I'm getting errors when I try to add index to that property! If I use the following for that same property:

modelBuilder.Entity(entityType).HasIndex(propName).IsUnique();

I get this error: Column 'RecordVersion' in table 'Master.ProductCategories' is of a type that is invalid for use as a key column in an index.

Code to repro

This is a little too convoluted for this example maybe, but I wanted to provide code as-is. Maybe it has something to do with the fact that I use abstract base class for my entities, or whatever.

This is entity:

public abstract class MasterEntityVersioned : MasterEntity
{
    [Rowver]
    public ulong RecordVersion { get; set; }
}

In OnModelCreating I enumerate each property with RowverAttribute and do on them:

                        modelBuilder.Entity(entityType)
                            .Property(propName)
                            .HasConversion<byte[]>()
                            .IsRowVersion();

and:

                        modelBuilder.Entity(entityType).HasIndex(propName).IsUnique();

Verbose output

This is the error I get after update-database -verbose:

Column 'RecordVersion' in table 'Master.ProductCategories' is of a type that is invalid for use as a key column in an index.

Provider and version information

EF Core version: 3.1
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET Core 3.1
Operating system: Win 10 latest
IDE: Visual Studio 2019

Other info

I think this might be somewhat related to #12434, but I decided to create separate issue, because the wording in there is not too clear for me, and also this particular case has maybe some interesting details (like it messes when you create index on that rowversion, but works otherwise).

[AndriySvyryd]

Related: #12436

[tx8821]

FYI, here is the code I ended up with finally, that currently works well with SQL Server rowversion and PostgreSQL xmin columns:

        foreach (var clrProp in entityType.ClrType.GetProperties())
        {
            if (clrProp.GetCustomAttribute<RowversionAttribute>() != null)
            {
                if (isPostgres)
                {
                    entityBuilder.Property(clrProp.Name)
                        .HasColumnName("xmin")
                        .HasColumnType("xid")
                        .IsRowVersion();
                }
                else
                {
                    entityBuilder.Property(clrProp.Name)
                        .HasColumnType("rowversion")
                        .IsRowVersion();

                    entityBuilder.HasIndex(clrProp.Name).IsUnique();
                }
            }
        }

Property type in model is ulong. As you can see, .HasConversion<byte[]>() is, apparently, not needed at all! The way it works now is not too bad, but needs to be documented explicitly.

[roji]

@tx8821 note that for PostgreSQL you have UseXminAsConcurrencyToken which is a clearer shortcut to the above (see docs).

[tx8821]

@roji Using UseXminAsConcurrencyToken is not an option in such scenario, because it assumes property name to be xmin, which is not really a good name semantically for SQL Server deployments. My code above works for same (user-defined) property name uniformly for Postgres and SQL Server.

[tx8821]

Actually looking at the code above, I think that none of the .HasColumnName() and .HasColumnType() calls should be built into EF Core itself, that should be implemented by particular db provider. E.g. Npgsql when detects that .IsRowVersion() was called on a property, it should add .HasColumnName("xmin").HasColumnType("xid") automatically. Also SQL Server provider should do the similar (add .HasColumnType("rowversion")). In such case, user just defines entityBuilder.Property(propName).IsRowVersion() and appropriate (different) code would be generated by the db provider.

But introducing these changes might be somewhat problematic, because IsRowVersion as it stands now, if I'm not mistaken, just marks the property to be used for concurrency checks. It currently does not have the row versioning semantics (which it should, because it is called quite literally "IsRowVersion"). So current IsRowVersion should be renamed to something like IsConcurrencyToken (or IsConcurrencyTokenAutoGenerated), and actual IsRowVersion should generate database-specific code for creating row versioning columns (like xmin and rowversion).

Long story short, IsRowVersion is terribly incorrect name, and if possible (maybe with .NET 5) it should either be renamed or changed to actually generate row versioning columns :)

[roji]

tl;dr this seems to have been fixed in 5.0, I don't see any additional problem to be dealt with.

On 3.1, with a byte[] property on SQL Server, no value conversion:

RecordVersion = table.Column<byte[]>(rowVersion: true, nullable: true)

On 3.1, with a ulong property and value conversion:

RecordVersion = table.Column<byte[]>(nullable: false)

However, with 5.0.9 the issue is gone and rowVersion: true does appear:

RecordVersion = table.Column<byte[]>(type: "rowversion", rowVersion: true, nullable: false)

On SQL Server, if IsRowVersion is specified on a property that isn't a byte[] (or isn't value-converted to one), the column seems to be ignored (e.g. a ulong property with IsRowversion and no value converter gets mapped to decimal(20,0). We could have add a SQL Server convention that sets a value converter to byte[] if the property is configured as a row version; though this would be a small breaking change and not sure how valuable it would be.

[roji]

(adding test in #25998)