Allow making a column non-nullable with SQL Server

Closes dotnet#21765
roji · Aug 26, 2022 · 0dca8c3 · 0dca8c3
1 parent 1c71c96
commit 0dca8c3
Show file tree

Hide file tree

Showing 3 changed files with 81 additions and 0 deletions.
diff --git a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
@@ -343,6 +343,42 @@ protected override void Generate(
             (oldDefaultValue, oldDefaultValueSql) = (null, null);
         }
 
+        // The column is being made non-nullable. Generate an update statement before doing that, to convert any existing null values to
+        // the default value (otherwise SQL Server fails).
+        if (!operation.IsNullable
+            && operation.OldColumn.IsNullable
+            && (operation.DefaultValueSql is not null || operation.DefaultValue is not null))
+        {
+            string defaultValueSql;
+            if (operation.DefaultValueSql is not null)
+            {
+                defaultValueSql = operation.DefaultValueSql;
+            }
+            else
+            {
+                Check.DebugAssert(operation.DefaultValue is not null, "operation.DefaultValue is not null");
+
+                var typeMapping = (columnType != null
+                        ? Dependencies.TypeMappingSource.FindMapping(operation.DefaultValue.GetType(), columnType)
+                        : null)
+                    ?? Dependencies.TypeMappingSource.GetMappingForValue(operation.DefaultValue);
+
+                defaultValueSql = typeMapping.GenerateSqlLiteral(operation.DefaultValue);
+            }
+
+            builder
+                .Append("UPDATE ")
+                .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Table, operation.Schema))
+                .Append(" SET ")
+                .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Name))
+                .Append(" = ")
+                .Append(defaultValueSql)
+                .Append(" WHERE ")
+                .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Name))
+                .Append(" IS NULL")
+                .AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator);
+        }
+
         if (alterStatementNeeded)
         {
             builder

diff --git a/test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs b/test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs
@@ -690,6 +690,29 @@ public virtual Task Alter_column_make_required()
                 Assert.False(column.IsNullable);
             });
 
+    [ConditionalFact]
+    public virtual Task Alter_column_make_required_with_null_data()
+        => Test(
+            builder => builder.Entity(
+                "People", e =>
+                {
+                    e.Property<int>("Id");
+                    e.Property<string>("SomeColumn");
+                    e.HasData(new Dictionary<string, object>
+                    {
+                        { "Id", 1 },
+                        { "SomeColumn", null }
+                    });
+                }),
+            builder => { },
+            builder => builder.Entity("People").Property<string>("SomeColumn").IsRequired(),
+            model =>
+            {
+                var table = Assert.Single(model.Tables);
+                var column = Assert.Single(table.Columns, c => c.Name != "Id");
+                Assert.False(column.IsNullable);
+            });
+
     [ConditionalFact]
     public virtual Task Alter_column_make_required_with_index()
         => Test(

diff --git a/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs
@@ -861,6 +861,23 @@ 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'[People]') AND [c].[name] = N'SomeColumn');
 IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [People] DROP CONSTRAINT [' + @var0 + '];');
+UPDATE [People] SET [SomeColumn] = N'' WHERE [SomeColumn] IS NULL;
+ALTER TABLE [People] ALTER COLUMN [SomeColumn] nvarchar(max) NOT NULL;
+ALTER TABLE [People] ADD DEFAULT N'' FOR [SomeColumn];");
+    }
+
+    public override async Task Alter_column_make_required_with_null_data()
+    {
+        await base.Alter_column_make_required_with_null_data();
+
+        AssertSql(
+            @"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'[People]') AND [c].[name] = N'SomeColumn');
+IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [People] DROP CONSTRAINT [' + @var0 + '];');
+UPDATE [People] SET [SomeColumn] = N'' WHERE [SomeColumn] IS NULL;
 ALTER TABLE [People] ALTER COLUMN [SomeColumn] nvarchar(max) NOT NULL;
 ALTER TABLE [People] ADD DEFAULT N'' FOR [SomeColumn];");
     }
@@ -878,6 +895,7 @@ 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'[People]') AND [c].[name] = N'SomeColumn');
 IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [People] DROP CONSTRAINT [' + @var0 + '];');
+UPDATE [People] SET [SomeColumn] = N'' WHERE [SomeColumn] IS NULL;
 ALTER TABLE [People] ALTER COLUMN [SomeColumn] nvarchar(450) NOT NULL;
 ALTER TABLE [People] ADD DEFAULT N'' FOR [SomeColumn];
 CREATE INDEX [IX_People_SomeColumn] ON [People] ([SomeColumn]);");
@@ -896,6 +914,7 @@ 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'[People]') AND [c].[name] = N'FirstName');
 IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [People] DROP CONSTRAINT [' + @var0 + '];');
+UPDATE [People] SET [FirstName] = N'' WHERE [FirstName] IS NULL;
 ALTER TABLE [People] ALTER COLUMN [FirstName] nvarchar(450) NOT NULL;
 ALTER TABLE [People] ADD DEFAULT N'' FOR [FirstName];
 CREATE INDEX [IX_People_FirstName_LastName] ON [People] ([FirstName], [LastName]);");
@@ -1127,6 +1146,7 @@ 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'[People]') AND [c].[name] = N'SomeColumn');
 IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [People] DROP CONSTRAINT [' + @var0 + '];');
+UPDATE [People] SET [SomeColumn] = N'' WHERE [SomeColumn] IS NULL;
 ALTER TABLE [People] ALTER COLUMN [SomeColumn] nvarchar(450) NOT NULL;
 ALTER TABLE [People] ADD DEFAULT N'' FOR [SomeColumn];
 CREATE INDEX [IX_People_SomeColumn] ON [People] ([SomeColumn]) INCLUDE ([SomeOtherColumn]);");
@@ -2053,6 +2073,7 @@ 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'[People]') AND [c].[name] = N'SomeField');
 IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [People] DROP CONSTRAINT [' + @var0 + '];');
+UPDATE [People] SET [SomeField] = N'' WHERE [SomeField] IS NULL;
 ALTER TABLE [People] ALTER COLUMN [SomeField] nvarchar(450) NOT NULL;
 ALTER TABLE [People] ADD DEFAULT N'' FOR [SomeField];",
             //
@@ -7921,6 +7942,7 @@ 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'[Entity]') AND [c].[name] = N'Name');
 IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [Entity] DROP CONSTRAINT [' + @var0 + '];');
+UPDATE [Entity] SET [Name] = N'' WHERE [Name] IS NULL;
 ALTER TABLE [Entity] ALTER COLUMN [Name] nvarchar(max) NOT NULL;
 ALTER TABLE [Entity] ADD DEFAULT N'' FOR [Name];");
     }