From 2eabf1ff774ea16c92041cb58de065e170e98f60 Mon Sep 17 00:00:00 2001
From: Shay Rojansky <roji@roji.org>
Date: Mon, 1 Mar 2021 19:50:23 +0100
Subject: [PATCH] Fix default constraint logic in SQL Server migrations
 (#24274)

Fixes #24272
---
 .../SqlServerMigrationsSqlGenerator.cs        | 10 ++++----
 .../Migrations/MigrationsSqlServerTest.cs     | 23 +++++++++++++++++++
 2 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
index 19f56523fe0..417cc968d7f 100644
--- a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
+++ b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
@@ -339,11 +339,14 @@ protected override void Generate(
                 || operation.Collation != operation.OldColumn.Collation
                 || HasDifferences(operation.GetAnnotations(), operation.OldColumn.GetAnnotations());
 
+            var (oldDefaultValue, oldDefaultValueSql) = (operation.OldColumn.DefaultValue, operation.OldColumn.DefaultValueSql);
+
             if (alterStatementNeeded
-                || !Equals(operation.DefaultValue, operation.OldColumn.DefaultValue)
-                || operation.DefaultValueSql != operation.OldColumn.DefaultValueSql)
+                || !Equals(operation.DefaultValue, oldDefaultValue)
+                || operation.DefaultValueSql != oldDefaultValueSql)
             {
                 DropDefaultConstraint(operation.Schema, operation.Table, operation.Name, builder);
+                (oldDefaultValue, oldDefaultValueSql) = (null, null);
             }
 
             if (alterStatementNeeded)
@@ -388,8 +391,7 @@ protected override void Generate(
                 builder.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator);
             }
 
-            if (operation.DefaultValue != null
-                || operation.DefaultValueSql != null)
+            if (!Equals(operation.DefaultValue, oldDefaultValue) || operation.DefaultValueSql != oldDefaultValueSql)
             {
                 builder
                     .Append("ALTER TABLE ")
diff --git a/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs
index bcc7f3e13a8..cfde56958fe 100644
--- a/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs
+++ b/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs
@@ -964,6 +964,29 @@ FROM [sys].[default_constraints] [d]
 ALTER TABLE [People] ADD DEFAULT N'Doe' FOR [Name];");
         }
 
+        [ConditionalFact]
+        public virtual async Task Alter_column_change_comment_with_default()
+        {
+            await Test(
+                builder => builder.Entity("People").Property<string>("Name").HasDefaultValue("Doe"),
+                builder => { },
+                builder => builder.Entity("People").Property<string>("Name")
+                    .HasComment("Some comment"),
+                model =>
+                {
+                    var nameColumn = Assert.Single(Assert.Single(model.Tables).Columns);
+                    Assert.Equal("(N'Doe')", nameColumn.DefaultValueSql);
+                    Assert.Equal("Some comment", nameColumn.Comment);
+                });
+
+            AssertSql(
+                @"DECLARE @defaultSchema AS sysname;
+SET @defaultSchema = SCHEMA_NAME();
+DECLARE @description AS sql_variant;
+SET @description = N'Some comment';
+EXEC sp_addextendedproperty 'MS_Description', @description, 'SCHEMA', @defaultSchema, 'TABLE', N'People', 'COLUMN', N'Name';");
+        }
+
         [ConditionalFact]
         public virtual async Task Alter_column_make_sparse()
         {