Fix some alter column logic

Fixes #524

Author: roji

src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs

@@ -316,6 +316,17 @@ protected override void Generate(AlterColumnOperation operation, IModel model, M
             var column = Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Name);
             var alterBase = $"ALTER TABLE {table} ALTER COLUMN {column} ";
 
+            // TYPE
+            builder.Append(alterBase)
+                .Append("TYPE ")
+                .Append(type)
+                .AppendLine(';');
+
+            // NOT NULL
+            builder.Append(alterBase)
+                .Append(operation.IsNullable ? "DROP NOT NULL" : "SET NOT NULL")
+                .AppendLine(';');
+
             CheckForOldValueGenerationAnnotation(operation);
 
             var oldStrategy = operation.OldColumn[NpgsqlAnnotationNames.ValueGenerationStrategy] as NpgsqlValueGenerationStrategy?;
@@ -401,7 +412,10 @@ protected override void Generate(AlterColumnOperation operation, IModel model, M
                                 Name = newSequenceName,
                                 ClrType = operation.ClrType
                             }, model, builder);
-                            defaultValueSql = $@"nextval('{Dependencies.SqlGenerationHelper.DelimitIdentifier(newSequenceName)}')";
+
+                            builder.Append(alterBase).Append("SET");
+                            DefaultValue(null, $@"nextval('{Dependencies.SqlGenerationHelper.DelimitIdentifier(newSequenceName)}')", builder);
+                            builder.AppendLine(';');
                             // Note: we also need to set the sequence ownership, this is done below after the ALTER COLUMN
                             break;
                         }
@@ -412,22 +426,10 @@ protected override void Generate(AlterColumnOperation operation, IModel model, M
                 }
             }
 
-            // TYPE
-            builder.Append(alterBase)
-                .Append("TYPE ")
-                .Append(type)
-                .AppendLine(';');
-
-            // NOT NULL
-            builder.Append(alterBase)
-                .Append(operation.IsNullable ? "DROP NOT NULL" : "SET NOT NULL")
-                .AppendLine(';');
-
             // DEFAULT.
-            // Note that identity columns don't have a regular default but trying
-            // to drop the default triggers an error.
-            if (newStrategy != NpgsqlValueGenerationStrategy.IdentityAlwaysColumn &&
-                newStrategy != NpgsqlValueGenerationStrategy.IdentityByDefaultColumn)
+            // 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)
             {
                 builder.Append(alterBase);
                 if (operation.DefaultValue != null || defaultValueSql != null)

test/EFCore.PG.FunctionalTests/NpgsqlMigrationSqlGeneratorTest.cs

@@ -471,9 +471,9 @@ public void AlterColumnOperation_to_identity()
                 });
 
             Assert.Equal(
-                @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" ADD GENERATED ALWAYS AS IDENTITY;" + EOL +
                 @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" TYPE integer;" + EOL +
-                @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL;" + EOL,
+                @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL;" + EOL +
+                @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" ADD GENERATED ALWAYS AS IDENTITY;" + EOL,
                 Sql);
         }
 
@@ -491,10 +491,10 @@ public void AlterColumnOperation_int_to_serial()
                 });
 
             Assert.Equal(
-                @"CREATE SEQUENCE ""People_IntKey_seq"" AS integer START WITH 1 INCREMENT BY 1 NO MINVALUE NO MAXVALUE NO CYCLE;" + EOL +
-                @"GO" + EOL + EOL +  // Note that GO here is just a delimiter introduced in the tests to indicate a batch boundary
                 @"ALTER TABLE ""People"" ALTER COLUMN ""IntKey"" TYPE integer;" + EOL +
                 @"ALTER TABLE ""People"" ALTER COLUMN ""IntKey"" SET NOT NULL;" + EOL +
+                @"CREATE SEQUENCE ""People_IntKey_seq"" AS integer START WITH 1 INCREMENT BY 1 NO MINVALUE NO MAXVALUE NO CYCLE;" + EOL +
+                @"GO" + EOL + EOL +  // Note that GO here is just a delimiter introduced in the tests to indicate a batch boundary
                 @"ALTER TABLE ""People"" ALTER COLUMN ""IntKey"" SET DEFAULT (nextval('""People_IntKey_seq""'));" + EOL +
                 @"ALTER SEQUENCE ""People_IntKey_seq"" OWNED BY ""People"".""IntKey"";" + EOL,
                 Sql);
@@ -514,10 +514,10 @@ public void AlterColumnOperation_long_to_bigserial()
                 });
 
             Assert.Equal(
-                @"CREATE SEQUENCE ""People_LongKey_seq"" START WITH 1 INCREMENT BY 1 NO MINVALUE NO MAXVALUE NO CYCLE;" + EOL +
-                @"GO" + EOL + EOL +  // Note that GO here is just a delimiter introduced in the tests to indicate a batch boundary
                 @"ALTER TABLE ""People"" ALTER COLUMN ""LongKey"" TYPE bigint;" + EOL +
                 @"ALTER TABLE ""People"" ALTER COLUMN ""LongKey"" SET NOT NULL;" + EOL +
+                @"CREATE SEQUENCE ""People_LongKey_seq"" START WITH 1 INCREMENT BY 1 NO MINVALUE NO MAXVALUE NO CYCLE;" + EOL +
+                @"GO" + EOL + EOL +  // Note that GO here is just a delimiter introduced in the tests to indicate a batch boundary
                 @"ALTER TABLE ""People"" ALTER COLUMN ""LongKey"" SET DEFAULT (nextval('""People_LongKey_seq""'));" + EOL +
                 @"ALTER SEQUENCE ""People_LongKey_seq"" OWNED BY ""People"".""LongKey"";" + EOL,
                 Sql);
@@ -540,9 +540,9 @@ public void AlterColumnOperation_identity_to_identity()
                 });
 
             Assert.Equal(
-                @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET GENERATED ALWAYS;" + EOL +
                 @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" TYPE integer;" + EOL +
-                @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL;" + EOL,
+                @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL;" + EOL +
+                @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET GENERATED ALWAYS;" + EOL,
                 Sql);
         }
 
@@ -562,13 +562,36 @@ public void AlterColumnOperation_serial_to_identity()
                     [NpgsqlAnnotationNames.ValueGenerationStrategy] = NpgsqlValueGenerationStrategy.IdentityAlwaysColumn
                 });
 
-            Assert.Equal(@"ALTER SEQUENCE ""People_Id_seq"" RENAME TO ""People_Id_old_seq"";" + EOL +
+            Assert.Equal(
+                @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" TYPE integer;" + EOL +
+                @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL;" + EOL +
+                @"ALTER SEQUENCE ""People_Id_seq"" RENAME TO ""People_Id_old_seq"";" + EOL +
                 @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" DROP DEFAULT;" + EOL +
                 @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" ADD GENERATED ALWAYS AS IDENTITY;" + EOL +
                 @"SELECT * FROM setval('""People_Id_seq""', nextval('""People_Id_old_seq""'), false);" + EOL +
-                @"DROP SEQUENCE ""People_Id_old_seq"";" + EOL +
-                @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" TYPE integer;" + EOL +
-                @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL;" + EOL,
+                @"DROP SEQUENCE ""People_Id_old_seq"";" + EOL,
+                Sql);
+        }
+
+        [Fact]
+        public void AlterColumnOperation_serial_change_type()
+        {
+            Generate(
+                new AlterColumnOperation
+                {
+                    Table = "People",
+                    Name = "Id",
+                    ClrType = typeof(long),
+                    OldColumn = new ColumnOperation
+                    {
+                        ClrType = typeof(int),
+                        [NpgsqlAnnotationNames.ValueGenerationStrategy] = NpgsqlValueGenerationStrategy.SerialColumn
+                    },
+                    [NpgsqlAnnotationNames.ValueGenerationStrategy] = NpgsqlValueGenerationStrategy.SerialColumn
+                });
+
+            Assert.Equal(@"ALTER TABLE ""People"" ALTER COLUMN ""Id"" TYPE bigint;" + EOL +
+                         @"ALTER TABLE ""People"" ALTER COLUMN ""Id"" SET NOT NULL;" + EOL,
                 Sql);
         }