From 078301e3c644b883ade2162648aa8efb2df9f02c Mon Sep 17 00:00:00 2001 From: maumar Date: Tue, 11 Mar 2025 23:01:48 -0700 Subject: [PATCH] Fix to #35162 - Regression from EF Core 8 to 9: MigrationBuilder.DropTable Causes Issues with Subsequent Table Recreation Port of https://github.com/dotnet/efcore/pull/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. --- .../SqlServerMigrationsSqlGenerator.cs | 25 +- .../Migrations/MigrationsTestBase.cs | 149 ++++++ .../Migrations/MigrationsSqlServerTest.cs | 441 ++++++++++++++++++ .../Migrations/MigrationsSqliteTest.cs | 5 + 4 files changed, 615 insertions(+), 5 deletions(-) diff --git a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs index 40573c91264..613f391200a 100644 --- a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs +++ b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs @@ -2549,10 +2549,16 @@ private IReadOnlyList RewriteOperations( var newRawSchema = renameTableOperation.NewSchema; var newSchema = newRawSchema ?? model?.GetDefaultSchema(); + var temporalTableInformation = BuildTemporalInformationFromMigrationOperation(schema, renameTableOperation); if (!temporalTableInformationMap.ContainsKey((tableName, rawSchema))) { - var temporalTableInformation = BuildTemporalInformationFromMigrationOperation(schema, renameTableOperation); temporalTableInformationMap[(tableName, rawSchema)] = temporalTableInformation; + } + + // we still need to check here - table with the new name could have existed before and have been deleted + // we want to preserve the original temporal info of that deleted table + if (!temporalTableInformationMap.ContainsKey((newTableName, newRawSchema))) + { temporalTableInformationMap[(newTableName, newRawSchema)] = temporalTableInformation; } @@ -2647,10 +2653,19 @@ private IReadOnlyList RewriteOperations( var schema = rawSchema ?? model?.GetDefaultSchema(); - // we are guaranteed to find entry here - we looped through all the operations earlier, - // info missing from operations we got from the model - // and in case of no/incomplete model we created dummy (non-temporal) entries - var temporalInformation = temporalTableInformationMap[(tableName, rawSchema)]; + TemporalOperationInformation temporalInformation; + if (operation is CreateTableOperation) + { + // for create table we always generate new temporal information from the operation itself + // just in case there was a table with that name before that got deleted/renamed + // also, temporal state (disabled versioning etc.) should always reset when creating a table + temporalInformation = BuildTemporalInformationFromMigrationOperation(schema, operation); + temporalTableInformationMap[(tableName, rawSchema)] = temporalInformation; + } + else + { + temporalInformation = temporalTableInformationMap[(tableName, rawSchema)]; + } switch (operation) { diff --git a/test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs b/test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs index f83e2575b6b..d91dafb6d63 100644 --- a/test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs @@ -3213,6 +3213,118 @@ public virtual Task Add_required_primitve_collection_with_custom_converter_and_c Assert.Single(customersTable.PrimaryKey!.Columns)); }); + [ConditionalFact] + public virtual Task Multiop_drop_table_and_create_the_same_table_in_one_migration() + => TestComposite( + [ + builder => builder.Entity( + "Customer", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.Property("Name"); + e.HasKey("Id"); + e.ToTable("Customers"); + }), + builder => { }, + builder => builder.Entity( + "Customer", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.Property("Name"); + e.HasKey("Id"); + + e.ToTable("Customers"); + }) + ]); + + [ConditionalFact] + public virtual Task Multiop_create_table_and_drop_it_in_one_migration() + => TestComposite( + [ + builder => { }, + builder => builder.Entity( + "Customer", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.Property("Name"); + e.HasKey("Id"); + + e.ToTable("Customers"); + }), + builder => { }, + ]); + + [ConditionalFact] + public virtual Task Multiop_rename_table_and_drop() + => TestComposite( + [ + builder => builder.Entity( + "Customer", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.Property("Name"); + e.HasKey("Id"); + + e.ToTable("Customers"); + }), + builder => builder.Entity( + "Customer", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.Property("Name"); + e.HasKey("Id"); + + e.ToTable("NewCustomers"); + }), + builder => { }, + ]); + + [ConditionalFact] + public virtual Task Multiop_rename_table_and_create_new_table_with_the_old_name() + => TestComposite( + [ + builder => builder.Entity( + "Customer", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.Property("Name"); + e.HasKey("Id"); + + e.ToTable("Customers"); + }), + builder => builder.Entity( + "Customer", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.Property("Name"); + e.HasKey("Id"); + + e.ToTable("NewCustomers"); + }), + builder => + { + builder.Entity( + "Customer", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.Property("Name"); + e.HasKey("Id"); + + e.ToTable("NewCustomers"); + }); + + builder.Entity( + "AnotherCustomer", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.Property("Name"); + e.HasKey("Id"); + + e.ToTable("Customers"); + }); + }, + ]); + protected class Person { public int Id { get; set; } @@ -3262,6 +3374,43 @@ protected virtual Task Test( MigrationsSqlGenerationOptions migrationsSqlGenerationOptions = MigrationsSqlGenerationOptions.Default) => Test(_ => { }, buildSourceAction, buildTargetAction, asserter, withConventions, migrationsSqlGenerationOptions); + protected virtual Task TestComposite( + List> buildActions, + bool withConventions = true, + MigrationsSqlGenerationOptions migrationsSqlGenerationOptions = MigrationsSqlGenerationOptions.Default) + { + if (buildActions.Count < 3) + { + throw new InvalidOperationException("You need at least 3 build actions for the composite case."); + } + + var context = CreateContext(); + var modelDiffer = context.GetService(); + var modelRuntimeInitializer = context.GetService(); + + var models = new List(); + for (var i = 0; i < buildActions.Count; i++) + { + var modelBuilder = CreateModelBuilder(withConventions); + buildActions[i](modelBuilder); + + var preSnapshotModel = modelRuntimeInitializer.Initialize( + (IModel)modelBuilder.Model, designTime: true, validationLogger: null); + + models.Add(preSnapshotModel); + } + + // build all migration operations going through each intermediate state of the model + var operations = new List(); + for (var i = 0; i < models.Count - 1; i++) + { + operations.AddRange( + modelDiffer.GetDifferences(models[i].GetRelationalModel(), models[i + 1].GetRelationalModel())); + } + + return Test(models.First(), models.Last(), operations, null, migrationsSqlGenerationOptions); + } + protected virtual Task Test( Action buildCommonAction, Action buildSourceAction, diff --git a/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs index b7998252e12..b1931dad046 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs @@ -3440,6 +3440,90 @@ await Test( """); } + public override async Task Multiop_drop_table_and_create_the_same_table_in_one_migration() + { + await base.Multiop_drop_table_and_create_the_same_table_in_one_migration(); + + AssertSql( + """ +DROP TABLE [Customers]; +""", + // + """ +CREATE TABLE [Customers] ( + [Id] int NOT NULL IDENTITY, + [Name] nvarchar(max) NULL, + CONSTRAINT [PK_Customers] PRIMARY KEY ([Id]) +); +"""); + } + + public override async Task Multiop_create_table_and_drop_it_in_one_migration() + { + await base.Multiop_create_table_and_drop_it_in_one_migration(); + + AssertSql( + """ +CREATE TABLE [Customers] ( + [Id] int NOT NULL IDENTITY, + [Name] nvarchar(max) NULL, + CONSTRAINT [PK_Customers] PRIMARY KEY ([Id]) +); +""", + // + """ +DROP TABLE [Customers]; +"""); + } + + public override async Task Multiop_rename_table_and_drop() + { + await base.Multiop_rename_table_and_drop(); + + AssertSql( + """ +ALTER TABLE [Customers] DROP CONSTRAINT [PK_Customers]; +""", + // + """ +EXEC sp_rename N'[Customers]', N'NewCustomers', 'OBJECT'; +""", + // + """ +ALTER TABLE [NewCustomers] ADD CONSTRAINT [PK_NewCustomers] PRIMARY KEY ([Id]); +""", + // + """ +DROP TABLE [NewCustomers]; +"""); + } + + public override async Task Multiop_rename_table_and_create_new_table_with_the_old_name() + { + await base.Multiop_rename_table_and_create_new_table_with_the_old_name(); + + AssertSql( + """ +ALTER TABLE [Customers] DROP CONSTRAINT [PK_Customers]; +""", + // + """ +EXEC sp_rename N'[Customers]', N'NewCustomers', 'OBJECT'; +""", + // + """ +ALTER TABLE [NewCustomers] ADD CONSTRAINT [PK_NewCustomers] PRIMARY KEY ([Id]); +""", + // + """ +CREATE TABLE [Customers] ( + [Id] int NOT NULL IDENTITY, + [Name] nvarchar(max) NULL, + CONSTRAINT [PK_Customers] PRIMARY KEY ([Id]) +); +"""); + } + [ConditionalFact] public virtual async Task Create_temporal_table_default_column_mappings_and_default_history_table() { @@ -10757,6 +10841,363 @@ CONSTRAINT [PK_HistoryTable] PRIMARY KEY ([Id]) """); } + [ConditionalFact] + public virtual async Task Temporal_multiop_drop_temporal_table_and_add_the_same_table_in_one_migration() + { + await TestComposite( + [ + builder => builder.Entity( + "Customer", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.Property("Name"); + e.Property("SystemTimeStart").ValueGeneratedOnAddOrUpdate(); + e.Property("SystemTimeEnd").ValueGeneratedOnAddOrUpdate(); + e.HasKey("Id"); + + e.ToTable( + "Customers", tb => tb.IsTemporal( + ttb => + { + ttb.UseHistoryTable("HistoryTable", "historySchema"); + ttb.HasPeriodStart("SystemTimeStart"); + ttb.HasPeriodEnd("SystemTimeEnd"); + })); + }), + builder => { }, + builder => builder.Entity( + "Customer", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.Property("Name"); + e.Property("SystemTimeStart").ValueGeneratedOnAddOrUpdate(); + e.Property("SystemTimeEnd").ValueGeneratedOnAddOrUpdate(); + e.HasKey("Id"); + + e.ToTable( + "Customers", tb => tb.IsTemporal( + ttb => + { + ttb.UseHistoryTable("HistoryTable", "historySchema"); + ttb.HasPeriodStart("SystemTimeStart"); + ttb.HasPeriodEnd("SystemTimeEnd"); + })); + }) + ]); + + AssertSql( +""" +ALTER TABLE [Customers] SET (SYSTEM_VERSIONING = OFF) +""", + // + """ +DROP TABLE [Customers]; +""", + // + """ +DROP TABLE [historySchema].[HistoryTable]; +""", + // + """ +IF SCHEMA_ID(N'historySchema') IS NULL EXEC(N'CREATE SCHEMA [historySchema];'); +""", + // + """ +CREATE TABLE [Customers] ( + [Id] int NOT NULL IDENTITY, + [Name] nvarchar(max) NULL, + [SystemTimeEnd] datetime2 GENERATED ALWAYS AS ROW END HIDDEN NOT NULL, + [SystemTimeStart] datetime2 GENERATED ALWAYS AS ROW START HIDDEN NOT NULL, + CONSTRAINT [PK_Customers] PRIMARY KEY ([Id]), + PERIOD FOR SYSTEM_TIME([SystemTimeStart], [SystemTimeEnd]) +) WITH (SYSTEM_VERSIONING = ON (HISTORY_TABLE = [historySchema].[HistoryTable])); +"""); + } + + [ConditionalFact] + public virtual async Task Temporal_multiop_create_temporal_and_drop() + { + await TestComposite( + [ + builder => { }, + builder => builder.Entity( + "Customer", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.Property("Name"); + e.Property("SystemTimeStart").ValueGeneratedOnAddOrUpdate(); + e.Property("SystemTimeEnd").ValueGeneratedOnAddOrUpdate(); + e.HasKey("Id"); + + e.ToTable( + "Customers", tb => tb.IsTemporal( + ttb => + { + ttb.UseHistoryTable("HistoryTable", "historySchema"); + ttb.HasPeriodStart("SystemTimeStart"); + ttb.HasPeriodEnd("SystemTimeEnd"); + })); + }), + builder => { }, + ]); + + AssertSql( +""" +IF SCHEMA_ID(N'historySchema') IS NULL EXEC(N'CREATE SCHEMA [historySchema];'); +""", + // + """ +CREATE TABLE [Customers] ( + [Id] int NOT NULL IDENTITY, + [Name] nvarchar(max) NULL, + [SystemTimeEnd] datetime2 GENERATED ALWAYS AS ROW END HIDDEN NOT NULL, + [SystemTimeStart] datetime2 GENERATED ALWAYS AS ROW START HIDDEN NOT NULL, + CONSTRAINT [PK_Customers] PRIMARY KEY ([Id]), + PERIOD FOR SYSTEM_TIME([SystemTimeStart], [SystemTimeEnd]) +) WITH (SYSTEM_VERSIONING = ON (HISTORY_TABLE = [historySchema].[HistoryTable])); +""", + // + """ +ALTER TABLE [Customers] SET (SYSTEM_VERSIONING = OFF) +""", + // + """ +DROP TABLE [Customers]; +""", + // + """ +DROP TABLE [historySchema].[HistoryTable]; +"""); + } + + [ConditionalFact] + public virtual async Task Temporal_multiop_rename_temporal_and_drop() + { + await TestComposite( + [ + builder => builder.Entity( + "Customer", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.Property("Name"); + e.Property("SystemTimeStart").ValueGeneratedOnAddOrUpdate(); + e.Property("SystemTimeEnd").ValueGeneratedOnAddOrUpdate(); + e.HasKey("Id"); + + e.ToTable( + "Customers", tb => tb.IsTemporal( + ttb => + { + ttb.UseHistoryTable("HistoryTable", "historySchema"); + ttb.HasPeriodStart("SystemTimeStart"); + ttb.HasPeriodEnd("SystemTimeEnd"); + })); + }), + builder => builder.Entity( + "Customer", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.Property("Name"); + e.Property("SystemTimeStart").ValueGeneratedOnAddOrUpdate(); + e.Property("SystemTimeEnd").ValueGeneratedOnAddOrUpdate(); + e.HasKey("Id"); + + e.ToTable( + "NewCustomers", tb => tb.IsTemporal( + ttb => + { + ttb.UseHistoryTable("HistoryTable", "historySchema"); + ttb.HasPeriodStart("SystemTimeStart"); + ttb.HasPeriodEnd("SystemTimeEnd"); + })); + }), + builder => { }, + ]); + + AssertSql( +""" +ALTER TABLE [Customers] SET (SYSTEM_VERSIONING = OFF) +""", + // + """ +ALTER TABLE [Customers] DROP CONSTRAINT [PK_Customers]; +""", + // + """ +EXEC sp_rename N'[Customers]', N'NewCustomers', 'OBJECT'; +""", + // + """ +ALTER TABLE [NewCustomers] ADD CONSTRAINT [PK_NewCustomers] PRIMARY KEY ([Id]); +""", + // + """ +DROP TABLE [NewCustomers]; +""", + // + """ +DROP TABLE [historySchema].[HistoryTable]; +"""); + } + + [ConditionalFact] + public virtual async Task Temporal_multiop_rename_period_drop_table_create_as_regular() + { + await TestComposite( + [ + builder => builder.Entity( + "Customer", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.Property("Name"); + e.Property("SystemTimeStart").ValueGeneratedOnAddOrUpdate(); + e.Property("SystemTimeEnd").ValueGeneratedOnAddOrUpdate(); + e.HasKey("Id"); + + e.ToTable( + "Customers", tb => tb.IsTemporal( + ttb => + { + ttb.UseHistoryTable("HistoryTable", "historySchema"); + ttb.HasPeriodStart("SystemTimeStart"); + ttb.HasPeriodEnd("SystemTimeEnd"); + })); + }), + builder => builder.Entity( + "Customer", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.Property("Name"); + e.Property("NewSystemTimeStart").ValueGeneratedOnAddOrUpdate(); + e.Property("SystemTimeEnd").ValueGeneratedOnAddOrUpdate(); + e.HasKey("Id"); + + e.ToTable( + "Customers", tb => tb.IsTemporal( + ttb => + { + ttb.UseHistoryTable("HistoryTable", "historySchema"); + ttb.HasPeriodStart("NewSystemTimeStart"); + ttb.HasPeriodEnd("SystemTimeEnd"); + })); + }), + builder => { }, + builder => builder.Entity( + "Customer", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.Property("Name"); + e.HasKey("Id"); + + e.ToTable("Customers"); + }), + ]); + + AssertSql( +""" +EXEC sp_rename N'[Customers].[SystemTimeStart]', N'NewSystemTimeStart', 'COLUMN'; +""", + // + """ +ALTER TABLE [Customers] SET (SYSTEM_VERSIONING = OFF) +""", + // + """ +DROP TABLE [Customers]; +""", + // + """ +DROP TABLE [historySchema].[HistoryTable]; +""", + // + """ +CREATE TABLE [Customers] ( + [Id] int NOT NULL IDENTITY, + [Name] nvarchar(max) NULL, + CONSTRAINT [PK_Customers] PRIMARY KEY ([Id]) +); +"""); + } + + [ConditionalFact] + public virtual async Task Temporal_multiop_rename_column_drop_table_create_as_regular() + { + await TestComposite( + [ + builder => builder.Entity( + "Customer", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.Property("Name"); + e.Property("SystemTimeStart").ValueGeneratedOnAddOrUpdate(); + e.Property("SystemTimeEnd").ValueGeneratedOnAddOrUpdate(); + e.HasKey("Id"); + + e.ToTable( + "Customers", tb => tb.IsTemporal( + ttb => + { + ttb.UseHistoryTable("HistoryTable", "historySchema"); + ttb.HasPeriodStart("SystemTimeStart"); + ttb.HasPeriodEnd("SystemTimeEnd"); + })); + }), + builder => builder.Entity( + "Customer", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.Property("NewName"); + e.Property("SystemTimeStart").ValueGeneratedOnAddOrUpdate(); + e.Property("SystemTimeEnd").ValueGeneratedOnAddOrUpdate(); + e.HasKey("Id"); + + e.ToTable( + "Customers", tb => tb.IsTemporal( + ttb => + { + ttb.UseHistoryTable("HistoryTable", "historySchema"); + ttb.HasPeriodStart("SystemTimeStart"); + ttb.HasPeriodEnd("SystemTimeEnd"); + })); + }), + builder => { }, + builder => builder.Entity( + "Customer", e => + { + e.Property("Id").ValueGeneratedOnAdd(); + e.Property("Name"); + e.HasKey("Id"); + + e.ToTable("Customers"); + }), + ]); + + AssertSql( +""" +EXEC sp_rename N'[Customers].[Name]', N'NewName', 'COLUMN'; +""", + // + """ +ALTER TABLE [Customers] SET (SYSTEM_VERSIONING = OFF) +""", + // + """ +DROP TABLE [Customers]; +""", + // + """ +DROP TABLE [historySchema].[HistoryTable]; +""", + // + """ +CREATE TABLE [Customers] ( + [Id] int NOT NULL IDENTITY, + [Name] nvarchar(max) NULL, + CONSTRAINT [PK_Customers] PRIMARY KEY ([Id]) +); +"""); + } + [ConditionalFact] public override async Task Add_required_primitive_collection_to_existing_table() { diff --git a/test/EFCore.Sqlite.FunctionalTests/Migrations/MigrationsSqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Migrations/MigrationsSqliteTest.cs index 79529ed9b1d..e3a82cc168d 100644 --- a/test/EFCore.Sqlite.FunctionalTests/Migrations/MigrationsSqliteTest.cs +++ b/test/EFCore.Sqlite.FunctionalTests/Migrations/MigrationsSqliteTest.cs @@ -2070,6 +2070,11 @@ public override Task Rename_sequence() public override Task Move_sequence() => AssertNotSupportedAsync(base.Move_sequence, SqliteStrings.SequencesNotSupported); + public override Task Multiop_rename_table_and_drop() + => AssertNotSupportedAsync( + base.Multiop_rename_table_and_drop, + SqliteStrings.InvalidMigrationOperation(nameof(DropPrimaryKeyOperation))); + [ConditionalFact] public override async Task Add_required_primitve_collection_to_existing_table() {