Address review by @roji

npgsql · Oct 30, 2018 · b3cca03 · b3cca03
1 parent e142aed
commit b3cca03
Show file tree

Hide file tree

Showing 11 changed files with 66 additions and 47 deletions.
diff --git a/src/EFCore.PG.NodaTime/Storage/Internal/NpgsqlNodaTimeTypeMappingSourcePlugin.cs b/src/EFCore.PG.NodaTime/Storage/Internal/NpgsqlNodaTimeTypeMappingSourcePlugin.cs
@@ -41,14 +41,14 @@ public class NpgsqlNodaTimeTypeMappingSourcePlugin : IRelationalTypeMappingSourc
         /// <summary>
         /// Constructs an instance of the <see cref="NpgsqlNodaTimeTypeMappingSourcePlugin"/> class.
         /// </summary>
-        public NpgsqlNodaTimeTypeMappingSourcePlugin()
+        public NpgsqlNodaTimeTypeMappingSourcePlugin([NotNull] ISqlGenerationHelper sqlGenerationHelper)
         {
-            _timestampLocalDateTimeRange = new NpgsqlRangeTypeMapping("tsrange", null, typeof(NpgsqlRange<LocalDateTime>), _timestampLocalDateTime);
-            _timestampInstantRange = new NpgsqlRangeTypeMapping("tsrange", null, typeof(NpgsqlRange<Instant>), _timestampInstant);
-            _timestamptzInstantRange = new NpgsqlRangeTypeMapping("tstzrange", null, typeof(NpgsqlRange<Instant>), _timestamptzInstant);
-            _timestamptzZonedDateTimeRange = new NpgsqlRangeTypeMapping("tstzrange", null, typeof(NpgsqlRange<ZonedDateTime>), _timestamptzZonedDateTime);
-            _timestamptzOffsetDateTimeRange = new NpgsqlRangeTypeMapping("tstzrange", null, typeof(NpgsqlRange<OffsetDateTime>), _timestamptzOffsetDateTime);
-            _dateRange = new NpgsqlRangeTypeMapping("daterange", null, typeof(NpgsqlRange<LocalDate>), _date);
+            _timestampLocalDateTimeRange = new NpgsqlRangeTypeMapping("tsrange", typeof(NpgsqlRange<LocalDateTime>), _timestampLocalDateTime, sqlGenerationHelper);
+            _timestampInstantRange = new NpgsqlRangeTypeMapping("tsrange", typeof(NpgsqlRange<Instant>), _timestampInstant, sqlGenerationHelper);
+            _timestamptzInstantRange = new NpgsqlRangeTypeMapping("tstzrange", typeof(NpgsqlRange<Instant>), _timestamptzInstant, sqlGenerationHelper);
+            _timestamptzZonedDateTimeRange = new NpgsqlRangeTypeMapping("tstzrange", typeof(NpgsqlRange<ZonedDateTime>), _timestamptzZonedDateTime, sqlGenerationHelper);
+            _timestamptzOffsetDateTimeRange = new NpgsqlRangeTypeMapping("tstzrange", typeof(NpgsqlRange<OffsetDateTime>), _timestamptzOffsetDateTime, sqlGenerationHelper);
+            _dateRange = new NpgsqlRangeTypeMapping("daterange", typeof(NpgsqlRange<LocalDate>), _date, sqlGenerationHelper);
 
             var storeTypeMappings = new Dictionary<string, RelationalTypeMapping[]>(StringComparer.OrdinalIgnoreCase)
             {

diff --git a/src/EFCore.PG/Infrastructure/Internal/NpgsqlOptionsExtension.cs b/src/EFCore.PG/Infrastructure/Internal/NpgsqlOptionsExtension.cs
@@ -90,6 +90,15 @@ public override bool ApplyServices(IServiceCollection services)
         // See https://github.com/aspnet/EntityFrameworkCore/pull/10091
         public override int? MinBatchSize => base.MinBatchSize ?? 2;
 
+        /// <summary>
+        /// Returns a copy of the current instance configured with the specified range mapping.
+        /// </summary>
+        [NotNull]
+        public virtual NpgsqlOptionsExtension WithRangeMapping<TSubtype>(
+            [NotNull] string rangeName,
+            [CanBeNull] string subtypeName)
+            => WithRangeMapping<TSubtype>(rangeName, null, subtypeName);
+
         /// <summary>
         /// Returns a copy of the current instance configured with the specified range mapping.
         /// </summary>

diff --git a/src/EFCore.PG/Metadata/PostgresRange.cs b/src/EFCore.PG/Metadata/PostgresRange.cs
@@ -33,7 +33,7 @@ public static PostgresRange GetOrAddPostgresRange(
             if (FindPostgresRange(annotatable, schema, name) is PostgresRange rangeType)
                 return rangeType;
 
-            rangeType = new PostgresRange(annotatable, BuildAnnotationName(schema, name));
+            rangeType = new PostgresRange(annotatable, BuildAnnotationName(annotatable, schema, name));
             rangeType.SetData(subtype, canonicalFunction, subtypeOpClass, collation, subtypeDiff);
             return rangeType;
         }
@@ -47,14 +47,22 @@ public static PostgresRange FindPostgresRange(
             Check.NotNull(annotatable, nameof(annotatable));
             Check.NotEmpty(name, nameof(name));
 
-            var annotationName = BuildAnnotationName(schema, name);
+            var annotationName = BuildAnnotationName(annotatable, schema, name);
 
             return annotatable[annotationName] == null ? null : new PostgresRange(annotatable, annotationName);
         }
 
         [NotNull]
-        static string BuildAnnotationName(string schema, string name)
-            => NpgsqlAnnotationNames.RangePrefix + (schema == null ? name : schema + '.' + name);
+        static string BuildAnnotationName(IAnnotatable annotatable, string schema, string name)
+        {
+            if (schema != null)
+                return $"{NpgsqlAnnotationNames.RangePrefix}{schema}.{name}";
+
+            if (annotatable[RelationalAnnotationNames.DefaultSchema] is string defaultSchema)
+                return $"{NpgsqlAnnotationNames.RangePrefix}{defaultSchema}.{name}";
+
+            return $"{NpgsqlAnnotationNames.RangePrefix}{name}";
+        }
 
         [NotNull]
         public static IEnumerable<PostgresRange> GetPostgresRanges([NotNull] IAnnotatable annotatable)
@@ -66,7 +74,7 @@ public static IEnumerable<PostgresRange> GetPostgresRanges([NotNull] IAnnotatabl
         [NotNull]
         public Annotatable Annotatable => (Annotatable)_annotatable;
 
-        [NotNull]
+        [CanBeNull]
         public string Schema => GetData().Schema;
 
         [NotNull]

diff --git a/src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs b/src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs
@@ -776,7 +776,7 @@ protected virtual void GenerateRangeStatements(
             foreach (var rangeTypeToDrop in PostgresRange.GetPostgresRanges(operation.OldDatabase)
                 .Where(oe => PostgresRange.GetPostgresRanges(operation).All(ne => ne.Name != oe.Name)))
             {
-                GenerateDropRange(rangeTypeToDrop, operation.OldDatabase, builder);
+                GenerateDropRange(rangeTypeToDrop, builder);
             }
 
             if (PostgresRange.GetPostgresRanges(operation).FirstOrDefault(nr =>
@@ -792,16 +792,14 @@ protected virtual void GenerateCreateRange(
             [NotNull] IModel model,
             [NotNull] MigrationCommandListBuilder builder)
         {
-            var schema = GetSchemaOrDefault(rangeType.Schema, model);
-
             // 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 (schema != null)
-                Generate(new EnsureSchemaOperation { Name = schema }, model, builder);
+            if (rangeType.Schema != null)
+                Generate(new EnsureSchemaOperation { Name=rangeType.Schema }, model, builder);
 
             builder
                 .Append("CREATE TYPE ")
-                .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(rangeType.Name, schema))
+                .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(rangeType.Name, rangeType.Schema))
                 .AppendLine($" AS RANGE (")
                 .IncrementIndent();
 
@@ -825,16 +823,11 @@ protected virtual void GenerateCreateRange(
                 .AppendLine(");");
         }
 
-        protected virtual void GenerateDropRange(
-            [NotNull] PostgresRange rangeType,
-            [CanBeNull] IAnnotatable oldDatabase,
-            [NotNull] MigrationCommandListBuilder builder)
+        protected virtual void GenerateDropRange([NotNull] PostgresRange rangeType, [NotNull] MigrationCommandListBuilder builder)
         {
-            var schema = GetSchemaOrDefault(rangeType.Schema, oldDatabase);
-
             builder
                 .Append("DROP TYPE ")
-                .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(rangeType.Name, schema))
+                .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(rangeType.Name, rangeType.Schema))
                 .AppendLine(";");
         }
 

diff --git a/src/EFCore.PG/Storage/Internal/Mapping/NpgsqlRangeTypeMapping.cs b/src/EFCore.PG/Storage/Internal/Mapping/NpgsqlRangeTypeMapping.cs
@@ -9,41 +9,43 @@ namespace Npgsql.EntityFrameworkCore.PostgreSQL.Storage.Internal.Mapping
 {
     public class NpgsqlRangeTypeMapping : NpgsqlTypeMapping
     {
-        [NotNull] static readonly NpgsqlSqlGenerationHelper SqlGenerationHelper =
-            new NpgsqlSqlGenerationHelper(new RelationalSqlGenerationHelperDependencies());
-
-        [CanBeNull] readonly string _storeTypeSchema;
-
-        [NotNull]
-        public override string StoreType => SqlGenerationHelper.DelimitIdentifier(base.StoreType, _storeTypeSchema);
+        [NotNull] readonly ISqlGenerationHelper _sqlGenerationHelper;
 
         public RelationalTypeMapping SubtypeMapping { get; }
 
+        public NpgsqlRangeTypeMapping(
+            [NotNull] string storeType,
+            [NotNull] Type clrType,
+            [NotNull] RelationalTypeMapping subtypeMapping,
+            [NotNull] ISqlGenerationHelper sqlGenerationHelper)
+            : this(storeType, null, clrType, subtypeMapping, sqlGenerationHelper) {}
+
         public NpgsqlRangeTypeMapping(
             [NotNull] string storeType,
             [CanBeNull] string storeTypeSchema,
             [NotNull] Type clrType,
-            [NotNull] RelationalTypeMapping subtypeMapping)
-            : base(storeType, clrType, GenerateNpgsqlDbType(subtypeMapping))
+            [NotNull] RelationalTypeMapping subtypeMapping,
+            [NotNull] ISqlGenerationHelper sqlGenerationHelper)
+            : base(sqlGenerationHelper.DelimitIdentifier(storeType, storeTypeSchema), clrType, GenerateNpgsqlDbType(subtypeMapping))
         {
-            _storeTypeSchema = storeTypeSchema;
             SubtypeMapping = subtypeMapping;
+            _sqlGenerationHelper = sqlGenerationHelper;
         }
 
         protected NpgsqlRangeTypeMapping(
             RelationalTypeMappingParameters parameters,
             NpgsqlDbType npgsqlDbType,
-            [CanBeNull] string storeTypeSchema,
-            [NotNull] RelationalTypeMapping subtypeMapping)
+            [NotNull] RelationalTypeMapping subtypeMapping,
+            [NotNull] ISqlGenerationHelper sqlGenerationHelper)
             : base(parameters, npgsqlDbType)
         {
-            _storeTypeSchema = storeTypeSchema;
             SubtypeMapping = subtypeMapping;
+            _sqlGenerationHelper = sqlGenerationHelper;
         }
 
         [NotNull]
         protected override RelationalTypeMapping Clone(RelationalTypeMappingParameters parameters)
-            => new NpgsqlRangeTypeMapping(parameters, NpgsqlDbType, _storeTypeSchema, SubtypeMapping);
+            => new NpgsqlRangeTypeMapping(parameters, NpgsqlDbType, SubtypeMapping, _sqlGenerationHelper);
 
         protected override string GenerateNonNullSqlLiteral(object value)
         {

diff --git a/src/EFCore.PG/Storage/Internal/NpgsqlTypeMappingSource.cs b/src/EFCore.PG/Storage/Internal/NpgsqlTypeMappingSource.cs
@@ -103,16 +103,17 @@ public class NpgsqlTypeMappingSource : RelationalTypeMappingSource
 
         public NpgsqlTypeMappingSource([NotNull] TypeMappingSourceDependencies dependencies,
             [NotNull] RelationalTypeMappingSourceDependencies relationalDependencies,
+            [NotNull] ISqlGenerationHelper sqlGenerationHelper,
             [CanBeNull] INpgsqlOptions npgsqlOptions=null)
             : base(dependencies, relationalDependencies)
         {
             // Initialize some mappings which depend on other mappings
-            _int4range = new NpgsqlRangeTypeMapping("int4range", null, typeof(NpgsqlRange<int>),      _int4);
-            _int8range = new NpgsqlRangeTypeMapping("int8range", null, typeof(NpgsqlRange<long>),     _int8);
-            _numrange  = new NpgsqlRangeTypeMapping("numrange",  null, typeof(NpgsqlRange<decimal>),  _numeric);
-            _tsrange   = new NpgsqlRangeTypeMapping("tsrange",   null, typeof(NpgsqlRange<DateTime>), _timestamp);
-            _tstzrange = new NpgsqlRangeTypeMapping("tstzrange", null, typeof(NpgsqlRange<DateTime>), _timestamptz);
-            _daterange = new NpgsqlRangeTypeMapping("daterange", null, typeof(NpgsqlRange<DateTime>), _timestamptz);
+            _int4range = new NpgsqlRangeTypeMapping("int4range", typeof(NpgsqlRange<int>),      _int4,        sqlGenerationHelper);
+            _int8range = new NpgsqlRangeTypeMapping("int8range", typeof(NpgsqlRange<long>),     _int8,        sqlGenerationHelper);
+            _numrange  = new NpgsqlRangeTypeMapping("numrange",  typeof(NpgsqlRange<decimal>),  _numeric,     sqlGenerationHelper);
+            _tsrange   = new NpgsqlRangeTypeMapping("tsrange",   typeof(NpgsqlRange<DateTime>), _timestamp,   sqlGenerationHelper);
+            _tstzrange = new NpgsqlRangeTypeMapping("tstzrange", typeof(NpgsqlRange<DateTime>), _timestamptz, sqlGenerationHelper);
+            _daterange = new NpgsqlRangeTypeMapping("daterange", typeof(NpgsqlRange<DateTime>), _timestamptz, sqlGenerationHelper);
 
             // Note that PostgreSQL has aliases to some built-in type name aliases (e.g. int4 for integer),
             // these are mapped as well.
@@ -252,7 +253,7 @@ public NpgsqlTypeMappingSource([NotNull] TypeMappingSourceDependencies dependenc
                         : throw new Exception($"Could not map range {rangeName}, no mapping was found for subtype {subtypeName}");
 
                 var rangeClrType = typeof(NpgsqlRange<>).MakeGenericType(subtypeClrType);
-                var rangeMapping = new NpgsqlRangeTypeMapping(rangeName, schemaName, rangeClrType, subtypeMapping);
+                var rangeMapping = new NpgsqlRangeTypeMapping(rangeName, schemaName, rangeClrType, subtypeMapping, sqlGenerationHelper);
                 StoreTypeMappings[rangeMapping.StoreType] = new RelationalTypeMapping[] { rangeMapping };
                 ClrTypeMappings[rangeMapping.ClrType] = rangeMapping;
             }

diff --git a/test/EFCore.PG.FunctionalTests/Scaffolding/NpgsqlDatabaseModelFactoryTest.cs b/test/EFCore.PG.FunctionalTests/Scaffolding/NpgsqlDatabaseModelFactoryTest.cs
@@ -1711,6 +1711,7 @@ line line
                             Array.Empty<ITypeMappingSourcePlugin>()
                         ),
                         new RelationalTypeMappingSourceDependencies(Array.Empty<IRelationalTypeMappingSourcePlugin>()),
+                        new NpgsqlSqlGenerationHelper(new RelationalSqlGenerationHelperDependencies()),
                         options
                     );
 

diff --git a/test/EFCore.PG.Plugins.FunctionalTests/NpgsqlNodaTimeTypeMappingTest.cs b/test/EFCore.PG.Plugins.FunctionalTests/NpgsqlNodaTimeTypeMappingTest.cs
@@ -118,7 +118,8 @@ public void GenerateSqlLiteral_returns_period_literal()
 
         #region Support
 
-        static readonly IRelationalTypeMappingSourcePlugin Mapper = new NpgsqlNodaTimeTypeMappingSourcePlugin();
+        static readonly IRelationalTypeMappingSourcePlugin Mapper =
+            new NpgsqlNodaTimeTypeMappingSourcePlugin(new NpgsqlSqlGenerationHelper(new RelationalSqlGenerationHelperDependencies()));
 
         static RelationalTypeMapping GetMapping(string storeType)
             => Mapper.FindMapping(new RelationalTypeMappingInfo(storeType));

diff --git a/test/EFCore.PG.Tests/Storage/NpgsqlTypeMappingTest.cs b/test/EFCore.PG.Tests/Storage/NpgsqlTypeMappingTest.cs
@@ -350,6 +350,7 @@ public void GenerateSqlLiteral_returns_ranking_normalization_literal() => Assert
                 Array.Empty<ITypeMappingSourcePlugin>()
             ),
             new RelationalTypeMappingSourceDependencies(Array.Empty<IRelationalTypeMappingSourcePlugin>()),
+            new NpgsqlSqlGenerationHelper(new RelationalSqlGenerationHelperDependencies()),
             new NpgsqlOptions()
         );
 

diff --git a/test/EFCore.PG.Tests/Update/NpgsqlModificationCommandBatchFactoryTest.cs b/test/EFCore.PG.Tests/Update/NpgsqlModificationCommandBatchFactoryTest.cs
@@ -27,6 +27,7 @@ public void Uses_MaxBatchSize_specified_in_NpgsqlOptionsExtension()
                     Array.Empty<ITypeMappingSourcePlugin>()
                 ),
                 new RelationalTypeMappingSourceDependencies(Array.Empty<IRelationalTypeMappingSourcePlugin>()),
+                new NpgsqlSqlGenerationHelper(new RelationalSqlGenerationHelperDependencies()),
                 new NpgsqlOptions()
             );
             var factory = new NpgsqlModificationCommandBatchFactory(
@@ -63,6 +64,7 @@ public void MaxBatchSize_is_optional()
                     Array.Empty<ITypeMappingSourcePlugin>()
                 ),
                 new RelationalTypeMappingSourceDependencies(Array.Empty<IRelationalTypeMappingSourcePlugin>()),
+                new NpgsqlSqlGenerationHelper(new RelationalSqlGenerationHelperDependencies()),
                 new NpgsqlOptions()
             );
             var factory = new NpgsqlModificationCommandBatchFactory(

diff --git a/test/EFCore.PG.Tests/Update/NpgsqlModificationCommandBatchTest.cs b/test/EFCore.PG.Tests/Update/NpgsqlModificationCommandBatchTest.cs
@@ -24,6 +24,7 @@ public void AddCommand_returns_false_when_max_batch_size_is_reached()
                     Array.Empty<ITypeMappingSourcePlugin>()
                 ),
                 new RelationalTypeMappingSourceDependencies(Array.Empty<IRelationalTypeMappingSourcePlugin>()),
+                new NpgsqlSqlGenerationHelper(new RelationalSqlGenerationHelperDependencies()),
                 new NpgsqlOptions()
             );