Schema enums follow up (part 2)

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>