Skip to content

Commit

Permalink
Fix to #35108 - Temporal table migration regression from EF Core 8 to 9
Browse files Browse the repository at this point in the history
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.

Fixes #35108
Also fixes #35148 which was the same underlying issue.
  • Loading branch information
maumar committed Dec 6, 2024
1 parent f57e57f commit 9f5e228
Show file tree
Hide file tree
Showing 2 changed files with 1,197 additions and 11 deletions.
136 changes: 125 additions & 11 deletions src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -1627,17 +1628,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 ");
Expand Down Expand Up @@ -2391,11 +2381,135 @@ private string Uniquify(string variableName, bool increase = true)
return _variableCounter == 0 ? variableName : variableName + _variableCounter;
}

private IReadOnlyList<MigrationOperation> FixLegacyTemporalAnnotations(IReadOnlyList<MigrationOperation> migrationOperations)
{
var resultOperations = new List<MigrationOperation>();
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 first, ColumnOperation second)
=> ColumnPropertiesAreTheSame(first, second) && AnnotationsAreTheSame(first, second);

// 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 first, ColumnOperation second)
=> first.ClrType == second.ClrType
&& first.Collation == second.Collation
&& first.ColumnType == second.ColumnType
&& first.Comment == second.Comment
&& first.ComputedColumnSql == second.ComputedColumnSql
&& Equals(first.DefaultValue, second.DefaultValue)
&& first.DefaultValueSql == second.DefaultValueSql
&& first.IsDestructiveChange == second.IsDestructiveChange
&& first.IsFixedLength == second.IsFixedLength
&& first.IsNullable == second.IsNullable
&& first.IsReadOnly == second.IsReadOnly
&& first.IsRowVersion == second.IsRowVersion
&& first.IsStored == second.IsStored
&& first.IsUnicode == second.IsUnicode
&& first.MaxLength == second.MaxLength
&& first.Precision == second.Precision
&& first.Scale == second.Scale;

static bool AnnotationsAreTheSame(ColumnOperation first, ColumnOperation second)
{
var firstAnnotations = first.GetAnnotations().OrderBy(x => x.Name).ToList();
var secondAnnotations = second.GetAnnotations().OrderBy(x => x.Name).ToList();

if (firstAnnotations.Count != secondAnnotations.Count)
{
return false;
}

return firstAnnotations.Zip(
secondAnnotations,
(f, s) => f.Name == s.Name && StructuralComparisons.StructuralEqualityComparer.Equals(f.Value, s.Value))
.All(x => x == true);
}
}

private IReadOnlyList<MigrationOperation> RewriteOperations(
IReadOnlyList<MigrationOperation> migrationOperations,
IModel? model,
MigrationsSqlGenerationOptions options)
{
migrationOperations = FixLegacyTemporalAnnotations(migrationOperations);

var operations = new List<MigrationOperation>();
var availableSchemas = new List<string>();

Expand Down
Loading

0 comments on commit 9f5e228

Please sign in to comment.