Skip to content

Commit

Permalink
Don't generate useless DDL when altering column
Browse files Browse the repository at this point in the history 
Fixes #1222
  • Loading branch information
@roji
roji committed Sep 4, 2020
1 parent 93697d6 commit 210fc07
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 87 deletions.
45 changes: 29 additions & 16 deletions src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,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)
{
Expand Down Expand Up @@ -425,30 +424,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);

Expand Down Expand Up @@ -518,6 +529,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(";");
Expand Down Expand Up @@ -622,20 +634,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)
Expand Down
113 changes: 48 additions & 65 deletions test/EFCore.PG.FunctionalTests/MigrationsNpgsqlTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -910,18 +910,15 @@ 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()
{
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 '';");
}

Expand All @@ -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 '';");
}

Expand All @@ -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 '';");
}

Expand Down Expand Up @@ -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]
Expand All @@ -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;");
}

Expand All @@ -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<int>("Id").HasDefaultValue(8);
e.HasKey("Id");
}),
builder => { },
builder => builder.Entity("People").Property<int>("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;");
}

Expand Down Expand Up @@ -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);");
}

Expand Down Expand Up @@ -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;");
}

Expand Down Expand Up @@ -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;");
Expand Down Expand Up @@ -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"";");
Expand Down Expand Up @@ -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"";");
Expand Down Expand Up @@ -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"";");
Expand All @@ -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]
Expand All @@ -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);
Expand Down Expand Up @@ -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]
Expand All @@ -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]
Expand All @@ -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]
Expand All @@ -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]
Expand All @@ -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()
Expand Down
Loading

0 comments on commit 210fc07

Please sign in to comment.