dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.79k stars 3.19k forks source link

Add test coverage for index recreation after altering computed columns #26656

Closed benstevens48 closed 3 years ago

benstevens48 commented 3 years ago

Description

If an index exists on a column before a migration and an identical index should exist after a migration, then the index is not added during the migration. This seems to make sense but it fails to take into account that an index on a column will be automatically dropped if the column is dropped. Certain migrations will cause a column to be dropped and re-added, such as changing the SQL of a computed column. You can test it using the code below.

Code

See also this sample solution

using Microsoft.EntityFrameworkCore;
using Microsoft.EntityFrameworkCore.Metadata.Builders;
using System;
using System.ComponentModel.DataAnnotations;
using System.ComponentModel.DataAnnotations.Schema;

namespace EfTest.DbModels {
    [Table("ProductItems")]
    public class ProductDbItem {
        [Key]
        [Column("Id")]
        public Guid Id { get; set; }
        [Required]
        [Column("DisplayName")]
        public string DisplayName { get; set; }
        [Column("TestCol")]
        public string TestCol { get; set; }
    }

    public class ProductEntityTypeConfiguration : IEntityTypeConfiguration<ProductDbItem> {
        public void Configure(EntityTypeBuilder<ProductDbItem> builder) {
            builder.Property(item => item.TestCol).HasComputedColumnSql("coalesce(\"DisplayName\", '')", true);
            builder.HasIndex(item => item.TestCol);
        }
    }
}

Generate an initial migration and script with the above model.

Now modify the HasComputedColumnSql to something else, e.g.

"coalesce(\"DisplayName\", '') || 'Test'"

Generate a migration from the previous migration.

The initial migration code loos like this

CREATE TABLE IF NOT EXISTS "__EFMigrationsHistory" (
    "MigrationId" character varying(150) NOT NULL,
    "ProductVersion" character varying(32) NOT NULL,
    CONSTRAINT "PK___EFMigrationsHistory" PRIMARY KEY ("MigrationId")
);

START TRANSACTION;

CREATE TABLE "ProductItems" (
    "Id" uuid NOT NULL,
    "DisplayName" text NOT NULL,
    "TestCol" text GENERATED ALWAYS AS (coalesce("DisplayName", '')) STORED,
    CONSTRAINT "PK_ProductItems" PRIMARY KEY ("Id")
);

CREATE INDEX "IX_ProductItems_TestCol" ON "ProductItems" ("TestCol");

INSERT INTO "__EFMigrationsHistory" ("MigrationId", "ProductVersion")
VALUES ('20211112165652_InitialMigration', '5.0.11');

COMMIT;

The next migration looks like this

START TRANSACTION;

ALTER TABLE "ProductItems" DROP COLUMN "TestCol";

ALTER TABLE "ProductItems" ADD "TestCol" text GENERATED ALWAYS AS (coalesce("DisplayName", '') || 'Test') STORED;

INSERT INTO "__EFMigrationsHistory" ("MigrationId", "ProductVersion")
VALUES ('20211112165736_ChangeCol', '5.0.11');

COMMIT;

Observe that the index is not re-added even though it will be dropped when the column is dropped.

Expected behavior

The index should be added after the migration.

There are a number of ways this could be done. One way would be to always drop indexes on columns that are dropped beforehand, then add the indexes again afterwards. (Dropping the index or checking if the index exists or calculating if the index will be automatically dropped is needed because I think the index may not be automatically dropped if it involves other columns).

Provider and version information

EF Core version: 5.0.11 Database provider: Any I assume but tested using Postgres Target framework: NET 5.0 Operating system: Windows 10 21H1 IDE: Visual Studio 2019 16.11

roji commented 3 years ago

This does work correctly in SQL Server which has specific code to drop and recreate the indexes, but the PostgreSQL provider doesn't this. We're missing migration test coverage for this, will add that.

Opened https://github.com/npgsql/efcore.pg/issues/2094 to track on the Npgsql side.

/cc @lauxjpn in case the Pomelo provider also needs this.