Refactor range metadata following 685 (#728)

Author: austindrenski

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

@@ -86,7 +86,7 @@ public override MethodCallCodeFragment GenerateFluentApi(IModel model, IAnnotati
                         enumTypeDef.Schema, enumTypeDef.Name, enumTypeDef.Labels);
             }
 
-            if (annotation.Name.StartsWith(NpgsqlAnnotationNames.RangePrefix))
+            if (annotation.Name.StartsWith(NpgsqlAnnotationNames.RangePrefix, StringComparison.Ordinal))
             {
                 var rangeTypeDef = new PostgresRange(model, annotation.Name);
 

src/EFCore.PG/Metadata/PostgresRange.cs

@@ -9,17 +9,47 @@
 
 namespace Npgsql.EntityFrameworkCore.PostgreSQL.Metadata
 {
+    /// <summary>
+    /// Represents the metadata for a PostgreSQL range.
+    /// </summary>
+    [PublicAPI]
     public class PostgresRange
     {
         [NotNull] readonly IAnnotatable _annotatable;
         [NotNull] readonly string _annotationName;
 
+        /// <summary>
+        /// Creates a <see cref="PostgresRange"/>.
+        /// </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 PostgresRange([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="PostgresRange"/> from or to the <see cref="IMutableAnnotatable"/>.
+        /// </summary>
+        /// <param name="annotatable">The annotatable from which to get or add the range.</param>
+        /// <param name="schema">The range schema or null to use the model's default schema.</param>
+        /// <param name="name">The range name.</param>
+        /// <param name="subtype">The range subtype.</param>
+        /// <param name="canonicalFunction"></param>
+        /// <param name="subtypeOpClass"></param>
+        /// <param name="collation"></param>
+        /// <param name="subtypeDiff"></param>
+        /// <returns>
+        /// The <see cref="PostgresRange"/> 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="subtype"/></exception>
+        [NotNull]
         public static PostgresRange GetOrAddPostgresRange(
             [NotNull] IMutableAnnotatable annotatable,
             [CanBeNull] string schema,
@@ -30,143 +60,175 @@ public static PostgresRange GetOrAddPostgresRange(
             string collation = null,
             string subtypeDiff = null)
         {
-            if (FindPostgresRange(annotatable, schema, name) is PostgresRange rangeType)
-                return rangeType;
+            Check.NotNull(annotatable, nameof(annotatable));
+            Check.NullButNotEmpty(schema, nameof(schema));
+            Check.NotEmpty(name, nameof(name));
+            Check.NotNull(subtype, nameof(subtype));
+
+            if (FindPostgresRange(annotatable, schema, name) is PostgresRange postgresRange)
+                return postgresRange;
+
+            var annotationName = BuildAnnotationName(schema, name);
 
-            rangeType = new PostgresRange(annotatable, BuildAnnotationName(annotatable, schema, name));
-            rangeType.SetData(subtype, canonicalFunction, subtypeOpClass, collation, subtypeDiff);
-            return rangeType;
+            return new PostgresRange(annotatable, annotationName)
+            {
+                Subtype = subtype,
+                CanonicalFunction = canonicalFunction,
+                SubtypeOpClass = subtypeOpClass,
+                Collation = collation,
+                SubtypeDiff = subtypeDiff,
+            };
         }
 
+        /// <summary>
+        /// Finds a <see cref="PostgresRange"/> in the <see cref="IAnnotatable"/>, or returns null if not found.
+        /// </summary>
+        /// <param name="annotatable">The annotatable to search for the range.</param>
+        /// <param name="schema">The range schema or null to use the model's default schema.</param>
+        /// <param name="name">The range name.</param>
+        /// <returns>
+        /// The <see cref="PostgresRange"/> 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 PostgresRange FindPostgresRange(
             [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(annotatable, schema, name);
+            var annotationName = BuildAnnotationName(schema, name);
 
             return annotatable[annotationName] == null ? null : new PostgresRange(annotatable, annotationName);
         }
 
         [NotNull]
-        static string BuildAnnotationName([NotNull] IAnnotatable annotatable, [CanBeNull] string schema, [NotNull] string name)
-            => !string.IsNullOrEmpty(schema)
+        static string BuildAnnotationName(string schema, string name)
+            => schema != null
                 ? $"{NpgsqlAnnotationNames.RangePrefix}{schema}.{name}"
-                : annotatable[RelationalAnnotationNames.DefaultSchema] is string defaultSchema && !string.IsNullOrEmpty(defaultSchema)
-                    ? $"{NpgsqlAnnotationNames.RangePrefix}{defaultSchema}.{name}"
-                    : $"{NpgsqlAnnotationNames.RangePrefix}{name}";
-
+                : $"{NpgsqlAnnotationNames.RangePrefix}{name}";
+
+        /// <summary>
+        /// Gets the collection of <see cref="PostgresRange"/> stored in the <see cref="IAnnotatable"/>.
+        /// </summary>
+        /// <param name="annotatable">The annotatable to search for <see cref="PostgresRange"/> annotations.</param>
+        /// <returns>
+        /// The collection of <see cref="PostgresRange"/> stored in the <see cref="IAnnotatable"/>.
+        /// </returns>
+        /// <exception cref="ArgumentNullException"><paramref name="annotatable"/></exception>
         [NotNull]
+        [ItemNotNull]
         public static IEnumerable<PostgresRange> GetPostgresRanges([NotNull] IAnnotatable annotatable)
             => Check.NotNull(annotatable, nameof(annotatable))
                     .GetAnnotations()
                     .Where(a => a.Name.StartsWith(NpgsqlAnnotationNames.RangePrefix, StringComparison.Ordinal))
                     .Select(a => new PostgresRange(annotatable, a.Name));
 
+        /// <summary>
+        /// The <see cref="Annotatable"/> that stores the range.
+        /// </summary>
         [NotNull]
         public Annotatable Annotatable => (Annotatable)_annotatable;
 
+        /// <summary>
+        /// The range schema or null to represent the default schema.
+        /// </summary>
         [CanBeNull]
-        public string Schema => GetData().Schema ?? (string)_annotatable[RelationalAnnotationNames.DefaultSchema];
+        public string Schema => GetData().Schema;
 
+        /// <summary>
+        /// The range name.
+        /// </summary>
         [NotNull]
         public string Name => GetData().Name;
 
+        /// <summary>
+        /// The subtype of the range.
+        /// </summary>
         [NotNull]
         public string Subtype
         {
             get => GetData().Subtype;
-            set
-            {
-                var x = GetData();
-                var (_, _, _, canonicalFunction, subtypeOpClass, collation, subtypeDiff) = GetData();
-                SetData(value, canonicalFunction, subtypeOpClass, collation, subtypeDiff);
-            }
+            set => SetData(subtype: value);
         }
 
+        /// <summary>
+        /// The function defining a "step" in a discrete range.
+        /// </summary>
         [CanBeNull]
         public string CanonicalFunction
         {
             get => GetData().CanonicalFunction;
-            set
-            {
-                var x = GetData();
-                var (_, _, subtype, _, subtypeOpClass, collation, subtypeDiff) = GetData();
-                SetData(subtype, value, subtypeOpClass, collation, subtypeDiff);
-            }
+            set => SetData(canonicalFunction: value);
         }
 
+        /// <summary>
+        /// The operator class to use.
+        /// </summary>
         [CanBeNull]
         public string SubtypeOpClass
         {
             get => GetData().SubtypeOpClass;
-            set
-            {
-                var x = GetData();
-                var (_, _, subtype, canonicalFunction, _, collation, subtypeDiff) = GetData();
-                SetData(subtype, canonicalFunction, value, collation, subtypeDiff);
-            }
+            set => SetData(subtypeOpClass: value);
         }
 
+        /// <summary>
+        /// The collation to use.
+        /// </summary>
         [CanBeNull]
         public string Collation
         {
             get => GetData().Collation;
-            set
-            {
-                var x = GetData();
-                var (_, _, subtype, canonicalFunction, subtypeOpClass, _, subtypeDiff) = GetData();
-                SetData(subtype, canonicalFunction, subtypeOpClass, value, subtypeDiff);
-            }
+            set => SetData(collation: value);
         }
 
+        /// <summary>
+        /// The function defining a difference in subtype values.
+        /// </summary>
         [CanBeNull]
         public string SubtypeDiff
         {
             get => GetData().SubtypeDiff;
-            set
-            {
-                var x = GetData();
-                var (_, _, subtype, canonicalFunction, subtypeOpClass, collation, _) = GetData();
-                SetData(subtype, canonicalFunction, subtypeOpClass, collation, value);
-            }
+            set => SetData(subtypeDiff: value);
         }
 
-        (string Schema, string Name, string Subtype, string CanonicalFunction, string SubtypeOpClass, string Collation,
-            string SubtypeDiff) GetData()
-            => !(Annotatable[_annotationName] is string annotationValue)
-                ? (null, null, null, null, null, null, null)
-                : Deserialize(_annotationName, annotationValue);
+        (string Schema, string Name, string Subtype, string CanonicalFunction, string SubtypeOpClass, string Collation, string SubtypeDiff) GetData()
+            => Deserialize(Annotatable.FindAnnotation(_annotationName));
 
-        void SetData(string subtype, string canonicalFunction, string subtypeOpClass, string collation, string subtypeDiff)
-            => Annotatable[_annotationName] = $"{subtype},{canonicalFunction},{subtypeOpClass},{collation},{subtypeDiff}";
+        void SetData(string subtype = null, string canonicalFunction = null, string subtypeOpClass = null, string collation = null, string subtypeDiff = null)
+            => Annotatable[_annotationName] =
+                $"{subtype ?? Subtype},{canonicalFunction ?? CanonicalFunction},{subtypeOpClass ?? SubtypeOpClass},{collation ?? Collation},{subtypeDiff ?? SubtypeDiff}";
 
-        static (string Schema, string Name, string Subtype, string CanonicalFunction, string SubtypeOpClass, string collation, string SubtypeDiff)
-            Deserialize([NotNull] string annotationName, [NotNull] string annotationValue)
+        static (string Schema, string Name, string Subtype, string CanonicalFunction, string SubtypeOpClass, string Collation, string SubtypeDiff)
+            Deserialize([CanBeNull] IAnnotation annotation)
         {
-            Check.NotEmpty(annotationValue, nameof(annotationValue));
+            if (annotation == null || !(annotation.Value is string value) || string.IsNullOrEmpty(value))
+                return (null, null, null, null, null, null, null);
 
-            var elements = annotationValue.Split(',').ToArray();
+            var elements = value.Split(',');
             if (elements.Length != 5)
-                throw new ArgumentException("Cannot parse range annotation value: " + annotationValue);
+                throw new ArgumentException($"Cannot parse range annotation value: {value}");
+
             for (var i = 0; i < 5; i++)
                 if (elements[i] == "")
                     elements[i] = null;
 
+            // 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/range name, let somebody complain first.
-            var schemaAndName = annotationName.Substring(NpgsqlAnnotationNames.RangePrefix.Length).Split('.');
+            var schemaAndName = annotation.Name.Substring(NpgsqlAnnotationNames.RangePrefix.Length).Split('.');
             switch (schemaAndName.Length)
             {
             case 1:
                 return (null, schemaAndName[0], elements[0], elements[1], elements[2], elements[3], elements[4]);
             case 2:
                 return (schemaAndName[0], schemaAndName[1], elements[0], elements[1], elements[2], elements[3], elements[4]);
             default:
-                throw new ArgumentException("Cannot parse enum name from annotation: " + annotationName);
+                throw new ArgumentException($"Cannot parse range name from annotation: {annotation.Name}");
             }
         }
     }

src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs

@@ -765,7 +765,7 @@ protected virtual void GenerateRangeStatements(
             foreach (var rangeTypeToDrop in operation.Npgsql().OldPostgresRanges
                 .Where(oe => operation.Npgsql().PostgresRanges.All(ne => ne.Name != oe.Name)))
             {
-                GenerateDropRange(rangeTypeToDrop, builder);
+                GenerateDropRange(rangeTypeToDrop, model, builder);
             }
 
             if (operation.Npgsql().PostgresRanges.FirstOrDefault(nr =>
@@ -781,14 +781,16 @@ protected virtual void GenerateCreateRange(
             [NotNull] IModel model,
             [NotNull] MigrationCommandListBuilder builder)
         {
+            var schema = rangeType.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 ranges, so we always ensure schema on range creation.
-            if (rangeType.Schema != null)
-                Generate(new EnsureSchemaOperation { Name = rangeType.Schema }, model, builder);
+            if (schema != null)
+                Generate(new EnsureSchemaOperation { Name = schema }, model, builder);
 
             builder
                 .Append("CREATE TYPE ")
-                .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(rangeType.Name, rangeType.Schema))
+                .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(rangeType.Name, schema))
                 .AppendLine($" AS RANGE (")
                 .IncrementIndent();
 
@@ -812,11 +814,16 @@ protected virtual void GenerateCreateRange(
                 .AppendLine(");");
         }
 
-        protected virtual void GenerateDropRange([NotNull] PostgresRange rangeType, [NotNull] MigrationCommandListBuilder builder)
+        protected virtual void GenerateDropRange(
+            [NotNull] PostgresRange rangeType,
+            [NotNull] IModel model,
+            [NotNull] MigrationCommandListBuilder builder)
         {
+            var schema = rangeType.Schema ?? model.Relational().DefaultSchema;
+
             builder
                 .Append("DROP TYPE ")
-                .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(rangeType.Name, rangeType.Schema))
+                .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(rangeType.Name, schema))
                 .AppendLine(";");
         }