Skip to content

Commit

Permalink
Fix to #27423 - Renaming and dropping columns with Temporal Tables ge…
Browse files Browse the repository at this point in the history 
…nerates faulty migration

Problem is that when we drop column from the history table we need to disable versioning (and drop columns in both tables separately). However, since we have disabled the period, renaming (and other operations) also needs to happen for both tables now and we don't do it. So the column is only renamed for the temporal table and the name in history table stays the same. When we try to switch versioning back on the exception is thrown.

Fix is to flow the information to ColumnOperations that the column is part of a temporal table and when we process the migrations, if the versioning for the temporal table has been disabled, mirror the necessary operation(s) to the history table also.

Fixes #27423
  • Loading branch information
@maumar
maumar committed Mar 9, 2022
1 parent e1a3ee7 commit a5bc598
Show file tree
Hide file tree
Showing 4 changed files with 905 additions and 17 deletions.
13 changes: 6 additions & 7 deletions src/EFCore.SqlServer/Metadata/Internal/SqlServerAnnotationProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -264,13 +264,12 @@ public override IEnumerable<IAnnotation> For(IColumn column, bool designTime)
? periodEndProperty.GetColumnName(storeObjectIdentifier)
: periodEndPropertyName;

if (column.Name == periodStartColumnName
|| column.Name == periodEndColumnName)
{
yield return new Annotation(SqlServerAnnotationNames.IsTemporal, true);
yield return new Annotation(SqlServerAnnotationNames.TemporalPeriodStartColumnName, periodStartColumnName);
yield return new Annotation(SqlServerAnnotationNames.TemporalPeriodEndColumnName, periodEndColumnName);
}
// TODO: issue #27459 - we want to avoid having those annotations on every column
yield return new Annotation(SqlServerAnnotationNames.IsTemporal, true);
yield return new Annotation(SqlServerAnnotationNames.TemporalHistoryTableName, entityType.GetHistoryTableName());
yield return new Annotation(SqlServerAnnotationNames.TemporalHistoryTableSchema, entityType.GetHistoryTableSchema());
yield return new Annotation(SqlServerAnnotationNames.TemporalPeriodStartColumnName, periodStartColumnName);
yield return new Annotation(SqlServerAnnotationNames.TemporalPeriodEndColumnName, periodEndColumnName);
}
}
}
31 changes: 31 additions & 0 deletions src/EFCore.SqlServer/Migrations/Internal/SqlServerMigrationsAnnotationProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,4 +104,35 @@ public override IEnumerable<IAnnotation> ForRename(ITable table)
table[SqlServerAnnotationNames.TemporalHistoryTableSchema]);
}
}

/// <inheritdoc />
public override IEnumerable<IAnnotation> ForRename(IColumn column)
{
if (column.Table[SqlServerAnnotationNames.IsTemporal] as bool? == true)
{
yield return new Annotation(SqlServerAnnotationNames.IsTemporal, true);

yield return new Annotation(
SqlServerAnnotationNames.TemporalHistoryTableName,
column.Table[SqlServerAnnotationNames.TemporalHistoryTableName]);

yield return new Annotation(
SqlServerAnnotationNames.TemporalHistoryTableSchema,
column.Table[SqlServerAnnotationNames.TemporalHistoryTableSchema]);

if (column[SqlServerAnnotationNames.TemporalPeriodStartColumnName] is string periodStartColumnName)
{
yield return new Annotation(
SqlServerAnnotationNames.TemporalPeriodStartColumnName,
periodStartColumnName);
}

if (column[SqlServerAnnotationNames.TemporalPeriodEndColumnName] is string periodEndColumnName)
{
yield return new Annotation(
SqlServerAnnotationNames.TemporalPeriodEndColumnName,
periodEndColumnName);
}
}
}
}
119 changes: 109 additions & 10 deletions src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2355,21 +2355,47 @@ alterTableOperation.OldTable[SqlServerAnnotationNames.TemporalHistoryTableSchema
// if only difference is in temporal annotations being removed or history table changed etc - we can ignore this operation
if (!CanSkipAlterColumnOperation(alterColumnOperation.OldColumn, alterColumnOperation))
{
operations.Add(operation);

// when modifying a period column, we need to perform the operations as a normal column first, and only later enable period
// removing the period information now, so that when we generate SQL that modifies the column we won't be making them auto generated as period
// (making column auto generated is not allowed in ALTER COLUMN statement)
// in later operation we enable the period and the period columns get set to auto generated automatically
if (alterColumnOperation[SqlServerAnnotationNames.IsTemporal] as bool? == true
&& alterColumnOperation.OldColumn[SqlServerAnnotationNames.IsTemporal] is null)
//
// if the column is not period we just remove temporal information - it's no longer needed and could affect the generated sql
// we will generate all the necessary operations involved with temporal tables here
alterColumnOperation.RemoveAnnotation(SqlServerAnnotationNames.IsTemporal);
alterColumnOperation.RemoveAnnotation(SqlServerAnnotationNames.TemporalPeriodStartColumnName);
alterColumnOperation.RemoveAnnotation(SqlServerAnnotationNames.TemporalPeriodEndColumnName);
alterColumnOperation.RemoveAnnotation(SqlServerAnnotationNames.TemporalHistoryTableName);
alterColumnOperation.RemoveAnnotation(SqlServerAnnotationNames.TemporalHistoryTableSchema);

// this is the case where we are not converting from normal table to temporal
// just a normal modification to a column on a temporal table
// in that case we need to double check if we need have disabled versioning earlier in this migration
// if so, we need to mirror the operation to the history table
if (alterColumnOperation.OldColumn[SqlServerAnnotationNames.IsTemporal] as bool? == true)
{
alterColumnOperation.RemoveAnnotation(SqlServerAnnotationNames.IsTemporal);
alterColumnOperation.RemoveAnnotation(SqlServerAnnotationNames.TemporalPeriodStartColumnName);
alterColumnOperation.RemoveAnnotation(SqlServerAnnotationNames.TemporalPeriodEndColumnName);
alterColumnOperation.OldColumn.RemoveAnnotation(SqlServerAnnotationNames.IsTemporal);
alterColumnOperation.OldColumn.RemoveAnnotation(SqlServerAnnotationNames.TemporalPeriodStartColumnName);
alterColumnOperation.OldColumn.RemoveAnnotation(SqlServerAnnotationNames.TemporalPeriodEndColumnName);
alterColumnOperation.OldColumn.RemoveAnnotation(SqlServerAnnotationNames.TemporalHistoryTableName);
alterColumnOperation.OldColumn.RemoveAnnotation(SqlServerAnnotationNames.TemporalHistoryTableSchema);

if (versioningMap.ContainsKey((table, schema)))
{
var alterHistoryTableColumn = CopyColumnOperation<AlterColumnOperation>(alterColumnOperation);
alterHistoryTableColumn.Table = historyTableName!;
alterHistoryTableColumn.Schema = historyTableSchema;
alterHistoryTableColumn.OldColumn = CopyColumnOperation<AddColumnOperation>(alterColumnOperation.OldColumn);
alterHistoryTableColumn.OldColumn.Table = historyTableName!;
alterHistoryTableColumn.OldColumn.Schema = historyTableSchema;

operations.Add(alterHistoryTableColumn);
}

// TODO: test what happens if default value just changes (from temporal to temporal)
}

operations.Add(operation);
}

break;
Expand All @@ -2396,13 +2422,17 @@ alterTableOperation.OldTable[SqlServerAnnotationNames.TemporalHistoryTableSchema
break;

case AddColumnOperation addColumnOperation:
operations.Add(addColumnOperation);

// when adding a period column, we need to add it as a normal column first, and only later enable period
// removing the period information now, so that when we generate SQL that adds the column we won't be making them auto generated as period
// it won't work, unless period is enabled
// but we can't enable period without adding the columns first - chicken and egg
if (addColumnOperation[SqlServerAnnotationNames.IsTemporal] as bool? == true)
{
addColumnOperation.RemoveAnnotation(SqlServerAnnotationNames.IsTemporal);
addColumnOperation.RemoveAnnotation(SqlServerAnnotationNames.TemporalHistoryTableName);
addColumnOperation.RemoveAnnotation(SqlServerAnnotationNames.TemporalHistoryTableSchema);
addColumnOperation.RemoveAnnotation(SqlServerAnnotationNames.TemporalPeriodStartColumnName);
addColumnOperation.RemoveAnnotation(SqlServerAnnotationNames.TemporalPeriodEndColumnName);

Expand All @@ -2411,14 +2441,48 @@ alterTableOperation.OldTable[SqlServerAnnotationNames.TemporalHistoryTableSchema
{
addColumnOperation.DefaultValue = DateTime.MaxValue;
}

// when adding (non-period) column to an exisiting temporal table we need to check if we have disabled the period
// due to some other operations in the same migration (e.g. delete column)
// if so, we need to also add the same column to history table
if (addColumnOperation.Name != periodStartColumnName
&& addColumnOperation.Name != periodEndColumnName)
{
if (versioningMap.ContainsKey((table, schema)))
{
var addHistoryTableColumnOperation = CopyColumnOperation<AddColumnOperation>(addColumnOperation);
addHistoryTableColumnOperation.Table = historyTableName!;
addHistoryTableColumnOperation.Schema = historyTableSchema;

operations.Add(addHistoryTableColumnOperation);
}
}
}

break;

case RenameColumnOperation renameColumnOperation:
operations.Add(renameColumnOperation);

// if we disabled period for the temporal table and now we are renaming the column,
// we need to also rename this same column in history table
if (versioningMap.ContainsKey((table, schema)))
{
var renameHistoryTableColumnOperation = new RenameColumnOperation
{
IsDestructiveChange = renameColumnOperation.IsDestructiveChange,
Name = renameColumnOperation.Name,
NewName = renameColumnOperation.NewName,
Table = historyTableName!,
Schema = historyTableSchema
};

operations.Add(renameHistoryTableColumnOperation);
}

operations.Add(addColumnOperation);
break;

default:
// CreateTableOperation
// RenameColumnOperation
operations.Add(operation);
break;
}
Expand Down Expand Up @@ -2607,6 +2671,7 @@ static bool CanSkipAlterColumnOperation(ColumnOperation first, ColumnOperation s
&& ColumnOperationsOnlyDifferByTemporalTableAnnotation(first, second)
&& ColumnOperationsOnlyDifferByTemporalTableAnnotation(second, first);

// 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
Expand Down Expand Up @@ -2651,5 +2716,39 @@ static bool ColumnOperationsOnlyDifferByTemporalTableAnnotation(ColumnOperation
|| a.Name == SqlServerAnnotationNames.TemporalPeriodStartColumnName
|| a.Name == SqlServerAnnotationNames.TemporalPeriodEndColumnName);
}

static TOperation CopyColumnOperation<TOperation>(ColumnOperation source)
where TOperation : ColumnOperation, new()
{
var result = new TOperation
{
ClrType = source.ClrType,
Collation = source.Collation,
ColumnType = source.ColumnType,
Comment = source.Comment,
ComputedColumnSql = source.ComputedColumnSql,
DefaultValue = source.DefaultValue,
DefaultValueSql = source.DefaultValueSql,
IsDestructiveChange = source.IsDestructiveChange,
IsFixedLength = source.IsFixedLength,
IsNullable = source.IsNullable,
IsRowVersion = source.IsRowVersion,
IsStored = source.IsStored,
IsUnicode = source.IsUnicode,
MaxLength = source.MaxLength,
Name = source.Name,
Precision = source.Precision,
Scale = source.Scale,
Table = source.Table,
Schema = source.Schema
};

foreach (var annotation in source.GetAnnotations())
{
result.AddAnnotation(annotation.Name, annotation.Value);
}

return result;
}
}
}
Loading

0 comments on commit a5bc598

Please sign in to comment.