From e167e910362c3456b487d873120b46d78191f82e Mon Sep 17 00:00:00 2001 From: Brice Lambson Date: Thu, 23 Jul 2020 15:45:04 -0700 Subject: [PATCH] Migrations: Fix some bugs Changes: - Synthesize DEFAULT value when column altered to NOT NULL (fixes #19882) - SQL Server: Rebuild indexes when column collation is altered (fixes #21547) --- .../Internal/MigrationsModelDiffer.cs | 2 +- .../SqlServerMigrationsSqlGenerator.cs | 4 +- .../Internal/MigrationsModelDifferTest.cs | 42 +++++++++++++++++ .../MigrationsSqlServerTest.cs | 47 +++++++++++++++---- 4 files changed, 83 insertions(+), 12 deletions(-) diff --git a/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs b/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs index 3dad0821c41..0fd3fb548b1 100644 --- a/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs +++ b/src/EFCore.Relational/Migrations/Internal/MigrationsModelDiffer.cs @@ -1011,7 +1011,7 @@ protected virtual IEnumerable Diff( Initialize( alterColumnOperation, target, targetTypeMapping, - target.IsNullable, targetMigrationsAnnotations, inline: true); + target.IsNullable, targetMigrationsAnnotations, inline: !source.IsNullable); Initialize( alterColumnOperation.OldColumn, source, sourceTypeMapping, diff --git a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs index 8a9c7b415ae..3c541735837 100644 --- a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs +++ b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs @@ -283,7 +283,9 @@ protected override void Generate( operation.Name, operation.OldColumn, model); - narrowed = type != oldType || !operation.IsNullable && operation.OldColumn.IsNullable; + narrowed = type != oldType + || operation.Collation != operation.OldColumn.Collation + || !operation.IsNullable && operation.OldColumn.IsNullable; } if (narrowed) diff --git a/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs b/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs index a9f543fe9fd..011b3824bcd 100644 --- a/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs +++ b/test/EFCore.Relational.Tests/Migrations/Internal/MigrationsModelDifferTest.cs @@ -1912,6 +1912,48 @@ public void Alter_column_nullability() }); } + [ConditionalFact] + public void Alter_column_nullability_to_required() + { + Execute( + source => source.Entity( + "Bison", + x => + { + x.ToTable("Bison", "dbo"); + x.Property("Id"); + x.Property("Name") + .HasColumnType("nvarchar(30)") + .IsRequired(false) + .HasDefaultValueSql("CreateBisonName()"); + }), + target => target.Entity( + "Bison", + x => + { + x.ToTable("Bison", "dbo"); + x.Property("Id"); + x.Property("Name") + .HasColumnType("nvarchar(30)") + .IsRequired() + .HasDefaultValueSql("CreateBisonName()"); + }), + operations => + { + Assert.Equal(1, operations.Count); + + var operation = Assert.IsType(operations[0]); + Assert.Equal("dbo", operation.Schema); + Assert.Equal("Bison", operation.Table); + Assert.Equal("Name", operation.Name); + Assert.Equal(typeof(string), operation.ClrType); + Assert.Equal("nvarchar(30)", operation.ColumnType); + Assert.False(operation.IsNullable); + Assert.Equal(string.Empty, operation.DefaultValue); + Assert.Equal("CreateBisonName()", operation.DefaultValueSql); + }); + } + [ConditionalFact] public void Alter_column_type() { diff --git a/test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs index 4714ecdc45e..db65d1fd6b2 100644 --- a/test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs @@ -502,7 +502,8 @@ 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'SomeColumn'); IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [People] DROP CONSTRAINT [' + @var0 + '];'); -ALTER TABLE [People] ALTER COLUMN [SomeColumn] nvarchar(max) NOT NULL;"); +ALTER TABLE [People] ALTER COLUMN [SomeColumn] nvarchar(max) NOT NULL; +ALTER TABLE [People] ADD DEFAULT N'' FOR [SomeColumn];"); } [ConditionalFact] @@ -519,6 +520,7 @@ FROM [sys].[default_constraints] [d] 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 [SomeColumn] nvarchar(450) NOT NULL; +ALTER TABLE [People] ADD DEFAULT N'' FOR [SomeColumn]; CREATE INDEX [IX_People_SomeColumn] ON [People] ([SomeColumn]);"); } @@ -536,6 +538,7 @@ FROM [sys].[default_constraints] [d] WHERE ([d].[parent_object_id] = OBJECT_ID(N'[People]') AND [c].[name] = N'FirstName'); IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [People] DROP CONSTRAINT [' + @var0 + '];'); ALTER TABLE [People] ALTER COLUMN [FirstName] nvarchar(450) NOT NULL; +ALTER TABLE [People] ADD DEFAULT N'' FOR [FirstName]; CREATE INDEX [IX_People_FirstName_LastName] ON [People] ([FirstName], [LastName]);"); } @@ -661,6 +664,37 @@ FROM [sys].[default_constraints] [d] ALTER TABLE [People] ALTER COLUMN [Name] nvarchar(max) COLLATE German_PhoneBook_CI_AS NULL;"); } + [ConditionalFact] + public virtual async Task Alter_column_set_collation_with_index() + { + await Test( + builder => builder.Entity( + "People", e => + { + e.Property("Name"); + e.HasIndex("Name"); + }), + builder => { }, + builder => builder.Entity("People").Property("Name") + .UseCollation(NonDefaultCollation), + model => + { + var nameColumn = Assert.Single(Assert.Single(model.Tables).Columns); + Assert.Equal(NonDefaultCollation, nameColumn.Collation); + }); + + AssertSql( + @"DROP INDEX [IX_People_Name] ON [People]; +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] ALTER COLUMN [Name] nvarchar(450) COLLATE German_PhoneBook_CI_AS NULL; +CREATE INDEX [IX_People_Name] ON [People] ([Name]);"); + } + [ConditionalFact] public override async Task Alter_column_reset_collation() { @@ -711,6 +745,7 @@ FROM [sys].[default_constraints] [d] 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 [SomeColumn] nvarchar(450) NOT NULL; +ALTER TABLE [People] ADD DEFAULT N'' FOR [SomeColumn]; CREATE INDEX [IX_People_SomeColumn] ON [People] ([SomeColumn]) INCLUDE ([SomeOtherColumn]);"); } @@ -1470,7 +1505,7 @@ public override async Task Add_primary_key_composite_with_name() public virtual async Task Add_primary_key_nonclustered() { await Test( - builder => builder.Entity("People").Property("SomeField"), + builder => builder.Entity("People").Property("SomeField").IsRequired().HasMaxLength(450), builder => { }, builder => builder.Entity("People").HasKey("SomeField").IsClustered(false), model => @@ -1482,14 +1517,6 @@ await Test( }); 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'SomeField'); -IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [People] DROP CONSTRAINT [' + @var0 + '];'); -ALTER TABLE [People] ALTER COLUMN [SomeField] nvarchar(450) NOT NULL;", - // @"ALTER TABLE [People] ADD CONSTRAINT [PK_People] PRIMARY KEY NONCLUSTERED ([SomeField]);"); }