Pomelo.EntityFrameworkCore.MySql icon indicating copy to clipboard operation
Pomelo.EntityFrameworkCore.MySql copied to clipboard

Migration: ValueGeneratedOnUpdate

Open JeanRessouche opened this issue 6 years ago • 10 comments

Not sure if it is a bug, i know that under MS Sql server we can't do it without a trigger but using MySql it should be supported.

Eventually, I misunderstand how it is supposed to work (maybe it's not supposed to tell the db to do the value generation but the dbContext/EF should set the data during SaveChanges, anyway that's not working either, DateUpdate remain empty.

Steps to reproduce

  • In code first, add a datetime property to a new or existing entity and in the EntityTypeConfiguration.Configure define it with ValueGeneratedOnUpdate.

  • Create a migration (Add-Migration..)

    public class TestFoo
    {
        public DateTime DateInsert { get; set; }

        public DateTime? DateUpdate { get; set; }
    }
    public class TestFooConfiguration : IEntityTypeConfiguration<TestFoo>
    {
        public void Configure(EntityTypeBuilder<TestFoo> entity)
        {
            // table name
            entity.ToTable("test_foo");

            // primary key
            entity.HasKey(e => e.Id).HasName("pk_test_foo");

            // properties options
            entity.Property(e => e.DateInsert).HasDefaultValueSql("CURRENT_TIMESTAMP").ValueGeneratedOnAdd();

            entity.Property(e => e.DateUpdate).HasDefaultValueSql("CURRENT_TIMESTAMP").ValueGeneratedOnUpdate();
        }
    }

The issue

The generated migrating script doesn't reflect the ValueGeneratedOnUpdate property.

protected override void Up(MigrationBuilder migrationBuilder)
{
    migrationBuilder.AddColumn<DateTime>(
         name: "DateInsert",
         table: "test_foo",
         nullable: false,
         defaultValueSql: "CURRENT_TIMESTAMP"
    );

    migrationBuilder.AddColumn<DateTime>(
         name: "DateUpdate",
         table: "test_foo",
         nullable: true, 
         defaultValueSql: "CURRENT_TIMESTAMP"
    );
}

Therefore the generated SQL will be

DateUpdate datetime DEFAULT NULL ON UPDATE CURRENT_TIMESTAMP,

ALTER TABLE test_foo ADD COLUMN DateUpdate DATETIME DEFAULT NULL;

Instead of

ALTER TABLE test_foo ADD COLUMN DateUpdate DATETIME DEFAULT NULL ON UPDATE CURRENT_TIMESTAMP;

Further technical details

MySQL version: 5.5.5-10.3.7 Operating system: Windows 2016 server Pomelo.EntityFrameworkCore.MySql version: 2.1.2

JeanRessouche avatar Sep 20 '18 20:09 JeanRessouche

My team is running into the same thing right now.

This is our model configuration:

builder
    .Property(x => x.CreatedOn)
    .HasDefaultValueSql("NOW(6)")
    .ValueGeneratedOnAdd();

builder
    .Property(x => x.UpdatedOn)
    .HasDefaultValueSql("NOW(6)")
    .ValueGeneratedOnAddOrUpdate();

The DDL created:

  `CreatedOn` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6),
  `UpdatedOn` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6),

Is there any way to add the missing ON UPDATE statement through the configuration without having to manually specify the table altogether?

MySQL version: 5.7.24 Operating system: Ubuntu 16.04 Pomelo.EntityFrameworkCore.MySql version: 2.1.2

mariotacke avatar Nov 12 '18 22:11 mariotacke

I just tested setting the HasDefaultValueSql parameter to NOW(6) ON UPDATE NOW(6) which appears to work and generates the proper DDL.

Workaround

builder
    .Property(x => x.CreatedOn)
    .HasDefaultValueSql("NOW(6)")
    .ValueGeneratedOnAdd();

builder
    .Property(x => x.UpdatedOn)
    .HasDefaultValueSql("NOW(6) ON UPDATE NOW(6)")
    .ValueGeneratedOnAddOrUpdate();

However, this smells funny. I assume the default value string is simply injected into the generated DDL. In this case it works because the ON UPDATE statement immediately follows the default value. The statement generation should probably happen through ValueGeneratedOnUpdate and ValueGeneratedOnAddOrUpdate instead though.

mariotacke avatar Nov 12 '18 22:11 mariotacke

You'll have to look at ColumnDefinition in MySqlMigrationsSqlGenerator class to see what is occurring.

https://github.com/PomeloFoundation/Pomelo.EntityFrameworkCore.MySql/blob/e2bf0fd6e3b3c903d75993041bf26f541f7c0885/src/EFCore.MySql/Migrations/MySqlMigrationsSqlGenerator.cs#L938-L998

Automatic Initialization and Updating for TIMESTAMP documents the syntax for automatic initialization.

mguinness avatar Nov 13 '18 00:11 mguinness

https://github.com/PomeloFoundation/Pomelo.EntityFrameworkCore.MySql/blob/c12d7ca837b0df9d9d1104c6cae6476e1ee189e6/src/EFCore.MySql/Metadata/MySqlPropertyAnnotations.cs#L72-L78

I believe this is the issue. When a default value (or default value SQL) is set, the value generation strategy is short-circuited to null. This isn't intuitive and I don't think it is correct behavior.

crozone avatar Jul 18 '19 04:07 crozone

OK I think I have a workaround for the Inserted and Updated timestamps:

            entity.Property(d => d.CreatedTime).ValueGeneratedOnAdd();
            entity.Property(d => d.UpdatedTime).ValueGeneratedOnAddOrUpdate();

            entity.Property(d => d.CreatedTime).Metadata.BeforeSaveBehavior = PropertySaveBehavior.Ignore;
            entity.Property(d => d.CreatedTime).Metadata.AfterSaveBehavior = PropertySaveBehavior.Ignore;
            entity.Property(d => d.UpdatedTime).Metadata.BeforeSaveBehavior = PropertySaveBehavior.Ignore;
            entity.Property(d => d.UpdatedTime).Metadata.AfterSaveBehavior = PropertySaveBehavior.Ignore;

Then check the migration and make sure that defaultValue isn't set, or is set to null. If it defaults to defaultValue: new DateTime(1, 1, 1, 0, 0, 0, 0, DateTimeKind.Unspecified)), delete the line.

This should generate the sql datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6); and datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6); correctly.

crozone avatar Jul 22 '19 00:07 crozone

@crozone I've had the same issue and your workaround didn't seem to make a difference for me. I'll resort to the @mariotacke's workaround for now. Details below

EntityConfiguration.cs

builder.Property(item => item.CreatedUtc)
                .HasDefaultValueSql("UTC_TIMESTAMP()")
                .ValueGeneratedOnAdd();

            builder.Property(item => item.UpdatedUtc)
                .HasDefaultValueSql("UTC_TIMESTAMP()")
                .ValueGeneratedOnAddOrUpdate(); 

Migration generated

migrationBuilder.AlterColumn<DateTime>(
                name: "UpdatedUtc",
                table: "Item",
                type: "datetime(6)",
                nullable: false,
                oldClrType: typeof(DateTime),
                oldDefaultValueSql: "UTC_TIMESTAMP()");

            migrationBuilder.AlterColumn<DateTime>(
                name: "CreatedUtc",
                table: "Item",
                type: "datetime(6)",
                nullable: false,
                oldClrType: typeof(DateTime),
                oldDefaultValueSql: "UTC_TIMESTAMP()");

MySQL version: 8.0.19 Operating system: Windows 10 Pro 1904 Pomelo.EntityFrameworkCore.MySql version: 3.1.1

ATLSAPI avatar Jul 02 '20 16:07 ATLSAPI

@ATLSAPI UTC_TIMESTAMP cannot be used as DEFAULT. Only CURRENT_TIMESTAMP (or its synonyms) can be used. You can verify this by trying to run the following statement in the MySQL command line tool (or Workbench):

CREATE TABLE `IceCreamsUtc` (
	`IceCreamId` int NOT NULL AUTO_INCREMENT,
	`Name` longtext CHARACTER SET utf8mb4 NULL,
	`CreatedUtc` datetime(6) NOT NULL DEFAULT UTC_TIMESTAMP(6),
	CONSTRAINT `PK_IceCreams` PRIMARY KEY (`IceCreamId`)
);

This SQL statement above will result in an error, while the following one will work:

CREATE TABLE `IceCreams` (
	`IceCreamId` int NOT NULL AUTO_INCREMENT,
	`Name` longtext CHARACTER SET utf8mb4 NULL,
	`Created` datetime NOT NULL DEFAULT CURRENT_TIMESTAMP,
	CONSTRAINT `PK_IceCreams` PRIMARY KEY (`IceCreamId`)
);

Since MySQL 8.0.13 however, you can use UTC_TIMESTAMP with the new explicit DEFAULT syntax (enclosing the default "value" in parenthesis):

CREATE TABLE `IceCreamsUtcMySql8_0_13` (
	`IceCreamId` int NOT NULL AUTO_INCREMENT,
	`Name` longtext CHARACTER SET utf8mb4 NULL,
	`CreatedUtc` datetime(6) NOT NULL DEFAULT (UTC_TIMESTAMP(6)),
	CONSTRAINT `PK_IceCreams` PRIMARY KEY (`IceCreamId`)
);

But this will only work as a default value, not in conjunction with ON UPDATE. That still only works with CURRENT_TIMESTAMP and not with UTC_TIMESTAMP (see 11.2.5 Automatic Initialization and Updating for TIMESTAMP and DATETIME).

Also, we haven't added support for the new explicit DEFAULT syntax yet, though we have prepared the code base for it.


There are also some existing issues around ValueGeneratedOnUpdate, where we don't generate an ON UPDATE CURRENT_TIMESTAMP clause for it, and using a nullable DateTime in conjunction with ValueGeneratedOnAddOrUpdate will not generate one as well (but would be legal in MySQL).


I did some testing, take a look at the following code:

using System;
using System.Diagnostics;
using System.Linq;
using Microsoft.EntityFrameworkCore;
using Microsoft.Extensions.Logging;

namespace IssueConsoleTemplate
{
    public class IceCream
    {
        public int IceCreamId { get; set; }
        public string Name { get; set; }

        ////////
        // Add before generating the migration:
        //

        // public DateTime Created { get; set; }
        // public DateTime Updated { get; set; }
        // public DateTime Modified { get; set; }
                    
        //
        ////////
    }

    public class Context : DbContext
    {
        public DbSet<IceCream> IceCreams { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            optionsBuilder
                .UseMySql(
                    "server=127.0.0.1;port=3306;user=root;password=;database=Issue687",
                    b => b.ServerVersion("8.0.20-mysql"))
                .UseLoggerFactory(
                    LoggerFactory.Create(
                        b => b
                            .AddConsole()
                            .AddFilter(level => level >= LogLevel.Information)))
                .EnableSensitiveDataLogging()
                .EnableDetailedErrors();
        }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            modelBuilder.Entity<IceCream>(
                entity =>
                {
                    ////////
                    // Add before generating the migration:
                    //
                    
                    // entity.Property(e => e.Created)
                    //     .ValueGeneratedOnAdd();
                    //
                    // // This will result in:
                    // // ALTER TABLE `IceCreams` ADD `Updated` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6);
                    // entity.Property(e => e.Updated)
                    //     .ValueGeneratedOnAddOrUpdate(); // ValueGeneratedOnUpdate will currently not work
                    //                                     // and the property also cannot be nullable.
                    //
                    // // This will (unexpectedly) result in (no ON UPDATE clause):
                    // // ALTER TABLE `IceCreams` ADD `Modified` datetime(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6);
                    // entity.Property(e => e.Modified)
                    //     .HasDefaultValueSql("CURRENT_TIMESTAMP(6)")
                    //     .ValueGeneratedOnAddOrUpdate();
                    
                    //
                    ////////
                    
                    entity.HasData(
                        new IceCream
                        {
                            IceCreamId = 1,
                            Name = "Vanilla",
                        },
                        new IceCream
                        {
                            IceCreamId = 2,
                            Name = "Chocolate",
                        },
                        new IceCream
                        {
                            IceCreamId = 3,
                            Name = "Matcha",
                        });
                });
        }
    }

    internal static class Program
    {
        private static void Main()
        {
            using var context = new Context();

            //context.Database.EnsureDeleted();
            //context.Database.EnsureCreated();

            var iceCreams = context.IceCreams
                .OrderBy(i => i.IceCreamId)
                .ToList();

            var vanilla = iceCreams.First();
            vanilla.Name += "-Bourbon";

            context.SaveChanges();

            vanilla = context.IceCreams.First(i => i.IceCreamId == 1);
            
            ////////
            // Add before generating the migration:
            //
            
            // Debug.Assert(vanilla.Created != vanilla.Updated);
            // Debug.Assert(vanilla.Updated != null && vanilla.Updated != default(DateTime));
            // Debug.Assert(vanilla.Modified == vanilla.Updated);
                     
            //
            ////////
        }
    }
}

Before adding the time related properties, the initial migration looks like this:

using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.Migrations;

namespace IssueConsoleTemplate.Migrations
{
    public partial class Initial : Migration
    {
        protected override void Up(MigrationBuilder migrationBuilder)
        {
            migrationBuilder.CreateTable(
                name: "IceCreams",
                columns: table => new
                {
                    IceCreamId = table.Column<int>(nullable: false)
                        .Annotation("MySql:ValueGenerationStrategy", MySqlValueGenerationStrategy.IdentityColumn),
                    Name = table.Column<string>(nullable: true)
                },
                constraints: table =>
                {
                    table.PrimaryKey("PK_IceCreams", x => x.IceCreamId);
                });

            migrationBuilder.InsertData(
                table: "IceCreams",
                columns: new[] { "IceCreamId", "Name" },
                values: new object[] { 1, "Vanilla" });

            migrationBuilder.InsertData(
                table: "IceCreams",
                columns: new[] { "IceCreamId", "Name" },
                values: new object[] { 2, "Chocolate" });

            migrationBuilder.InsertData(
                table: "IceCreams",
                columns: new[] { "IceCreamId", "Name" },
                values: new object[] { 3, "Matcha" });
        }

        protected override void Down(MigrationBuilder migrationBuilder)
        {
            migrationBuilder.DropTable(
                name: "IceCreams");
        }
    }
}

After adding the time related properties and another migration, the new migration then looks like this:

using System;
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.Migrations;

namespace IssueConsoleTemplate.Migrations
{
    public partial class AddTimeRelatedProperties : Migration
    {
        protected override void Up(MigrationBuilder migrationBuilder)
        {
            migrationBuilder.AddColumn<DateTime>(
                name: "Created",
                table: "IceCreams",
                nullable: false)
                .Annotation("MySql:ValueGenerationStrategy", MySqlValueGenerationStrategy.IdentityColumn);

            migrationBuilder.AddColumn<DateTime>(
                name: "Modified",
                table: "IceCreams",
                nullable: false,
                defaultValueSql: "CURRENT_TIMESTAMP(6)");

            migrationBuilder.AddColumn<DateTime>(
                name: "Updated",
                table: "IceCreams",
                nullable: false)
                .Annotation("MySql:ValueGenerationStrategy", MySqlValueGenerationStrategy.ComputedColumn);
        }

        protected override void Down(MigrationBuilder migrationBuilder)
        {
            migrationBuilder.DropColumn(
                name: "Created",
                table: "IceCreams");

            migrationBuilder.DropColumn(
                name: "Modified",
                table: "IceCreams");

            migrationBuilder.DropColumn(
                name: "Updated",
                table: "IceCreams");
        }
    }
}

So, from the looks of it, to make your code work, remove the .HasDefaultValueSql("UTC_TIMESTAMP()") lines in your code, and regenerate the migration. Since this would generate local times, you might need to set the time zone for the server or the per-session time zone (see 5.1.14 MySQL Server Time Zone Support).

Another approach would be to use triggers.

lauxjpn avatar Jul 02 '20 19:07 lauxjpn

You should use DateTimeOffset based on current source code.

shcant avatar Jul 22 '20 07:07 shcant

You should use DateTimeOffset based on current source code

While it is generally a good idea to use System.DateTimeOffset over System.DateTime, both translate to datetime(6) database-side. System.DateTimeOffset will just save the date as UTC (but will not preserve the explicit timezone information), while System.DateTime will just be saved as is.

Since even System.DateTimeOffset results in a datetime(6) column, those columns can only be used with .HasDefaultValueSql("CURRENT_TIMESTAMP(6)") as well.

A default value of UTC_TIMESTAMP does not work with any column, unless the new explicit DEFAULT syntax is being used (but even then it will not work with ON UPDATE).

lauxjpn avatar Jul 22 '20 14:07 lauxjpn

You should use DateTimeOffset based on current source code

While it is generally a good idea to use System.DateTimeOffset over System.DateTime, both translate to datetime(6) database-side. System.DateTimeOffset will just save the date as UTC (but will not preserve the explicit timezone information), while System.DateTime will just be saved as is.

Since even System.DateTimeOffset results in a datetime(6) column, those columns can only be used with .HasDefaultValueSql("CURRENT_TIMESTAMP(6)") as well.

A default value of UTC_TIMESTAMP does not work with any column, unless the new explicit DEFAULT syntax is being used (but even then it will not work with ON UPDATE).

That's why I'm thinking about using the unix milliseconds instead, which is long type, so that it only depends on the server time being correct which is easy to control.

nicholasyin avatar Aug 10 '22 04:08 nicholasyin