From ab5926380bf15f90db624b739f100bbe1f6bca6b Mon Sep 17 00:00:00 2001 From: Brice Lambson Date: Thu, 16 Jul 2020 14:02:45 -0700 Subject: [PATCH] SQLite Migrations: Test table rebuilds --- .../SqliteMigrationsSqlGenerator.cs | 172 +++---- .../SqliteMigrationsSqlGeneratorTest.cs | 427 ++++++++++++++++++ 2 files changed, 489 insertions(+), 110 deletions(-) diff --git a/src/EFCore.Sqlite.Core/Migrations/SqliteMigrationsSqlGenerator.cs b/src/EFCore.Sqlite.Core/Migrations/SqliteMigrationsSqlGenerator.cs index 85452850245..373b817207d 100644 --- a/src/EFCore.Sqlite.Core/Migrations/SqliteMigrationsSqlGenerator.cs +++ b/src/EFCore.Sqlite.Core/Migrations/SqliteMigrationsSqlGenerator.cs @@ -94,7 +94,6 @@ private IReadOnlyList RewriteOperations( case DropColumnOperation dropColumnOperation: { var rebuild = rebuilds.GetOrAddNew((dropColumnOperation.Table, dropColumnOperation.Schema)); - rebuild.OldColumns.Remove(dropColumnOperation.Name); rebuild.OperationsToReplace.Add(dropColumnOperation); rebuild.DropColumnsDeferred.Add(dropColumnOperation.Name); @@ -128,13 +127,7 @@ private IReadOnlyList RewriteOperations( { var rebuild = rebuilds.GetOrAddNew((alterColumnOperation.Table, alterColumnOperation.Schema)); rebuild.OperationsToReplace.Add(alterColumnOperation); - - if (alterColumnOperation.OldColumn.IsNullable && !alterColumnOperation.IsNullable) - { - var overrides = rebuild.NewlyRequiredColumns.GetOrAddNew(alterColumnOperation.Name); - overrides.DefaultValueSql = alterColumnOperation.DefaultValueSql; - overrides.DefaultValue = alterColumnOperation.DefaultValue; - } + rebuild.AlterColumnsDeferred.Add(alterColumnOperation.Name, alterColumnOperation); operations.Add(alterColumnOperation); @@ -144,9 +137,11 @@ private IReadOnlyList RewriteOperations( case CreateIndexOperation createIndexOperation: { if (rebuilds.TryGetValue((createIndexOperation.Table, createIndexOperation.Schema), out var rebuild) - && rebuild.AddColumnsDeferred.Intersect(createIndexOperation.Columns).Any()) + && (rebuild.AddColumnsDeferred.Keys.Intersect(createIndexOperation.Columns).Any() + || rebuild.RenameColumnsDeferred.Keys.Intersect(createIndexOperation.Columns).Any())) { rebuild.OperationsToReplace.Add(createIndexOperation); + rebuild.CreateIndexesDeferred.Add(createIndexOperation.Name); } operations.Add(createIndexOperation); @@ -168,14 +163,7 @@ private IReadOnlyList RewriteOperations( Name = renameIndexOperation.Name }); - var createOperation = CreateIndexOperation.For(index); - operations.Add(createOperation); - - if (rebuilds.TryGetValue((renameIndexOperation.Table, renameIndexOperation.Schema), out var rebuild) - && rebuild.AddColumnsDeferred.Intersect(index.Columns.Select(c => c.Name)).Any()) - { - rebuild.OperationsToReplace.Add(createOperation); - } + operations.Add(CreateIndexOperation.For(index)); } else { @@ -190,9 +178,8 @@ private IReadOnlyList RewriteOperations( if (rebuilds.TryGetValue((addColumnOperation.Table, addColumnOperation.Schema), out var rebuild) && rebuild.DropColumnsDeferred.Contains(addColumnOperation.Name)) { - rebuild.OldColumns.Add(addColumnOperation.Name, null); rebuild.OperationsToReplace.Add(addColumnOperation); - rebuild.AddColumnsDeferred.Add(addColumnOperation.Name); + rebuild.AddColumnsDeferred.Add(addColumnOperation.Name, addColumnOperation); } else if (addColumnOperation.Comment != null) { @@ -208,26 +195,11 @@ private IReadOnlyList RewriteOperations( { if (rebuilds.TryGetValue((renameColumnOperation.Table, renameColumnOperation.Schema), out var rebuild)) { - if (rebuild.AddColumnsDeferred.Contains(renameColumnOperation.Name)) + if (rebuild.DropColumnsDeferred.Contains(renameColumnOperation.NewName)) { - rebuild.OldColumns.Add( - renameColumnOperation.NewName, - rebuild.OldColumns.Remove(renameColumnOperation.Name, out var oldName) - ? oldName - : renameColumnOperation.Name); rebuild.OperationsToReplace.Add(renameColumnOperation); - rebuild.AddColumnsDeferred.Add(renameColumnOperation.NewName); - } - else if (rebuild.DropColumnsDeferred.Contains(renameColumnOperation.NewName)) - { - rebuild.OldColumns.Add( - renameColumnOperation.NewName, - rebuild.OldColumns.Remove(renameColumnOperation.Name, out var oldName) - ? oldName - : renameColumnOperation.Name); - rebuild.OperationsToReplace.Add(renameColumnOperation); - rebuild.AddColumnsDeferred.Add(renameColumnOperation.NewName); rebuild.DropColumnsDeferred.Add(renameColumnOperation.Name); + rebuild.RenameColumnsDeferred.Add(renameColumnOperation.NewName, renameColumnOperation); } } @@ -254,6 +226,7 @@ private IReadOnlyList RewriteOperations( case DropIndexOperation _: case DropSchemaOperation _: case DropSequenceOperation _: + case DropTableOperation _: case EnsureSchemaOperation _: case RenameSequenceOperation _: case RestartSequenceOperation _: @@ -263,21 +236,6 @@ private IReadOnlyList RewriteOperations( break; } - case DropTableOperation dropTableOperation: - { - if (rebuilds.Remove((dropTableOperation.Name, dropTableOperation.Schema), out var rebuild)) - { - foreach (var operationToReplace in rebuild.OperationsToReplace) - { - operations.Remove(operationToReplace); - } - } - - operations.Add(operation); - - break; - } - case DeleteDataOperation _: case InsertDataOperation _: case UpdateDataOperation _: @@ -307,12 +265,15 @@ private IReadOnlyList RewriteOperations( } } + var skippedRebuilds = new List<(string Table, string Schema)>(); var indexesToRebuild = new List(); foreach (var rebuild in rebuilds) { var table = model?.GetRelationalModel().FindTable(rebuild.Key.Table, rebuild.Key.Schema); if (table == null) { + skippedRebuilds.Add(rebuild.Key); + continue; } @@ -348,7 +309,10 @@ private IReadOnlyList RewriteOperations( Name = column.Name, ColumnType = column.StoreType, IsNullable = column.IsNullable, - DefaultValue = column.DefaultValue, + DefaultValue = rebuild.Value.AddColumnsDeferred.TryGetValue(column.Name, out var originalOperation) + && !originalOperation.IsNullable + ? originalOperation.DefaultValue + : column.DefaultValue, DefaultValueSql = column.DefaultValueSql, ComputedColumnSql = column.ComputedColumnSql, IsStored = column.IsStored, @@ -377,18 +341,27 @@ private IReadOnlyList RewriteOperations( createTableOperation.AddAnnotations(table.GetAnnotations()); operations.Add(createTableOperation); - indexesToRebuild.AddRange(table.Indexes); + foreach (var index in table.Indexes) + { + if (index.IsUnique && rebuild.Value.CreateIndexesDeferred.Contains(index.Name)) + { + var createIndexOperation = CreateIndexOperation.For(index); + createIndexOperation.Table = createTableOperation.Name; + operations.Add(createIndexOperation); + } + else + { + indexesToRebuild.Add(index); + } + } var intoBuilder = new StringBuilder(); var selectBuilder = new StringBuilder(); var first = true; foreach (var column in table.Columns) { - var oldColumnName = rebuild.Value.OldColumns.TryGetValue(column.Name, out var value) - ? value - : column.Name; if (column.ComputedColumnSql != null - || oldColumnName == null) + || rebuild.Value.AddColumnsDeferred.ContainsKey(column.Name)) { continue; } @@ -405,54 +378,33 @@ private IReadOnlyList RewriteOperations( intoBuilder.Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(column.Name)); - var isNewlyRequired = rebuild.Value.NewlyRequiredColumns.TryGetValue(column.Name, out var overrides); - if (isNewlyRequired) + var defaultValue = rebuild.Value.AlterColumnsDeferred.TryGetValue(column.Name, out var alterColumnOperation) + && !alterColumnOperation.IsNullable + && alterColumnOperation.OldColumn.IsNullable + ? alterColumnOperation.DefaultValue + : null; + if (defaultValue != null) { selectBuilder.Append("IFNULL("); } - selectBuilder.Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(oldColumnName)); + selectBuilder.Append( + Dependencies.SqlGenerationHelper.DelimitIdentifier( + rebuild.Value.RenameColumnsDeferred.TryGetValue(column.Name, out var renameColumnOperation) + ? renameColumnOperation.Name + : column.Name)); - if (isNewlyRequired) + if (defaultValue != null) { - selectBuilder.Append(", "); - - var defaultValueSql = overrides.DefaultValueSql - ?? column.DefaultValueSql; - if (defaultValueSql != null) - { - selectBuilder.Append(defaultValueSql); - } - else - { - var defaultValue = overrides.DefaultValue - ?? column.DefaultValue; - if (defaultValue == null) - { - var propertyMapping = column.PropertyMappings.First(); - var property = propertyMapping.Property; - var typeMapping = propertyMapping.TypeMapping; - var clrType = ((property.GetValueConverter() ?? (property.FindRelationalTypeMapping() ?? typeMapping)?.Converter)?.ProviderClrType ?? typeMapping.ClrType).UnwrapNullableType(); - - defaultValue = clrType == typeof(string) - ? string.Empty - : clrType.IsArray - ? Array.CreateInstance(clrType.GetElementType(), 0) - : clrType.UnwrapNullableType().GetDefaultValue(); - } - - var defaultValueTypeMapping = column.StoreType != null - ? Dependencies.TypeMappingSource.FindMapping(defaultValue.GetType(), column.StoreType) - : null; - if (defaultValueTypeMapping == null) - { - defaultValueTypeMapping = Dependencies.TypeMappingSource.GetMappingForValue(defaultValue); - } - - selectBuilder.Append(defaultValueTypeMapping.GenerateSqlLiteral(defaultValue)); - } - - selectBuilder.Append(")"); + var defaultValueTypeMapping = (column.StoreType == null + ? null + : Dependencies.TypeMappingSource.FindMapping(defaultValue.GetType(), column.StoreType)) + ?? Dependencies.TypeMappingSource.GetMappingForValue(defaultValue); + + selectBuilder + .Append(", ") + .Append(defaultValueTypeMapping.GenerateSqlLiteral(defaultValue)) + .Append(")"); } } @@ -461,7 +413,7 @@ private IReadOnlyList RewriteOperations( { Sql = new StringBuilder() .Append("INSERT INTO ") - .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier("ef_temp_" + table.Name)) + .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(createTableOperation.Name)) .Append(" (") .Append(intoBuilder) .AppendLine(")") @@ -470,11 +422,16 @@ private IReadOnlyList RewriteOperations( .AppendLine() .Append("FROM ") .Append(table.Name) - .AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator) + .Append(Dependencies.SqlGenerationHelper.StatementTerminator) .ToString() }); } + foreach (var skippedRebuild in skippedRebuilds) + { + rebuilds.Remove(skippedRebuild); + } + if (rebuilds.Any()) { operations.Add( @@ -1095,19 +1052,14 @@ protected override void Generate(DropSequenceOperation operation, IModel model, #endregion - private sealed class RequiredColumnOverrides - { - public string DefaultValueSql { get; set; } - public object DefaultValue { get; set; } - } - private sealed class RebuildContext { public ICollection OperationsToReplace { get; } = new List(); - public IDictionary OldColumns { get; } = new Dictionary(); - public IDictionary NewlyRequiredColumns { get; } = new Dictionary(); - public ICollection AddColumnsDeferred { get; } = new HashSet(); + public IDictionary AddColumnsDeferred { get; } = new Dictionary(); public ICollection DropColumnsDeferred { get; } = new HashSet(); + public IDictionary AlterColumnsDeferred = new Dictionary(); + public IDictionary RenameColumnsDeferred = new Dictionary(); + public ICollection CreateIndexesDeferred { get; } = new HashSet(); public ICollection OperationsToWarnFor { get; } = new List(); } } diff --git a/test/EFCore.Sqlite.Tests/Migrations/SqliteMigrationsSqlGeneratorTest.cs b/test/EFCore.Sqlite.Tests/Migrations/SqliteMigrationsSqlGeneratorTest.cs index 542b09d1bc9..7f27b0345cd 100644 --- a/test/EFCore.Sqlite.Tests/Migrations/SqliteMigrationsSqlGeneratorTest.cs +++ b/test/EFCore.Sqlite.Tests/Migrations/SqliteMigrationsSqlGeneratorTest.cs @@ -611,6 +611,425 @@ public override void UpdateDataOperation_required_args_multiple_rows() WHERE ""First Name"" = 'Daenerys'; SELECT changes(); +"); + } + + [ConditionalFact] + public virtual void AddPrimaryKey_throws_when_no_model() + { + var ex = Assert.Throws( + () => Generate( + new AddPrimaryKeyOperation + { + Table = "Blogs", + Name = "PK_Blogs", + Columns = new[] { "Id" } + })); + + Assert.Equal(SqliteStrings.InvalidMigrationOperation("AddPrimaryKeyOperation"), ex.Message); + } + + [ConditionalFact] + public virtual void AddUniqueConstraint_throws_when_no_model() + { + var ex = Assert.Throws( + () => Generate( + new AddUniqueConstraintOperation + { + Table = "Blogs", + Name = "AK_Blogs_Uri", + Columns = new[] { "Uri" } + })); + + Assert.Equal(SqliteStrings.InvalidMigrationOperation("AddUniqueConstraintOperation"), ex.Message); + } + + [ConditionalFact] + public virtual void AddCheckConstraint_throws_when_no_model() + { + var ex = Assert.Throws( + () => Generate( + new AddCheckConstraintOperation + { + Table = "Blogs", + Name = "CK_Blogs_Rating", + Sql = "Rating BETWEEN 1 AND 5" + })); + + Assert.Equal(SqliteStrings.InvalidMigrationOperation("AddCheckConstraintOperation"), ex.Message); + } + + [ConditionalFact] + public virtual void AlterTable_mostly_works_when_no_model() + { + Generate( + new AlterTableOperation + { + Name = "Blogs", + Comment = "The Blogs table" + }); + + Assert.Empty(Sql); + } + + [ConditionalFact] + public virtual void DropForeignKey_throws_when_no_model() + { + var ex = Assert.Throws( + () => Generate( + new DropForeignKeyOperation + { + Table = "Posts", + Name = "FK_Posts_BlogId" + })); + + Assert.Equal(SqliteStrings.InvalidMigrationOperation("DropForeignKeyOperation"), ex.Message); + } + + [ConditionalFact] + public virtual void DropPrimaryKey_throws_when_no_model() + { + var ex = Assert.Throws( + () => Generate( + new DropPrimaryKeyOperation + { + Table = "Blogs", + Name = "PK_Blogs" + })); + + Assert.Equal(SqliteStrings.InvalidMigrationOperation("DropPrimaryKeyOperation"), ex.Message); + } + + [ConditionalFact] + public virtual void DropUniqueConstraint_throws_when_no_model() + { + var ex = Assert.Throws( + () => Generate( + new DropUniqueConstraintOperation + { + Table = "Blogs", + Name = "AK_Blogs_Uri" + })); + + Assert.Equal(SqliteStrings.InvalidMigrationOperation("DropUniqueConstraintOperation"), ex.Message); + } + + [ConditionalFact] + public virtual void DropColumn_throws_when_no_model() + { + var ex = Assert.Throws( + () => Generate( + new DropColumnOperation + { + Table = "Posts", + Name = "Rating" + })); + + Assert.Equal(SqliteStrings.InvalidMigrationOperation("DropColumnOperation"), ex.Message); + } + + [ConditionalFact] + public virtual void AddColumnOperation_with_comment_mostly_works_when_no_model() + { + Generate( + new AddColumnOperation + { + Table = "Blogs", + ClrType = typeof(string), + Name = "Summary", + Comment = "A short description" + }); + + AssertSql( + @"ALTER TABLE ""Blogs"" ADD ""Summary"" TEXT NOT NULL; +"); + } + + [ConditionalFact] + public virtual void DropColumn_defers_subsequent_RenameColumn() + { + Generate( + modelBuilder => modelBuilder.Entity("Blog").Property("Name"), + migrationBuilder => + { + migrationBuilder.DropColumn( + name: "Name", + table: "Blog"); + migrationBuilder.RenameColumn( + name: "Title", + table: "Blog", + newName: "Name"); + }); + + AssertSql( + @"CREATE TABLE ""ef_temp_Blog"" ( + ""Name"" TEXT NULL +); +GO + +INSERT INTO ""ef_temp_Blog"" (""Name"") +SELECT ""Title"" +FROM Blog; +GO + +PRAGMA foreign_keys = 0; +GO + +DROP TABLE ""Blog""; +GO + +ALTER TABLE ""ef_temp_Blog"" RENAME TO ""Blog""; +GO + +PRAGMA foreign_keys = 1; +"); + } + + [ConditionalFact] + public virtual void Deferred_RenameColumn_defers_subsequent_AddColumn() + { + Generate( + modelBuilder => modelBuilder.Entity( + "Blog", x => + { + x.Property("Title"); + x.Property("Name"); + }), + migrationBuilder => + { + migrationBuilder.DropColumn( + name: "Name", + table: "Blog"); + migrationBuilder.RenameColumn( + name: "Title", + table: "Blog", + newName: "Name"); + migrationBuilder.AddColumn( + name: "Title", + table: "Blog", + nullable: true); + }); + + AssertSql( + @"CREATE TABLE ""ef_temp_Blog"" ( + ""Name"" TEXT NULL, + ""Title"" TEXT NULL +); +GO + +INSERT INTO ""ef_temp_Blog"" (""Name"") +SELECT ""Title"" +FROM Blog; +GO + +PRAGMA foreign_keys = 0; +GO + +DROP TABLE ""Blog""; +GO + +ALTER TABLE ""ef_temp_Blog"" RENAME TO ""Blog""; +GO + +PRAGMA foreign_keys = 1; +"); + } + + [ConditionalFact] + public virtual void Deferred_RenameColumn_defers_subsequent_CreateIndex_unique() + { + Generate( + modelBuilder => modelBuilder.Entity( + "Blog", x => + { + x.Property("Name"); + x.HasIndex("Name").IsUnique(); + }), + migrationBuilder => + { + migrationBuilder.DropColumn( + name: "Name", + table: "Blog"); + migrationBuilder.RenameColumn( + name: "Title", + table: "Blog", + newName: "Name"); + migrationBuilder.CreateIndex( + name: "IX_Blog_Name", + table: "Blog", + column: "Name", + unique: true); + }); + + AssertSql( + @"CREATE TABLE ""ef_temp_Blog"" ( + ""Name"" TEXT NULL +); +GO + +CREATE UNIQUE INDEX ""IX_Blog_Name"" ON ""ef_temp_Blog"" (""Name""); +GO + +INSERT INTO ""ef_temp_Blog"" (""Name"") +SELECT ""Title"" +FROM Blog; +GO + +PRAGMA foreign_keys = 0; +GO + +DROP TABLE ""Blog""; +GO + +ALTER TABLE ""ef_temp_Blog"" RENAME TO ""Blog""; +GO + +PRAGMA foreign_keys = 1; +"); + } + + [ConditionalFact] + public virtual void DropColumn_defers_subsequent_AddColumn_required() + { + Generate( + modelBuilder => modelBuilder.Entity( + "Blog", + x => + { + x.Property("Id"); + x.Property("Name").IsRequired(); + }), + migrationBuilder => + { + migrationBuilder.DropColumn( + name: "Name", + table: "Blog"); + migrationBuilder.AddColumn( + name: "Name", + table: "Blog", + nullable: false, + defaultValue: "Overridden"); + }); + + AssertSql( + @"CREATE TABLE ""ef_temp_Blog"" ( + ""Id"" INTEGER NOT NULL CONSTRAINT ""PK_Blog"" PRIMARY KEY AUTOINCREMENT, + ""Name"" TEXT NOT NULL DEFAULT 'Overridden' +); +GO + +INSERT INTO ""ef_temp_Blog"" (""Id"") +SELECT ""Id"" +FROM Blog; +GO + +PRAGMA foreign_keys = 0; +GO + +DROP TABLE ""Blog""; +GO + +ALTER TABLE ""ef_temp_Blog"" RENAME TO ""Blog""; +GO + +PRAGMA foreign_keys = 1; +"); + } + + [ConditionalFact] + public virtual void Deferred_AddColumn_defers_subsequent_CreateIndex() + { + Generate( + modelBuilder => modelBuilder.Entity( + "Blog", + x => + { + x.Property("Id"); + x.Property("Name"); + x.HasIndex("Name"); + }), + migrationBuilder => + { + migrationBuilder.DropColumn( + name: "Name", + table: "Blog"); + migrationBuilder.AddColumn( + name: "Name", + table: "Blog"); + migrationBuilder.CreateIndex( + name: "IX_Blog_Name", + table: "Blog", + column: "Name"); + + }); + + AssertSql( + @"CREATE TABLE ""ef_temp_Blog"" ( + ""Id"" INTEGER NOT NULL CONSTRAINT ""PK_Blog"" PRIMARY KEY AUTOINCREMENT, + ""Name"" TEXT NULL +); +GO + +INSERT INTO ""ef_temp_Blog"" (""Id"") +SELECT ""Id"" +FROM Blog; +GO + +PRAGMA foreign_keys = 0; +GO + +DROP TABLE ""Blog""; +GO + +ALTER TABLE ""ef_temp_Blog"" RENAME TO ""Blog""; +GO + +PRAGMA foreign_keys = 1; +GO + +CREATE INDEX ""IX_Blog_Name"" ON ""Blog"" (""Name""); +"); + } + + [ConditionalFact] + public virtual void RenameTable_preserves_pending_rebuilds() + { + Generate( + modelBuilder => modelBuilder.Entity("Blog").Property("Id"), + migrationBuilder => + { + migrationBuilder.DropColumn( + name: "Name", + table: "Blogs"); + migrationBuilder.RenameTable( + name: "Blogs", + newName: "Blog"); + }); + + AssertSql( + @"ALTER TABLE ""Blogs"" RENAME TO ""Blog""; +GO + +CREATE TABLE ""ef_temp_Blog"" ( + ""Id"" INTEGER NOT NULL CONSTRAINT ""PK_Blog"" PRIMARY KEY AUTOINCREMENT +); +GO + +INSERT INTO ""ef_temp_Blog"" (""Id"") +SELECT ""Id"" +FROM Blog; +GO + +PRAGMA foreign_keys = 0; +GO + +DROP TABLE ""Blog""; +GO + +ALTER TABLE ""ef_temp_Blog"" RENAME TO ""Blog""; +GO + +PRAGMA foreign_keys = 1; "); } @@ -623,5 +1042,13 @@ public SqliteMigrationsSqlGeneratorTest() .OptionsBuilder).Options) { } + + protected virtual void Generate(Action buildAction, Action migrateAction) + { + var migrationBuilder = new MigrationBuilder(activeProvider: null); + migrateAction(migrationBuilder); + + Generate(buildAction, migrationBuilder.Operations.ToArray()); + } } }