Skip to content

Commit

Permalink
Fix to #26676 - EF Core 6 - IsTemporal adds an unnecessary migration …
Browse files Browse the repository at this point in the history
…when DefaultSchema is configured at the DbContext Level

Problem was a discrepancy between RelationalModel build in runtime and the current model (from OnModelCreating) - relational model would set history table schema annotation using the table schema or default schema, however current model wouldn't set those annotations.
Model differ would then pick up on those differences and create migration to set the schema for history table. When building actual migration sql we already compensated for the difference and not generate actual sql code, but the problem persists.

Fix is to improve the temporal convention so that history table schema in current model (more) closely resembles the temporal table schema and therefore is similar to RelationalModel generated in runtime.

Fixes #26676
  • Loading branch information
maumar committed Jan 18, 2022
1 parent 6523335 commit 66e9565
Show file tree
Hide file tree
Showing 6 changed files with 1,100 additions and 140 deletions.
11 changes: 2 additions & 9 deletions src/EFCore.SqlServer/Extensions/SqlServerEntityTypeExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@ namespace Microsoft.EntityFrameworkCore;
/// </remarks>
public static class SqlServerEntityTypeExtensions
{
private const string DefaultHistoryTableNameSuffix = "History";

/// <summary>
/// Returns a value indicating whether the entity type is mapped to a memory-optimized table.
/// </summary>
Expand Down Expand Up @@ -202,11 +200,7 @@ public static void SetPeriodEndPropertyName(this IMutableEntityType entityType,
? throw new InvalidOperationException(CoreStrings.RuntimeModelMissingData)
: entityType[SqlServerAnnotationNames.TemporalHistoryTableName] is string historyTableName
? historyTableName
: entityType[SqlServerAnnotationNames.IsTemporal] as bool? == true
? entityType.GetTableName() is string tableName
? tableName + DefaultHistoryTableNameSuffix
: null
: null;
: null;

/// <summary>
/// Sets a value representing the name of the history table associated with the entity mapped to a temporal table.
Expand Down Expand Up @@ -252,8 +246,7 @@ public static void SetHistoryTableName(this IMutableEntityType entityType, strin
public static string? GetHistoryTableSchema(this IReadOnlyEntityType entityType)
=> (entityType is RuntimeEntityType)
? throw new InvalidOperationException(CoreStrings.RuntimeModelMissingData)
: entityType[SqlServerAnnotationNames.TemporalHistoryTableSchema] as string
?? entityType[RelationalAnnotationNames.Schema] as string;
: entityType[SqlServerAnnotationNames.TemporalHistoryTableSchema] as string;

/// <summary>
/// Sets a value representing the schema of the history table associated with the entity mapped to a temporal table.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@ namespace Microsoft.EntityFrameworkCore.Metadata.Conventions;
/// </remarks>
public class SqlServerTemporalConvention : IEntityTypeAnnotationChangedConvention, ISkipNavigationForeignKeyChangedConvention
{
private const string PeriodStartDefaultName = "PeriodStart";
private const string PeriodEndDefaultName = "PeriodEnd";
private const string DefaultPeriodStartName = "PeriodStart";
private const string DefaultPeriodEndName = "PeriodEnd";
private const string DefaultHistoryTableNameSuffix = "History";

/// <summary>
/// Creates a new instance of <see cref="SqlServerTemporalConvention" />.
Expand Down Expand Up @@ -56,12 +57,18 @@ public virtual void ProcessEntityTypeAnnotationChanged(
{
if (entityTypeBuilder.Metadata.GetPeriodStartPropertyName() == null)
{
entityTypeBuilder.HasPeriodStart(PeriodStartDefaultName);
entityTypeBuilder.HasPeriodStart(DefaultPeriodStartName);
}

if (entityTypeBuilder.Metadata.GetPeriodEndPropertyName() == null)
{
entityTypeBuilder.HasPeriodEnd(PeriodEndDefaultName);
entityTypeBuilder.HasPeriodEnd(DefaultPeriodEndName);
}

if (entityTypeBuilder.Metadata.GetHistoryTableName() == null)
{
var tableName = entityTypeBuilder.Metadata.GetTableName();
entityTypeBuilder.UseHistoryTableName(tableName + DefaultHistoryTableNameSuffix);
}

foreach (var skipLevelNavigation in entityTypeBuilder.Metadata.GetSkipNavigations())
Expand Down Expand Up @@ -118,6 +125,23 @@ public virtual void ProcessEntityTypeAnnotationChanged(
periodPropertyBuilder?.HasColumnName(periodPropertyName);
}
}

if (name == RelationalAnnotationNames.Schema
&& entityTypeBuilder.Metadata.IsTemporal())
{
// if table schema changes, also update the history table schema
entityTypeBuilder.UseHistoryTableSchema(annotation?.Value as string);
}

if (name == SqlServerAnnotationNames.TemporalHistoryTableName)
{
// when setting up history table (name),
// also by convention set up it's schema to be the same as the temporal table
// we have to use annotation directly, so that we don't use the default model schema
// in case table schema is not explicitly set
var tableSchemaAnnotation = entityTypeBuilder.Metadata.FindAnnotation(RelationalAnnotationNames.Schema);
entityTypeBuilder.UseHistoryTableSchema(tableSchemaAnnotation?.Value as string);
}
}

/// <inheritdoc />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,8 @@ protected override void Generate(
if (operation[SqlServerAnnotationNames.IsTemporal] as bool? == true)
{
var historyTableSchema = operation[SqlServerAnnotationNames.TemporalHistoryTableSchema] as string
?? model?.GetDefaultSchema();
?? operation.Schema;

var needsExec = historyTableSchema == null;
var subBuilder = needsExec
? new MigrationCommandListBuilder(Dependencies)
Expand Down Expand Up @@ -1261,7 +1262,7 @@ protected override void Generate(
{
var historyTableName = operation[SqlServerAnnotationNames.TemporalHistoryTableName] as string;
var historyTableSchema = operation[SqlServerAnnotationNames.TemporalHistoryTableSchema] as string
?? model?.GetDefaultSchema();
?? operation.Schema ?? model?.GetDefaultSchema();
var periodStartColumnName = operation[SqlServerAnnotationNames.TemporalPeriodStartColumnName] as string;
var periodEndColumnName = operation[SqlServerAnnotationNames.TemporalPeriodEndColumnName] as string;

Expand Down Expand Up @@ -2261,7 +2262,7 @@ private IReadOnlyList<MigrationOperation> RewriteOperations(
schema ??= model?.GetDefaultSchema();
var historyTableName = operation[SqlServerAnnotationNames.TemporalHistoryTableName] as string;
var historyTableSchema = operation[SqlServerAnnotationNames.TemporalHistoryTableSchema] as string
?? model?.GetDefaultSchema();
?? schema;
var periodStartColumnName = operation[SqlServerAnnotationNames.TemporalPeriodStartColumnName] as string;
var periodEndColumnName = operation[SqlServerAnnotationNames.TemporalPeriodEndColumnName] as string;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2123,6 +2123,7 @@ public virtual void Temporal_table_information_is_stored_in_snapshot_minimal_set
b.ToTable(tb => tb.IsTemporal(ttb =>
{
ttb.UseHistoryTable(""EntityWithStringPropertyHistory"");
ttb
.HasPeriodStart(""PeriodStart"")
.HasColumnName(""PeriodStart"");
Expand All @@ -2138,7 +2139,7 @@ public virtual void Temporal_table_information_is_stored_in_snapshot_minimal_set
"Microsoft.EntityFrameworkCore.Migrations.ModelSnapshotSqlServerTest+EntityWithStringProperty");
var annotations = temporalEntity.GetAnnotations().ToList();

Assert.Equal(5, annotations.Count);
Assert.Equal(6, annotations.Count);
Assert.Contains(annotations, a => a.Name == SqlServerAnnotationNames.IsTemporal && a.Value as bool? == true);
Assert.Contains(
annotations,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1062,6 +1062,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
{
entity.ToTable(tb => tb.IsTemporal(ttb =>
{
ttb.UseHistoryTable(""CustomerHistory"");
ttb
.HasPeriodStart(""PeriodStart"")
.HasColumnName(""PeriodStart"");
Expand Down
Loading

0 comments on commit 66e9565

Please sign in to comment.