Schema enums follow up (part 2) (#685)

* Follow-up to schema-enums (605, 724)

- Refactors `PostgresEnum` to properly handle schema qualification
  without breaking the standard patterns for migration operations.

- Late-access default schema (SQL generation time).

- Return `IReadOnlyList<string>` for `PostgresEnum.Labels` since
  items of that collection are not writable (i.e. returns a copy
  from the annotation value).

- Match the semantics of `Sequence.cs` such that
  `PostgresEnum.Schema` returns the annotation schema when it is
  not null, otherwise null indicates the default schema.

Author: austindrenski

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

@@ -75,7 +75,7 @@ public override MethodCallCodeFragment GenerateFluentApi(IModel model, IAnnotati
                     extension.Name);
             }
 
-            if (annotation.Name.StartsWith(NpgsqlAnnotationNames.EnumPrefix))
+            if (annotation.Name.StartsWith(NpgsqlAnnotationNames.EnumPrefix, StringComparison.Ordinal))
             {
                 var enumTypeDef = new PostgresEnum(model, annotation.Name);
 

src/EFCore.PG/Metadata/PostgresEnum.cs

@@ -9,102 +9,182 @@
 
 namespace Npgsql.EntityFrameworkCore.PostgreSQL.Metadata
 {
+    /// <summary>
+    /// Represents the metadata for a PostgreSQL enum.
+    /// </summary>
+    [PublicAPI]
     public class PostgresEnum
     {
-        readonly IAnnotatable _annotatable;
-        readonly string _annotationName;
-
-        internal PostgresEnum(IAnnotatable annotatable, string annotationName)
+        [NotNull] readonly IAnnotatable _annotatable;
+        [NotNull] readonly string _annotationName;
+
+        /// <summary>
+        /// Creates a <see cref="PostgresEnum"/>.
+        /// </summary>
+        /// <param name="annotatable">The annotatable to search for the annotation.</param>
+        /// <param name="annotationName">The annotation name to search for in the annotatable.</param>
+        /// <exception cref="ArgumentNullException"><paramref name="annotatable"/></exception>
+        /// <exception cref="ArgumentNullException"><paramref name="annotationName"/></exception>
+        internal PostgresEnum([NotNull] IAnnotatable annotatable, [NotNull] string annotationName)
         {
-            _annotatable = annotatable;
-            _annotationName = annotationName;
+            _annotatable = Check.NotNull(annotatable, nameof(annotatable));
+            _annotationName = Check.NotNull(annotationName, nameof(annotationName));
         }
 
+        /// <summary>
+        /// Gets or adds a <see cref="PostgresEnum"/> from or to the <see cref="IMutableAnnotatable"/>.
+        /// </summary>
+        /// <param name="annotatable">The annotatable from which to get or add the enum.</param>
+        /// <param name="schema">The enum schema or null to use the model's default schema.</param>
+        /// <param name="name">The enum name.</param>
+        /// <param name="labels">The enum labels.</param>
+        /// <returns>
+        /// The <see cref="PostgresEnum"/> from the <see cref="IMutableAnnotatable"/>.
+        /// </returns>
+        /// <exception cref="ArgumentException"><paramref name="schema"/></exception>
+        /// <exception cref="ArgumentNullException"><paramref name="annotatable"/></exception>
+        /// <exception cref="ArgumentNullException"><paramref name="name"/></exception>
+        /// <exception cref="ArgumentNullException"><paramref name="labels"/></exception>
+        [NotNull]
         public static PostgresEnum GetOrAddPostgresEnum(
             [NotNull] IMutableAnnotatable annotatable,
             [CanBeNull] string schema,
             [NotNull] string name,
             [NotNull] string[] labels)
         {
+            Check.NotNull(annotatable, nameof(annotatable));
+            Check.NullButNotEmpty(schema, nameof(schema));
+            Check.NotEmpty(name, nameof(name));
+            Check.NotNull(labels, nameof(labels));
+
             if (FindPostgresEnum(annotatable, schema, name) is PostgresEnum enumType)
                 return enumType;
 
-            enumType = new PostgresEnum(annotatable, BuildAnnotationName(schema, name));
-            enumType.SetData(labels);
-            return enumType;
+            var annotationName = BuildAnnotationName(schema, name);
+
+            return new PostgresEnum(annotatable, annotationName) { Labels = labels };
         }
 
+        /// <summary>
+        /// Gets or adds a <see cref="PostgresEnum"/> from or to the <see cref="IMutableAnnotatable"/>.
+        /// </summary>
+        /// <param name="annotatable">The annotatable from which to get or add the enum.</param>
+        /// <param name="name">The enum name.</param>
+        /// <param name="labels">The enum labels.</param>
+        /// <returns>
+        /// The <see cref="PostgresEnum"/> from the <see cref="IMutableAnnotatable"/>.
+        /// </returns>
+        /// <exception cref="ArgumentNullException"><paramref name="annotatable"/></exception>
+        /// <exception cref="ArgumentNullException"><paramref name="name"/></exception>
+        /// <exception cref="ArgumentNullException"><paramref name="labels"/></exception>
+        [NotNull]
         public static PostgresEnum GetOrAddPostgresEnum(
             [NotNull] IMutableAnnotatable annotatable,
             [NotNull] string name,
             [NotNull] string[] labels)
             => GetOrAddPostgresEnum(annotatable, null, name, labels);
 
+        /// <summary>
+        /// Finds a <see cref="PostgresEnum"/> in the <see cref="IAnnotatable"/>, or returns null if not found.
+        /// </summary>
+        /// <param name="annotatable">The annotatable to search for the enum.</param>
+        /// <param name="schema">The enum schema or null to use the model's default schema.</param>
+        /// <param name="name">The enum name.</param>
+        /// <returns>
+        /// The <see cref="PostgresEnum"/> from the <see cref="IAnnotatable"/>.
+        /// </returns>
+        /// <exception cref="ArgumentException"><paramref name="schema"/></exception>
+        /// <exception cref="ArgumentNullException"><paramref name="annotatable"/></exception>
+        /// <exception cref="ArgumentNullException"><paramref name="name"/></exception>
+        [CanBeNull]
         public static PostgresEnum FindPostgresEnum(
             [NotNull] IAnnotatable annotatable,
             [CanBeNull] string schema,
             [NotNull] string name)
         {
             Check.NotNull(annotatable, nameof(annotatable));
+            Check.NullButNotEmpty(schema, nameof(schema));
             Check.NotEmpty(name, nameof(name));
 
             var annotationName = BuildAnnotationName(schema, name);
 
             return annotatable[annotationName] == null ? null : new PostgresEnum(annotatable, annotationName);
         }
 
+        [NotNull]
         static string BuildAnnotationName(string schema, string name)
-            => NpgsqlAnnotationNames.EnumPrefix + (schema == null ? name : schema + '.' + name);
-
+            => schema != null
+                ? $"{NpgsqlAnnotationNames.EnumPrefix}{schema}.{name}"
+                : $"{NpgsqlAnnotationNames.EnumPrefix}{name}";
+
+        /// <summary>
+        /// Gets the collection of <see cref="PostgresEnum"/> stored in the <see cref="IAnnotatable"/>.
+        /// </summary>
+        /// <param name="annotatable">The annotatable to search for <see cref="PostgresEnum"/> annotations.</param>
+        /// <returns>
+        /// The collection of <see cref="PostgresEnum"/> stored in the <see cref="IAnnotatable"/>.
+        /// </returns>
+        /// <exception cref="ArgumentNullException"><paramref name="annotatable"/></exception>
+        [NotNull]
+        [ItemNotNull]
         public static IEnumerable<PostgresEnum> GetPostgresEnums([NotNull] IAnnotatable annotatable)
-        {
-            Check.NotNull(annotatable, nameof(annotatable));
-
-            return annotatable.GetAnnotations()
-                .Where(a => a.Name.StartsWith(NpgsqlAnnotationNames.EnumPrefix, StringComparison.Ordinal))
-                .Select(a => new PostgresEnum(annotatable, a.Name));
-        }
-
+            => Check.NotNull(annotatable, nameof(annotatable))
+                    .GetAnnotations()
+                    .Where(a => a.Name.StartsWith(NpgsqlAnnotationNames.EnumPrefix, StringComparison.Ordinal))
+                    .Select(a => new PostgresEnum(annotatable, a.Name));
+
+        /// <summary>
+        /// The <see cref="Annotatable"/> that stores the enum.
+        /// </summary>
+        [NotNull]
         public Annotatable Annotatable => (Annotatable)_annotatable;
 
+        /// <summary>
+        /// The enum schema or null to represent the default schema.
+        /// </summary>
+        [CanBeNull]
         public string Schema => GetData().Schema;
 
+        /// <summary>
+        /// The enum name.
+        /// </summary>
+        [NotNull]
         public string Name => GetData().Name;
 
-        public string[] Labels
+        /// <summary>
+        /// The enum labels.
+        /// </summary>
+        [NotNull]
+        public IReadOnlyList<string> Labels
         {
             get => GetData().Labels;
             set => SetData(value);
         }
 
         (string Schema, string Name, string[] Labels) GetData()
-        {
-            return !(Annotatable[_annotationName] is string annotationValue)
-                ? (null, null, null)
-                : Deserialize(_annotationName, annotationValue);
-        }
+            => Deserialize(Annotatable.FindAnnotation(_annotationName));
 
-        void SetData(string[] labels)
+        void SetData([NotNull] IEnumerable<string> labels)
             => Annotatable[_annotationName] = string.Join(",", labels);
 
-        static (string schema, string name, string[] labels) Deserialize(
-            [NotNull] string annotationName,
-            [NotNull] string annotationValue)
+        static (string Schema, string Name, string[] Labels) Deserialize([CanBeNull] IAnnotation annotation)
         {
-            Check.NotEmpty(annotationValue, nameof(annotationValue));
+            if (annotation == null || !(annotation.Value is string value) || string.IsNullOrEmpty(value))
+                return (null, null, null);
 
-            var labels = annotationValue.Split(',').ToArray();
+            var labels = value.Split(',');
 
+            // TODO: This would be a safer operation if we stored schema and name in the annotation value (see Sequence.cs).
             // Yes, this doesn't support dots in the schema/enum name, let somebody complain first.
-            var schemaAndName = annotationName.Substring(NpgsqlAnnotationNames.EnumPrefix.Length).Split('.');
+            var schemaAndName = annotation.Name.Substring(NpgsqlAnnotationNames.EnumPrefix.Length).Split('.');
             switch (schemaAndName.Length)
             {
             case 1:
                 return (null, schemaAndName[0], labels);
             case 2:
                 return (schemaAndName[0], schemaAndName[1], labels);
             default:
-                throw new ArgumentException("Cannot parse enum name from annotation: " + annotationName);
+                throw new ArgumentException($"Cannot parse enum name from annotation: {annotation.Name}");
             }
         }
     }

src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs

@@ -638,6 +638,7 @@ protected override void Generate(
             MigrationCommandListBuilder builder)
         {
             Check.NotNull(operation, nameof(operation));
+            Check.NotNull(model, nameof(model));
             Check.NotNull(builder, nameof(builder));
 
             GenerateEnumStatements(operation, model, builder);
@@ -691,7 +692,7 @@ protected virtual void GenerateEnumStatements(
             foreach (var enumTypeToDrop in operation.Npgsql().OldPostgresEnums
                 .Where(oe => operation.Npgsql().PostgresEnums.All(ne => ne.Name != oe.Name)))
             {
-                GenerateDropEnum(enumTypeToDrop, operation.OldDatabase, builder);
+                GenerateDropEnum(enumTypeToDrop, model, builder);
             }
 
             // TODO: Some forms of enum alterations are actually supported...
@@ -708,7 +709,7 @@ protected virtual void GenerateCreateEnum(
             [NotNull] IModel model,
             [NotNull] MigrationCommandListBuilder builder)
         {
-            var schema = GetSchemaOrDefault(enumType.Schema, model);
+            var schema = enumType.Schema ?? model.Relational().DefaultSchema;
 
             // Schemas are normally created (or rather ensured) by the model differ, which scans all tables, sequences
             // and other database objects. However, it isn't aware of enums, so we always ensure schema on enum creation.
@@ -723,10 +724,10 @@ protected virtual void GenerateCreateEnum(
             var stringTypeMapping = Dependencies.TypeMappingSource.GetMapping(typeof(string));
 
             var labels = enumType.Labels;
-            for (var i = 0; i < labels.Length; i++)
+            for (var i = 0; i < labels.Count; i++)
             {
                 builder.Append(stringTypeMapping.GenerateSqlLiteral(labels[i]));
-                if (i < labels.Length - 1)
+                if (i < labels.Count - 1)
                     builder.Append(", ");
             }
 
@@ -735,10 +736,10 @@ protected virtual void GenerateCreateEnum(
 
         protected virtual void GenerateDropEnum(
             [NotNull] PostgresEnum enumType,
-            [CanBeNull] IAnnotatable oldDatabase,
+            [NotNull] IModel model,
             [NotNull] MigrationCommandListBuilder builder)
         {
-            var schema = GetSchemaOrDefault(enumType.Schema, oldDatabase);
+            var schema = enumType.Schema ?? model.Relational().DefaultSchema;
 
             builder
                 .Append("DROP TYPE ")
@@ -1117,10 +1118,6 @@ static string GenerateStorageParameterValue(object value)
 
         #region Helpers
 
-        [CanBeNull]
-        static string GetSchemaOrDefault([CanBeNull] string schema, [CanBeNull] IAnnotatable model)
-            => schema ?? model?.FindAnnotation(RelationalAnnotationNames.DefaultSchema)?.Value as string;
-
         /// <summary>
         /// True if <see cref="_postgresVersion"/> is null, greater than, or equal to the specified version.
         /// </summary>