diff --git a/src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs b/src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs index 3e96d255f..6d0029eb6 100644 --- a/src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs +++ b/src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs @@ -305,7 +305,6 @@ protected override void Generate(AlterColumnOperation operation, IModel model, M var column = model?.GetRelationalModel().FindTable(operation.Table, operation.Schema) ?.Columns.FirstOrDefault(c => c.Name == operation.Name); - var type = operation.ColumnType ?? GetColumnType(operation.Schema, operation.Table, operation.Name, operation, model); if (operation.ComputedColumnSql != null) { @@ -348,30 +347,42 @@ protected override void Generate(AlterColumnOperation operation, IModel model, M } string newSequenceName = null; - var defaultValueSql = operation.DefaultValueSql; var alterBase = $"ALTER TABLE {DelimitIdentifier(operation.Table, operation.Schema)} " + $"ALTER COLUMN {DelimitIdentifier(operation.Name)} "; // TYPE + COLLATION - builder.Append(alterBase) - .Append("TYPE ") - .Append(type); + var type = operation.ColumnType ?? + GetColumnType(operation.Schema, operation.Table, operation.Name, operation, model); + var oldType = IsOldColumnSupported(model) + ? operation.OldColumn.ColumnType ?? + GetColumnType(operation.Schema, operation.Table, operation.Name, operation.OldColumn, model) + : null; // If a collation was defined on the column specifically, via the standard EF mechanism, it will be // available in operation.Collation (as usual). If not, there may be a model-wide default column collation, // which gets transmitted via the Npgsql-specific annotation. var oldCollation = (string)(operation.OldColumn.Collation ?? operation.OldColumn[NpgsqlAnnotationNames.DefaultColumnCollation]); var newCollation = (string)(operation.Collation ?? operation[NpgsqlAnnotationNames.DefaultColumnCollation]); - if (newCollation != oldCollation) - builder.Append(" COLLATE ").Append(DelimitIdentifier(newCollation ?? "default")); - builder.AppendLine(";"); + if (type != oldType || newCollation != oldCollation) + { + builder.Append(alterBase) + .Append("TYPE ") + .Append(type); - // NOT NULL - builder.Append(alterBase) - .Append(operation.IsNullable ? "DROP NOT NULL" : "SET NOT NULL") - .AppendLine(";"); + if (newCollation != oldCollation) + builder.Append(" COLLATE ").Append(DelimitIdentifier(newCollation ?? "default")); + + builder.AppendLine(";"); + } + + if (operation.IsNullable != operation.OldColumn.IsNullable) + { + builder.Append(alterBase) + .Append(operation.IsNullable ? "DROP NOT NULL" : "SET NOT NULL") + .AppendLine(";"); + } CheckForOldValueGenerationAnnotation(operation); @@ -441,6 +452,7 @@ protected override void Generate(AlterColumnOperation operation, IModel model, M { case NpgsqlValueGenerationStrategy.IdentityAlwaysColumn: case NpgsqlValueGenerationStrategy.IdentityByDefaultColumn: + builder.Append(alterBase).AppendLine("DROP DEFAULT;"); builder.Append(alterBase).Append("ADD"); IdentityDefinition(operation, builder); builder.AppendLine(";"); @@ -545,20 +557,21 @@ protected override void Generate(AlterColumnOperation operation, IModel model, M // DEFAULT. // Note that defaults values for value-generated columns (identity, serial) are managed above. This is // only for regular columns with user-specified default settings. - if (newStrategy == null) + if (newStrategy == null && + (operation.DefaultValueSql != operation.OldColumn.DefaultValueSql || + !Equals(operation.DefaultValue, operation.OldColumn.DefaultValue))) { builder.Append(alterBase); - if (operation.DefaultValue != null || defaultValueSql != null) + if (operation.DefaultValue != null || operation.DefaultValueSql != null) { builder.Append("SET"); - DefaultValue(operation.DefaultValue, defaultValueSql, type, builder); + DefaultValue(operation.DefaultValue, operation.DefaultValueSql, type, builder); } else builder.Append("DROP DEFAULT"); builder.AppendLine(";"); } - // A sequence has been created because this column was altered to be a serial. // Change the sequence's ownership. if (newSequenceName != null) diff --git a/test/EFCore.PG.FunctionalTests/MigrationsNpgsqlTest.cs b/test/EFCore.PG.FunctionalTests/MigrationsNpgsqlTest.cs index 857f51a69..065664b8e 100644 --- a/test/EFCore.PG.FunctionalTests/MigrationsNpgsqlTest.cs +++ b/test/EFCore.PG.FunctionalTests/MigrationsNpgsqlTest.cs @@ -910,9 +910,7 @@ public override async Task Alter_column_change_type() await base.Alter_column_change_type(); AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""SomeColumn"" TYPE bigint; -ALTER TABLE ""People"" ALTER COLUMN ""SomeColumn"" SET NOT NULL; -ALTER TABLE ""People"" ALTER COLUMN ""SomeColumn"" DROP DEFAULT;"); + @"ALTER TABLE ""People"" ALTER COLUMN ""SomeColumn"" TYPE bigint;"); } public override async Task Alter_column_make_required() @@ -920,8 +918,7 @@ public override async Task Alter_column_make_required() await base.Alter_column_make_required(); AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""SomeColumn"" TYPE text; -ALTER TABLE ""People"" ALTER COLUMN ""SomeColumn"" SET NOT NULL; + @"ALTER TABLE ""People"" ALTER COLUMN ""SomeColumn"" SET NOT NULL; ALTER TABLE ""People"" ALTER COLUMN ""SomeColumn"" SET DEFAULT '';"); } @@ -930,8 +927,7 @@ public override async Task Alter_column_make_required_with_index() await base.Alter_column_make_required_with_index(); AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""SomeColumn"" TYPE text; -ALTER TABLE ""People"" ALTER COLUMN ""SomeColumn"" SET NOT NULL; + @"ALTER TABLE ""People"" ALTER COLUMN ""SomeColumn"" SET NOT NULL; ALTER TABLE ""People"" ALTER COLUMN ""SomeColumn"" SET DEFAULT '';"); } @@ -940,8 +936,7 @@ public override async Task Alter_column_make_required_with_composite_index() await base.Alter_column_make_required_with_composite_index(); AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""FirstName"" TYPE text; -ALTER TABLE ""People"" ALTER COLUMN ""FirstName"" SET NOT NULL; + @"ALTER TABLE ""People"" ALTER COLUMN ""FirstName"" SET NOT NULL; ALTER TABLE ""People"" ALTER COLUMN ""FirstName"" SET DEFAULT '';"); } @@ -1012,36 +1007,24 @@ public override async Task Alter_column_add_comment() { await base.Alter_column_add_comment(); - // TODO: #1222 AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" TYPE integer; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" DROP DEFAULT; -COMMENT ON COLUMN ""People"".""Id"" IS 'Some comment';"); + @"COMMENT ON COLUMN ""People"".""Id"" IS 'Some comment';"); } public override async Task Alter_column_change_comment() { await base.Alter_column_change_comment(); - // TODO: #1222 AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" TYPE integer; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" DROP DEFAULT; -COMMENT ON COLUMN ""People"".""Id"" IS 'Some comment2';"); + @"COMMENT ON COLUMN ""People"".""Id"" IS 'Some comment2';"); } public override async Task Alter_column_remove_comment() { await base.Alter_column_remove_comment(); - // TODO: #1222 AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" TYPE integer; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" DROP DEFAULT; -COMMENT ON COLUMN ""People"".""Id"" IS NULL;"); + @"COMMENT ON COLUMN ""People"".""Id"" IS NULL;"); } [Fact] @@ -1066,8 +1049,7 @@ await Test( }); AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" TYPE integer; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL; + @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" DROP DEFAULT; ALTER TABLE ""People"" ALTER COLUMN ""Id"" ADD GENERATED BY DEFAULT AS IDENTITY;"); } @@ -1093,8 +1075,33 @@ await Test( }); AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" TYPE integer; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL; + @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" DROP DEFAULT; +ALTER TABLE ""People"" ALTER COLUMN ""Id"" ADD GENERATED ALWAYS AS IDENTITY;"); + } + + [Fact] + public virtual async Task Alter_column_make_default_into_identity() + { + await Test( + builder => builder.Entity( + "People", e => + { + e.Property("Id").HasDefaultValue(8); + e.HasKey("Id"); + }), + builder => { }, + builder => builder.Entity("People").Property("Id") + .UseIdentityAlwaysColumn(), + model => + { + var table = Assert.Single(model.Tables); + var column = Assert.Single(table.Columns); + Assert.Equal(NpgsqlValueGenerationStrategy.IdentityAlwaysColumn, column[NpgsqlAnnotationNames.ValueGenerationStrategy]); + Assert.Null(column[NpgsqlAnnotationNames.IdentityOptions]); + }); + + AssertSql( + @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" DROP DEFAULT; ALTER TABLE ""People"" ALTER COLUMN ""Id"" ADD GENERATED ALWAYS AS IDENTITY;"); } @@ -1127,8 +1134,7 @@ await Test( }); AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" TYPE integer; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL; + @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" DROP DEFAULT; ALTER TABLE ""People"" ALTER COLUMN ""Id"" ADD GENERATED BY DEFAULT AS IDENTITY (START WITH 10 INCREMENT BY 2 MAXVALUE 2000);"); } @@ -1158,9 +1164,7 @@ await Test( }); AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" TYPE integer; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET INCREMENT BY 2; + @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET INCREMENT BY 2; ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET CYCLE;"); } @@ -1190,9 +1194,7 @@ await Test( }); AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" TYPE integer; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" RESTART WITH 1; + @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" RESTART WITH 1; ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET INCREMENT BY 1; ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NO CYCLE; ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET CACHE 1;"); @@ -1222,9 +1224,7 @@ await Test( }); AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" TYPE integer; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL; -CREATE SEQUENCE ""People_Id_seq"" AS integer START WITH 1 INCREMENT BY 1 NO MINVALUE NO MAXVALUE NO CYCLE;", + @"CREATE SEQUENCE ""People_Id_seq"" AS integer START WITH 1 INCREMENT BY 1 NO MINVALUE NO MAXVALUE NO CYCLE;", // @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET DEFAULT (nextval('""People_Id_seq""')); ALTER SEQUENCE ""People_Id_seq"" OWNED BY ""People"".""Id"";"); @@ -1255,9 +1255,7 @@ await Test( }); AssertSql( - @"ALTER TABLE some_schema.""People"" ALTER COLUMN ""Id"" TYPE integer; -ALTER TABLE some_schema.""People"" ALTER COLUMN ""Id"" SET NOT NULL; -CREATE SEQUENCE some_schema.""People_Id_seq"" AS integer START WITH 1 INCREMENT BY 1 NO MINVALUE NO MAXVALUE NO CYCLE;", + @"CREATE SEQUENCE some_schema.""People_Id_seq"" AS integer START WITH 1 INCREMENT BY 1 NO MINVALUE NO MAXVALUE NO CYCLE;", // @"ALTER TABLE some_schema.""People"" ALTER COLUMN ""Id"" SET DEFAULT (nextval('some_schema.""People_Id_seq""')); ALTER SEQUENCE some_schema.""People_Id_seq"" OWNED BY some_schema.""People"".""Id"";"); @@ -1288,9 +1286,7 @@ await Test( }); AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" TYPE bigint; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL; -CREATE SEQUENCE ""People_Id_seq"" START WITH 1 INCREMENT BY 1 NO MINVALUE NO MAXVALUE NO CYCLE;", + @"CREATE SEQUENCE ""People_Id_seq"" START WITH 1 INCREMENT BY 1 NO MINVALUE NO MAXVALUE NO CYCLE;", // @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET DEFAULT (nextval('""People_Id_seq""')); ALTER SEQUENCE ""People_Id_seq"" OWNED BY ""People"".""Id"";"); @@ -1317,9 +1313,7 @@ await Test( }); AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" TYPE integer; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET GENERATED ALWAYS;"); + @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET GENERATED ALWAYS;"); } [Fact] @@ -1343,9 +1337,7 @@ await Test( }); AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" TYPE integer; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL; -ALTER SEQUENCE ""People_Id_seq"" RENAME TO ""People_Id_old_seq""; + @"ALTER SEQUENCE ""People_Id_seq"" RENAME TO ""People_Id_old_seq""; ALTER TABLE ""People"" ALTER COLUMN ""Id"" DROP DEFAULT; ALTER TABLE ""People"" ALTER COLUMN ""Id"" ADD GENERATED ALWAYS AS IDENTITY; SELECT * FROM setval('""People_Id_seq""', nextval('""People_Id_old_seq""'), false); @@ -1378,8 +1370,7 @@ await Test( }); AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" TYPE bigint; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL;"); + @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" TYPE bigint;"); } [Fact] @@ -1404,9 +1395,7 @@ await Test( }); AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" TYPE integer; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL; -ALTER TABLE ""People"" ALTER COLUMN ""Id"" RESTART WITH 20;"); + @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" RESTART WITH 20;"); } [Fact] @@ -1415,9 +1404,7 @@ public override async Task Alter_column_set_collation() await base.Alter_column_set_collation(); AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""Name"" TYPE text COLLATE ""POSIX""; -ALTER TABLE ""People"" ALTER COLUMN ""Name"" DROP NOT NULL; -ALTER TABLE ""People"" ALTER COLUMN ""Name"" DROP DEFAULT;"); + @"ALTER TABLE ""People"" ALTER COLUMN ""Name"" TYPE text COLLATE ""POSIX"";"); } [Fact] @@ -1426,9 +1413,7 @@ public override async Task Alter_column_reset_collation() await base.Alter_column_reset_collation(); AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""Name"" TYPE text COLLATE ""default""; -ALTER TABLE ""People"" ALTER COLUMN ""Name"" DROP NOT NULL; -ALTER TABLE ""People"" ALTER COLUMN ""Name"" DROP DEFAULT;"); + @"ALTER TABLE ""People"" ALTER COLUMN ""Name"" TYPE text COLLATE ""default"";"); } [Fact] @@ -1451,9 +1436,7 @@ await Test( }); AssertSql( - @"ALTER TABLE ""People"" ALTER COLUMN ""Name"" TYPE text COLLATE ""C""; -ALTER TABLE ""People"" ALTER COLUMN ""Name"" DROP NOT NULL; -ALTER TABLE ""People"" ALTER COLUMN ""Name"" DROP DEFAULT;"); + @"ALTER TABLE ""People"" ALTER COLUMN ""Name"" TYPE text COLLATE ""C"";"); } public override async Task Drop_column() diff --git a/test/EFCore.PG.Tests/Migrations/NpgsqlMigrationsSqlGeneratorTest.cs b/test/EFCore.PG.Tests/Migrations/NpgsqlMigrationsSqlGeneratorTest.cs index daa08f842..c1afe26a4 100644 --- a/test/EFCore.PG.Tests/Migrations/NpgsqlMigrationsSqlGeneratorTest.cs +++ b/test/EFCore.PG.Tests/Migrations/NpgsqlMigrationsSqlGeneratorTest.cs @@ -4,6 +4,7 @@ using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.EntityFrameworkCore.Infrastructure; using Microsoft.EntityFrameworkCore.Metadata; +using Microsoft.EntityFrameworkCore.Metadata.Internal; using Microsoft.EntityFrameworkCore.Migrations; using Microsoft.EntityFrameworkCore.Migrations.Operations; using Microsoft.Extensions.DependencyInjection; @@ -161,8 +162,6 @@ public override void AlterColumnOperation_without_column_type() AssertSql( @"ALTER TABLE ""People"" ALTER COLUMN ""LuckyNumber"" TYPE integer; -ALTER TABLE ""People"" ALTER COLUMN ""LuckyNumber"" SET NOT NULL; -ALTER TABLE ""People"" ALTER COLUMN ""LuckyNumber"" DROP DEFAULT; "); } @@ -432,7 +431,11 @@ public void CreateIndexOperation_collation() public void Alter_column_change_serial_to_identity_idempotent(MigrationsSqlGenerationOptions options) { Generate( - modelBuilder => modelBuilder.Entity().Property("Id").UseSerialColumn(), + modelBuilder => + { + modelBuilder.HasAnnotation(CoreAnnotationNames.ProductVersion, "3.1.0"); + modelBuilder.Entity().Property("Id").UseSerialColumn(); + }, new[] { new AlterColumnOperation @@ -456,9 +459,7 @@ public void Alter_column_change_serial_to_identity_idempotent(MigrationsSqlGener options); AssertSql( - $@"ALTER TABLE ""Person"" ALTER COLUMN ""Id"" TYPE integer; -ALTER TABLE ""Person"" ALTER COLUMN ""Id"" SET NOT NULL; -ALTER SEQUENCE ""Person_Id_seq"" RENAME TO ""Person_Id_old_seq""; + $@"ALTER SEQUENCE ""Person_Id_seq"" RENAME TO ""Person_Id_old_seq""; ALTER TABLE ""Person"" ALTER COLUMN ""Id"" DROP DEFAULT; ALTER TABLE ""Person"" ALTER COLUMN ""Id"" ADD GENERATED BY DEFAULT AS IDENTITY; {(options == MigrationsSqlGenerationOptions.Idempotent ? "PERFORM" : "SELECT")} * FROM setval('""Person_Id_seq""', nextval('""Person_Id_old_seq""'), false);