Skip to content

Commit

Permalink
SqlServer Migrations: Don't generate unnecessary ALTER TABLE
Browse files Browse the repository at this point in the history 
Fixes #21364
  • Loading branch information
@bricelam
bricelam committed Aug 25, 2020
1 parent 9638c0c commit ff60c29
Show file tree
Hide file tree
Showing 2 changed files with 143 additions and 59 deletions.
127 changes: 87 additions & 40 deletions src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -264,14 +264,18 @@ protected override void Generate(
ClrType = operation.ClrType,
ColumnType = operation.ColumnType,
IsUnicode = operation.IsUnicode,
IsFixedLength = operation.IsFixedLength,
MaxLength = operation.MaxLength,
Precision = operation.Precision,
Scale = operation.Scale,
IsRowVersion = operation.IsRowVersion,
IsNullable = operation.IsNullable,
DefaultValue = operation.DefaultValue,
DefaultValueSql = operation.DefaultValueSql,
ComputedColumnSql = operation.ComputedColumnSql,
IsStored = operation.IsStored,
IsFixedLength = operation.IsFixedLength
Comment = operation.Comment,
Collation = operation.Collation
};
addColumnOperation.AddAnnotations(operation.GetAnnotations());

Expand All @@ -280,16 +284,16 @@ protected override void Generate(
DropIndexes(indexesToRebuild, builder);
Generate(dropColumnOperation, model, builder, terminate: false);
builder.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator);
Generate(addColumnOperation, model, builder, terminate: false);
builder.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator);
Generate(addColumnOperation, model, builder);
CreateIndexes(indexesToRebuild, builder);
builder.EndCommand(suppressTransaction: IsMemoryOptimized(operation, model, operation.Schema, operation.Table));

return;
}

var narrowed = false;
if (IsOldColumnSupported(model))
var oldColumnSupported = IsOldColumnSupported(model);
if (oldColumnSupported)
{
if (IsIdentity(operation) != IsIdentity(operation.OldColumn))
{
Expand Down Expand Up @@ -321,44 +325,68 @@ protected override void Generate(
DropIndexes(indexesToRebuild, builder);
}

DropDefaultConstraint(operation.Schema, operation.Table, operation.Name, builder);
var alterStatementNeeded = narrowed
|| !oldColumnSupported
|| operation.ClrType != operation.OldColumn.ClrType
|| operation.ColumnType != operation.OldColumn.ColumnType
|| operation.IsUnicode != operation.OldColumn.IsUnicode
|| operation.IsFixedLength != operation.OldColumn.IsFixedLength
|| operation.MaxLength != operation.OldColumn.MaxLength
|| operation.Precision != operation.OldColumn.Precision
|| operation.Scale != operation.OldColumn.Scale
|| operation.IsRowVersion != operation.OldColumn.IsRowVersion
|| operation.IsNullable != operation.OldColumn.IsNullable
|| operation.Collation != operation.OldColumn.Collation
|| HasDifferences(operation.GetAnnotations(), operation.OldColumn.GetAnnotations());

builder
.Append("ALTER TABLE ")
.Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Table, operation.Schema))
.Append(" ALTER COLUMN ");

// NB: DefaultValue, DefaultValueSql, and identity are handled elsewhere. Don't copy them here.
var definitionOperation = new AlterColumnOperation
{
Schema = operation.Schema,
Table = operation.Table,
Name = operation.Name,
ClrType = operation.ClrType,
ColumnType = operation.ColumnType,
IsUnicode = operation.IsUnicode,
IsFixedLength = operation.IsFixedLength,
MaxLength = operation.MaxLength,
IsRowVersion = operation.IsRowVersion,
IsNullable = operation.IsNullable,
ComputedColumnSql = operation.ComputedColumnSql,
Collation = operation.Collation,
OldColumn = operation.OldColumn
};
definitionOperation.AddAnnotations(
operation.GetAnnotations().Where(
a => a.Name != SqlServerAnnotationNames.ValueGenerationStrategy
&& a.Name != SqlServerAnnotationNames.Identity));

ColumnDefinition(
operation.Schema,
operation.Table,
operation.Name,
definitionOperation,
model,
builder);
if (alterStatementNeeded
|| !Equals(operation.DefaultValue, operation.OldColumn.DefaultValue)
|| operation.DefaultValueSql != operation.OldColumn.DefaultValueSql)
{
DropDefaultConstraint(operation.Schema, operation.Table, operation.Name, builder);
}

builder.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator);
if (alterStatementNeeded)
{
builder
.Append("ALTER TABLE ")
.Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Table, operation.Schema))
.Append(" ALTER COLUMN ");

// NB: ComputedColumnSql, IsStored, DefaultValue, DefaultValueSql, Comment, ValueGenerationStrategy, and Identity are
// handled elsewhere. Don't copy them here.
var definitionOperation = new AlterColumnOperation
{
Schema = operation.Schema,
Table = operation.Table,
Name = operation.Name,
ClrType = operation.ClrType,
ColumnType = operation.ColumnType,
IsUnicode = operation.IsUnicode,
IsFixedLength = operation.IsFixedLength,
MaxLength = operation.MaxLength,
Precision = operation.Precision,
Scale = operation.Scale,
IsRowVersion = operation.IsRowVersion,
IsNullable = operation.IsNullable,
Collation = operation.Collation,
OldColumn = operation.OldColumn
};
definitionOperation.AddAnnotations(
operation.GetAnnotations().Where(
a => a.Name != SqlServerAnnotationNames.ValueGenerationStrategy
&& a.Name != SqlServerAnnotationNames.Identity));

ColumnDefinition(
operation.Schema,
operation.Table,
operation.Name,
definitionOperation,
model,
builder);

builder.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator);
}

if (operation.DefaultValue != null
|| operation.DefaultValueSql != null)
Expand Down Expand Up @@ -2002,5 +2030,24 @@ private void GenerateExecWhenIdempotent(

generate(builder);
}

private static bool HasDifferences(IEnumerable<IAnnotation> source, IEnumerable<IAnnotation> target)
{
var targetAnnotations = target.ToDictionary(a => a.Name);

var count = 0;
foreach (var sourceAnnotation in source)
{
if (!targetAnnotations.TryGetValue(sourceAnnotation.Name, out var targetAnnotation)
|| !Equals(sourceAnnotation.Value, targetAnnotation.Value))
{
return true;
}

count++;
}

return count != targetAnnotations.Count;
}
}
}
75 changes: 56 additions & 19 deletions test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -596,19 +596,46 @@ public override async Task Alter_column_add_comment()
{
await base.Alter_column_add_comment();

AssertSql(
@"DECLARE @defaultSchema AS sysname;
SET @defaultSchema = SCHEMA_NAME();
DECLARE @description AS sql_variant;
SET @description = N'Some comment';
EXEC sp_addextendedproperty 'MS_Description', @description, 'SCHEMA', @defaultSchema, 'TABLE', N'People', 'COLUMN', N'Id';");
}

[ConditionalFact]
public virtual async Task Alter_computed_column_add_comment()
{
await Test(
builder => builder.Entity("People", x =>
{
x.Property<int>("Id");
x.Property<int>("SomeColumn").HasComputedColumnSql("42");
}),
builder => { },
builder => builder.Entity("People").Property<int>("SomeColumn").HasComment("Some comment"),
model =>
{
var table = Assert.Single(model.Tables);
var column = Assert.Single(table.Columns.Where(c => c.Name == "SomeColumn"));
Assert.Equal("Some comment", column.Comment);
});

AssertSql(
@"DECLARE @var0 sysname;
SELECT @var0 = [d].[name]
FROM [sys].[default_constraints] [d]
INNER JOIN [sys].[columns] [c] ON [d].[parent_column_id] = [c].[column_id] AND [d].[parent_object_id] = [c].[object_id]
WHERE ([d].[parent_object_id] = OBJECT_ID(N'[People]') AND [c].[name] = N'Id');
WHERE ([d].[parent_object_id] = OBJECT_ID(N'[People]') AND [c].[name] = N'SomeColumn');
IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [People] DROP CONSTRAINT [' + @var0 + '];');
ALTER TABLE [People] ALTER COLUMN [Id] int NOT NULL;
ALTER TABLE [People] DROP COLUMN [SomeColumn];
ALTER TABLE [People] ADD [SomeColumn] AS 42;
DECLARE @defaultSchema AS sysname;
SET @defaultSchema = SCHEMA_NAME();
DECLARE @description AS sql_variant;
SET @description = N'Some comment';
EXEC sp_addextendedproperty 'MS_Description', @description, 'SCHEMA', @defaultSchema, 'TABLE', N'People', 'COLUMN', N'Id';");
EXEC sp_addextendedproperty 'MS_Description', @description, 'SCHEMA', @defaultSchema, 'TABLE', N'People', 'COLUMN', N'SomeColumn';");
}

[ConditionalFact]
Expand All @@ -617,14 +644,7 @@ public override async Task Alter_column_change_comment()
await base.Alter_column_change_comment();

AssertSql(
@"DECLARE @var0 sysname;
SELECT @var0 = [d].[name]
FROM [sys].[default_constraints] [d]
INNER JOIN [sys].[columns] [c] ON [d].[parent_column_id] = [c].[column_id] AND [d].[parent_object_id] = [c].[object_id]
WHERE ([d].[parent_object_id] = OBJECT_ID(N'[People]') AND [c].[name] = N'Id');
IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [People] DROP CONSTRAINT [' + @var0 + '];');
ALTER TABLE [People] ALTER COLUMN [Id] int NOT NULL;
DECLARE @defaultSchema AS sysname;
@"DECLARE @defaultSchema AS sysname;
SET @defaultSchema = SCHEMA_NAME();
DECLARE @description AS sql_variant;
EXEC sp_dropextendedproperty 'MS_Description', 'SCHEMA', @defaultSchema, 'TABLE', N'People', 'COLUMN', N'Id';
Expand All @@ -638,14 +658,7 @@ public override async Task Alter_column_remove_comment()
await base.Alter_column_remove_comment();

AssertSql(
@"DECLARE @var0 sysname;
SELECT @var0 = [d].[name]
FROM [sys].[default_constraints] [d]
INNER JOIN [sys].[columns] [c] ON [d].[parent_column_id] = [c].[column_id] AND [d].[parent_object_id] = [c].[object_id]
WHERE ([d].[parent_object_id] = OBJECT_ID(N'[People]') AND [c].[name] = N'Id');
IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [People] DROP CONSTRAINT [' + @var0 + '];');
ALTER TABLE [People] ALTER COLUMN [Id] int NOT NULL;
DECLARE @defaultSchema AS sysname;
@"DECLARE @defaultSchema AS sysname;
SET @defaultSchema = SCHEMA_NAME();
DECLARE @description AS sql_variant;
EXEC sp_dropextendedproperty 'MS_Description', 'SCHEMA', @defaultSchema, 'TABLE', N'People', 'COLUMN', N'Id';");
Expand Down Expand Up @@ -910,6 +923,30 @@ FROM [sys].[default_constraints] [d]
ALTER TABLE [People] ALTER COLUMN [IdentityColumn] bigint NOT NULL;");
}

[ConditionalFact]
public virtual async Task Alter_column_change_default()
{
await Test(
builder => builder.Entity("People").Property<string>("Name"),
builder => { },
builder => builder.Entity("People").Property<string>("Name")
.HasDefaultValue("Doe"),
model =>
{
var nameColumn = Assert.Single(Assert.Single(model.Tables).Columns);
Assert.Equal("(N'Doe')", nameColumn.DefaultValueSql);
});

AssertSql(
@"DECLARE @var0 sysname;
SELECT @var0 = [d].[name]
FROM [sys].[default_constraints] [d]
INNER JOIN [sys].[columns] [c] ON [d].[parent_column_id] = [c].[column_id] AND [d].[parent_object_id] = [c].[object_id]
WHERE ([d].[parent_object_id] = OBJECT_ID(N'[People]') AND [c].[name] = N'Name');
IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [People] DROP CONSTRAINT [' + @var0 + '];');
ALTER TABLE [People] ADD DEFAULT N'Doe' FOR [Name];");
}

public override async Task Drop_column()
{
await base.Drop_column();
Expand Down

0 comments on commit ff60c29

Please sign in to comment.