Fix to #35108 - Temporal table migration regression from EF Core 8 to 9 (#35289)

In 9 we changed the way we process migration of temporal tables. One of the changes was drastically reducing the number of annotations for columns which are part of temporal tables. This however caused regressions for cases where migration code was created using EF8 (and containing those legacy annotations) but then executed using EF9 tooling. Specifically, extra annotations were generating a number of superfluous Alter Column operations (which were only modifying those annotations). In EF8 we had logic to weed out those operations, but it was removed in EF9.

Fix is to remove all the legacy annotations on column operations before we start processing them. We no longer rely on them, but rather use annotations on Table operations and/or relational model. The only exception is CreateColumnOperation, so for it we convert old annotations to TemporalIsPeriodStartColumn and TemporalIsPeriodEndColumn where appropriate. Also, we are bringing back logic from EF8 which removed unnecessary AlterColumnOperations if the old and new columns are the same after the legacy temporal annotations have been removed.

Author: maumar

src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Collections;
 using System.Globalization;
 using System.Text;
 using Microsoft.EntityFrameworkCore.SqlServer.Internal;
@@ -1599,17 +1600,6 @@ protected override void ColumnDefinition(
         var isPeriodStartColumn = operation[SqlServerAnnotationNames.TemporalIsPeriodStartColumn] as bool? == true;
         var isPeriodEndColumn = operation[SqlServerAnnotationNames.TemporalIsPeriodEndColumn] as bool? == true;
 
-        // falling back to legacy annotations, in case the migration was generated using pre-9.0 bits
-        if (!isPeriodStartColumn && !isPeriodEndColumn)
-        {
-            if (operation[SqlServerAnnotationNames.TemporalPeriodStartColumnName] is string periodStartColumnName
-                && operation[SqlServerAnnotationNames.TemporalPeriodEndColumnName] is string periodEndColumnName)
-            {
-                isPeriodStartColumn = operation.Name == periodStartColumnName;
-                isPeriodEndColumn = operation.Name == periodEndColumnName;
-            }
-        }
-
         if (isPeriodStartColumn || isPeriodEndColumn)
         {
             builder.Append(" GENERATED ALWAYS AS ROW ");
@@ -2363,11 +2353,140 @@ private string Uniquify(string variableName, bool increase = true)
         return _variableCounter == 0 ? variableName : variableName + _variableCounter;
     }
 
+    private IReadOnlyList<MigrationOperation> FixLegacyTemporalAnnotations(IReadOnlyList<MigrationOperation> migrationOperations)
+    {
+        // short-circuit for non-temporal migrations (which is the majority)
+        if (migrationOperations.All(o => o[SqlServerAnnotationNames.IsTemporal] as bool? != true))
+        {
+            return migrationOperations;
+        }
+
+        var resultOperations = new List<MigrationOperation>(migrationOperations.Count);
+        foreach (var migrationOperation in migrationOperations)
+        {
+            var isTemporal = migrationOperation[SqlServerAnnotationNames.IsTemporal] as bool? == true;
+            if (!isTemporal)
+            {
+                resultOperations.Add(migrationOperation);
+                continue;
+            }
+
+            switch (migrationOperation)
+            {
+                case CreateTableOperation createTableOperation:
+
+                    foreach (var column in createTableOperation.Columns)
+                    {
+                        NormalizeTemporalAnnotationsForAddColumnOperation(column);
+                    }
+
+                    resultOperations.Add(migrationOperation);
+                    break;
+
+                case AddColumnOperation addColumnOperation:
+                    NormalizeTemporalAnnotationsForAddColumnOperation(addColumnOperation);
+                    resultOperations.Add(addColumnOperation);
+                    break;
+
+                case AlterColumnOperation alterColumnOperation:
+                    RemoveLegacyTemporalColumnAnnotations(alterColumnOperation);
+                    RemoveLegacyTemporalColumnAnnotations(alterColumnOperation.OldColumn);
+                    if (!CanSkipAlterColumnOperation(alterColumnOperation, alterColumnOperation.OldColumn))
+                    {
+                        resultOperations.Add(alterColumnOperation);
+                    }
+
+                    break;
+
+                case DropColumnOperation dropColumnOperation:
+                    RemoveLegacyTemporalColumnAnnotations(dropColumnOperation);
+                    resultOperations.Add(dropColumnOperation);
+                    break;
+
+                case RenameColumnOperation renameColumnOperation:
+                    RemoveLegacyTemporalColumnAnnotations(renameColumnOperation);
+                    resultOperations.Add(renameColumnOperation);
+                    break;
+
+                default:
+                    resultOperations.Add(migrationOperation);
+                    break;
+            }
+        }
+
+        return resultOperations;
+
+        static void NormalizeTemporalAnnotationsForAddColumnOperation(AddColumnOperation addColumnOperation)
+        {
+            var periodStartColumnName = addColumnOperation[SqlServerAnnotationNames.TemporalPeriodStartColumnName] as string;
+            var periodEndColumnName = addColumnOperation[SqlServerAnnotationNames.TemporalPeriodEndColumnName] as string;
+            if (periodStartColumnName == addColumnOperation.Name)
+            {
+                addColumnOperation.AddAnnotation(SqlServerAnnotationNames.TemporalIsPeriodStartColumn, true);
+            }
+            else if (periodEndColumnName == addColumnOperation.Name)
+            {
+                addColumnOperation.AddAnnotation(SqlServerAnnotationNames.TemporalIsPeriodEndColumn, true);
+            }
+
+            RemoveLegacyTemporalColumnAnnotations(addColumnOperation);
+        }
+
+        static void RemoveLegacyTemporalColumnAnnotations(MigrationOperation operation)
+        {
+            operation.RemoveAnnotation(SqlServerAnnotationNames.IsTemporal);
+            operation.RemoveAnnotation(SqlServerAnnotationNames.TemporalHistoryTableName);
+            operation.RemoveAnnotation(SqlServerAnnotationNames.TemporalHistoryTableSchema);
+            operation.RemoveAnnotation(SqlServerAnnotationNames.TemporalPeriodStartColumnName);
+            operation.RemoveAnnotation(SqlServerAnnotationNames.TemporalPeriodEndColumnName);
+        }
+
+        static bool CanSkipAlterColumnOperation(ColumnOperation column, ColumnOperation oldColumn)
+            => ColumnPropertiesAreTheSame(column, oldColumn) && AnnotationsAreTheSame(column, oldColumn);
+
+        // don't compare name, table or schema - they are not being set in the model differ (since they should always be the same)
+        static bool ColumnPropertiesAreTheSame(ColumnOperation column, ColumnOperation oldColumn)
+            => column.ClrType == oldColumn.ClrType
+                && column.Collation == oldColumn.Collation
+                && column.ColumnType == oldColumn.ColumnType
+                && column.Comment == oldColumn.Comment
+                && column.ComputedColumnSql == oldColumn.ComputedColumnSql
+                && Equals(column.DefaultValue, oldColumn.DefaultValue)
+                && column.DefaultValueSql == oldColumn.DefaultValueSql
+                && column.IsDestructiveChange == oldColumn.IsDestructiveChange
+                && column.IsFixedLength == oldColumn.IsFixedLength
+                && column.IsNullable == oldColumn.IsNullable
+                && column.IsReadOnly == oldColumn.IsReadOnly
+                && column.IsRowVersion == oldColumn.IsRowVersion
+                && column.IsStored == oldColumn.IsStored
+                && column.IsUnicode == oldColumn.IsUnicode
+                && column.MaxLength == oldColumn.MaxLength
+                && column.Precision == oldColumn.Precision
+                && column.Scale == oldColumn.Scale;
+
+        static bool AnnotationsAreTheSame(ColumnOperation column, ColumnOperation oldColumn)
+        {
+            var columnAnnotations = column.GetAnnotations().ToList();
+            var oldColumnAnnotations = oldColumn.GetAnnotations().ToList();
+
+            if (columnAnnotations.Count != oldColumnAnnotations.Count)
+            {
+                return false;
+            }
+
+            return columnAnnotations.Zip(oldColumnAnnotations)
+                .All(x => x.First.Name == x.Second.Name
+                    && StructuralComparisons.StructuralEqualityComparer.Equals(x.First.Value, x.Second.Value));
+        }
+    }
+
     private IReadOnlyList<MigrationOperation> RewriteOperations(
         IReadOnlyList<MigrationOperation> migrationOperations,
         IModel? model,
         MigrationsSqlGenerationOptions options)
     {
+        migrationOperations = FixLegacyTemporalAnnotations(migrationOperations);
+
         var operations = new List<MigrationOperation>();
         var availableSchemas = new List<string>();