Add a warning for decimal keys for SqlServer (#21713)

Fixes #17817

src/EFCore.SqlServer/Diagnostics/Internal/SqlServerLoggingDefinitions.cs

@@ -13,6 +13,14 @@ namespace Microsoft.EntityFrameworkCore.SqlServer.Diagnostics.Internal
     /// </summary>
     public class SqlServerLoggingDefinitions : RelationalLoggingDefinitions
     {
+        /// <summary>
+        ///     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.
+        /// </summary>
+        public EventDefinitionBase LogDecimalTypeKey;
+
         /// <summary>
         ///     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

src/EFCore.SqlServer/Diagnostics/SqlServerEventId.cs

@@ -27,6 +27,7 @@ private enum Id
             DecimalTypeDefaultWarning = CoreEventId.ProviderBaseId,
             ByteIdentityColumnWarning,
             ConflictingValueGenerationStrategiesWarning,
+            DecimalTypeKeyWarning,
 
             // Scaffolding events
             ColumnFound = CoreEventId.ProviderDesignBaseId,
@@ -64,6 +65,19 @@ private enum Id
         private static readonly string _validationPrefix = DbLoggerCategory.Model.Validation.Name + ".";
         private static EventId MakeValidationId(Id id) => new EventId((int)id, _validationPrefix + id);
 
+        /// <summary>
+        ///     <para>
+        ///         Decimal column is part of the key.
+        ///     </para>
+        ///     <para>
+        ///         This event is in the <see cref="DbLoggerCategory.Model.Validation" /> category.
+        ///     </para>
+        ///     <para>
+        ///         This event uses the <see cref="PropertyEventData" /> payload when used with a <see cref="DiagnosticSource" />.
+        ///     </para>
+        /// </summary>
+        public static readonly EventId DecimalTypeKeyWarning = MakeValidationId(Id.DecimalTypeKeyWarning);
+
         /// <summary>
         ///     <para>
         ///         No explicit type for a decimal column.

src/EFCore.SqlServer/Internal/SqlServerLoggerExtensions.cs

@@ -16,6 +16,43 @@ namespace Microsoft.EntityFrameworkCore.SqlServer.Internal
     /// </summary>
     public static class SqlServerLoggerExtensions
     {
+        /// <summary>
+        ///     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.
+        /// </summary>
+        public static void DecimalTypeKeyWarning(
+            [NotNull] this IDiagnosticsLogger<DbLoggerCategory.Model.Validation> diagnostics,
+            [NotNull] IProperty property)
+        {
+            var definition = SqlServerResources.LogDecimalTypeKey(diagnostics);
+
+            if (diagnostics.ShouldLog(definition))
+            {
+                definition.Log(diagnostics, property.Name, property.DeclaringEntityType.DisplayName());
+            }
+
+            if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled))
+            {
+                var eventData = new PropertyEventData(
+                    definition,
+                    DecimalTypeKeyWarning,
+                    property);
+
+                diagnostics.DispatchEventData(definition, eventData, diagnosticSourceEnabled, simpleLogEnabled);
+            }
+        }
+
+        private static string DecimalTypeKeyWarning(EventDefinitionBase definition, EventData payload)
+        {
+            var d = (EventDefinition<string, string>)definition;
+            var p = (PropertyEventData)payload;
+            return d.GenerateMessage(
+                p.Property.Name,
+                p.Property.DeclaringEntityType.DisplayName());
+        }
+
         /// <summary>
         ///     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

src/EFCore.SqlServer/Internal/SqlServerModelValidator.cs

@@ -55,7 +55,7 @@ public override void Validate(IModel model, IDiagnosticsLogger<DbLoggerCategory.
 
             base.Validate(model, logger);
 
-            ValidateDefaultDecimalMapping(model, logger);
+            ValidateDecimalColumns(model, logger);
             ValidateByteIdentityMapping(model, logger);
             ValidateNonKeyValueGeneration(model, logger);
         }
@@ -66,32 +66,37 @@ public override void Validate(IModel model, IDiagnosticsLogger<DbLoggerCategory.
         ///     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.
         /// </summary>
-        protected virtual void ValidateDefaultDecimalMapping(
+        protected virtual void ValidateDecimalColumns(
             [NotNull] IModel model, [NotNull] IDiagnosticsLogger<DbLoggerCategory.Model.Validation> logger)
         {
-            foreach (var property in model.GetEntityTypes()
+            foreach (IConventionProperty property in model.GetEntityTypes()
                 .SelectMany(t => t.GetDeclaredProperties())
-                .Where(
-                    p => p.ClrType.UnwrapNullableType() == typeof(decimal)
+                .Where(p => p.ClrType.UnwrapNullableType() == typeof(decimal)
                         && !p.IsForeignKey()))
             {
-                var valueConverterConfigurationSource = (property as IConventionProperty)?.GetValueConverterConfigurationSource();
+                var valueConverterConfigurationSource = property.GetValueConverterConfigurationSource();
                 var valueConverterProviderType = property.GetValueConverter()?.ProviderClrType;
                 if (!ConfigurationSource.Convention.Overrides(valueConverterConfigurationSource)
                     && typeof(decimal) != valueConverterProviderType)
                 {
                     continue;
                 }
 
-                var columnTypeConfigurationSource = (property as IConventionProperty)?.GetColumnTypeConfigurationSource();
-                var typeMappingConfigurationSource = (property as IConventionProperty)?.GetTypeMappingConfigurationSource();
-                if ((columnTypeConfigurationSource == null
-                        && ConfigurationSource.Convention.Overrides(typeMappingConfigurationSource))
+                var columnTypeConfigurationSource = property.GetColumnTypeConfigurationSource();
+                if (((columnTypeConfigurationSource == null
+                        && ConfigurationSource.Convention.Overrides(property.GetTypeMappingConfigurationSource()))
                     || (columnTypeConfigurationSource != null
                         && ConfigurationSource.Convention.Overrides(columnTypeConfigurationSource)))
+                    && (ConfigurationSource.Convention.Overrides(property.GetPrecisionConfigurationSource())
+                        || ConfigurationSource.Convention.Overrides(property.GetScaleConfigurationSource())))
                 {
                     logger.DecimalTypeDefaultWarning(property);
                 }
+
+                if (property.IsKey())
+                {
+                    logger.DecimalTypeKeyWarning(property);
+                }
             }
         }
 

src/EFCore.SqlServer/Properties/SqlServerStrings.resx

@@ -142,7 +142,7 @@
     <value>An exception has been raised that is likely due to a transient failure. Consider enabling transient error resiliency by adding 'EnableRetryOnFailure()' to the 'UseSqlServer' call.</value>
   </data>
   <data name="LogDefaultDecimalTypeColumn" xml:space="preserve">
-    <value>No type was specified for the decimal column '{property}' on entity type '{entityType}'. This will cause values to be silently truncated if they do not fit in the default precision and scale. Explicitly specify the SQL server column type that can accommodate all the values using 'HasColumnType()' or specify a ValueConverter.</value>
+    <value>No type was specified for the decimal property '{property}' on entity type '{entityType}'. This will cause values to be silently truncated if they do not fit in the default precision and scale. Explicitly specify the SQL server column type that can accommodate all the values using 'HasColumnType()', specify precision and scale using 'HasPrecision()' or configure a value converter using 'HasConversion()'.</value>
     <comment>Warning SqlServerEventId.DecimalTypeDefaultWarning string string</comment>
   </data>
   <data name="LogByteIdentityColumn" xml:space="preserve">
@@ -269,4 +269,8 @@
   <data name="DuplicateIndexFillFactorMismatch" xml:space="preserve">
     <value>The indexes {index1} on '{entityType1}' and {index2} on '{entityType2}' are both mapped to '{table}.{indexName}' but with different fill factor configuration.</value>
   </data>
+  <data name="LogDecimalTypeKey" xml:space="preserve">
+    <value>The decimal property '{property}' is part of a key on entity type '{entityType}'. If the configured precision and scale don't match the column type in the database this will cause values to be silently truncated if they do not fit in the default precision and scale. Consider using a different property as the key or make sure that the database column type matches the model configuration and enable decimal rounding warnings using 'SET NUMERIC_ROUNDABORT ON'.</value>
+    <comment>Warning SqlServerEventId.DecimalTypeKeyWarning string string</comment>
+  </data>
 </root>

test/EFCore.Specification.Tests/ServiceProviderFixtureBase.cs

@@ -16,8 +16,7 @@ public abstract class ServiceProviderFixtureBase : FixtureBase
         private ListLoggerFactory _listLoggerFactory;
 
         public ListLoggerFactory ListLoggerFactory
-            => _listLoggerFactory
-                ?? (_listLoggerFactory = (ListLoggerFactory)ServiceProvider.GetRequiredService<ILoggerFactory>());
+            => _listLoggerFactory ??= (ListLoggerFactory)ServiceProvider.GetRequiredService<ILoggerFactory>();
 
         protected ServiceProviderFixtureBase()
         {

test/EFCore.SqlServer.FunctionalTests/BuiltInDataTypesSqlServerTest.cs

@@ -3101,8 +3101,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
             public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder builder)
             {
                 var options = base.AddOptions(builder).ConfigureWarnings(
-                    c => c
-                        .Log(SqlServerEventId.DecimalTypeDefaultWarning));
+                    c => c.Log(SqlServerEventId.DecimalTypeDefaultWarning));
 
                 new SqlServerDbContextOptionsBuilder(options).MinBatchSize(1);
 

test/EFCore.SqlServer.FunctionalTests/SqlServerFixture.cs

@@ -22,6 +22,7 @@ public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder build
                 w =>
                 {
                     w.Log(SqlServerEventId.ByteIdentityColumnWarning);
+                    w.Log(SqlServerEventId.DecimalTypeKeyWarning);
                 });
     }
 }

test/EFCore.SqlServer.FunctionalTests/UpdatesSqlServerFixture.cs

@@ -1,6 +1,7 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using Microsoft.EntityFrameworkCore.Diagnostics;
 using Microsoft.EntityFrameworkCore.TestModels.UpdatesModel;
 using Microsoft.EntityFrameworkCore.TestUtilities;
 
@@ -10,6 +11,13 @@ public class UpdatesSqlServerFixture : UpdatesRelationalFixture
     {
         protected override ITestStoreFactory TestStoreFactory => SqlServerTestStoreFactory.Instance;
 
+        public override DbContextOptionsBuilder AddOptions(DbContextOptionsBuilder builder)
+            => base.AddOptions(builder).ConfigureWarnings(
+                w =>
+                {
+                    w.Log(SqlServerEventId.DecimalTypeKeyWarning);
+                });
+
         protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext context)
         {
             base.OnModelCreating(modelBuilder, context);

test/EFCore.SqlServer.Tests/Infrastructure/SqlServerModelValidatorTest.cs

@@ -347,6 +347,19 @@ public virtual void Detects_incompatible_non_clustered_shared_key()
                 modelBuilder.Model);
         }
 
+        [ConditionalFact]
+        public virtual void Detects_decimal_keys()
+        {
+            var modelBuilder = CreateConventionalModelBuilder();
+            modelBuilder.Entity<Animal>()
+                .Property<decimal>("Price").HasPrecision(18, 2);
+            modelBuilder.Entity<Animal>().HasKey("Price");
+
+            VerifyWarning(
+                SqlServerResources.LogDecimalTypeKey(new TestLogger<SqlServerLoggingDefinitions>())
+                    .GenerateMessage("Price", nameof(Animal)), modelBuilder.Model);
+        }
+
         [ConditionalFact]
         public virtual void Detects_default_decimal_mapping()
         {
@@ -369,6 +382,18 @@ public virtual void Detects_default_nullable_decimal_mapping()
                     .GenerateMessage("Price", nameof(Animal)), modelBuilder.Model);
         }
 
+        [ConditionalFact]
+        public virtual void Does_not_warn_if_decimal_column_has_precision_and_scale()
+        {
+            var modelBuilder = CreateConventionalModelBuilder();
+            modelBuilder.Entity<Animal>()
+                .Property<decimal>("Price").HasPrecision(18, 2);
+
+            VerifyLogDoesNotContain(
+                SqlServerResources.LogDefaultDecimalTypeColumn(new TestLogger<SqlServerLoggingDefinitions>())
+                    .GenerateMessage("Price", nameof(Animal)), modelBuilder.Model);
+        }
+
         [ConditionalFact]
         public virtual void Does_not_warn_if_default_decimal_mapping_has_non_decimal_to_decimal_value_converter()
         {