From bd86e8f36fe80631875ac42fc83949f60ac40bd4 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Mon, 29 Jul 2019 22:09:45 +0200 Subject: [PATCH] Refactor comment support * Removed comment-related migration generation APIs from relational, implementation is now only in SqlServerMigrationsSqlGenerator. * Made extended property support more general so we can use them for other purposes if needed. Fixes #16799 Fixes #16800 --- .../Migrations/MigrationsSqlGenerator.cs | 46 ---- .../SqlServerMigrationsSqlGenerator.cs | 246 ++++++++++++------ .../SqlServerMigrationSqlGeneratorTest.cs | 26 +- 3 files changed, 177 insertions(+), 141 deletions(-) diff --git a/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs b/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs index aefe3b6ca3d..4f24fbe421b 100644 --- a/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs +++ b/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs @@ -177,13 +177,6 @@ protected virtual void Generate( ColumnDefinition(operation, model, builder); - if (operation.Comment != null) - { - builder.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator); - - GenerateComment(operation, model, builder, operation.Comment, operation.Schema, operation.Table, operation.Name); - } - if (terminate) { builder.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator); @@ -568,16 +561,6 @@ protected virtual void Generate( builder.Append(")"); - if (operation.Comment != null) - { - GenerateComment(operation, model, builder, operation.Comment, operation.Schema, operation.Name); - } - - foreach (var column in operation.Columns.Where(c => c.Comment != null)) - { - GenerateComment(operation, model, builder, column.Comment, operation.Schema, operation.Name, column.Name); - } - if (terminate) { builder.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator); @@ -1633,35 +1616,6 @@ protected virtual void EndStatement( builder.EndCommand(suppressTransaction); } - /// - /// - /// Can be overridden by database providers to build commands for applying comments to tables and columns - /// by making calls on the given . - /// - /// - /// Note that the default implementation of this method does nothing because there is no common metadata - /// relating to this operation. Providers only need to override this method if they have some provider-specific - /// annotations that must be handled. - /// - /// - /// The operation. - /// The target model which may be null if the operations exist without a model. - /// The command builder to use to build the commands. - /// The comment to be applied. - /// The schema of the table. - /// The name of the table. - /// The column name if comment is being applied to a column. - protected virtual void GenerateComment( - [NotNull] MigrationOperation operation, - [CanBeNull] IModel model, - [NotNull] MigrationCommandListBuilder builder, - [NotNull] string comment, - [NotNull] string schema, - [NotNull] string table, - [CanBeNull] string columnName = null) - { - } - /// /// Concatenates the given column names into a /// separated list. diff --git a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs index 2c37b591f78..15a0eac4552 100644 --- a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs +++ b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs @@ -117,6 +117,10 @@ protected override void Generate( MigrationCommandListBuilder builder, bool terminate) { + if (!terminate && operation.Comment != null) + { + throw new ArgumentException($"When generating migrations SQL for {nameof(AddColumnOperation)}, can't produce unterminated SQL with comments"); + } var valueGenerationStrategy = operation[ SqlServerAnnotationNames.ValueGenerationStrategy] as SqlServerValueGenerationStrategy?; @@ -132,9 +136,14 @@ protected override void Generate( if (terminate) { - builder - .AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator) - .EndCommand(suppressTransaction: IsMemoryOptimized(operation, model, operation.Schema, operation.Table)); + builder.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator); + + if (operation.Comment != null) + { + AddDropComment(builder, operation.Comment, null, operation.Schema, operation.Table, operation.Name); + } + + builder.EndCommand(suppressTransaction: IsMemoryOptimized(operation, model, operation.Schema, operation.Table)); } } @@ -337,14 +346,7 @@ protected override void Generate( if (operation.OldColumn.Comment != operation.Comment) { - if (operation.OldColumn.Comment != null) - { - EndStatement(builder); - - GenerateDropExtendedProperty(builder, model, "Comment", operation.Schema, operation.Table, operation.Name); - } - - GenerateComment(operation, model, builder, operation.Comment, operation.Schema, operation.Table, operation.Name); + AddDropComment(builder, operation.Comment, operation.OldColumn.Comment, operation.Schema, operation.Table, operation.Name); } if (narrowed) @@ -458,6 +460,11 @@ protected override void Generate( MigrationCommandListBuilder builder, bool terminate = true) { + if (!terminate && operation.Comment != null) + { + throw new ArgumentException($"When generating migrations SQL for {nameof(CreateTableOperation)}, can't produce unterminated SQL with comments"); + } + base.Generate(operation, model, builder, terminate: false); var memoryOptimized = IsMemoryOptimized(operation); @@ -474,12 +481,19 @@ protected override void Generate( } } - if (terminate) + builder.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator); + + if (operation.Comment != null) { - builder - .AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator) - .EndCommand(suppressTransaction: memoryOptimized); + AddDropComment(builder, operation.Comment, null, operation.Schema, operation.Name); + } + + foreach (var column in operation.Columns.Where(c => c.Comment != null)) + { + AddDropComment(builder, column.Comment, null, operation.Schema, operation.Name, column.Name); } + + builder.EndCommand(suppressTransaction: memoryOptimized); } /// @@ -1631,117 +1645,193 @@ protected virtual void CreateIndexes( } /// - /// Generates SQL to create comment extended properties on table and columns. + /// + /// Generates add and drop commands for comments on tables and columns. + /// /// - /// The operation. - /// The target model which may be null if the operations exist without a model. /// The command builder to use to build the commands. - /// The comment to be applied. + /// The new comment to be applied. + /// The previous comment. /// The schema of the table. /// The name of the table. /// The column name if comment is being applied to a column. - protected override void GenerateComment( - MigrationOperation operation, - IModel model, - MigrationCommandListBuilder builder, - string comment, - string schema, - string table, - string columnName = null) + protected virtual void AddDropComment( + [NotNull] MigrationCommandListBuilder builder, + [CanBeNull] string comment, + [CanBeNull] string oldComment, + [NotNull] string schema, + [NotNull] string table, + [CanBeNull] string columnName = null) { - Check.NotNull(operation, nameof(operation)); - Check.NotNull(builder, nameof(builder)); - Check.NotNull(table, nameof(table)); + if (comment == oldComment) + { + return; + } - if (comment != null) + if (oldComment != null) { - EndStatement(builder); + GenerateDropExtendedProperty(builder, + "Comment", + "Schema", schema, + "Table", table, + columnName == null ? null : "Column", columnName); + } - GenerateCreateExtendedProperty(builder, model, "Comment", comment, schema, table, columnName); + if (comment != null) + { + GenerateAddExtendedProperty(builder, + "Comment", comment, + "Schema", schema, + "Table", table, + columnName == null ? null : "Column", columnName); } } /// - /// Generates SQL to create a extended property on table and columns. + /// Generates SQL to create a extended property. /// + /// + /// See https://docs.microsoft.com/en-us/sql/relational-databases/system-stored-procedures/sp-addextendedproperty-transact-sql?view=sql-server-2017 + /// /// The command builder to use to build the commands. - /// The target model which may be null if the operations exist without a model. /// The name of the extended property. /// The value of the extended property. - /// The schema of the table. - /// The name of the table. - /// The column name if comment is being applied to a column. - protected virtual void GenerateCreateExtendedProperty( + /// + /// The type of level 0 object. + /// Valid inputs are ASSEMBLY, CONTRACT, EVENT NOTIFICATION, FILEGROUP, MESSAGE TYPE, PARTITION FUNCTION, PARTITION SCHEME, + /// REMOTE SERVICE BINDING, ROUTE, SCHEMA, SERVICE, USER, TRIGGER, TYPE, PLAN GUIDE, and NULL. + /// + /// The name of the level 0 object type specified. + /// + /// The type of level 1 object. + /// Valid inputs are AGGREGATE, DEFAULT, FUNCTION, LOGICAL FILE NAME, PROCEDURE, QUEUE, RULE, SEQUENCE, SYNONYM, TABLE, + /// TABLE_TYPE, TYPE, VIEW, XML SCHEMA COLLECTION, and NULL. + /// + /// The name of the level 0 object type specified. + /// + /// The type of level 2 object. + /// Valid inputs are COLUMN, CONSTRAINT, EVENT NOTIFICATION, INDEX, PARAMETER, TRIGGER, and NULL. + /// + /// The name of the level 2 object type specified. + protected virtual void GenerateAddExtendedProperty( [NotNull] MigrationCommandListBuilder builder, - [CanBeNull] IModel model, [NotNull] string name, - [NotNull] string value, - [CanBeNull] string schema, - [NotNull] string table, - [CanBeNull] string columnName = null) + [CanBeNull] string value, + [CanBeNull] string level0Type = null, + [CanBeNull] string level0Name = null, + [CanBeNull] string level1Type = null, + [CanBeNull] string level1Name = null, + [CanBeNull] string level2Type = null, + [CanBeNull] string level2Name = null) { Check.NotNull(builder, nameof(builder)); Check.NotNull(name, nameof(name)); - Check.NotNull(value, nameof(value)); - Check.NotNull(table, nameof(table)); var stringTypeMapping = Dependencies.TypeMappingSource.GetMapping(typeof(string)); - builder - .Append("EXEC sp_addextendedproperty @name = ") - .Append(stringTypeMapping.GenerateSqlLiteral(name)) - .Append(", @value = ") - .Append(stringTypeMapping.GenerateSqlLiteral(value)) - .Append(", @level0type = N'Schema', @level0name = ") - .Append(stringTypeMapping.GenerateSqlLiteral(schema ?? model?.GetDefaultSchema())) - .Append(", @level1type = N'Table', @level1name = ") - .Append(stringTypeMapping.GenerateSqlLiteral(table)); + builder.Append("EXEC sp_addextendedproperty @name = ").Append(stringTypeMapping.GenerateSqlLiteral(name)); + if (value != null) + { + builder.Append(", @value = ").Append(stringTypeMapping.GenerateSqlLiteral(value)); + } - if (columnName != null) + if (level0Type != null) { builder - .Append(", @level2type = N'Column', @level2name = ") - .Append(stringTypeMapping.GenerateSqlLiteral(columnName)); + .Append(", @level0type = ") + .Append(stringTypeMapping.GenerateSqlLiteral(level0Type)) + .Append(", @level0name = ") + .Append(stringTypeMapping.GenerateSqlLiteral(level0Name)); + + if (level1Type != null) + { + builder + .Append(", @level1type = ") + .Append(stringTypeMapping.GenerateSqlLiteral(level1Type)) + .Append(", @level1name = ") + .Append(stringTypeMapping.GenerateSqlLiteral(level1Name)); + + if (level2Type != null) + { + builder + .Append(", @level2type = ") + .Append(stringTypeMapping.GenerateSqlLiteral(level2Type)) + .Append(", @level2name = ") + .Append(stringTypeMapping.GenerateSqlLiteral(level2Name)); + } + } } + + builder.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator); } /// - /// Generates SQL to drop a extended property on table and columns. + /// Generates SQL to drop a extended property. /// /// The command builder to use to build the commands. - /// The target model which may be null if the operations exist without a model. /// The name of the extended property. - /// The schema of the table. - /// The name of the table. - /// The column name if comment is being applied to a column. + /// + /// The type of level 0 object. + /// Valid inputs are ASSEMBLY, CONTRACT, EVENT NOTIFICATION, FILEGROUP, MESSAGE TYPE, PARTITION FUNCTION, PARTITION SCHEME, + /// REMOTE SERVICE BINDING, ROUTE, SCHEMA, SERVICE, USER, TRIGGER, TYPE, PLAN GUIDE, and NULL. + /// + /// The name of the level 0 object type specified. + /// + /// The type of level 1 object. + /// Valid inputs are AGGREGATE, DEFAULT, FUNCTION, LOGICAL FILE NAME, PROCEDURE, QUEUE, RULE, SEQUENCE, SYNONYM, TABLE, + /// TABLE_TYPE, TYPE, VIEW, XML SCHEMA COLLECTION, and NULL. + /// + /// The name of the level 0 object type specified. + /// + /// The type of level 2 object. + /// Valid inputs are COLUMN, CONSTRAINT, EVENT NOTIFICATION, INDEX, PARAMETER, TRIGGER, and NULL. + /// + /// The name of the level 2 object type specified. protected virtual void GenerateDropExtendedProperty( [NotNull] MigrationCommandListBuilder builder, - [CanBeNull] IModel model, [NotNull] string name, - [CanBeNull] string schema, - [NotNull] string table, - [CanBeNull] string columnName = null) + [CanBeNull] string level0Type = null, + [CanBeNull] string level0Name = null, + [CanBeNull] string level1Type = null, + [CanBeNull] string level1Name = null, + [CanBeNull] string level2Type = null, + [CanBeNull] string level2Name = null) { Check.NotNull(builder, nameof(builder)); Check.NotNull(name, nameof(name)); - Check.NotNull(table, nameof(table)); var stringTypeMapping = Dependencies.TypeMappingSource.GetMapping(typeof(string)); - builder - .Append("EXEC sp_dropextendedproperty @name = ") - .Append(stringTypeMapping.GenerateSqlLiteral(name)) - .Append(", @level0type = N'Schema', @level0name = ") - .Append(stringTypeMapping.GenerateSqlLiteral(schema ?? model?.GetDefaultSchema())) - .Append(", @level1type = N'Table', @level1name = ") - .Append(stringTypeMapping.GenerateSqlLiteral(table)); + builder.Append("EXEC sp_dropextendedproperty @name = ").Append(stringTypeMapping.GenerateSqlLiteral(name)); - if (columnName != null) + if (level0Type != null) { builder - .Append(", @level2type = N'Column', @level2name = ") - .Append(stringTypeMapping.GenerateSqlLiteral(columnName)); + .Append(", @level0type = ") + .Append(stringTypeMapping.GenerateSqlLiteral(level0Type)) + .Append(", @level0name = ") + .Append(stringTypeMapping.GenerateSqlLiteral(level0Name)); + + if (level1Type != null) + { + builder + .Append(", @level1type = ") + .Append(stringTypeMapping.GenerateSqlLiteral(level1Type)) + .Append(", @level1name = ") + .Append(stringTypeMapping.GenerateSqlLiteral(level1Name)); + + if (level2Type != null) + { + builder + .Append(", @level2type = ") + .Append(stringTypeMapping.GenerateSqlLiteral(level2Type)) + .Append(", @level2name = ") + .Append(stringTypeMapping.GenerateSqlLiteral(level2Name)); + } + } } + + builder.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator); } /// diff --git a/test/EFCore.SqlServer.FunctionalTests/SqlServerMigrationSqlGeneratorTest.cs b/test/EFCore.SqlServer.FunctionalTests/SqlServerMigrationSqlGeneratorTest.cs index 3ba05e82705..ab56f1ef700 100644 --- a/test/EFCore.SqlServer.FunctionalTests/SqlServerMigrationSqlGeneratorTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/SqlServerMigrationSqlGeneratorTest.cs @@ -30,10 +30,8 @@ PRIMARY KEY ([Id]), UNIQUE ([SSN]), CHECK (SSN > 0), FOREIGN KEY ([EmployerId]) REFERENCES [Companies] ([Id]) -)GO - -EXEC sp_addextendedproperty @name = N'Comment', @value = N'Table comment', @level0type = N'Schema', @level0name = N'dbo', @level1type = N'Table', @level1name = N'People'GO - +); +EXEC sp_addextendedproperty @name = N'Comment', @value = N'Table comment', @level0type = N'Schema', @level0name = N'dbo', @level1type = N'Table', @level1name = N'People'; EXEC sp_addextendedproperty @name = N'Comment', @value = N'Employer ID comment', @level0type = N'Schema', @level0name = N'dbo', @level1type = N'Table', @level1name = N'People', @level2type = N'Column', @level2name = N'EmployerId'; "); } @@ -242,8 +240,6 @@ public virtual void AddColumnOperation_with_comment() AssertSql( @"ALTER TABLE [People] ADD [FullName] nvarchar(max) NOT NULL; -GO - EXEC sp_addextendedproperty @name = N'Comment', @value = N'My comment', @level0type = N'Schema', @level0name = NULL, @level1type = N'Table', @level1name = N'People', @level2type = N'Column', @level2name = N'FullName'; "); } @@ -874,9 +870,8 @@ FROM [sys].[default_constraints] [d] WHERE ([d].[parent_object_id] = OBJECT_ID(N'[dbo].[People]') AND [c].[name] = N'LuckyNumber'); IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [dbo].[People] DROP CONSTRAINT [' + @var0 + '];'); ALTER TABLE [dbo].[People] ALTER COLUMN [LuckyNumber] int NOT NULL; -GO - -EXEC sp_addextendedproperty @name = N'Comment', @value = N'My Comment', @level0type = N'Schema', @level0name = N'dbo', @level1type = N'Table', @level1name = N'People', @level2type = N'Column', @level2name = N'LuckyNumber'"); +EXEC sp_addextendedproperty @name = N'Comment', @value = N'My Comment', @level0type = N'Schema', @level0name = N'dbo', @level1type = N'Table', @level1name = N'People', @level2type = N'Column', @level2name = N'LuckyNumber'; +"); } [ConditionalFact] @@ -914,11 +909,9 @@ FROM [sys].[default_constraints] [d] WHERE ([d].[parent_object_id] = OBJECT_ID(N'[dbo].[People]') AND [c].[name] = N'Name'); IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [dbo].[People] DROP CONSTRAINT [' + @var0 + '];'); ALTER TABLE [dbo].[People] ALTER COLUMN [Name] nvarchar(max) NOT NULL; -GO - -EXEC sp_dropextendedproperty @name = N'Comment', @level0type = N'Schema', @level0name = N'dbo', @level1type = N'Table', @level1name = N'People', @level2type = N'Column', @level2name = N'Name'GO - -EXEC sp_addextendedproperty @name = N'Comment', @value = N'My Comment 2', @level0type = N'Schema', @level0name = N'dbo', @level1type = N'Table', @level1name = N'People', @level2type = N'Column', @level2name = N'Name'"); +EXEC sp_dropextendedproperty @name = N'Comment', @level0type = N'Schema', @level0name = N'dbo', @level1type = N'Table', @level1name = N'People', @level2type = N'Column', @level2name = N'Name'; +EXEC sp_addextendedproperty @name = N'Comment', @value = N'My Comment 2', @level0type = N'Schema', @level0name = N'dbo', @level1type = N'Table', @level1name = N'People', @level2type = N'Column', @level2name = N'Name'; +"); } [ConditionalFact] @@ -955,9 +948,8 @@ FROM [sys].[default_constraints] [d] WHERE ([d].[parent_object_id] = OBJECT_ID(N'[dbo].[People]') AND [c].[name] = N'Name'); IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [dbo].[People] DROP CONSTRAINT [' + @var0 + '];'); ALTER TABLE [dbo].[People] ALTER COLUMN [Name] nvarchar(max) NOT NULL; -GO - -EXEC sp_dropextendedproperty @name = N'Comment', @level0type = N'Schema', @level0name = N'dbo', @level1type = N'Table', @level1name = N'People', @level2type = N'Column', @level2name = N'Name'"); +EXEC sp_dropextendedproperty @name = N'Comment', @level0type = N'Schema', @level0name = N'dbo', @level1type = N'Table', @level1name = N'People', @level2type = N'Column', @level2name = N'Name'; +"); } [ConditionalFact]