New EF Core comment support

Closes #892

src/EFCore.PG/Design/Internal/NpgsqlAnnotationCodeGenerator.cs

@@ -1,12 +1,10 @@
 using System;
 using System.Diagnostics;
-using System.Linq;
 using JetBrains.Annotations;
 using Microsoft.EntityFrameworkCore;
 using Microsoft.EntityFrameworkCore.Design;
 using Microsoft.EntityFrameworkCore.Infrastructure;
 using Microsoft.EntityFrameworkCore.Metadata;
-using Npgsql.EntityFrameworkCore.PostgreSQL.Infrastructure.Internal;
 using Npgsql.EntityFrameworkCore.PostgreSQL.Metadata;
 using Npgsql.EntityFrameworkCore.PostgreSQL.Metadata.Internal;
 using Npgsql.EntityFrameworkCore.PostgreSQL.Utilities;
@@ -129,9 +127,6 @@ public override MethodCallCodeFragment GenerateFluentApi(IEntityType entityType,
             Check.NotNull(entityType, nameof(entityType));
             Check.NotNull(annotation, nameof(annotation));
 
-            if (annotation.Name == NpgsqlAnnotationNames.Comment)
-                return new MethodCallCodeFragment(nameof(NpgsqlEntityTypeBuilderExtensions.HasComment), annotation.Value);
-
             if (annotation.Name == NpgsqlAnnotationNames.UnloggedTable)
                 return new MethodCallCodeFragment(nameof(NpgsqlEntityTypeBuilderExtensions.IsUnlogged), annotation.Value);
 
@@ -164,9 +159,6 @@ public override MethodCallCodeFragment GenerateFluentApi(IProperty property, IAn
                     identityOptions.MaxValue,
                     identityOptions.IsCyclic ? true : (bool?)null,
                     identityOptions.NumbersToCache == 1 ? null : (long?)identityOptions.NumbersToCache);
-
-            case NpgsqlAnnotationNames.Comment:
-                return new MethodCallCodeFragment(nameof(NpgsqlPropertyBuilderExtensions.HasComment), annotation.Value);
             }
 
             return null;

src/EFCore.PG/Extensions/NpgsqlEntityTypeBuilderExtensions.cs

@@ -130,41 +130,6 @@ public static bool CanSetStorageParameter(
 
         #endregion Storage parameters
 
-        #region Comment
-
-        /// <summary>
-        ///     Configures the comment set on the table when targeting Npgsql.
-        /// </summary>
-        /// <param name="entityTypeBuilder"> The builder for the entity type being configured. </param>
-        /// <param name="comment"> The name of the table. </param>
-        /// <returns> The same builder instance so that multiple calls can be chained. </returns>
-        public static EntityTypeBuilder HasComment(
-            [NotNull] this EntityTypeBuilder entityTypeBuilder,
-            [CanBeNull] string comment)
-        {
-            Check.NotNull(entityTypeBuilder, nameof(entityTypeBuilder));
-            Check.NullButNotEmpty(comment, nameof(comment));
-
-            NpgsqlEntityTypeExtensions.SetComment(entityTypeBuilder.Metadata, comment);
-
-            return entityTypeBuilder;
-        }
-
-        /// <summary>
-        ///     Configures the comment set on the table when targeting Npgsql.
-        /// </summary>
-        /// <typeparam name="TEntity"> The entity type being configured. </typeparam>
-        /// <param name="entityTypeBuilder"> The builder for the entity type being configured. </param>
-        /// <param name="comment"> The name of the table. </param>
-        /// <returns> The same builder instance so that multiple calls can be chained. </returns>
-        public static EntityTypeBuilder<TEntity> HasComment<TEntity>(
-            [NotNull] this EntityTypeBuilder<TEntity> entityTypeBuilder,
-            [CanBeNull] string comment)
-            where TEntity : class
-        => (EntityTypeBuilder<TEntity>)HasComment((EntityTypeBuilder)entityTypeBuilder, comment);
-
-        #endregion Comment
-
         #region Unlogged Table
 
         /// <summary>
@@ -291,6 +256,40 @@ public static EntityTypeBuilder<TEntity> UseCockroachDbInterleaveInParent<TEntit
 
         #region Obsolete
 
+
+        /// <summary>
+        /// Configures a comment to be applied on the table.
+        /// </summary>
+        /// <param name="entityTypeBuilder">The builder for the entity type being configured.</param>
+        /// <param name="comment">The comment for the table.</param>
+        /// <returns>The same builder instance so that multiple calls can be chained.</returns>
+        [Obsolete("Use HasComment")]
+        public static EntityTypeBuilder ForNpgsqlHasComment(
+            [NotNull] this EntityTypeBuilder entityTypeBuilder,
+            [CanBeNull] string comment)
+        {
+            Check.NotNull(entityTypeBuilder, nameof(entityTypeBuilder));
+            Check.NullButNotEmpty(comment, nameof(comment));
+
+            entityTypeBuilder.HasComment(comment);
+
+            return entityTypeBuilder;
+        }
+
+        /// <summary>
+        /// Configures a comment to be applied on the table.
+        /// </summary>
+        /// <typeparam name="TEntity">The entity type being configured.</typeparam>
+        /// <param name="entityTypeBuilder">The builder for the entity type being configured.</param>
+        /// <param name="comment">The comment for the table.</param>
+        /// <returns>The same builder instance so that multiple calls can be chained.</returns>
+        [Obsolete("Use HasComment")]
+        public static EntityTypeBuilder<TEntity> ForNpgsqlHasComment<TEntity>(
+            [NotNull] this EntityTypeBuilder<TEntity> entityTypeBuilder,
+            [CanBeNull] string comment)
+            where TEntity : class
+            => (EntityTypeBuilder<TEntity>)ForNpgsqlHasComment((EntityTypeBuilder)entityTypeBuilder, comment);
+
         /// <summary>
         /// Configures using the auto-updating system column <c>xmin</c> as the optimistic concurrency token.
         /// </summary>

src/EFCore.PG/Extensions/NpgsqlEntityTypeExtensions.cs

@@ -30,12 +30,6 @@ public static void SetStorageParameter([NotNull] this IMutableEntityType entityT
         public static void SetStorageParameter([NotNull] this IConventionEntityType entityType, string parameterName, object parameterValue, bool fromDataAnnotation = false)
             => entityType.SetOrRemoveAnnotation(NpgsqlAnnotationNames.StorageParameterPrefix + parameterName, parameterValue, fromDataAnnotation);
 
-        public static string GetComment([NotNull] this IEntityType entityType)
-            => (string)entityType[NpgsqlAnnotationNames.Comment];
-
-        public static void SetComment([NotNull] this IMutableEntityType entityType, [CanBeNull] string comment)
-            => entityType.SetOrRemoveAnnotation(NpgsqlAnnotationNames.Comment, comment);
-
         public static bool GetIsUnlogged([NotNull] this IEntityType entityType)
             => entityType[NpgsqlAnnotationNames.UnloggedTable] as bool? ?? false;
 

src/EFCore.PG/Extensions/NpgsqlPropertyBuilderExtensions.cs

@@ -612,40 +612,40 @@ public static bool CanSetIdentityOptions(
 
         #endregion Identity options
 
-        #region Comment
+        #region Obsolete
+
 
         /// <summary>
-        /// Configures the comment set on the column when targeting Npgsql.
+        /// Configures a comment to be applied to the column.
         /// </summary>
-        /// <param name="propertyBuilder"> The builder for the property being configured.</param>
-        /// <param name="comment"> The comment of the column.</param>
+        /// <param name="propertyBuilder">The builder for the property being configured.</param>
+        /// <param name="comment">The comment for the column.</param>
         /// <returns>The same builder instance so that multiple calls can be chained.</returns>
-        public static PropertyBuilder HasComment(
+        [Obsolete("Use HasComment")]
+        public static PropertyBuilder ForNpgsqlHasComment(
             [NotNull] this PropertyBuilder propertyBuilder,
             [CanBeNull] string comment)
         {
             Check.NotNull(propertyBuilder, nameof(propertyBuilder));
             Check.NullButNotEmpty(comment, nameof(comment));
 
-            NpgsqlPropertyExtensions.SetComment(propertyBuilder.Metadata, comment);
+            propertyBuilder.HasComment(comment);
 
             return propertyBuilder;
         }
 
         /// <summary>
-        /// Configures the comment set on the column when targeting Npgsql.
+        /// Configures a comment to be applied to the column.
         /// </summary>
-        /// <param name="propertyBuilder"> The builder for the property being configured.</param>
-        /// <param name="comment"> The comment of the column.</param>
+        /// <typeparam name="TEntity">The entity type being configured.</typeparam>
+        /// <param name="propertyBuilder">The builder for the property being configured.</param>
+        /// <param name="comment">The comment for the column.</param>
         /// <returns>The same builder instance so that multiple calls can be chained.</returns>
-        public static PropertyBuilder<TEntity> HasComment<TEntity>(
+        [Obsolete("Use HasComment")]
+        public static PropertyBuilder<TEntity> ForNpgsqlHasComment<TEntity>(
             [NotNull] this PropertyBuilder<TEntity> propertyBuilder,
             [CanBeNull] string comment)
-        => (PropertyBuilder<TEntity>)HasComment((PropertyBuilder)propertyBuilder, comment);
-
-        #endregion Comment
-
-        #region Obsolete
+            => (PropertyBuilder<TEntity>)ForNpgsqlHasComment((PropertyBuilder)propertyBuilder, comment);
 
         /// <summary>
         /// Configures the property to use a sequence-based hi-lo pattern to generate values for new entities,

src/EFCore.PG/Extensions/NpgsqlPropertyExtensions.cs

@@ -477,15 +477,5 @@ public static void RemoveIdentityOptions([NotNull] this IConventionProperty prop
             => property.RemoveAnnotation(NpgsqlAnnotationNames.IdentityOptions);
 
         #endregion Identity sequence options
-
-        #region Comment
-
-        public static string GetComment([NotNull] this IProperty property)
-            => (string)property[NpgsqlAnnotationNames.Comment];
-
-        public static void SetComment([NotNull] this IMutableProperty property, [CanBeNull] string comment)
-            => property.SetOrRemoveAnnotation(NpgsqlAnnotationNames.Comment, comment);
-
-        #endregion Comment
     }
 }

src/EFCore.PG/Metadata/Internal/NpgsqlAnnotationNames.cs

@@ -22,7 +22,6 @@ public static class NpgsqlAnnotationNames
         public const string DatabaseTemplate = Prefix + "DatabaseTemplate";
         public const string Tablespace = Prefix + "Tablespace";
         public const string StorageParameterPrefix = Prefix + "StorageParameter:";
-        public const string Comment = Prefix + "Comment";
         public const string UnloggedTable = Prefix + "UnloggedTable";
         public const string IdentityOptions = Prefix + "IdentitySequenceOptions";
 
@@ -47,5 +46,8 @@ public static class NpgsqlAnnotationNames
 
         [Obsolete("Replaced by ValueGenerationStrategy.SerialColumn")]
         public const string ValueGeneratedOnAdd = Prefix + "ValueGeneratedOnAdd";
+
+        [Obsolete("Replaced by built-in EF Core support, use HasComment on entities or properties.")]
+        public const string Comment = Prefix + "Comment";
     }
 }

src/EFCore.PG/Migrations/Internal/NpgsqlMigrationsAnnotationProvider.cs

@@ -18,8 +18,6 @@ public NpgsqlMigrationsAnnotationProvider([NotNull] MigrationsAnnotationProvider
 
         public override IEnumerable<IAnnotation> For(IEntityType entityType)
         {
-            if (NpgsqlEntityTypeExtensions.GetComment(entityType) is string comment)
-                yield return new Annotation(NpgsqlAnnotationNames.Comment, comment);
             if (entityType.GetIsUnlogged())
                 yield return new Annotation(NpgsqlAnnotationNames.UnloggedTable, entityType.GetIsUnlogged());
             if (entityType[CockroachDbAnnotationNames.InterleaveInParent] != null)
@@ -47,9 +45,6 @@ public override IEnumerable<IAnnotation> For(IProperty property)
                     }
                 }
             }
-
-            if (NpgsqlPropertyExtensions.GetComment(property) is string comment)
-                yield return new Annotation(NpgsqlAnnotationNames.Comment, comment);
         }
 
         public override IEnumerable<IAnnotation> For(IIndex index)

src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs

@@ -63,8 +63,13 @@ protected override void Generate(
             CreateTableOperation operation,
             IModel model,
             MigrationCommandListBuilder builder,
-            bool terminate)
+            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");
+            }
+
             operation.Columns.RemoveAll(c => IsSystemColumn(c.Name));
 
             builder.Append("CREATE ");
@@ -114,7 +119,7 @@ protected override void Generate(
             }
 
             // Comment on the table
-            if (operation[NpgsqlAnnotationNames.Comment] is string comment && comment.Length > 0)
+            if (operation.Comment != null)
             {
                 builder.AppendLine(';');
 
@@ -124,13 +129,13 @@ protected override void Generate(
                     .Append("COMMENT ON TABLE ")
                     .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Name, operation.Schema))
                     .Append(" IS ")
-                    .Append(stringTypeMapping.GenerateSqlLiteral(comment));
+                    .Append(stringTypeMapping.GenerateSqlLiteral(operation.Comment));
             }
 
             // Comments on the columns
-            foreach (var columnOp in operation.Columns.Where(c => c[NpgsqlAnnotationNames.Comment] != null))
+            foreach (var columnOp in operation.Columns.Where(c => c.Comment != null))
             {
-                var columnComment = columnOp[NpgsqlAnnotationNames.Comment];
+                var columnComment = columnOp.Comment;
                 builder.AppendLine(';');
 
                 var stringTypeMapping = Dependencies.TypeMappingSource.GetMapping(typeof(string));
@@ -200,18 +205,15 @@ protected override void Generate(AlterTableOperation operation, IModel model, Mi
             }
 
             // Comment
-            var oldComment = operation.OldTable[NpgsqlAnnotationNames.Comment] as string;
-            var newComment = operation[NpgsqlAnnotationNames.Comment] as string;
-
-            if (oldComment != newComment)
+            if (operation.Comment != operation.OldTable.Comment)
             {
                 var stringTypeMapping = Dependencies.TypeMappingSource.GetMapping(typeof(string));
 
                 builder
                     .Append("COMMENT ON TABLE ")
                     .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Name, operation.Schema))
                     .Append(" IS ")
-                    .Append(stringTypeMapping.GenerateSqlLiteral(newComment));
+                    .Append(stringTypeMapping.GenerateSqlLiteral(operation.Comment));
 
                 builder.AppendLine(';');
                 madeChanges = true;
@@ -241,7 +243,7 @@ protected override void Generate(
             DropColumnOperation operation,
             IModel model,
             MigrationCommandListBuilder builder,
-            bool terminate)
+            bool terminate = true)
         {
             // Never touch system columns
             if (IsSystemColumn(operation.Name))
@@ -254,15 +256,20 @@ protected override void Generate(
             AddColumnOperation operation,
             IModel model,
             MigrationCommandListBuilder builder,
-            bool terminate)
+            bool terminate = true)
         {
+            if (!terminate && operation.Comment != null)
+            {
+                throw new ArgumentException($"When generating migrations SQL for {nameof(AddColumnOperation)}, can't produce unterminated SQL with comments");
+            }
+
             // Never touch system columns
             if (IsSystemColumn(operation.Name))
                 return;
 
             base.Generate(operation, model, builder, terminate: false);
 
-            if (operation[NpgsqlAnnotationNames.Comment] is string comment && comment.Length > 0)
+            if (operation.Comment != null)
             {
                 builder.AppendLine(';');
 
@@ -274,7 +281,7 @@ protected override void Generate(
                     .Append('.')
                     .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Name))
                     .Append(" IS ")
-                    .Append(stringTypeMapping.GenerateSqlLiteral(comment));
+                    .Append(stringTypeMapping.GenerateSqlLiteral(operation.Comment));
             }
 
             if (terminate)
@@ -550,10 +557,7 @@ protected override void Generate(AlterColumnOperation operation, IModel model, M
             }
 
             // Comment
-            var oldComment = operation.OldColumn[NpgsqlAnnotationNames.Comment] as string;
-            var newComment = operation[NpgsqlAnnotationNames.Comment] as string;
-
-            if (oldComment != newComment)
+            if (operation.Comment != operation.OldColumn.Comment)
             {
                 var stringTypeMapping = Dependencies.TypeMappingSource.GetMapping(typeof(string));
 
@@ -563,7 +567,7 @@ protected override void Generate(AlterColumnOperation operation, IModel model, M
                     .Append('.')
                     .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Name))
                     .Append(" IS ")
-                    .Append(stringTypeMapping.GenerateSqlLiteral(newComment))
+                    .Append(stringTypeMapping.GenerateSqlLiteral(operation.Comment))
                     .AppendLine(';');
             }
 

src/EFCore.PG/Scaffolding/Internal/NpgsqlDatabaseModelFactory.cs

@@ -222,7 +222,7 @@ ns.nspname NOT IN ('pg_catalog', 'information_schema') AND
                     };
 
                     if (reader.GetValueOrDefault<string>("description") is string comment)
-                        table[NpgsqlAnnotationNames.Comment] = comment;
+                        table.Comment = comment;
 
                     tables.Add(table);
                 }
@@ -433,7 +433,7 @@ nspname NOT IN ('pg_catalog', 'information_schema') AND
                         }
 
                         if (record.GetValueOrDefault<string>("description") is string comment)
-                            column[NpgsqlAnnotationNames.Comment] = comment;
+                            column.Comment = comment;
 
                         table.Columns.Add(column);
                     }