Full OldTable on AlterTableOperation, further comment support

Changed AlterTableOperation.OldTable from Annotatable to TableOperation,
and did various fixes throughout to support comment alteration on tables.

Some comment fixes and refactoring.

Fixes #16819
Fixes #16798

src/EFCore.Design/Migrations/Design/CSharpMigrationOperationGenerator.cs

@@ -768,6 +768,14 @@ protected virtual void Generate([NotNull] AlterTableOperation operation, [NotNul
                         .Append(Code.Literal(operation.Comment));
                 }
 
+                if (operation.OldTable.Comment != null)
+                {
+                    builder
+                        .AppendLine(",")
+                        .Append("oldComment: ")
+                        .Append(Code.Literal(operation.OldTable.Comment));
+                }
+
                 builder.Append(")");
 
                 Annotations(operation.GetAnnotations(), builder);

src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs

@@ -557,39 +557,41 @@ protected virtual IEnumerable<MigrationOperation> Diff(
                 };
             }
 
-            var operations = DiffAnnotations(source, target)
-                .Concat(Diff(source.GetProperties(), target.GetProperties(), diffContext))
-                .Concat(Diff(source.GetKeys(), target.GetKeys(), diffContext))
-                .Concat(Diff(source.GetIndexes(), target.GetIndexes(), diffContext))
-                .Concat(Diff(source.GetCheckConstraints(), target.GetCheckConstraints(), diffContext));
-            foreach (var operation in operations)
-            {
-                yield return operation;
-            }
-
-            DiffData(source, target, diffContext);
-        }
-
-        private IEnumerable<MigrationOperation> DiffAnnotations(
-            [NotNull] TableMapping source,
-            [NotNull] TableMapping target)
-        {
             // Validation should ensure that all the relevant annotations for the collocated entity types are the same
             var sourceMigrationsAnnotations = MigrationsAnnotations.For(source.EntityTypes[0]).ToList();
             var targetMigrationsAnnotations = MigrationsAnnotations.For(target.EntityTypes[0]).ToList();
-            if (HasDifferences(sourceMigrationsAnnotations, targetMigrationsAnnotations))
+
+            if (source.GetComment() != target.GetComment()
+                || HasDifferences(sourceMigrationsAnnotations, targetMigrationsAnnotations))
             {
                 var alterTableOperation = new AlterTableOperation
                 {
                     Name = target.Name,
                     Schema = target.Schema,
-                    Comment = target.GetComment()
+                    Comment = target.GetComment(),
+                    OldTable =
+                    {
+                        Comment = source.GetComment()
+                    }
                 };
-                alterTableOperation.AddAnnotations(targetMigrationsAnnotations);
 
+                alterTableOperation.AddAnnotations(targetMigrationsAnnotations);
                 alterTableOperation.OldTable.AddAnnotations(sourceMigrationsAnnotations);
+
                 yield return alterTableOperation;
             }
+
+            var operations = Diff(source.GetProperties(), target.GetProperties(), diffContext)
+                .Concat(Diff(source.GetKeys(), target.GetKeys(), diffContext))
+                .Concat(Diff(source.GetIndexes(), target.GetIndexes(), diffContext))
+                .Concat(Diff(source.GetCheckConstraints(), target.GetCheckConstraints(), diffContext));
+
+            foreach (var operation in operations)
+            {
+                yield return operation;
+            }
+
+            DiffData(source, target, diffContext);
         }
 
         /// <summary>

src/EFCore.Relational/Migrations/MigrationBuilder.cs

@@ -480,19 +480,25 @@ public virtual AlterOperationBuilder<AlterSequenceOperation> AlterSequence(
         /// <param name="name"> The table name. </param>
         /// <param name="schema"> The schema that contains the table, or <c>null</c> to use the default schema. </param>
         /// <param name="comment"> A comment to associate with the table. </param>
+        /// <param name="oldComment"> The previous comment to associate with the table. </param>
         /// <returns> A builder to allow annotations to be added to the operation. </returns>
         public virtual AlterOperationBuilder<AlterTableOperation> AlterTable(
             [NotNull] string name,
             [CanBeNull] string schema = null,
-            [CanBeNull] string comment = null)
+            [CanBeNull] string comment = null,
+            [CanBeNull] string oldComment = null)
         {
             Check.NotEmpty(name, nameof(name));
 
             var operation = new AlterTableOperation
             {
                 Schema = schema,
                 Name = name,
-                Comment = comment
+                Comment = comment,
+                OldTable = new TableOperation
+                {
+                    Comment = oldComment
+                }
             };
             Operations.Add(operation);
 

src/EFCore.Relational/Migrations/Operations/AlterTableOperation.cs

@@ -10,7 +10,7 @@ namespace Microsoft.EntityFrameworkCore.Migrations.Operations
     /// <summary>
     ///     A <see cref="MigrationOperation" /> to alter an existing table.
     /// </summary>
-    public class AlterTableOperation : MigrationOperation, IAlterMigrationOperation
+    public class AlterTableOperation : TableOperation, IAlterMigrationOperation
     {
         /// <summary>
         ///     The name of the table.
@@ -22,15 +22,10 @@ public class AlterTableOperation : MigrationOperation, IAlterMigrationOperation
         /// </summary>
         public virtual string Schema { get; [param: CanBeNull] set; }
 
-        /// <summary>
-        ///     Comment for this table
-        /// </summary>
-        public virtual string Comment { get; [param: CanBeNull] set; }
-
         /// <summary>
         ///     An operation representing the table as it was before being altered.
         /// </summary>
-        public virtual Annotatable OldTable { get; [param: NotNull] set; } = new Annotatable();
+        public virtual TableOperation OldTable { get; [param: NotNull] set; } = new TableOperation();
 
         /// <inheritdoc />
         IMutableAnnotatable IAlterMigrationOperation.OldAnnotations => OldTable;

src/EFCore.Relational/Migrations/Operations/CreateTableOperation.cs

@@ -9,7 +9,7 @@ namespace Microsoft.EntityFrameworkCore.Migrations.Operations
     /// <summary>
     ///     A <see cref="MigrationOperation" /> for creating a new table.
     /// </summary>
-    public class CreateTableOperation : MigrationOperation
+    public class CreateTableOperation : TableOperation
     {
         /// <summary>
         ///     The name of the table.
@@ -21,11 +21,6 @@ public class CreateTableOperation : MigrationOperation
         /// </summary>
         public virtual string Schema { get; [param: CanBeNull] set; }
 
-        /// <summary>
-        ///     Comment for this table
-        /// </summary>
-        public virtual string Comment { get; [param: CanBeNull] set; }
-
         /// <summary>
         ///     The <see cref="AddPrimaryKeyOperation" /> representing the creation of the primary key for the table.
         /// </summary>

src/EFCore.Relational/Migrations/Operations/TableOperation.cs

@@ -0,0 +1,19 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using JetBrains.Annotations;
+
+namespace Microsoft.EntityFrameworkCore.Migrations.Operations
+{
+    /// <summary>
+    ///     A <see cref="MigrationOperation" /> for operations on tables.
+    ///     See also <see cref="CreateTableOperation" /> and <see cref="AlterTableOperation" />.
+    /// </summary>
+    public class TableOperation : MigrationOperation
+    {
+        /// <summary>
+        ///     Comment for this table
+        /// </summary>
+        public virtual string Comment { get; [param: CanBeNull] set; }
+    }
+}

src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs

@@ -40,6 +40,7 @@ public class SqlServerMigrationsSqlGenerator : MigrationsSqlGenerator
         private IReadOnlyList<MigrationOperation> _operations;
         private int _variableCounter;
 
+        private const string DefaultSchema = "dbo";
         /// <summary>
         ///     Creates a new <see cref="SqlServerMigrationsSqlGenerator"/> instance.
         /// </summary>
@@ -137,7 +138,7 @@ protected override void Generate(
 
                 if (operation.Comment != null)
                 {
-                    AddDropComment(builder, operation.Comment, null, operation.Schema, operation.Table, operation.Name);
+                    GenerateColumnComment(builder, model, operation.Comment, null, operation.Schema, operation.Table, operation.Name);
                 }
 
                 builder.EndCommand(suppressTransaction: IsMemoryOptimized(operation, model, operation.Schema, operation.Table));
@@ -339,7 +340,7 @@ protected override void Generate(
 
             if (operation.OldColumn.Comment != operation.Comment)
             {
-                AddDropComment(builder, operation.Comment, operation.OldColumn.Comment, operation.Schema, operation.Table, operation.Name);
+                GenerateColumnComment(builder, model, operation.Comment, operation.OldColumn.Comment, operation.Schema, operation.Table, operation.Name);
             }
 
             if (narrowed)
@@ -478,12 +479,12 @@ protected override void Generate(
 
             if (operation.Comment != null)
             {
-                AddDropComment(builder, operation.Comment, null, operation.Schema, operation.Name);
+                GenerateTableComment(builder, model, operation.Comment, null, operation.Schema, operation.Name);
             }
 
             foreach (var column in operation.Columns.Where(c => c.Comment != null))
             {
-                AddDropComment(builder, column.Comment, null, operation.Schema, operation.Name, column.Name);
+                GenerateColumnComment(builder, model, column.Comment, null, operation.Schema, operation.Name, column.Name);
             }
 
             builder.EndCommand(suppressTransaction: memoryOptimized);
@@ -671,7 +672,7 @@ protected override void Generate(EnsureSchemaOperation operation, IModel model,
             Check.NotNull(operation, nameof(operation));
             Check.NotNull(builder, nameof(builder));
 
-            if (string.Equals(operation.Name, "DBO", StringComparison.OrdinalIgnoreCase))
+            if (string.Equals(operation.Name, DefaultSchema, StringComparison.OrdinalIgnoreCase))
             {
                 return;
             }
@@ -965,7 +966,12 @@ protected override void Generate(AlterTableOperation operation, IModel model, Mi
                 throw new InvalidOperationException(SqlServerStrings.AlterMemoryOptimizedTable);
             }
 
-            base.Generate(operation, model, builder);
+            if (operation.OldTable.Comment != operation.Comment)
+            {
+                GenerateTableComment(builder, model, operation.Comment, operation.OldTable.Comment, operation.Schema, operation.Name);
+            }
+
+            builder.EndCommand(suppressTransaction: IsMemoryOptimized(operation, model, operation.Schema, operation.Name));
         }
 
         /// <summary>
@@ -1636,35 +1642,82 @@ protected virtual void CreateIndexes(
 
         /// <summary>
         ///     <para>
-        ///         Generates add and drop commands for comments on tables and columns.
+        ///         Generates add and drop commands for comments on tables.
+        ///     </para>
+        /// </summary>
+        /// <param name="builder"> The command builder to use to build the commands. </param>
+        /// <param name="model"> The target model which may be <c>null</c> if the operations exist without a model. </param>
+        /// <param name="comment"> The new comment to be applied. </param>
+        /// <param name="oldComment"> The previous comment. </param>
+        /// <param name="schema"> The schema of the table. </param>
+        /// <param name="table"> The name of the table. </param>
+        protected virtual void GenerateTableComment(
+            [NotNull] MigrationCommandListBuilder builder,
+            [CanBeNull] IModel model,
+            [CanBeNull] string comment,
+            [CanBeNull] string oldComment,
+            [CanBeNull] string schema,
+            [NotNull] string table)
+        {
+            if (comment == oldComment)
+            {
+                return;
+            }
+
+            schema ??= model?.GetDefaultSchema() ?? DefaultSchema;
+
+            if (oldComment != null)
+            {
+                GenerateDropExtendedProperty(builder,
+                    "Comment",
+                    "Schema", schema,
+                    "Table", table);
+            }
+
+            if (comment != null)
+            {
+                GenerateAddExtendedProperty(builder,
+                    "Comment", comment,
+                    "Schema", schema,
+                    "Table", table);
+            }
+        }
+
+        /// <summary>
+        ///     <para>
+        ///         Generates add and drop commands for comments on columns.
         ///     </para>
         /// </summary>
         /// <param name="builder"> The command builder to use to build the commands. </param>
+        /// <param name="model"> The target model which may be <c>null</c> if the operations exist without a model. </param>
         /// <param name="comment"> The new comment to be applied. </param>
         /// <param name="oldComment"> The previous comment. </param>
         /// <param name="schema"> The schema of the table. </param>
         /// <param name="table"> The name of the table. </param>
-        /// <param name="columnName"> The column name if comment is being applied to a column. </param>
-        protected virtual void AddDropComment(
+        /// <param name="columnName"> The name of the column. </param>
+        protected virtual void GenerateColumnComment(
             [NotNull] MigrationCommandListBuilder builder,
+            [CanBeNull] IModel model,
             [CanBeNull] string comment,
             [CanBeNull] string oldComment,
-            [NotNull] string schema,
+            [CanBeNull] string schema,
             [NotNull] string table,
-            [CanBeNull] string columnName = null)
+            [NotNull] string columnName)
         {
             if (comment == oldComment)
             {
                 return;
             }
 
+            schema ??= model?.GetDefaultSchema() ?? DefaultSchema;
+
             if (oldComment != null)
             {
                 GenerateDropExtendedProperty(builder,
                     "Comment",
                     "Schema", schema,
                     "Table", table,
-                    columnName == null ? null : "Column", columnName);
+                    "Column", columnName);
             }
 
             if (comment != null)
@@ -1673,7 +1726,7 @@ protected virtual void AddDropComment(
                     "Comment", comment,
                     "Schema", schema,
                     "Table", table,
-                    columnName == null ? null : "Column", columnName);
+                    "Column", columnName);
             }
         }
 

test/EFCore.Design.Tests/Migrations/Design/CSharpMigrationOperationGeneratorTest.cs

@@ -778,17 +778,23 @@ public void AlterTableOperation_all_args()
                 {
                     Name = "Customer",
                     Schema = "dbo",
-                    Comment = "My Comment"
+                    Comment = "My Comment 2",
+                    OldTable =
+                    {
+                        Comment = "My Comment"
+                    }
                 },
                 "mb.AlterTable(" + _eol +
                 "    name: \"Customer\"," + _eol +
                 "    schema: \"dbo\"," + _eol +
-                "    comment: \"My Comment\");",
+                "    comment: \"My Comment 2\"," + _eol +
+                "    oldComment: \"My Comment\");",
                 o =>
                 {
                     Assert.Equal("Customer", o.Name);
                     Assert.Equal("dbo", o.Schema);
-                    Assert.Equal("My Comment", o.Comment);
+                    Assert.Equal("My Comment 2", o.Comment);
+                    Assert.Equal("My Comment", o.OldTable.Comment);
                 });
         }
 

test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs

@@ -620,6 +620,38 @@ public void Create_table_columns_handles_self_referencing_one_to_one()
                 });
         }
 
+        [ConditionalFact]
+        public void Alter_table_comment()
+        {
+            Execute(
+                source => source.Entity(
+                    "MountainLion",
+                    x =>
+                    {
+                        x.ToTable("MountainLion", "dbo");
+                        x.Property<int>("Id");
+                        x.HasComment("Old comment");
+                    }),
+                target => target.Entity(
+                    "MountainLion",
+                    x =>
+                    {
+                        x.ToTable("MountainLion", "dbo");
+                        x.Property<int>("Id");
+                        x.HasComment("New comment");
+                    }),
+                operations =>
+                {
+                    Assert.Equal(1, operations.Count);
+
+                    var operation = Assert.IsType<AlterTableOperation>(operations[0]);
+                    Assert.Equal("dbo", operation.Schema);
+                    Assert.Equal("MountainLion", operation.Name);
+                    Assert.Equal("New comment", operation.Comment);
+                    Assert.Equal("Old comment", operation.OldTable.Comment);
+                });
+        }
+
         [ConditionalFact]
         public void Rename_table()
         {