SQL Server Migrations: Handle AlterColumn(nullable: false)

[bricelam]

Found this while working on #19882. When changing a property to required (aka NOT NULL), the database currently throws.

Cannot insert the value NULL into column 'MyColumn', table 'MyTable'; column does not allow nulls. UPDATE fails.
The statement has been terminated.

We could handle this better by translating it as follows.

UPDATE [MyTable]
SET [MyColumn] = N''
WHERE [MyColumn] IS NULL;

ALTER TABLE [MyTable] ALTER COLUMN [MyColumn] nvarchar(max) NOT NULL;

[bricelam]

Note, this only fails when the column actually contains nulls.

[rdadkins]

This also fails when attempting to add a default value to a previously nullable column (Azure SQL, EF Core 5.0).

Current Model:

public class Point
{
  public decimal? Latitude { get; set; }

  public decimal? Longitude { get; set; }
}

Desired Model:

public class Point
{
  public decimal Latitude { get; set; }

  public decimal Longitude { get; set; }
}

Migration:

Up()
{
  migrationBuilder.AlterColumn<decimal>(
    name: "Latitude",
    table: "Point",
    type: "decimal(11,8)",
    nullable: false,
    defaultValue: 0m,
    oldClrType: typeof(decimal),
    oldType: "decimal(11,8)",
    oldNullable: true);

  migrationBuilder.AlterColumn<decimal>(
    name: "Longitude",
    table: "Point",
    type: "decimal(11,8)",
    nullable: false,
    defaultValue: 0m,
    oldClrType: typeof(decimal),
    oldType: "decimal(11,8)",
    oldNullable: true);
}

Generated SQL:

...
DECLARE @var0 sysname;
SELECT @var0 = [d].[name]
FROM [sys].[default_constraints] [d]
INNER JOIN [sys].[columns] [c] ON [d].[parent_column_id] = [c].[column_id] AND [d].[parent_object_id] = [c].[object_id]
WHERE ([d].[parent_object_id] = OBJECT_ID(N'[Point]') AND [c].[name] = N'Latitude');
IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [Point] DROP CONSTRAINT [' + @var0 + '];');
ALTER TABLE [Request] ALTER COLUMN [Latitude] decimal(11,8) NOT NULL;
ALTER TABLE [Request] ADD DEFAULT 0.0 FOR [Latitude];
GO

DECLARE @var1 sysname;
SELECT @var1 = [d].[name]
FROM [sys].[default_constraints] [d]
INNER JOIN [sys].[columns] [c] ON [d].[parent_column_id] = [c].[column_id] AND [d].[parent_object_id] = [c].[object_id]
WHERE ([d].[parent_object_id] = OBJECT_ID(N'[Point]') AND [c].[name] = N'Longitude');
IF @var1 IS NOT NULL EXEC(N'ALTER TABLE [Point] DROP CONSTRAINT [' + @var1 + '];');
ALTER TABLE [Request] ALTER COLUMN [Longitude] decimal(11,8) NOT NULL;
ALTER TABLE [Request] ADD DEFAULT 0.0 FOR [Longitude];
GO
...

It seems that the simple fix would be to have default values have precedence over the NOT NULL alteration.

[HerrNiklasRaab]

Same here, but in my case, the column I want to make non-nullable has no null values, but it still fails to migrate. This seems different from what is described by @bricelam : #21765 (comment)

[roji]

Are we sure it's a good idea to implicitly/silently convert NULLs to the C# default? It may be safer to require the user to do the explicit gesture of including the above, so they're clear on the decision they're making (after all, the database itself doesn't do this implicitly, so I'm not sure we should either).

[bricelam]

The original idea was that we'd generate a data loss warning, then you'd review the migration to specify the value you wanted. We just use the CLR default as a starting point since this is the value that would be there if the property was required when the entity was inserted.

[roji]

Note npgsql/efcore.pg#2131 which was just opened, and where the CLR default (empty string) is an invalid value for the column type (json). Of course, we could allow providers to specify default values for type mappings, but this may also point towards forcing users to deal with this up-front rather than trying to do this for them.

A data loss warning may be good enough, though I think there's a difference between more classical, clearly-intentional data loss scenarios (you removed a property) and this more subtle scenario - I'm usually a fan of just exposing the database to users without doing any magic, and databases simply don't allow this. Am also not sure users would realize the possible danger just by seeing the proposed UPDATE in the migration code - maybe a specific/explicit warning would help with that.