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

Add support for default value expressions and automatically use them for longtext column default values (MySQL 8.0.13+/MariaDB 10.2.1) #1476

Closed
WardenDrew opened this issue Jul 21, 2021 · 4 comments · Fixed by #1498

Comments

@WardenDrew
Copy link
Contributor

WardenDrew commented Jul 21, 2021

When assigning a default value to a text column code-first, via .HasDefaultValue("test") the default value is not set at the database. This was tested both with a normal migration resulting in a .AddColumn() call as well as with a fresh migration resulting in a .CreateTable() call. Both migrations generated SQL that was missing setting the default value on the SerializedTimeZoneInfo Text column.

MySQL has supported this via an expression literal DEFAULT ('abc') since 8.0.13
https://dev.mysql.com/doc/refman/8.0/en/data-type-defaults.html#data-type-defaults-explicit

MariaDB has supported setting literal defaults DEFAULT 'abc' and expression literals DEFAULT ('abc') since 10.2.1
https://mariadb.com/kb/en/mariadb-1021-release-notes/

Steps to reproduce

Simplified Model

public partial class Company : BaseEntity
{
    public string StartOfWeek { get; set; }
    public string SerializedTimeZoneInfo { get; set; } // Added this property
}

Model Builder

modelBuilder.Entity<Company>(entityBuilder =>
{
    entityBuilder .ToTable("Company");

    entityBuilder .Property(e => e.StartOfWeek)
        .IsRequired()
        .HasColumnType("varchar(9)")
        .HasDefaultValue("sunday"); // added this default value

    entityBuilder .Property(e => e.SerializedTimeZoneInfo) // added this property
        .IsRequired()
        .HasColumnType("text")
        .HasDefaultValue("test")
});

Generated Migration Builder

public partial class Company_TimeZoneInfo : Migration
{
    protected override void Up(MigrationBuilder migrationBuilder)
    {
        migrationBuilder.AlterColumn<string>(
            name: "StartOfWeek",
            table: "Company",
            type: "varchar(9)",
            nullable: false,
            defaultValue: "sunday",
            collation: "utf8mb4_unicode_ci",
            oldClrType: typeof(string),
            oldType: "varchar(9)")
            .Annotation("MySql:CharSet", "utf8mb4")
            .OldAnnotation("MySql:CharSet", "utf8mb4")
            .OldAnnotation("Relational:Collation", "utf8mb4_unicode_ci");

        migrationBuilder.AddColumn<string>(
            name: "SerializedTimeZoneInfo",
            table: "Company",
            type: "text",
            nullable: false,
            defaultValue: "test",
            collation: "utf8mb4_unicode_ci")
            .Annotation("MySql:CharSet", "utf8mb4");
    }

    /* Down() Ommitted */
}

Generated SQL Script

START TRANSACTION;

ALTER TABLE `Company` MODIFY COLUMN `StartOfWeek` varchar(9) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT 'sunday';

ALTER TABLE `Company` ADD `SerializedTimeZoneInfo` text CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL;

INSERT INTO `__EFMigrationsHistory` (`MigrationId`, `ProductVersion`)
VALUES ('20210720182003_Company_TimeZoneInfo', '5.0.5');

COMMIT;

Further technical details

MySQL version: MariaDB 10.5.9
Operating system: Windows 10 20H1 & Ubuntu 20.04 LTS
Pomelo.EntityFrameworkCore.MySql version: 5.0.8
Microsoft.AspNetCore.App version: 5.0.0

@WardenDrew
Copy link
Contributor Author

WardenDrew commented Jul 21, 2021

#615 References the other side of the coin to this issue. And the commit to address it appears to still be in effect today:
79ca284#diff-aef69ec0dc36c14bdcbee5af1faffb4a4c2c26f5029f7b24f8eb51c21f375cfc

While MySQL 5.7 does indeed lack support for this, versions after 8.0.13 (and MariaDB after 10.2.1) has added support for this.

@mguinness
Copy link
Collaborator

You probably need to look at MySqlMigrationsSqlGenerator for the code that emits the default value for a column.

if (columnType.IndexOf("blob", StringComparison.OrdinalIgnoreCase) < 0 &&
columnType.IndexOf("text", StringComparison.OrdinalIgnoreCase) < 0 &&
columnType.IndexOf("json", StringComparison.OrdinalIgnoreCase) < 0 &&
!isSpatialStoreType)
{
DefaultValue(operation.DefaultValue, operation.DefaultValueSql, columnType, builder);
}

To enable it only for certain versions you would have to add the logic to both MySqlServerVersion and MariaDbServerVersion classes.

If you or anyone else would like to give it a shot we welcome contributions.

@lauxjpn lauxjpn self-assigned this Sep 5, 2021
@lauxjpn lauxjpn added this to the 6.0.0-preview.6 milestone Sep 5, 2021
@lauxjpn lauxjpn changed the title defaultValue ignored for text columns Add support for default value expressions and automatically use them for longtext column default values (MySQL 8.0.13+/MariaDB 10.2.1) Sep 7, 2021
@kaupov
Copy link

kaupov commented Nov 22, 2021

Hi, any change to add this fix also to version 5?

@lauxjpn
Copy link
Collaborator

lauxjpn commented Nov 23, 2021

@kaupov No, we usually do not add features to patch releases of branches that are already in maintenance mode. We only add features to the next major version, so they can get proper testing before release.

Since the feature is a bit more involved, it is unlikely to be backported.
You need to use 6.0 instead, or override MySqlMigrationsSqlGenerator with your own custom implementation (base on the #1498 implementation) and inject it into EF Core.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants