Skip to content

Commit 85957ac

Browse files
maumarmaumar
authored
Fix to #35162 - Regression from EF Core 8 to 9: MigrationBuilder.DropTable Causes Issues with Subsequent Table Recreation (#35776)
Port of #35764 Fixes #35162 Description 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. Customer impact Some manually modified/squashed migrations no longer work when they are being applied, throwing an unhelpful exception (The given key '...' was not present in the dictionary.'). No good workaround apart from breaking the migrations apart. How found Reported by multiple customers on 9. Regression Yes. Testing Added infra to test these types of scenarios (containing intermediate migration state), added multiple tests both for regular and temporal tables. Risk Low - Very targeted fix, accessing the temporal information dictionary in a more robust way. No quirk, since migrations are design time, so hard to set switches for them.
1 parent f718f7a commit 85957ac

File tree

4 files changed

+615
-5
lines changed

4 files changed

+615
-5
lines changed

‎src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs

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

2552+
var temporalTableInformation = BuildTemporalInformationFromMigrationOperation(schema, renameTableOperation);
25522553
if (!temporalTableInformationMap.ContainsKey((tableName, rawSchema)))
25532554
{
2554-
var temporalTableInformation = BuildTemporalInformationFromMigrationOperation(schema, renameTableOperation);
25552555
temporalTableInformationMap[(tableName, rawSchema)] = temporalTableInformation;
2556+
}
2557+
2558+
// we still need to check here - table with the new name could have existed before and have been deleted
2559+
// we want to preserve the original temporal info of that deleted table
2560+
if (!temporalTableInformationMap.ContainsKey((newTableName, newRawSchema)))
2561+
{
25562562
temporalTableInformationMap[(newTableName, newRawSchema)] = temporalTableInformation;
25572563
}
25582564

@@ -2647,10 +2653,19 @@ private IReadOnlyList<MigrationOperation> RewriteOperations(
26472653

26482654
var schema = rawSchema ?? model?.GetDefaultSchema();
26492655

2650-
// we are guaranteed to find entry here - we looped through all the operations earlier,
2651-
// info missing from operations we got from the model
2652-
// and in case of no/incomplete model we created dummy (non-temporal) entries
2653-
var temporalInformation = temporalTableInformationMap[(tableName, rawSchema)];
2656+
TemporalOperationInformation temporalInformation;
2657+
if (operation is CreateTableOperation)
2658+
{
2659+
// for create table we always generate new temporal information from the operation itself
2660+
// just in case there was a table with that name before that got deleted/renamed
2661+
// also, temporal state (disabled versioning etc.) should always reset when creating a table
2662+
temporalInformation = BuildTemporalInformationFromMigrationOperation(schema, operation);
2663+
temporalTableInformationMap[(tableName, rawSchema)] = temporalInformation;
2664+
}
2665+
else
2666+
{
2667+
temporalInformation = temporalTableInformationMap[(tableName, rawSchema)];
2668+
}
26542669

26552670
switch (operation)
26562671
{

‎test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs

+149
Original file line numberDiff line numberDiff line change
@@ -3213,6 +3213,118 @@ public virtual Task Add_required_primitve_collection_with_custom_converter_and_c
32133213
Assert.Single(customersTable.PrimaryKey!.Columns));
32143214
});
32153215

3216+
[ConditionalFact]
3217+
public virtual Task Multiop_drop_table_and_create_the_same_table_in_one_migration()
3218+
=> TestComposite(
3219+
[
3220+
builder => builder.Entity(
3221+
"Customer", e =>
3222+
{
3223+
e.Property<int>("Id").ValueGeneratedOnAdd();
3224+
e.Property<string>("Name");
3225+
e.HasKey("Id");
3226+
e.ToTable("Customers");
3227+
}),
3228+
builder => { },
3229+
builder => builder.Entity(
3230+
"Customer", e =>
3231+
{
3232+
e.Property<int>("Id").ValueGeneratedOnAdd();
3233+
e.Property<string>("Name");
3234+
e.HasKey("Id");
3235+
3236+
e.ToTable("Customers");
3237+
})
3238+
]);
3239+
3240+
[ConditionalFact]
3241+
public virtual Task Multiop_create_table_and_drop_it_in_one_migration()
3242+
=> TestComposite(
3243+
[
3244+
builder => { },
3245+
builder => builder.Entity(
3246+
"Customer", e =>
3247+
{
3248+
e.Property<int>("Id").ValueGeneratedOnAdd();
3249+
e.Property<string>("Name");
3250+
e.HasKey("Id");
3251+
3252+
e.ToTable("Customers");
3253+
}),
3254+
builder => { },
3255+
]);
3256+
3257+
[ConditionalFact]
3258+
public virtual Task Multiop_rename_table_and_drop()
3259+
=> TestComposite(
3260+
[
3261+
builder => builder.Entity(
3262+
"Customer", e =>
3263+
{
3264+
e.Property<int>("Id").ValueGeneratedOnAdd();
3265+
e.Property<string>("Name");
3266+
e.HasKey("Id");
3267+
3268+
e.ToTable("Customers");
3269+
}),
3270+
builder => builder.Entity(
3271+
"Customer", e =>
3272+
{
3273+
e.Property<int>("Id").ValueGeneratedOnAdd();
3274+
e.Property<string>("Name");
3275+
e.HasKey("Id");
3276+
3277+
e.ToTable("NewCustomers");
3278+
}),
3279+
builder => { },
3280+
]);
3281+
3282+
[ConditionalFact]
3283+
public virtual Task Multiop_rename_table_and_create_new_table_with_the_old_name()
3284+
=> TestComposite(
3285+
[
3286+
builder => builder.Entity(
3287+
"Customer", e =>
3288+
{
3289+
e.Property<int>("Id").ValueGeneratedOnAdd();
3290+
e.Property<string>("Name");
3291+
e.HasKey("Id");
3292+
3293+
e.ToTable("Customers");
3294+
}),
3295+
builder => builder.Entity(
3296+
"Customer", e =>
3297+
{
3298+
e.Property<int>("Id").ValueGeneratedOnAdd();
3299+
e.Property<string>("Name");
3300+
e.HasKey("Id");
3301+
3302+
e.ToTable("NewCustomers");
3303+
}),
3304+
builder =>
3305+
{
3306+
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("NewCustomers");
3314+
});
3315+
3316+
builder.Entity(
3317+
"AnotherCustomer", e =>
3318+
{
3319+
e.Property<int>("Id").ValueGeneratedOnAdd();
3320+
e.Property<string>("Name");
3321+
e.HasKey("Id");
3322+
3323+
e.ToTable("Customers");
3324+
});
3325+
},
3326+
]);
3327+
32163328
protected class Person
32173329
{
32183330
public int Id { get; set; }
@@ -3262,6 +3374,43 @@ protected virtual Task Test(
32623374
MigrationsSqlGenerationOptions migrationsSqlGenerationOptions = MigrationsSqlGenerationOptions.Default)
32633375
=> Test(_ => { }, buildSourceAction, buildTargetAction, asserter, withConventions, migrationsSqlGenerationOptions);
32643376

3377+
protected virtual Task TestComposite(
3378+
List<Action<ModelBuilder>> buildActions,
3379+
bool withConventions = true,
3380+
MigrationsSqlGenerationOptions migrationsSqlGenerationOptions = MigrationsSqlGenerationOptions.Default)
3381+
{
3382+
if (buildActions.Count < 3)
3383+
{
3384+
throw new InvalidOperationException("You need at least 3 build actions for the composite case.");
3385+
}
3386+
3387+
var context = CreateContext();
3388+
var modelDiffer = context.GetService<IMigrationsModelDiffer>();
3389+
var modelRuntimeInitializer = context.GetService<IModelRuntimeInitializer>();
3390+
3391+
var models = new List<IModel>();
3392+
for (var i = 0; i < buildActions.Count; i++)
3393+
{
3394+
var modelBuilder = CreateModelBuilder(withConventions);
3395+
buildActions[i](modelBuilder);
3396+
3397+
var preSnapshotModel = modelRuntimeInitializer.Initialize(
3398+
(IModel)modelBuilder.Model, designTime: true, validationLogger: null);
3399+
3400+
models.Add(preSnapshotModel);
3401+
}
3402+
3403+
// build all migration operations going through each intermediate state of the model
3404+
var operations = new List<MigrationOperation>();
3405+
for (var i = 0; i < models.Count - 1; i++)
3406+
{
3407+
operations.AddRange(
3408+
modelDiffer.GetDifferences(models[i].GetRelationalModel(), models[i + 1].GetRelationalModel()));
3409+
}
3410+
3411+
return Test(models.First(), models.Last(), operations, null, migrationsSqlGenerationOptions);
3412+
}
3413+
32653414
protected virtual Task Test(
32663415
Action<ModelBuilder> buildCommonAction,
32673416
Action<ModelBuilder> buildSourceAction,

0 commit comments

Comments
 (0)