From 09bc34fd07977b86813909d3a61e0ee9049bb6de Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Thu, 1 Dec 2022 13:06:55 +0100 Subject: [PATCH] Correct date/time mapping with precision (#2580) Closes #2579 (cherry picked from commit 9573961411ef9643c10d4cc6a57b03ee8ead62b4) --- .../Internal/LegacyTimestampInstantMapping.cs | 9 +++++ .../NpgsqlNodaTimeTypeMappingSourcePlugin.cs | 18 +++------ .../Storage/Internal/TimeMapping.cs | 9 +++++ .../Storage/Internal/TimeTzMapping.cs | 9 +++++ .../Internal/TimestampLocalDateTimeMapping.cs | 9 +++++ .../Internal/TimestampTzInstantMapping.cs | 9 +++++ .../TimestampTzOffsetDateTimeMapping.cs | 9 +++++ .../TimestampTzZonedDateTimeMapping.cs | 9 +++++ .../Internal/Mapping/NpgsqlTimeTypeMapping.cs | 9 +++++ .../Mapping/NpgsqlTimeTzTypeMapping.cs | 9 +++++ .../Internal/NpgsqlTypeMappingSource.cs | 10 ++--- .../NpgsqlNodaTimeTypeMappingTest.cs | 37 ++++++++++--------- .../Storage/NpgsqlTypeMappingTest.cs | 6 +++ 13 files changed, 117 insertions(+), 35 deletions(-) diff --git a/src/EFCore.PG.NodaTime/Storage/Internal/LegacyTimestampInstantMapping.cs b/src/EFCore.PG.NodaTime/Storage/Internal/LegacyTimestampInstantMapping.cs index 9ad60bc3e..0fb0e4b20 100644 --- a/src/EFCore.PG.NodaTime/Storage/Internal/LegacyTimestampInstantMapping.cs +++ b/src/EFCore.PG.NodaTime/Storage/Internal/LegacyTimestampInstantMapping.cs @@ -62,6 +62,15 @@ public override RelationalTypeMapping Clone(string storeType, int? size) public override CoreTypeMapping Clone(ValueConverter? converter) => new LegacyTimestampInstantMapping(Parameters.WithComposedConverter(converter)); + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + protected override string ProcessStoreType(RelationalTypeMappingParameters parameters, string storeType, string _) + => parameters.Precision is null ? storeType : $"timestamp({parameters.Precision}) without time zone"; + /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to /// the same compatibility standards as public APIs. It may be changed or removed without notice in diff --git a/src/EFCore.PG.NodaTime/Storage/Internal/NpgsqlNodaTimeTypeMappingSourcePlugin.cs b/src/EFCore.PG.NodaTime/Storage/Internal/NpgsqlNodaTimeTypeMappingSourcePlugin.cs index 3e14e1907..dd16660f3 100644 --- a/src/EFCore.PG.NodaTime/Storage/Internal/NpgsqlNodaTimeTypeMappingSourcePlugin.cs +++ b/src/EFCore.PG.NodaTime/Storage/Internal/NpgsqlNodaTimeTypeMappingSourcePlugin.cs @@ -236,7 +236,8 @@ public NpgsqlNodaTimeTypeMappingSourcePlugin(ISqlGenerationHelper sqlGenerationH /// doing so can result in application failures when updating to a new Entity Framework Core release. /// public virtual RelationalTypeMapping? FindMapping(in RelationalTypeMappingInfo mappingInfo) - => FindExistingMapping(mappingInfo) ?? FindArrayMapping(mappingInfo); + => FindBaseMapping(mappingInfo)?.Clone(mappingInfo) + ?? FindArrayMapping(mappingInfo)?.Clone(mappingInfo); /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to @@ -244,7 +245,7 @@ public NpgsqlNodaTimeTypeMappingSourcePlugin(ISqlGenerationHelper sqlGenerationH /// any release. You should only use it directly in your code with extreme caution and knowing that /// doing so can result in application failures when updating to a new Entity Framework Core release. /// - protected virtual RelationalTypeMapping? FindExistingMapping(in RelationalTypeMappingInfo mappingInfo) + protected virtual RelationalTypeMapping? FindBaseMapping(in RelationalTypeMappingInfo mappingInfo) { var clrType = mappingInfo.ClrType; var storeTypeName = mappingInfo.StoreTypeName; @@ -289,16 +290,9 @@ public NpgsqlNodaTimeTypeMappingSourcePlugin(ISqlGenerationHelper sqlGenerationH } } - if (clrType is null || !ClrTypeMappings.TryGetValue(clrType, out var mapping)) - { - return null; - } - - // All PostgreSQL date/time types accept a precision except for date - // TODO: Cache size/precision/scale mappings? - return mappingInfo.Precision.HasValue && mapping.ClrType != typeof(LocalDate) - ? mapping.Clone($"{mapping.StoreType}({mappingInfo.Precision.Value})", null) - : mapping; + return clrType is not null && ClrTypeMappings.TryGetValue(clrType, out var mapping) + ? mapping + : null; } /// diff --git a/src/EFCore.PG.NodaTime/Storage/Internal/TimeMapping.cs b/src/EFCore.PG.NodaTime/Storage/Internal/TimeMapping.cs index 4b0da9695..668d9c063 100644 --- a/src/EFCore.PG.NodaTime/Storage/Internal/TimeMapping.cs +++ b/src/EFCore.PG.NodaTime/Storage/Internal/TimeMapping.cs @@ -67,6 +67,15 @@ public override RelationalTypeMapping Clone(string storeType, int? size) public override CoreTypeMapping Clone(ValueConverter? converter) => new TimeMapping(Parameters.WithComposedConverter(converter)); + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + protected override string ProcessStoreType(RelationalTypeMappingParameters parameters, string storeType, string _) + => parameters.Precision is null ? storeType : $"time({parameters.Precision})"; + /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to /// the same compatibility standards as public APIs. It may be changed or removed without notice in diff --git a/src/EFCore.PG.NodaTime/Storage/Internal/TimeTzMapping.cs b/src/EFCore.PG.NodaTime/Storage/Internal/TimeTzMapping.cs index 2832cfc8e..89e4794bf 100644 --- a/src/EFCore.PG.NodaTime/Storage/Internal/TimeTzMapping.cs +++ b/src/EFCore.PG.NodaTime/Storage/Internal/TimeTzMapping.cs @@ -79,6 +79,15 @@ public override RelationalTypeMapping Clone(string storeType, int? size) public override CoreTypeMapping Clone(ValueConverter? converter) => new TimeTzMapping(Parameters.WithComposedConverter(converter)); + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + protected override string ProcessStoreType(RelationalTypeMappingParameters parameters, string storeType, string _) + => parameters.Precision is null ? storeType : $"timestamp({parameters.Precision}) with time zone"; + /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to /// the same compatibility standards as public APIs. It may be changed or removed without notice in diff --git a/src/EFCore.PG.NodaTime/Storage/Internal/TimestampLocalDateTimeMapping.cs b/src/EFCore.PG.NodaTime/Storage/Internal/TimestampLocalDateTimeMapping.cs index c2026c3cf..e3b58b91d 100644 --- a/src/EFCore.PG.NodaTime/Storage/Internal/TimestampLocalDateTimeMapping.cs +++ b/src/EFCore.PG.NodaTime/Storage/Internal/TimestampLocalDateTimeMapping.cs @@ -66,6 +66,15 @@ public override RelationalTypeMapping Clone(string storeType, int? size) public override CoreTypeMapping Clone(ValueConverter? converter) => new TimestampLocalDateTimeMapping(Parameters.WithComposedConverter(converter)); + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + protected override string ProcessStoreType(RelationalTypeMappingParameters parameters, string storeType, string _) + => parameters.Precision is null ? storeType : $"timestamp({parameters.Precision}) without time zone"; + /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to /// the same compatibility standards as public APIs. It may be changed or removed without notice in diff --git a/src/EFCore.PG.NodaTime/Storage/Internal/TimestampTzInstantMapping.cs b/src/EFCore.PG.NodaTime/Storage/Internal/TimestampTzInstantMapping.cs index abf7f893c..8f65dd0a4 100644 --- a/src/EFCore.PG.NodaTime/Storage/Internal/TimestampTzInstantMapping.cs +++ b/src/EFCore.PG.NodaTime/Storage/Internal/TimestampTzInstantMapping.cs @@ -56,6 +56,15 @@ public override RelationalTypeMapping Clone(string storeType, int? size) public override CoreTypeMapping Clone(ValueConverter? converter) => new TimestampTzInstantMapping(Parameters.WithComposedConverter(converter)); + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + protected override string ProcessStoreType(RelationalTypeMappingParameters parameters, string storeType, string _) + => parameters.Precision is null ? storeType : $"timestamp({parameters.Precision}) with time zone"; + /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to /// the same compatibility standards as public APIs. It may be changed or removed without notice in diff --git a/src/EFCore.PG.NodaTime/Storage/Internal/TimestampTzOffsetDateTimeMapping.cs b/src/EFCore.PG.NodaTime/Storage/Internal/TimestampTzOffsetDateTimeMapping.cs index 8a40deacd..96dbf2212 100644 --- a/src/EFCore.PG.NodaTime/Storage/Internal/TimestampTzOffsetDateTimeMapping.cs +++ b/src/EFCore.PG.NodaTime/Storage/Internal/TimestampTzOffsetDateTimeMapping.cs @@ -66,6 +66,15 @@ public override RelationalTypeMapping Clone(string storeType, int? size) public override CoreTypeMapping Clone(ValueConverter? converter) => new TimestampTzOffsetDateTimeMapping(Parameters.WithComposedConverter(converter)); + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + protected override string ProcessStoreType(RelationalTypeMappingParameters parameters, string storeType, string _) + => parameters.Precision is null ? storeType : $"timestamp({parameters.Precision}) with time zone"; + /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to /// the same compatibility standards as public APIs. It may be changed or removed without notice in diff --git a/src/EFCore.PG.NodaTime/Storage/Internal/TimestampTzZonedDateTimeMapping.cs b/src/EFCore.PG.NodaTime/Storage/Internal/TimestampTzZonedDateTimeMapping.cs index 5647ccc49..0d71c4bcf 100644 --- a/src/EFCore.PG.NodaTime/Storage/Internal/TimestampTzZonedDateTimeMapping.cs +++ b/src/EFCore.PG.NodaTime/Storage/Internal/TimestampTzZonedDateTimeMapping.cs @@ -61,6 +61,15 @@ public override RelationalTypeMapping Clone(string storeType, int? size) public override CoreTypeMapping Clone(ValueConverter? converter) => new TimestampTzZonedDateTimeMapping(Parameters.WithComposedConverter(converter)); + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + protected override string ProcessStoreType(RelationalTypeMappingParameters parameters, string storeType, string _) + => parameters.Precision is null ? storeType : $"timestamp({parameters.Precision}) with time zone"; + /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to /// the same compatibility standards as public APIs. It may be changed or removed without notice in diff --git a/src/EFCore.PG/Storage/Internal/Mapping/NpgsqlTimeTypeMapping.cs b/src/EFCore.PG/Storage/Internal/Mapping/NpgsqlTimeTypeMapping.cs index 16b0a142a..7d4b16b5d 100644 --- a/src/EFCore.PG/Storage/Internal/Mapping/NpgsqlTimeTypeMapping.cs +++ b/src/EFCore.PG/Storage/Internal/Mapping/NpgsqlTimeTypeMapping.cs @@ -36,6 +36,15 @@ protected NpgsqlTimeTypeMapping(RelationalTypeMappingParameters parameters) protected override RelationalTypeMapping Clone(RelationalTypeMappingParameters parameters) => new NpgsqlTimeTypeMapping(parameters); + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + protected override string ProcessStoreType(RelationalTypeMappingParameters parameters, string storeType, string _) + => parameters.Precision is null ? storeType : $"time({parameters.Precision}) without time zone"; + /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to /// the same compatibility standards as public APIs. It may be changed or removed without notice in diff --git a/src/EFCore.PG/Storage/Internal/Mapping/NpgsqlTimeTzTypeMapping.cs b/src/EFCore.PG/Storage/Internal/Mapping/NpgsqlTimeTzTypeMapping.cs index 1fc923a21..cbd453429 100644 --- a/src/EFCore.PG/Storage/Internal/Mapping/NpgsqlTimeTzTypeMapping.cs +++ b/src/EFCore.PG/Storage/Internal/Mapping/NpgsqlTimeTzTypeMapping.cs @@ -34,6 +34,15 @@ protected NpgsqlTimeTzTypeMapping(RelationalTypeMappingParameters parameters) protected override RelationalTypeMapping Clone(RelationalTypeMappingParameters parameters) => new NpgsqlTimeTzTypeMapping(parameters); + /// + /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to + /// the same compatibility standards as public APIs. It may be changed or removed without notice in + /// any release. You should only use it directly in your code with extreme caution and knowing that + /// doing so can result in application failures when updating to a new Entity Framework Core release. + /// + protected override string ProcessStoreType(RelationalTypeMappingParameters parameters, string storeType, string _) + => parameters.Precision is null ? storeType : $"time({parameters.Precision}) with time zone"; + /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to /// the same compatibility standards as public APIs. It may be changed or removed without notice in diff --git a/src/EFCore.PG/Storage/Internal/NpgsqlTypeMappingSource.cs b/src/EFCore.PG/Storage/Internal/NpgsqlTypeMappingSource.cs index 0985dc8bd..0217fe952 100644 --- a/src/EFCore.PG/Storage/Internal/NpgsqlTypeMappingSource.cs +++ b/src/EFCore.PG/Storage/Internal/NpgsqlTypeMappingSource.cs @@ -493,11 +493,11 @@ protected virtual void SetupEnumMappings(ISqlGenerationHelper sqlGenerationHelpe /// protected override RelationalTypeMapping? FindMapping(in RelationalTypeMappingInfo mappingInfo) => // First, try any plugins, allowing them to override built-in mappings (e.g. NodaTime) - base.FindMapping(mappingInfo) ?? - FindBaseMapping(mappingInfo)?.Clone(mappingInfo) ?? - FindArrayMapping(mappingInfo)?.Clone(mappingInfo) ?? - FindRowValueMapping(mappingInfo)?.Clone(mappingInfo) ?? - FindUserRangeMapping(mappingInfo); + base.FindMapping(mappingInfo) + ?? FindBaseMapping(mappingInfo)?.Clone(mappingInfo) + ?? FindArrayMapping(mappingInfo)?.Clone(mappingInfo) + ?? FindRowValueMapping(mappingInfo)?.Clone(mappingInfo) + ?? FindUserRangeMapping(mappingInfo); /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to diff --git a/test/EFCore.PG.NodaTime.FunctionalTests/NpgsqlNodaTimeTypeMappingTest.cs b/test/EFCore.PG.NodaTime.FunctionalTests/NpgsqlNodaTimeTypeMappingTest.cs index d1fb616f1..fbbc52138 100644 --- a/test/EFCore.PG.NodaTime.FunctionalTests/NpgsqlNodaTimeTypeMappingTest.cs +++ b/test/EFCore.PG.NodaTime.FunctionalTests/NpgsqlNodaTimeTypeMappingTest.cs @@ -26,9 +26,15 @@ public void Instant_maps_to_timestamp_legacy() { var mapping = GetMapping(typeof(Instant), "timestamp"); Assert.Same(typeof(Instant), mapping.ClrType); - Assert.Equal("timestamp without time zone", mapping.StoreType); + Assert.Equal("timestamp", mapping.StoreType); } + [Fact] + public void Instant_with_precision() + => Assert.Equal( + "timestamp(3) with time zone", + Mapper.FindMapping(typeof(Instant), "timestamp with time zone", precision: 3)!.StoreType); + [Fact] public void GenerateSqlLiteral_returns_LocalDateTime_literal() { @@ -520,26 +526,21 @@ public void GenerateCodeLiteral_returns_OffsetTime_literal() [Fact] public void Duration_is_properly_mapped() - { - var mapping = GetMapping(typeof(Duration)); - - Assert.Equal("interval", mapping.StoreType); - Assert.Same(typeof(Duration), mapping.ClrType); - - Assert.Same(mapping, GetMapping(typeof(Duration), "interval")); - } + => Assert.All(new[] { GetMapping(typeof(Duration)), GetMapping(typeof(Duration), "interval") }, + m => + { + Assert.Equal("interval", m.StoreType); + Assert.Same(typeof(Duration), m.ClrType); + }); [Fact] public void Period_is_properly_mapped() - { - var mapping = GetMapping(typeof(Period)); - - Assert.Equal("interval", mapping.StoreType); - Assert.Same(typeof(Period), mapping.ClrType); - - Assert.Same(mapping, GetMapping(typeof(Period))); - Assert.Same(mapping, GetMapping(typeof(Period), "interval")); - } + => Assert.All(new[] { GetMapping(typeof(Period)), GetMapping(typeof(Period), "interval") }, + m => + { + Assert.Equal("interval", m.StoreType); + Assert.Same(typeof(Period), m.ClrType); + }); [Fact] public void GenerateSqlLiteral_returns_Period_literal() diff --git a/test/EFCore.PG.Tests/Storage/NpgsqlTypeMappingTest.cs b/test/EFCore.PG.Tests/Storage/NpgsqlTypeMappingTest.cs index 89205139b..875029759 100644 --- a/test/EFCore.PG.Tests/Storage/NpgsqlTypeMappingTest.cs +++ b/test/EFCore.PG.Tests/Storage/NpgsqlTypeMappingTest.cs @@ -28,6 +28,12 @@ public void Timestamp_maps_to_DateTime_by_default() public void Timestamptz_maps_to_DateTime_by_default() => Assert.Same(typeof(DateTime), GetMapping("timestamp with time zone").ClrType); + [Fact] + public void DateTime_with_precision() + => Assert.Equal( + "timestamp(3) with time zone", + Mapper.FindMapping(typeof(DateTime), "timestamp with time zone", precision: 3)!.StoreType); + [Fact] public void GenerateSqlLiteral_returns_date_literal() {