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);
@@ -1053,6 +1061,13 @@ protected virtual void Generate([NotNull] CreateTableOperation operation, [NotNu
                                 .Append(Code.UnknownLiteral(column.DefaultValue));
                         }
 
+                        if (column.Comment != null)
+                        {
+                            builder
+                                .Append(", comment: ")
+                                .Append(Code.Literal(operation.Comment));
+                        }
+
                         builder.Append(")");
 
                         using (builder.Indent())

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);
 
@@ -679,7 +685,7 @@ public virtual OperationBuilder<CreateCheckConstraintOperation> CreateCheckConst
         /// <param name="constraints">
         ///     A delegate allowing constraints to be applied over the columns configured by the 'columns' delegate above.
         /// </param>
-        /// <param name="comment"> A comment to been applied to the table. </param>
+        /// <param name="comment"> A comment to be applied to the table. </param>
         /// <returns> A <see cref="CreateTableBuilder{TColumns}" /> to allow further configuration to be chained. </returns>
         public virtual CreateTableBuilder<TColumns> CreateTable<TColumns>(
             [NotNull] string name,

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/Builders/ColumnsBuilder.cs

@@ -44,6 +44,7 @@ public ColumnsBuilder([NotNull] CreateTableOperation createTableOperation)
         /// <param name="defaultValueSql"> The SQL expression to use for the column's default constraint. </param>
         /// <param name="computedColumnSql"> The SQL expression to use to compute the column value. </param>
         /// <param name="fixedLength"> Indicates whether or not the column is constrained to fixed-length data. </param>
+        /// <param name="comment"> A comment to be applied to the table. </param>
         /// <returns> The same builder so that multiple calls can be chained. </returns>
         public virtual OperationBuilder<AddColumnOperation> Column<T>(
             [CanBeNull] string type = null,
@@ -55,7 +56,8 @@ public virtual OperationBuilder<AddColumnOperation> Column<T>(
             [CanBeNull] object defaultValue = null,
             [CanBeNull] string defaultValueSql = null,
             [CanBeNull] string computedColumnSql = null,
-            bool? fixedLength = null)
+            bool? fixedLength = null,
+            [CanBeNull] string comment = null)
         {
             var operation = new AddColumnOperation
             {
@@ -71,7 +73,8 @@ public virtual OperationBuilder<AddColumnOperation> Column<T>(
                 DefaultValue = defaultValue,
                 DefaultValueSql = defaultValueSql,
                 ComputedColumnSql = computedColumnSql,
-                IsFixedLength = fixedLength
+                IsFixedLength = fixedLength,
+                Comment = comment
             };
             _createTableOperation.Columns.Add(operation);
 

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; }
+    }
+}