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.65k stars 3.15k forks source link

Computed column incorrectly created as nullable #32451

Open sliekens opened 9 months ago

sliekens commented 9 months ago

File a bug

Hello,

I'm using SQL Server 2019 with EF Core 7.0 migrations,

I have a table with 2 columns:

I want to add a computed SortDate column which coalesces these two into a not null date for sorting the rows.

My approach might not be the best, but I found that my code incorrectly creates the computed column as a nullable column.

Include your code

My fluent API config:

    modelBuilder.Property(document => document.SortDate)
        .HasColumnName("SortDate")
        .HasColumnType("date")
        .HasComputedColumnSql("coalesce(DocumentDate, cast(CreatedAt as date))", stored: true)
        .IsRequired();

The corresponding Migration:

(Note it contains nullable: false)

migrationBuilder.AddColumn<DateTime>(
    name: "SortDate",
    schema: "dbo",
    table: "Documents",
    type: "date",
    nullable: false,
    computedColumnSql: "coalesce(DocumentDate, cast(CreatedAt as date))",
    stored: true)

The generated SQL using dotnet ef migrations script:

EXEC(N'ALTER TABLE [dbo].[Documents] ADD [SortDate] AS coalesce(DocumentDate, cast(CreatedAt as date)) PERSISTED');

And I believe this is wrong, it should contain NOT NULL:

Expected:

EXEC(N'ALTER TABLE [dbo].[Documents] ADD [SortDate] AS coalesce(DocumentDate, cast(CreatedAt as date)) PERSISTED NOT NULL');

Without NOT NULL, I believe SQL Server will use some heuristic to determine whether the computation can return NULL. I think SQL Server believes my computation can return a NULL because I use a cast.

I think it's a good idea to always specify NULL or NOT NULL in the generated SQL for computed columns.

Include provider and version information

EF Core version: 7.0.0 Database provider: Microsoft.EntityFrameworkCore.SqlServer Target framework: NET 6.0 Operating system: Debian 11 IDE: VS Code

roji commented 9 months ago

I think it's a good idea to always specify NULL or NOT NULL in the generated SQL for computed columns.

Is there any particular reason you think that's important, i.e. are you seeing a bug or behavior change resulting from EF not explicitly specifying NOT NULL?

Note that SQL Server only allows specifying NOT NULL on PERSISTED computed columns, not on non-persisted ones.

sliekens commented 9 months ago

Thank you for your supersonic reply,

I am doing some things with computed columns in temporal tables where the nullability of the column does matter a lot: nullability of columns must match in the main table and in the history table. It's a bit unfortunate that I have to drop down to raw SQL because the migration builder does not add the NOT NULL constraint.

I also just think it's wrong that the column is created as nullable because I did specify IsRequired() in Fluent API and the corresponding property on my entity is also not a nullable type:

public DateTime SortDate { get; private set; }

Aside from that, I don't think the mismatch would cause any application errors. So I understand this is not a priority issue.

gulbanana commented 3 weeks ago

Here's a workaround: subclass SqlServerMigrationsSqlGenerator, install it with DbContextOptionsBuilder.ReplaceService<IMigrationsSqlGenerator, YourSubclassName>(), and override this method:

protected override void ComputedColumnDefinition(string? schema, string table, string name, ColumnOperation operation, IModel? model, MigrationCommandListBuilder builder)
{
    base.ComputedColumnDefinition(schema, table, name, operation, model, builder);
    if (operation.IsStored == true)
    {
        builder.Append(operation.IsNullable ? " NULL" : " NOT NULL");
    }
}