Skip to content

Commit c96f8a2

Browse files
maumarmaumar
committed
Targeted fix for #35162 - Regression from EF Core 8 to 9: MigrationBuilder.DropTable Causes Issues with Subsequent Table Recreation
Problem was that in 9.0 we changed how we process temporal tables, tracking the temporal table information as we process migrations. However, we didn't take into account the cases where users would manually modify their migrations, or squash multiple migrations together, resulting in corrupted dictionary access for cases where a table was deleted and created in the same migration. This fix just makes the temporal information map handling more robust, so those errors don't happen anymore. There is a broader issue with temporal tables but those will be addressed in a different PR. Fixes #35162
1 parent ba7e7bc commit c96f8a2

File tree

4 files changed

+281
-5
lines changed

4 files changed

+281
-5
lines changed

Diff for: ‎src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs

+20-5
Original file line numberDiff line numberDiff line change
@@ -2577,10 +2577,16 @@ private IReadOnlyList<MigrationOperation> RewriteOperations(
25772577
var newRawSchema = renameTableOperation.NewSchema;
25782578
var newSchema = newRawSchema ?? model?.GetDefaultSchema();
25792579

2580+
var temporalTableInformation = BuildTemporalInformationFromMigrationOperation(schema, renameTableOperation);
25802581
if (!temporalTableInformationMap.ContainsKey((tableName, rawSchema)))
25812582
{
2582-
var temporalTableInformation = BuildTemporalInformationFromMigrationOperation(schema, renameTableOperation);
25832583
temporalTableInformationMap[(tableName, rawSchema)] = temporalTableInformation;
2584+
}
2585+
2586+
// we still need to check here - table with the new name could have existed before and have been deleted
2587+
// we want to preserve the original temporal info of that deleted table
2588+
if (!temporalTableInformationMap.ContainsKey((newTableName, newRawSchema)))
2589+
{
25842590
temporalTableInformationMap[(newTableName, newRawSchema)] = temporalTableInformation;
25852591
}
25862592

@@ -2675,10 +2681,19 @@ private IReadOnlyList<MigrationOperation> RewriteOperations(
26752681

26762682
var schema = rawSchema ?? model?.GetDefaultSchema();
26772683

2678-
// we are guaranteed to find entry here - we looped through all the operations earlier,
2679-
// info missing from operations we got from the model
2680-
// and in case of no/incomplete model we created dummy (non-temporal) entries
2681-
var temporalInformation = temporalTableInformationMap[(tableName, rawSchema)];
2684+
TemporalOperationInformation temporalInformation;
2685+
if (operation is CreateTableOperation)
2686+
{
2687+
// for create table we always generate new temporal information from the operation itself
2688+
// just in case there was a table with that name before that got deleted/renamed
2689+
// also, temporal state (disabled versioning etc.) should always reset when creating a table
2690+
temporalInformation = BuildTemporalInformationFromMigrationOperation(schema, operation);
2691+
temporalTableInformationMap[(tableName, rawSchema)] = temporalInformation;
2692+
}
2693+
else
2694+
{
2695+
temporalInformation = temporalTableInformationMap[(tableName, rawSchema)];
2696+
}
26822697

26832698
switch (operation)
26842699
{

Diff for: ‎test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs

+172
Original file line numberDiff line numberDiff line change
@@ -3256,6 +3256,122 @@ public virtual Task Add_required_primitve_collection_with_custom_converter_and_c
32563256
Assert.Single(customersTable.PrimaryKey!.Columns));
32573257
});
32583258

3259+
[ConditionalFact]
3260+
public virtual Task Multiop_drop_table_and_create_the_same_table_in_one_migration()
3261+
=> TestComposite(
3262+
[
3263+
builder => builder.Entity(
3264+
"Customer", e =>
3265+
{
3266+
e.Property<int>("Id").ValueGeneratedOnAdd();
3267+
e.Property<string>("Name");
3268+
e.HasKey("Id");
3269+
e.ToTable("Customers");
3270+
}),
3271+
builder => { },
3272+
builder => builder.Entity(
3273+
"Customer", e =>
3274+
{
3275+
e.Property<int>("Id").ValueGeneratedOnAdd();
3276+
e.Property<string>("Name");
3277+
e.HasKey("Id");
3278+
3279+
e.ToTable("Customers");
3280+
})
3281+
],
3282+
model => { });
3283+
3284+
[ConditionalFact]
3285+
public virtual Task Multiop_create_table_and_drop_it_in_one_migration()
3286+
=> TestComposite(
3287+
[
3288+
builder => { },
3289+
builder => builder.Entity(
3290+
"Customer", e =>
3291+
{
3292+
e.Property<int>("Id").ValueGeneratedOnAdd();
3293+
e.Property<string>("Name");
3294+
e.HasKey("Id");
3295+
3296+
e.ToTable("Customers");
3297+
}),
3298+
builder => { },
3299+
],
3300+
model => { });
3301+
3302+
[ConditionalFact]
3303+
public virtual Task Multiop_rename_table_and_drop()
3304+
=> TestComposite(
3305+
[
3306+
builder => builder.Entity(
3307+
"Customer", e =>
3308+
{
3309+
e.Property<int>("Id").ValueGeneratedOnAdd();
3310+
e.Property<string>("Name");
3311+
e.HasKey("Id");
3312+
3313+
e.ToTable("Customers");
3314+
}),
3315+
builder => builder.Entity(
3316+
"Customer", e =>
3317+
{
3318+
e.Property<int>("Id").ValueGeneratedOnAdd();
3319+
e.Property<string>("Name");
3320+
e.HasKey("Id");
3321+
3322+
e.ToTable("NewCustomers");
3323+
}),
3324+
builder => { },
3325+
],
3326+
model => { });
3327+
3328+
[ConditionalFact]
3329+
public virtual Task Multiop_rename_table_and_create_new_table_with_the_old_name()
3330+
=> TestComposite(
3331+
[
3332+
builder => builder.Entity(
3333+
"Customer", e =>
3334+
{
3335+
e.Property<int>("Id").ValueGeneratedOnAdd();
3336+
e.Property<string>("Name");
3337+
e.HasKey("Id");
3338+
3339+
e.ToTable("Customers");
3340+
}),
3341+
builder => builder.Entity(
3342+
"Customer", e =>
3343+
{
3344+
e.Property<int>("Id").ValueGeneratedOnAdd();
3345+
e.Property<string>("Name");
3346+
e.HasKey("Id");
3347+
3348+
e.ToTable("NewCustomers");
3349+
}),
3350+
builder =>
3351+
{
3352+
builder.Entity(
3353+
"Customer", e =>
3354+
{
3355+
e.Property<int>("Id").ValueGeneratedOnAdd();
3356+
e.Property<string>("Name");
3357+
e.HasKey("Id");
3358+
3359+
e.ToTable("NewCustomers");
3360+
});
3361+
3362+
builder.Entity(
3363+
"AnotherCustomer", e =>
3364+
{
3365+
e.Property<int>("Id").ValueGeneratedOnAdd();
3366+
e.Property<string>("Name");
3367+
e.HasKey("Id");
3368+
3369+
e.ToTable("Customers");
3370+
});
3371+
},
3372+
],
3373+
model => { });
3374+
32593375
protected class Person
32603376
{
32613377
public int Id { get; set; }
@@ -3305,6 +3421,62 @@ protected virtual Task Test(
33053421
MigrationsSqlGenerationOptions migrationsSqlGenerationOptions = MigrationsSqlGenerationOptions.Default)
33063422
=> Test(_ => { }, buildSourceAction, buildTargetAction, asserter, withConventions, migrationsSqlGenerationOptions);
33073423

3424+
protected virtual Task TestComposite(
3425+
List<Action<ModelBuilder>> buildActions,
3426+
Action<DatabaseModel> asserter,
3427+
bool withConventions = true,
3428+
MigrationsSqlGenerationOptions migrationsSqlGenerationOptions = MigrationsSqlGenerationOptions.Default)
3429+
{
3430+
if (buildActions.Count < 3)
3431+
{
3432+
throw new InvalidOperationException("You need at least 3 build actions for the composite case.");
3433+
}
3434+
3435+
var context = CreateContext();
3436+
var modelDiffer = context.GetService<IMigrationsModelDiffer>();
3437+
var modelRuntimeInitializer = context.GetService<IModelRuntimeInitializer>();
3438+
3439+
var models = new List<IModel>();
3440+
var preSnapshotSourceModel = default(IModel);
3441+
for (var i = 0; i < buildActions.Count; i++)
3442+
{
3443+
var modelBuilder = CreateModelBuilder(withConventions);
3444+
buildActions[i](modelBuilder);
3445+
3446+
var preSnapshotModel = modelRuntimeInitializer.Initialize(
3447+
(IModel)modelBuilder.Model, designTime: true, validationLogger: null);
3448+
3449+
if (i == 0)
3450+
{
3451+
// Round-trip the source model through a snapshot, compiling it and then extracting it back again.
3452+
// This simulates the real-world migration flow and can expose errors in snapshot generation
3453+
// we only do this for the starting model, the subsequent models are just for generating funky migration operations
3454+
// in the scenario that we want to simulate, those additional models would not have been backed up by the model snapshot
3455+
// as they either were manually added migration ops, or result of squashing
3456+
var migrationsCodeGenerator = Fixture.TestHelpers.CreateDesignServiceProvider().GetRequiredService<IMigrationsCodeGenerator>();
3457+
var sourceModelSnapshot = migrationsCodeGenerator.GenerateSnapshot(
3458+
modelSnapshotNamespace: null, typeof(DbContext), "MigrationsTestSnapshot", preSnapshotModel);
3459+
var sourceModel = BuildModelFromSnapshotSource(sourceModelSnapshot);
3460+
models.Add(sourceModel);
3461+
preSnapshotSourceModel = preSnapshotModel;
3462+
}
3463+
else
3464+
{
3465+
models.Add(preSnapshotModel);
3466+
}
3467+
}
3468+
3469+
// build all migration operations going through each intermediate state of the model
3470+
var operations = new List<MigrationOperation>();
3471+
for (var i = 0; i < models.Count - 1; i++)
3472+
{
3473+
operations.AddRange(
3474+
modelDiffer.GetDifferences(models[i].GetRelationalModel(), models[i + 1].GetRelationalModel()));
3475+
}
3476+
3477+
return Test(preSnapshotSourceModel, models.Last(), operations, asserter, migrationsSqlGenerationOptions);
3478+
}
3479+
33083480
protected virtual Task Test(
33093481
Action<ModelBuilder> buildCommonAction,
33103482
Action<ModelBuilder> buildSourceAction,

Diff for: ‎test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs

+84
Original file line numberDiff line numberDiff line change
@@ -3440,6 +3440,90 @@ await Test(
34403440
""");
34413441
}
34423442

3443+
public override async Task Multiop_drop_table_and_create_the_same_table_in_one_migration()
3444+
{
3445+
await base.Multiop_drop_table_and_create_the_same_table_in_one_migration();
3446+
3447+
AssertSql(
3448+
"""
3449+
DROP TABLE [Customers];
3450+
""",
3451+
//
3452+
"""
3453+
CREATE TABLE [Customers] (
3454+
[Id] int NOT NULL IDENTITY,
3455+
[Name] nvarchar(max) NULL,
3456+
CONSTRAINT [PK_Customers] PRIMARY KEY ([Id])
3457+
);
3458+
""");
3459+
}
3460+
3461+
public override async Task Multiop_create_table_and_drop_it_in_one_migration()
3462+
{
3463+
await base.Multiop_create_table_and_drop_it_in_one_migration();
3464+
3465+
AssertSql(
3466+
"""
3467+
CREATE TABLE [Customers] (
3468+
[Id] int NOT NULL IDENTITY,
3469+
[Name] nvarchar(max) NULL,
3470+
CONSTRAINT [PK_Customers] PRIMARY KEY ([Id])
3471+
);
3472+
""",
3473+
//
3474+
"""
3475+
DROP TABLE [Customers];
3476+
""");
3477+
}
3478+
3479+
public override async Task Multiop_rename_table_and_drop()
3480+
{
3481+
await base.Multiop_rename_table_and_drop();
3482+
3483+
AssertSql(
3484+
"""
3485+
ALTER TABLE [Customers] DROP CONSTRAINT [PK_Customers];
3486+
""",
3487+
//
3488+
"""
3489+
EXEC sp_rename N'[Customers]', N'NewCustomers', 'OBJECT';
3490+
""",
3491+
//
3492+
"""
3493+
ALTER TABLE [NewCustomers] ADD CONSTRAINT [PK_NewCustomers] PRIMARY KEY ([Id]);
3494+
""",
3495+
//
3496+
"""
3497+
DROP TABLE [NewCustomers];
3498+
""");
3499+
}
3500+
3501+
public override async Task Multiop_rename_table_and_create_new_table_with_the_old_name()
3502+
{
3503+
await base.Multiop_rename_table_and_create_new_table_with_the_old_name();
3504+
3505+
AssertSql(
3506+
"""
3507+
ALTER TABLE [Customers] DROP CONSTRAINT [PK_Customers];
3508+
""",
3509+
//
3510+
"""
3511+
EXEC sp_rename N'[Customers]', N'NewCustomers', 'OBJECT';
3512+
""",
3513+
//
3514+
"""
3515+
ALTER TABLE [NewCustomers] ADD CONSTRAINT [PK_NewCustomers] PRIMARY KEY ([Id]);
3516+
""",
3517+
//
3518+
"""
3519+
CREATE TABLE [Customers] (
3520+
[Id] int NOT NULL IDENTITY,
3521+
[Name] nvarchar(max) NULL,
3522+
CONSTRAINT [PK_Customers] PRIMARY KEY ([Id])
3523+
);
3524+
""");
3525+
}
3526+
34433527
[ConditionalFact]
34443528
public virtual async Task Create_temporal_table_default_column_mappings_and_default_history_table()
34453529
{

Diff for: ‎test/EFCore.Sqlite.FunctionalTests/Migrations/MigrationsSqliteTest.cs

+5
Original file line numberDiff line numberDiff line change
@@ -2070,6 +2070,11 @@ public override Task Rename_sequence()
20702070
public override Task Move_sequence()
20712071
=> AssertNotSupportedAsync(base.Move_sequence, SqliteStrings.SequencesNotSupported);
20722072

2073+
public override Task Multiop_rename_table_and_drop()
2074+
=> AssertNotSupportedAsync(
2075+
base.Multiop_rename_table_and_drop,
2076+
SqliteStrings.InvalidMigrationOperation(nameof(DropPrimaryKeyOperation)));
2077+
20732078
[ConditionalFact]
20742079
public override async Task Add_required_primitve_collection_to_existing_table()
20752080
{

0 commit comments

Comments
 (0)