Altering a nullable column to not null generates invalid SQL commands for migration

[joakimriedel]

File a bug

Altering a nullable column to not null of a temporal table generates invalid idempotent script. The script does not include necessary commands to disable system versioning and alter the history table data, which cause an error running the script due to null column values in history. (see remark)

Include your code

The migration will be generated similar to;

            migrationBuilder.AlterColumn<bool>(
                name: "ColumnName",
                table: "TableName",
                type: "bit",
                nullable: false,
                defaultValue: false,
                oldClrType: typeof(bool),
                oldType: "bit",
                oldNullable: true)
                .Annotation("SqlServer:IsTemporal", true)
                .Annotation("SqlServer:TemporalHistoryTableName", "TableNameHistory")
                .Annotation("SqlServer:TemporalHistoryTableSchema", null)
                .Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
                .Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart")
                .OldAnnotation("SqlServer:IsTemporal", true)
                .OldAnnotation("SqlServer:TemporalHistoryTableName", "TableNameHistory")
                .OldAnnotation("SqlServer:TemporalHistoryTableSchema", null)
                .OldAnnotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
                .OldAnnotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");

Generating the idempotent script would output something like;

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20231016135852_Migration')
BEGIN
    DECLARE @var116 sysname;
    SELECT @var116 = [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'[TableName]') AND [c].[name] = N'ColumnName');
    IF @var116 IS NOT NULL EXEC(N'ALTER TABLE [TableName] DROP CONSTRAINT [' + @var116 + '];');
    EXEC(N'UPDATE [TableName] SET [ColumnName] = CAST(0 AS bit) WHERE [ColumnName] IS NULL');
    ALTER TABLE [TableName] ALTER COLUMN [ColumnName] bit NOT NULL;
    ALTER TABLE [TableName] ADD DEFAULT CAST(0 AS bit) FOR [ColumnName];
END;
GO

But I would expect it to be something like;

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20231016135852_Migration')
BEGIN
    ALTER TABLE [TableName] SET (SYSTEM_VERSIONING = OFF)
END;
GO

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20231016135852_Migration')
BEGIN
    DECLARE @var116 sysname;
    SELECT @var116 = [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'[TableName]') AND [c].[name] = N'ColumnName');
    IF @var116 IS NOT NULL EXEC(N'ALTER TABLE [TableName] DROP CONSTRAINT [' + @var116 + '];');

    EXEC(N'UPDATE [TableName] SET [ColumnName] = CAST(0 AS bit) WHERE [ColumnName] IS NULL');
    ALTER TABLE [TableName] ALTER COLUMN [ColumnName] bit NOT NULL;
    ALTER TABLE [TableName] ADD DEFAULT CAST(0 AS bit) FOR [ColumnName];

    EXEC(N'UPDATE [TableNameHistory] SET [ColumnName] = CAST(0 AS bit) WHERE [ColumnName] IS NULL');
    ALTER TABLE [TableNameHistory] ALTER COLUMN [ColumnName] bit NOT NULL;
    ALTER TABLE [TableNameHistory] ADD DEFAULT CAST(0 AS bit) FOR [ColumnName];
END;
GO

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20231016135852_Migration')
BEGIN
    DECLARE @historyTableSchema sysname = SCHEMA_NAME()
    EXEC(N'ALTER TABLE [TableName] SET (SYSTEM_VERSIONING = ON (HISTORY_TABLE = [' + @historyTableSchema + '].[TableNameHistory]))')
END;
GO

I am also facing a similar issue when renaming a column;

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20230924183929_Migration')
BEGIN
    EXEC sp_rename N'[TableName].[OldName]', N'NewName', N'COLUMN';
END;
GO

instead of

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20230924183929_Migration')
BEGIN
    ALTER TABLE [TableName] SET (SYSTEM_VERSIONING = OFF)
END;
GO

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20230924183929_Migration')
BEGIN
    EXEC sp_rename N'[TableName].[OldName]', N'NewName', N'COLUMN';
    EXEC sp_rename N'[TableNameHistory].[OldName]', N'NewName', N'COLUMN';
END;
GO

IF NOT EXISTS(SELECT * FROM [__EFMigrationsHistory] WHERE [MigrationId] = N'20230924183929_Migration')
BEGIN
    DECLARE @historyTableSchema sysname = SCHEMA_NAME()
    EXEC(N'ALTER TABLE [TableName] SET (SYSTEM_VERSIONING = ON (HISTORY_TABLE = [' + @historyTableSchema + '].[TableNameHistory]))')
END;
GO

even though it's marked temporal in migration code

            migrationBuilder.RenameColumn(
                name: "OldName",
                table: "TableName",
                newName: "NewName")
                .Annotation("SqlServer:IsTemporal", true)
                .Annotation("SqlServer:TemporalHistoryTableName", "TableNameHistory")
                .Annotation("SqlServer:TemporalHistoryTableSchema", null)
                .Annotation("SqlServer:TemporalPeriodEndColumnName", "PeriodEnd")
                .Annotation("SqlServer:TemporalPeriodStartColumnName", "PeriodStart");

DROPPING columns however, works perfectly and the necessary disabling of SYSTEM_VERSIONING gets written to the idempotent script.

Exception

Trying to execute dotnet ef database update fails with the following exception.

Microsoft.Data.SqlClient.SqlException (0x80131904): Cannot insert the value NULL into column 'ColumnName', table 'Database.dbo.TableNameHistory'; column does not allow nulls. UPDATE fails.
The statement has been terminated.
   at Microsoft.Data.SqlClient.SqlConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at Microsoft.Data.SqlClient.SqlInternalConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at Microsoft.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, Boolean callerHasConnectionLock, Boolean asyncClose)
   at Microsoft.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, Boolean& dataReady)
   at Microsoft.Data.SqlClient.SqlCommand.RunExecuteNonQueryTds(String methodName, Boolean isAsync, Int32 timeout, Boolean asyncWrite)
   at Microsoft.Data.SqlClient.SqlCommand.InternalExecuteNonQuery(TaskCompletionSource`1 completion, Boolean sendToPipe, Int32 timeout, Boolean& usedCache, Boolean asyncWrite, Boolean inRetry, String methodName)
   at Microsoft.Data.SqlClient.SqlCommand.ExecuteNonQuery()
   at Microsoft.EntityFrameworkCore.Storage.RelationalCommand.ExecuteNonQuery(RelationalCommandParameterObject parameterObject)
   at Microsoft.EntityFrameworkCore.Migrations.MigrationCommand.ExecuteNonQuery(IRelationalConnection connection, IReadOnlyDictionary`2 parameterValues)
   at Microsoft.EntityFrameworkCore.Migrations.Internal.MigrationCommandExecutor.ExecuteNonQuery(IEnumerable`1 migrationCommands, IRelationalConnection connection)
   at Microsoft.EntityFrameworkCore.Migrations.Internal.Migrator.Migrate(String targetMigration)
   at Microsoft.EntityFrameworkCore.Design.Internal.MigrationsOperations.UpdateDatabase(String targetMigration, String connectionString, String contextType)
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.UpdateDatabaseImpl(String targetMigration, String connectionString, String contextType)
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.UpdateDatabase.<>c__DisplayClass0_0.<.ctor>b__0()
   at Microsoft.EntityFrameworkCore.Design.OperationExecutor.OperationBase.Execute(Action action)

Include provider and version information

EF Core version: 7.0.11
Database provider: Microsoft.EntityFrameworkCore.SqlServer
Target framework: .NET 7

[ajcvickers]

/cc @bricelam @maumar

[maumar]

this is still a problem in the current bits. We should disable period before nullable -> non-nullable alter column operations, like we do for dropcolumn. There are also some other operations that require it, listed in the remarks linked in the bug report.
Also we should test our migrations with data (at least in some cases) - this bug would have slipped through the cracks, even if we had this exact scenario covered in our tests. It only repros if there is a row with a null value for the column that is to be converted to non-nullable.

@joakimriedel do you see the exception for rename case as well, or just noting the fact that we don't disable period for the rename case (but there is no exception as is)?

[joakimriedel]

@maumar the rename case was a bit different, I did not get any exception running the migration locally on mssqllocaldb , but the idempotent script blew up running on Azure SQL complaining about the history table not being renamed or something. I had to manually disable versioning to run the rename on both tables to upgrade our production servers.

[maumar]

@joakimriedel thanks for the info, I will look into the rename case bit deeper then. If you still have the exception message that Azure SQL gave you, can you provide it?

[joakimriedel]

@maumar unfortunately that migration was a few months back and I didn't file an issue since it thought it was a temporary issue. I could try to see if I can recreate it.

[maumar]

@joakimriedel that would definitely help us pin point the problem. Alternatively, we could just blanket disable versioning for all the alter/rename operations, but I'd really like to avoid that, because this could cause failures in other scenarios. Temporal tables migrations is really messy because of the shared state between operations.

[joakimriedel]

@maumar OK so I restored the database back to before the migration with renamed column and tried it again, which refreshed my memory.

Renaming the column does not error, in fact, it does not do anything at all.

I can call sp_rename how many times I want, it just won't rename and it does not throw any errors. The error I got previously was due to the next step in the migration using the new column name (while the db still had the old name).

To actually make sp_rename rename the column, versioning has to be disabled and you have to call sp_rename on both the temporary table and the history table and then enable versioning again.

[maumar]

@joakimriedel thanks a lot for the extra info!