From ec2f71b1ebecf94ae5a29c045cd178b6eb00252d Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Wed, 7 Aug 2019 14:04:04 +0200 Subject: [PATCH] Second wave of fixes --- .../CSharpMigrationOperationGenerator.cs | 7 + .../Migrations/MigrationBuilder.cs | 2 +- .../Operations/Builders/ColumnsBuilder.cs | 7 +- .../SqlServerMigrationsSqlGenerator.cs | 166 +++++++++--------- .../SqlServerMigrationSqlGeneratorTest.cs | 55 +++++- 5 files changed, 153 insertions(+), 84 deletions(-) diff --git a/src/EFCore.Design/Migrations/Design/CSharpMigrationOperationGenerator.cs b/src/EFCore.Design/Migrations/Design/CSharpMigrationOperationGenerator.cs index ea2c742d859..e3cfec93bd4 100644 --- a/src/EFCore.Design/Migrations/Design/CSharpMigrationOperationGenerator.cs +++ b/src/EFCore.Design/Migrations/Design/CSharpMigrationOperationGenerator.cs @@ -1061,6 +1061,13 @@ protected virtual void Generate([NotNull] CreateTableOperation operation, [NotNu .Append(Code.UnknownLiteral(column.DefaultValue)); } + if (column.Comment != null) + { + builder + .Append(", comment: ") + .Append(Code.Literal(operation.Comment)); + } + builder.Append(")"); using (builder.Indent()) diff --git a/src/EFCore.Relational/Migrations/MigrationBuilder.cs b/src/EFCore.Relational/Migrations/MigrationBuilder.cs index f1478cd4feb..28d63a50801 100644 --- a/src/EFCore.Relational/Migrations/MigrationBuilder.cs +++ b/src/EFCore.Relational/Migrations/MigrationBuilder.cs @@ -685,7 +685,7 @@ public virtual OperationBuilder CreateCheckConst /// /// A delegate allowing constraints to be applied over the columns configured by the 'columns' delegate above. /// - /// A comment to been applied to the table. + /// A comment to be applied to the table. /// A to allow further configuration to be chained. public virtual CreateTableBuilder CreateTable( [NotNull] string name, diff --git a/src/EFCore.Relational/Migrations/Operations/Builders/ColumnsBuilder.cs b/src/EFCore.Relational/Migrations/Operations/Builders/ColumnsBuilder.cs index 9554b377f26..c0142ab04a4 100644 --- a/src/EFCore.Relational/Migrations/Operations/Builders/ColumnsBuilder.cs +++ b/src/EFCore.Relational/Migrations/Operations/Builders/ColumnsBuilder.cs @@ -44,6 +44,7 @@ public ColumnsBuilder([NotNull] CreateTableOperation createTableOperation) /// The SQL expression to use for the column's default constraint. /// The SQL expression to use to compute the column value. /// Indicates whether or not the column is constrained to fixed-length data. + /// A comment to be applied to the table. /// The same builder so that multiple calls can be chained. public virtual OperationBuilder Column( [CanBeNull] string type = null, @@ -55,7 +56,8 @@ public virtual OperationBuilder Column( [CanBeNull] object defaultValue = null, [CanBeNull] string defaultValueSql = null, [CanBeNull] string computedColumnSql = null, - bool? fixedLength = null) + bool? fixedLength = null, + [CanBeNull] string comment = null) { var operation = new AddColumnOperation { @@ -71,7 +73,8 @@ public virtual OperationBuilder Column( DefaultValue = defaultValue, DefaultValueSql = defaultValueSql, ComputedColumnSql = computedColumnSql, - IsFixedLength = fixedLength + IsFixedLength = fixedLength, + Comment = comment }; _createTableOperation.Columns.Add(operation); diff --git a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs index 83749664676..089eaccc867 100644 --- a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs +++ b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Diagnostics; using System.Globalization; using System.IO; using System.Linq; @@ -40,7 +41,6 @@ public class SqlServerMigrationsSqlGenerator : MigrationsSqlGenerator private IReadOnlyList _operations; private int _variableCounter; - private const string DefaultSchema = "dbo"; /// /// Creates a new instance. /// @@ -138,7 +138,10 @@ protected override void Generate( if (operation.Comment != null) { - GenerateColumnComment(builder, model, operation.Comment, null, operation.Schema, operation.Table, operation.Name); + GenerateComment(builder, model, operation.Comment, null, + operation.Schema, + "Table", operation.Table, + "Column", operation.Name); } builder.EndCommand(suppressTransaction: IsMemoryOptimized(operation, model, operation.Schema, operation.Table)); @@ -340,7 +343,10 @@ protected override void Generate( if (operation.OldColumn.Comment != operation.Comment) { - GenerateColumnComment(builder, model, operation.Comment, operation.OldColumn.Comment, operation.Schema, operation.Table, operation.Name); + GenerateComment(builder, model, operation.Comment, operation.OldColumn.Comment, + operation.Schema, + "Table", operation.Table, + "Column", operation.Name); } if (narrowed) @@ -479,12 +485,22 @@ protected override void Generate( if (operation.Comment != null) { - GenerateTableComment(builder, model, operation.Comment, null, operation.Schema, operation.Name); + GenerateComment(builder, model, operation.Comment, null, operation.Schema, "Table", operation.Name); } + Console.WriteLine("BEFORE"); + foreach (var c in operation.Columns) + { + Console.WriteLine($"COLUMN: {c.Name}, comment={c.Comment}"); + } foreach (var column in operation.Columns.Where(c => c.Comment != null)) { - GenerateColumnComment(builder, model, column.Comment, null, operation.Schema, operation.Name, column.Name); + Console.WriteLine("COL"); + GenerateComment(builder, model, column.Comment, null, + operation.Schema, + "Table", operation.Name, + "Column", column.Name, + firstComment: false); } builder.EndCommand(suppressTransaction: memoryOptimized); @@ -672,7 +688,7 @@ protected override void Generate(EnsureSchemaOperation operation, IModel model, Check.NotNull(operation, nameof(operation)); Check.NotNull(builder, nameof(builder)); - if (string.Equals(operation.Name, DefaultSchema, StringComparison.OrdinalIgnoreCase)) + if (string.Equals(operation.Name, "dbo", StringComparison.OrdinalIgnoreCase)) { return; } @@ -968,7 +984,7 @@ protected override void Generate(AlterTableOperation operation, IModel model, Mi if (operation.OldTable.Comment != operation.Comment) { - GenerateTableComment(builder, model, operation.Comment, operation.OldTable.Comment, operation.Schema, operation.Name); + GenerateComment(builder, model, operation.Comment, operation.OldTable.Comment, operation.Schema, "Table", operation.Name); } builder.EndCommand(suppressTransaction: IsMemoryOptimized(operation, model, operation.Schema, operation.Name)); @@ -1642,7 +1658,7 @@ protected virtual void CreateIndexes( /// /// - /// Generates add and drop commands for comments on tables. + /// Generates add and drop commands for comments on columns. /// /// /// The command builder to use to build the commands. @@ -1650,84 +1666,72 @@ protected virtual void CreateIndexes( /// The new comment to be applied. /// The previous comment. /// The schema of the table. - /// The name of the table. - protected virtual void GenerateTableComment( + /// The type of the level1 object (Table, Index). + /// The name of the table or index. + /// The type of the level2 object (Column). + /// The name of the column. + /// + /// Indicates whether this is the first comment operation being generated in this batch. + /// Only the first operation will cause the @schema variable to be declared and set. + /// + protected virtual void GenerateComment( [NotNull] MigrationCommandListBuilder builder, [CanBeNull] IModel model, [CanBeNull] string comment, [CanBeNull] string oldComment, [CanBeNull] string schema, - [NotNull] string table) + [NotNull] string level1Type, + [NotNull] string level1Name, + [CanBeNull] string level2Type = null, + [CanBeNull] string level2Name = null, + bool firstComment = true) { if (comment == oldComment) { return; } - schema ??= model?.GetDefaultSchema() ?? DefaultSchema; - - if (oldComment != null) - { - GenerateDropExtendedProperty(builder, - "Comment", - "Schema", schema, - "Table", table); - } + var stringTypeMapping = Dependencies.TypeMappingSource.GetMapping(typeof(string)); - if (comment != null) + schema ??= model?.GetDefaultSchema(); + if (schema == null) { - GenerateAddExtendedProperty(builder, - "Comment", comment, - "Schema", schema, - "Table", table); + if (firstComment) + { + builder.Append("DECLARE @schema AS nvarchar(max)") + .AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator); + builder.Append("SET @schema = SCHEMA_NAME()") + .AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator); + } + schema = "@schema"; } - } - - /// - /// - /// Generates add and drop commands for comments on columns. - /// - /// - /// The command builder to use to build the commands. - /// The target model which may be null if the operations exist without a model. - /// The new comment to be applied. - /// The previous comment. - /// The schema of the table. - /// The name of the table. - /// The name of the column. - protected virtual void GenerateColumnComment( - [NotNull] MigrationCommandListBuilder builder, - [CanBeNull] IModel model, - [CanBeNull] string comment, - [CanBeNull] string oldComment, - [CanBeNull] string schema, - [NotNull] string table, - [NotNull] string columnName) - { - if (comment == oldComment) + else { - return; + schema = Literal(schema); } - schema ??= model?.GetDefaultSchema() ?? DefaultSchema; - if (oldComment != null) { - GenerateDropExtendedProperty(builder, - "Comment", - "Schema", schema, - "Table", table, - "Column", columnName); + GenerateDropExtendedProperty( + builder, + Literal("Comment"), + Literal("Schema"), schema, + Literal(level1Type), Literal(level1Name), + level2Type == null ? null : Literal(level2Type), + level2Type == null ? null : Literal(level2Name)); } if (comment != null) { GenerateAddExtendedProperty(builder, - "Comment", comment, - "Schema", schema, - "Table", table, - "Column", columnName); + Literal("Comment"), Literal(comment), + Literal("Schema"), schema, + Literal(level1Type), Literal(level1Name), + level2Type == null ? null : Literal(level2Type), + level2Type == null ? null : Literal(level2Name)); } + + string Literal(string s) => stringTypeMapping.GenerateSqlLiteral(s); } /// @@ -1770,37 +1774,38 @@ protected virtual void GenerateAddExtendedProperty( Check.NotNull(builder, nameof(builder)); Check.NotNull(name, nameof(name)); - var stringTypeMapping = Dependencies.TypeMappingSource.GetMapping(typeof(string)); - - builder.Append("EXEC sp_addextendedproperty @name = ").Append(stringTypeMapping.GenerateSqlLiteral(name)); + builder.Append("EXEC sp_addextendedproperty @name = ").Append(name); if (value != null) { - builder.Append(", @value = ").Append(stringTypeMapping.GenerateSqlLiteral(value)); + builder.Append(", @value = ").Append(value); } if (level0Type != null) { + Debug.Assert(level0Name != null); builder .Append(", @level0type = ") - .Append(stringTypeMapping.GenerateSqlLiteral(level0Type)) + .Append(level0Type) .Append(", @level0name = ") - .Append(stringTypeMapping.GenerateSqlLiteral(level0Name)); + .Append(level0Name); if (level1Type != null) { + Debug.Assert(level1Name != null); builder .Append(", @level1type = ") - .Append(stringTypeMapping.GenerateSqlLiteral(level1Type)) + .Append(level1Type) .Append(", @level1name = ") - .Append(stringTypeMapping.GenerateSqlLiteral(level1Name)); + .Append(level1Name); if (level2Type != null) { + Debug.Assert(level2Name != null); builder .Append(", @level2type = ") - .Append(stringTypeMapping.GenerateSqlLiteral(level2Type)) + .Append(level2Type) .Append(", @level2name = ") - .Append(stringTypeMapping.GenerateSqlLiteral(level2Name)); + .Append(level2Name); } } } @@ -1843,33 +1848,34 @@ protected virtual void GenerateDropExtendedProperty( Check.NotNull(builder, nameof(builder)); Check.NotNull(name, nameof(name)); - var stringTypeMapping = Dependencies.TypeMappingSource.GetMapping(typeof(string)); - - builder.Append("EXEC sp_dropextendedproperty @name = ").Append(stringTypeMapping.GenerateSqlLiteral(name)); + builder.Append("EXEC sp_dropextendedproperty @name = ").Append(name); if (level0Type != null) { + Debug.Assert(level0Name != null); builder .Append(", @level0type = ") - .Append(stringTypeMapping.GenerateSqlLiteral(level0Type)) + .Append(level0Type) .Append(", @level0name = ") - .Append(stringTypeMapping.GenerateSqlLiteral(level0Name)); + .Append(level0Name); if (level1Type != null) { + Debug.Assert(level1Name != null); builder .Append(", @level1type = ") - .Append(stringTypeMapping.GenerateSqlLiteral(level1Type)) + .Append(level1Type) .Append(", @level1name = ") - .Append(stringTypeMapping.GenerateSqlLiteral(level1Name)); + .Append(level1Name); if (level2Type != null) { + Debug.Assert(level2Name != null); builder .Append(", @level2type = ") - .Append(stringTypeMapping.GenerateSqlLiteral(level2Type)) + .Append(level2Type) .Append(", @level2name = ") - .Append(stringTypeMapping.GenerateSqlLiteral(level2Name)); + .Append(level2Name); } } } diff --git a/test/EFCore.SqlServer.FunctionalTests/SqlServerMigrationSqlGeneratorTest.cs b/test/EFCore.SqlServer.FunctionalTests/SqlServerMigrationSqlGeneratorTest.cs index ebc19b12e2e..e205792e0c9 100644 --- a/test/EFCore.SqlServer.FunctionalTests/SqlServerMigrationSqlGeneratorTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/SqlServerMigrationSqlGeneratorTest.cs @@ -35,6 +35,38 @@ FOREIGN KEY ([EmployerId]) REFERENCES [Companies] ([Id]) "); } + [ConditionalFact] + public void CreateTableOperation_default_schema_with_comment() + { + Generate( + new CreateTableOperation + { + Name = "People", + Columns = + { + new AddColumnOperation + { + Name = "Id", + Table = "People", + ClrType = typeof(int), + IsNullable = false, + Comment = "ID comment" + }, + }, + Comment = "Table comment" + }); + + AssertSql( + @"CREATE TABLE [People] ( + [Id] int NOT NULL +); +DECLARE @schema AS nvarchar(max); +SET @schema = SCHEMA_NAME(); +EXEC sp_addextendedproperty @name = N'Comment', @value = N'Table comment', @level0type = N'Schema', @level0name = @schema, @level1type = N'Table', @level1name = N'People'; +EXEC sp_addextendedproperty @name = N'Comment', @value = N'ID comment', @level0type = N'Schema', @level0name = @schema, @level1type = N'Table', @level1name = N'People', @level2type = N'Column', @level2name = N'Id'; +"); + } + public override void CreateIndexOperation_with_filter_where_clause() { base.CreateIndexOperation_with_filter_where_clause(); @@ -325,7 +357,28 @@ public virtual void AddColumnOperation_with_comment() AssertSql( @"ALTER TABLE [People] ADD [FullName] nvarchar(max) NOT NULL; -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'FullName'; +DECLARE @schema AS nvarchar(max); +SET @schema = SCHEMA_NAME(); +EXEC sp_addextendedproperty @name = N'Comment', @value = N'My comment', @level0type = N'Schema', @level0name = @schema, @level1type = N'Table', @level1name = N'People', @level2type = N'Column', @level2name = N'FullName'; +"); + } + + [ConditionalFact] + public virtual void AddColumnOperation_with_comment_non_default_schema() + { + Generate( + new AddColumnOperation + { + Schema = "my", + Table = "People", + Name = "FullName", + ClrType = typeof(string), + Comment = "My comment" + }); + + AssertSql( + @"ALTER TABLE [my].[People] ADD [FullName] nvarchar(max) NOT NULL; +EXEC sp_addextendedproperty @name = N'Comment', @value = N'My comment', @level0type = N'Schema', @level0name = N'my', @level1type = N'Table', @level1name = N'People', @level2type = N'Column', @level2name = N'FullName'; "); }