Value converters can lead to unexpected seed data operations in migrations

[lauxjpn]

Shouldn't the C# generator emit the ValueConverter.ModelClrType instead of the ValueConverter.ProviderClrType (line 375) for properties in the snapshot, when a ValueConverter is set?

https://github.com/aspnet/EntityFrameworkCore/blob/680b887408744f5163c1e09ae4216072665c402b/src/EFCore.Design/Migrations/Design/CSharpSnapshotGenerator.cs#L366-L376

With the current implementation, if we supply a ValueConverter to a type mapping, some strange side effects happen when adding migrations. These include regeneration of seed data or unnecessary AlterColumn() calls.

---

For example, our MySqlDateTimeTypeMapping can use a value converter to convert between byte[] (model type) and DateTime (provider type), to support Timestamp/RowVersion columns (which use the database type timestamp but should be implemented as byte[] in the model).

For this model

public class TestEntity
{
    [Key]
    public int Id { get; set; }

    [Required, Timestamp]
    public byte[] RowVersion { get; set; }
}

and this context

public class Issue792Context : DbContext
{
    public DbSet<TestEntity> TestEntities { get; set; }

    protected override void OnModelCreating(ModelBuilder modelBuilder)
    {
        // Debugger.Launch();
        modelBuilder.Entity<TestEntity>()
            .HasData(new TestEntity { Id = 1 });
    }
}

the following first migration is generated

protected override void Up(MigrationBuilder migrationBuilder)
{
    migrationBuilder.CreateTable(
        name: "TestEntities",
        columns: table => new
        {
            Id = table.Column<int>(nullable: false)
                .Annotation("MySql:ValueGenerationStrategy",
                            MySqlValueGenerationStrategy.IdentityColumn),

            /* Not sure if expecting table.Column<byte[]> here; works either way for us */
            RowVersion = table.Column<DateTime>(rowVersion: true, nullable: false)
                .Annotation("MySql:ValueGenerationStrategy",
                            MySqlValueGenerationStrategy.ComputedColumn)
        },
        constraints: table =>
        {
            table.PrimaryKey("PK_TestEntities", x => x.Id);
        });

    migrationBuilder.InsertData(
        table: "TestEntities",
        column: "Id",
        value: 1);
}

with the following snapshot:

        protected override void BuildModel(ModelBuilder modelBuilder)
        {
#pragma warning disable 612, 618
            modelBuilder
                .HasAnnotation("ProductVersion", "3.0.1-dev")
                .HasAnnotation("Relational:MaxIdentifierLength", 64);

            modelBuilder.Entity("IssueConsoleTemplate.TestEntity", b =>
                {
                    b.Property<int>("Id")
                        .ValueGeneratedOnAdd()
                        .HasColumnType("int");

                    /* Expecting b.Property<byte[]> here */
                    /* Side effect: .IsRequired() is missing */
                    b.Property<DateTime>("RowVersion")
                        .IsConcurrencyToken()
                        .ValueGeneratedOnAddOrUpdate()
                        .HasColumnType("timestamp(6)");

                    b.HasKey("Id");

                    b.ToTable("TestEntities");

                    b.HasData(
                        new
                        {
                            Id = 1
                        });
                });
#pragma warning restore 612, 618
        }

I am not sure, if ModelClrType needs to be emitted for the table.Column<> call (probably not), but I think it should be emitted for the b.Property<> call.

Adding an additional migration (without any change to the model), will now recreate the seed data, as the source and target property will be considered changed.

Notice that as a side effect of using ProviderClrType, the [Required] annotation does not result in .IsRequired() being emitted in the snapshot (though nullable: false made it into the Up() script), because ProviderClrType is System.DateTime in our example and is not nullable.
However, our ModelClrType is byte[] and therefore nullable, which now can result in AlterColumn() calls (that set the nullable state of the property) being generated for every migration.

https://github.com/aspnet/EntityFrameworkCore/blob/680b887408744f5163c1e09ae4216072665c402b/src/EFCore.Design/Migrations/Design/CSharpSnapshotGenerator.cs#L396-L401

---

This is tracked on our repo by PomeloFoundation/Pomelo.EntityFrameworkCore.MySql#792.

[ajcvickers]

@lauxjpn Thanks for reporting this, and thanks for your ongoing work on the MySQL provider. It does look like there is a bug here (@AndriySvyryd and @bricelam have fixed a few things in the snapshots recently) but using the store type in the snapshot is intentional.

The reason is that snapshots persist for a long time, even while the application and its types are evolving. Consider, for example, a MyMoneyType defined in an application and used for properties in the model with an appropriate value converter. If MyMoneyType is used in the snapshot and is then modified or deleted, it will likely break existing model snapshots. So we try to only use "provider types" in the snapshot, since these are likely to be stable.

The reason this should work is because all migrations ultimately cares about are what changes to make to the database. It doesn't matter if the model type changes as long as the store type stays the same. That being said, this is has been a significant source of bugs, of which this is likely one.

[lauxjpn]

[...] but using the store type in the snapshot is intentional.

Just to clarify, this issue is not about the use of database store types like varchar or datetime in the snapshot, but about the use of the provider (CLR) type ValueConverter.ProviderClrType instead of the model (CLR) type ValueConverter.ModelClrType when a value converter has been specified for a type mapping.

If MyMoneyType is used in the snapshot and is then modified or deleted, it will likely break existing model snapshots. So we try to only use "provider types" in the snapshot, since these are likely to be stable.

I understand. This is a reciprocity problem.

---

Let's assume the following two custom types exist:

public struct MyVanillaMoneyType : System.Double
{
    // [...]
}

public struct MyStrawberryMoneyType : System.Int32
{
    // [...]
}

We then define the following type mapping MyVanillaMoneyTypeMapping:

public class MyVanillaMoneyTypeMapping : MyMoneyTypeMapping
{
    public MyVanillaMoneyTypeMapping(
        [NotNull] string storeType,
        Type clrType,
        ValueConverter converter = null,
        ValueComparer comparer = null,
        int? precision = null)
        : this(
            new RelationalTypeMappingParameters(
                new CoreTypeMappingParameters(
                    clrType, // CLR type
                    converter,
                    comparer),
                storeType, // databse store type
                precision == null
                  ? StoreTypePostfix.None
                  : StoreTypePostfix.Precision,
                System.Data.DbType.Double, // DbType
                precision: precision))
    {
    }
    
    // [...]
}

Here MyVanillaMoneyTypeMapping just maps a supplied CLR type to a database store type of mymoney. The default implementation already does this correctly for MyVanillaMoneyTypeMapping. If we want to map another CLR type like MyStrawberryMoneyType, we can provide a ValueConverter.

For our example, we instantiate two different objects of this type mapping in our RelationalTypeMappingSource derived class like this:

private readonly RelationalTypeMapping _myVanillaMoneyTypeMapping
            = new MyVanillaMoneyTypeMapping(
                "mymoney", // database store type
                typeof(MyVanillaMoneyType)); // CLR type;

private readonly RelationalTypeMapping _myStrawberryMoneyTypeMapping
            = new MyVanillaMoneyTypeMapping(
                "mymoney", // database store type
                typeof(MyStrawberryMoneyType), // CLR type
                new StrawberryToVanillaConverter(), // value converter
                new StrawberryComparer()); // comparer

We have now mapped both CLR types, MyVanillaMoneyType and MyStrawberryMoneyType, to the database store type of mymoney (assuming some FindMapping() magic).

When we run this later, the value converter of _myStrawberryMoneyTypeMapping will have the following properties set:

StrawberryToVanillaConverter.ModelClrType = typeof(MyStrawberryMoneyType);
StrawberryToVanillaConverter.ProviderClrType = typeof(MyVanillaMoneyType);

Let's define our model:

public class MyIceCreamWallet
{
    public MyVanillaMoneyType VanillaMoneyProperty { get; set; }
    public MyStrawberryMoneyType StrawberryMoneyProperty { get; set; }
}

When we generate the initial migration, we get the following snapshot code:

protected override void BuildModel(ModelBuilder modelBuilder)
{
    modelBuilder.Entity("MyNamespace.MyIceCreamWallet", b =>
        {
            b.Property<MyVanillaMoneyType>("VanillaMoneyProperty")
                .HasColumnType("mymoney");

            /* This should be MyStrawberryMoneyType NOT MyVanillaMoneyType */
            b.Property<MyVanillaMoneyType>("StrawberryMoneyProperty")
                .HasColumnType("mymoney");

            b.ToTable("MyModelEntities");
        });
}

The line b.Property<MyVanillaMoneyType>("StrawberryMoneyProperty") should be b.Property<MyStrawberryMoneyType>("StrawberryMoneyProperty").

This is not the case, because the C# generator emits ValueConverter.ProviderClrType instead of ValueConverter.ModelClrType.

Hmm... always dangerous to use food related for examples. Guess I go buy some ice cream now =)

[AndriySvyryd]

The line b.Property("StrawberryMoneyProperty") should be b.Property("StrawberryMoneyProperty").

The migrations model differ calculates what needs to be changed in the database to reflect the changes between the snapshot and the current model. It does not care about ModelClrType, only the ProviderClrType is needed. In fact the snapshot doesn't have access to custom value converters, so if it only had the ModelClrType it would not be able to get the ProviderClrType.

[lauxjpn]

It seems that this leads to wrong calls in the Up()/Down() methods though, even if no actual change was made for the migration, because the differ compares apples and oranges, I mean MyVanillaMoneyType for the source property and MyStrawberryMoneyType for the target property.

To make this concrete again for the original problem with:

[Required, Timestamp]
public byte[] RowVersion { get; set; }

The following script is generated every time a migration (even without change) is added:

public partial class NoChange : Migration
{
    protected override void Up(MigrationBuilder migrationBuilder)
    {
        migrationBuilder.DeleteData(
            table: "TestEntities",
            keyColumn: "Id",
            keyValue: 1);

        migrationBuilder.InsertData(
            table: "TestEntities",
            column: "Id",
            value: 1);
    }

    protected override void Down(MigrationBuilder migrationBuilder)
    {
        migrationBuilder.DeleteData(
            table: "TestEntities",
            keyColumn: "Id",
            keyValue: 1);

        migrationBuilder.InsertData(
            table: "TestEntities",
            column: "Id",
            value: 1);
    }
}

The following code is responsible for that:
https://github.com/aspnet/EntityFrameworkCore/blob/6148e6bc5eda687b9b2698112af785bd733fa358/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs#L1797-L1859

For the model property RowVersion, the sourceProperty is of CLR type System.DateTime (because of the snapshot definition I am complaining about) with an implicit default value of DateTime.MinValue, and targetProperty is of CLR type System.Byte[] with an implicit default value of null.

This ultimately leads to the following line being true:

https://github.com/aspnet/EntityFrameworkCore/blob/6148e6bc5eda687b9b2698112af785bd733fa358/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs#L1833

Which results in the seed data being recreated:

https://github.com/aspnet/EntityFrameworkCore/blob/6148e6bc5eda687b9b2698112af785bd733fa358/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs#L1854

[AndriySvyryd]

@lauxjpn Yes, there are certainly issues in the seed data diffing code.

• For this particular case we shouldn't even be comparing the values as the property is ValueGenerated.OnAddOrUpdate
• In general if the ProviderClrType is not nullable and the current value is null then the converted value should be dafault(ProviderClrType) until Allow HasConversion/ValueConverters to convert nulls #13850 is fixed

[lauxjpn]

@AndriySvyryd While I look at it a bit closer, maybe we can make it work for now for the Pomelo provider by tweaking our BytesToDateTimeConverter implementation to handle the null / default cases properly.

[AndriySvyryd]

@lauxjpn That would only work after #13850 is fixed, currently we don't invoke the value converters for null values

[lauxjpn]

@AndriySvyryd Sorry, I forgot about the SanitizeConverter issue again.

After that is fixed, our BytesToDateTimeConverter should return DateTime.MinValue for null values and we should be fine:

https://github.com/aspnet/EntityFrameworkCore/blob/6148e6bc5eda687b9b2698112af785bd733fa358/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs#L1826-L1828

[lauxjpn]

Then there is still this issue left:

Notice that as a side effect of using ProviderClrType, the [Required] annotation does not result in .IsRequired() being emitted in the snapshot (though nullable: false made it into the Up() script), because ProviderClrType our source property is System.DateTime in our example and is not nullable, though our target property type (the actual model type) System.Byte[] is.

And if we drop the [Required] attribute like so:

[Timestamp]
public byte[] RowVersion { get; set; }

We suddenly get the following script generated on every migration:

public partial class NoChange : Migration
{
    protected override void Up(MigrationBuilder migrationBuilder)
    {
        migrationBuilder.AlterColumn<DateTime>(
            name: "RowVersion",
            table: "TestEntities",
            rowVersion: true,
            nullable: true,
            oldClrType: typeof(DateTime),
            oldType: "timestamp(6)",
            oldNullable: true)
            .Annotation("MySql:ValueGenerationStrategy",
                        MySqlValueGenerationStrategy.ComputedColumn);
    }

    protected override void Down(MigrationBuilder migrationBuilder)
    {
        migrationBuilder.AlterColumn<DateTime>(
            name: "RowVersion",
            table: "TestEntities",
            type: "timestamp(6)",
            nullable: true,
            oldClrType: typeof(DateTime),
            oldRowVersion: true,
            oldNullable: true)
            .OldAnnotation("MySql:ValueGenerationStrategy",
                        MySqlValueGenerationStrategy.ComputedColumn);
    }
}

That should also be due to the fact, that the source property is not nullable, but the target property is. The differ then assumes this is a change and creates the alter column operation.

---

I can open separate issues for them, if you want me to.

[AndriySvyryd]

I've added that fix to the PR.

[lauxjpn]

@AndriySvyryd Awesome, thanks!

[AndriySvyryd]

On a second look we can't yet mark them as non-nullable before #13850 is fixed as we can still send nulls