SqlServerValueGenerationStrategy which conflicts now causes warning (#…

…19887)

A SqlServerValueGenerationStrategy which conflicts with another value generation strategy now causes a warning instead of an error. The warning throws by default but that can be overridden if wanted.

src/EFCore.SqlServer/Diagnostics/ConflictingValueGenerationStrategiesEventData.cs

@@ -0,0 +1,53 @@
+// 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 System;
+using System.Diagnostics;
+using JetBrains.Annotations;
+using Microsoft.EntityFrameworkCore.Metadata;
+
+namespace Microsoft.EntityFrameworkCore.Diagnostics
+{
+    /// <summary>
+    ///     A <see cref="DiagnosticSource" /> event payload class for events that have
+    ///     a property.
+    /// </summary>
+    public class ConflictingValueGenerationStrategiesEventData : EventData
+    {
+        /// <summary>
+        ///     Constructs the event payload.
+        /// </summary>
+        /// <param name="eventDefinition"> The event definition. </param>
+        /// <param name="messageGenerator"> A delegate that generates a log message for this event. </param>
+        /// <param name="sqlServerValueGenerationStrategy"> The SQL Server value generation strategy. </param>
+        /// <param name="otherValueGenerationStrategy"> The other value generation strategy. </param>
+        /// <param name="property"> The property. </param>
+        public ConflictingValueGenerationStrategiesEventData(
+            [NotNull] EventDefinitionBase eventDefinition,
+            [NotNull] Func<EventDefinitionBase, EventData, string> messageGenerator,
+            SqlServerValueGenerationStrategy sqlServerValueGenerationStrategy,
+            [NotNull] string otherValueGenerationStrategy,
+            [NotNull] IProperty property)
+            : base(eventDefinition, messageGenerator)
+        {
+            SqlServerValueGenerationStrategy = sqlServerValueGenerationStrategy;
+            OtherValueGenerationStrategy = otherValueGenerationStrategy;
+            Property = property;
+        }
+
+        /// <summary>
+        ///     The SQL Server value generation strategy.
+        /// </summary>
+        public virtual SqlServerValueGenerationStrategy SqlServerValueGenerationStrategy { get; }
+
+        /// <summary>
+        ///     The other value generation strategy.
+        /// </summary>
+        public virtual string OtherValueGenerationStrategy { get; }
+
+        /// <summary>
+        ///     The property.
+        /// </summary>
+        public virtual IProperty Property { get; }
+    }
+}

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

@@ -140,5 +140,13 @@ public class SqlServerLoggingDefinitions : RelationalLoggingDefinitions
         ///     doing so can result in application failures when updating to a new Entity Framework Core release.
         /// </summary>
         public EventDefinitionBase LogReflexiveConstraintIgnored;
+
+        /// <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 LogConflictingValueGenerationStrategies;
     }
 }

src/EFCore.SqlServer/Diagnostics/SqlServerEventId.cs

@@ -26,6 +26,7 @@ private enum Id
             // Model validation events
             DecimalTypeDefaultWarning = CoreEventId.ProviderBaseId,
             ByteIdentityColumnWarning,
+            ConflictingValueGenerationStrategiesWarning,
 
             // Scaffolding events
             ColumnFound = CoreEventId.ProviderDesignBaseId,
@@ -89,6 +90,20 @@ private enum Id
         /// </summary>
         public static readonly EventId ByteIdentityColumnWarning = MakeValidationId(Id.ByteIdentityColumnWarning);
 
+        /// <summary>
+        ///     <para>
+        ///         There are conflicting value generation methods for a property.
+        ///     </para>
+        ///     <para>
+        ///         This event is in the <see cref="DbLoggerCategory.Model.Validation" /> category.
+        ///     </para>
+        ///     <para>
+        ///         This event uses the <see cref="ConflictingValueGenerationStrategiesEventData" />
+        ///         payload when used with a <see cref="DiagnosticSource" />.
+        ///     </para>
+        /// </summary>
+        public static readonly EventId ConflictingValueGenerationStrategiesWarning = MakeValidationId(Id.ConflictingValueGenerationStrategiesWarning);
+
         private static readonly string _scaffoldingPrefix = DbLoggerCategory.Scaffolding.Name + ".";
         private static EventId MakeScaffoldingId(Id id) => new EventId((int)id, _scaffoldingPrefix + id);
 

src/EFCore.SqlServer/Extensions/SqlServerDbContextOptionsBuilderExtensions.cs

@@ -169,9 +169,13 @@ var coreOptionsExtension
                 = optionsBuilder.Options.FindExtension<CoreOptionsExtension>()
                 ?? new CoreOptionsExtension();
 
-            coreOptionsExtension = coreOptionsExtension.WithWarningsConfiguration(
+            coreOptionsExtension = coreOptionsExtension.
+                WithWarningsConfiguration(
                 coreOptionsExtension.WarningsConfiguration.TryWithExplicit(
-                    RelationalEventId.AmbientTransactionWarning, WarningBehavior.Throw));
+                    RelationalEventId.AmbientTransactionWarning, WarningBehavior.Throw)).
+                WithWarningsConfiguration(
+                coreOptionsExtension.WarningsConfiguration.TryWithExplicit(
+                    SqlServerEventId.ConflictingValueGenerationStrategiesWarning, WarningBehavior.Throw));
 
             ((IDbContextOptionsBuilderInfrastructure)optionsBuilder).AddOrUpdateExtension(coreOptionsExtension);
         }

src/EFCore.SqlServer/Internal/SqlServerLoggerExtensions.cs

@@ -90,6 +90,50 @@ private static string ByteIdentityColumnWarning(EventDefinitionBase definition,
                 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
+        ///     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 ConflictingValueGenerationStrategiesWarning(
+            [NotNull] this IDiagnosticsLogger<DbLoggerCategory.Model.Validation> diagnostics,
+            SqlServerValueGenerationStrategy sqlServerValueGenerationStrategy,
+            [NotNull] string otherValueGenerationStrategy,
+            [NotNull] IProperty property)
+        {
+            var definition = SqlServerResources.LogConflictingValueGenerationStrategies(diagnostics);
+
+            if (diagnostics.ShouldLog(definition))
+            {
+                definition.Log(diagnostics, sqlServerValueGenerationStrategy.ToString(), otherValueGenerationStrategy,
+                    property.Name, property.DeclaringEntityType.DisplayName());
+            }
+
+            if (diagnostics.NeedsEventData(definition, out var diagnosticSourceEnabled, out var simpleLogEnabled))
+            {
+                var eventData = new ConflictingValueGenerationStrategiesEventData(
+                    definition,
+                    ConflictingValueGenerationStrategiesWarning,
+                    sqlServerValueGenerationStrategy,
+                    otherValueGenerationStrategy,
+                    property);
+
+                diagnostics.DispatchEventData(definition, eventData, diagnosticSourceEnabled, simpleLogEnabled);
+            }
+        }
+
+        private static string ConflictingValueGenerationStrategiesWarning(EventDefinitionBase definition, EventData payload)
+        {
+            var d = (EventDefinition<string, string, string, string>)definition;
+            var p = (ConflictingValueGenerationStrategiesEventData)payload;
+            return d.GenerateMessage(
+                p.SqlServerValueGenerationStrategy.ToString(),
+                p.OtherValueGenerationStrategy,
+                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/Metadata/Conventions/SqlServerStoreGenerationConvention.cs

@@ -6,6 +6,7 @@
 using Microsoft.EntityFrameworkCore.Diagnostics;
 using Microsoft.EntityFrameworkCore.Metadata.Builders;
 using Microsoft.EntityFrameworkCore.Metadata.Conventions.Infrastructure;
+using Microsoft.EntityFrameworkCore.SqlServer.Internal;
 using Microsoft.EntityFrameworkCore.SqlServer.Metadata.Internal;
 
 // ReSharper disable once CheckNamespace
@@ -102,25 +103,24 @@ protected override void Validate(IConventionProperty property)
             if (property.GetValueGenerationStrategyConfigurationSource() != null
                 && property.GetValueGenerationStrategy() != SqlServerValueGenerationStrategy.None)
             {
+                var generationStrategy = property.GetValueGenerationStrategy();
+
                 if (property.GetDefaultValue() != null)
                 {
-                    throw new InvalidOperationException(
-                        RelationalStrings.ConflictingColumnServerGeneration(
-                            "SqlServerValueGenerationStrategy", property.Name, "DefaultValue"));
+                    Dependencies.ValidationLogger.ConflictingValueGenerationStrategiesWarning(
+                        generationStrategy, "DefaultValue", property);
                 }
 
                 if (property.GetDefaultValueSql() != null)
                 {
-                    throw new InvalidOperationException(
-                        RelationalStrings.ConflictingColumnServerGeneration(
-                            "SqlServerValueGenerationStrategy", property.Name, "DefaultValueSql"));
+                    Dependencies.ValidationLogger.ConflictingValueGenerationStrategiesWarning(
+                        generationStrategy, "DefaultValueSql", property);
                 }
 
                 if (property.GetComputedColumnSql() != null)
                 {
-                    throw new InvalidOperationException(
-                        RelationalStrings.ConflictingColumnServerGeneration(
-                            "SqlServerValueGenerationStrategy", property.Name, "ComputedColumnSql"));
+                    Dependencies.ValidationLogger.ConflictingValueGenerationStrategiesWarning(
+                        generationStrategy, "ComputedColumnSql", property);
                 }
             }
 

src/EFCore.SqlServer/Properties/SqlServerStrings.resx

@@ -241,4 +241,8 @@
   <data name="FunctionOnClient" xml:space="preserve">
     <value>The '{methodName}' method is not supported because the query has switched to client-evaluation. Inspect the log to determine which query expressions are triggering client-evaluation.</value>
   </data>
+  <data name="LogConflictingValueGenerationStrategies" xml:space="preserve">
+    <value>Both the SqlServerValueGenerationStrategy {generationStrategy} and {otherGenerationStrategy} have been set on property '{propertyName}' on entity type '{entityName}'. Usually this is a mistake. Only use these at the same time if you are sure you understand the consequences.</value>
+    <comment>Warning SqlServerEventId.ConflictingValueGenerationStrategiesWarning string string string string</comment>
+  </data>
 </root>

test/EFCore.SqlServer.FunctionalTests/SqlServerValueGenerationConflictTest.cs

@@ -0,0 +1,150 @@
+// 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 System;
+using System.Linq;
+using Microsoft.EntityFrameworkCore.Diagnostics;
+using Microsoft.EntityFrameworkCore.Infrastructure;
+using Microsoft.EntityFrameworkCore.Metadata;
+using Microsoft.EntityFrameworkCore.Metadata.Conventions.Infrastructure;
+using Microsoft.EntityFrameworkCore.SqlServer.Diagnostics.Internal;
+using Microsoft.EntityFrameworkCore.SqlServer.Internal;
+using Microsoft.EntityFrameworkCore.TestUtilities;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging;
+using Xunit;
+
+// ReSharper disable InconsistentNaming
+namespace Microsoft.EntityFrameworkCore
+{
+    public class SqlServerValueGenerationStrategyThrowTest :
+        SqlServerValueGenerationConflictTest<SqlServerValueGenerationStrategyThrowTest.ThrowContext>
+    {
+        public SqlServerValueGenerationStrategyThrowTest(
+            SqlServerValueGenerationStrategyFixture<ThrowContext> fixture)
+            : base(fixture)
+        {
+        }
+
+        [ConditionalFact]
+        public virtual void SqlServerValueGeneration_conflicting_with_existing_ValueGeneration_strategy_throws()
+        {
+            var modelBuilder = CreateModelBuilder();
+            modelBuilder.Entity<Fred>()
+                .Property(e => e.Id)
+                .HasDefaultValueSql("2")
+                .UseHiLo();
+
+            Assert.Equal(
+                CoreStrings.WarningAsErrorTemplate(
+                    SqlServerEventId.ConflictingValueGenerationStrategiesWarning,
+                    SqlServerResources.LogConflictingValueGenerationStrategies(
+                        new TestLogger<SqlServerLoggingDefinitions>())
+                        .GenerateMessage(SqlServerValueGenerationStrategy.SequenceHiLo.ToString(), "DefaultValueSql", "Id", nameof(Fred)),
+                    "SqlServerEventId.ConflictingValueGenerationStrategiesWarning"),
+                Assert.Throws<InvalidOperationException>(() => Validate(modelBuilder)).Message);
+        }
+
+        public class ThrowContext : DbContext
+        {
+            public ThrowContext(DbContextOptions options)
+                : base(options)
+            {
+            }
+
+            public virtual DbSet<Fred> Freds { get; set; }
+
+            // use the normal behavior of ConflictingValueGenerationStrategiesWarning
+            // defined in UseSqlServer()
+            protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
+                => optionsBuilder.UseSqlServer();
+        }
+    }
+
+    public class SqlServerValueGenerationStrategyNoThrowTest :
+        SqlServerValueGenerationConflictTest<SqlServerValueGenerationStrategyNoThrowTest.NoThrowContext>
+    {
+        public SqlServerValueGenerationStrategyNoThrowTest(
+            SqlServerValueGenerationStrategyFixture<NoThrowContext> fixture)
+            : base(fixture)
+        {
+        }
+
+        [ConditionalFact]
+        public virtual void SqlServerValueGeneration_conflicting_with_existing_ValueGeneration_strategy_warns()
+        {
+            var modelBuilder = CreateModelBuilder();
+            modelBuilder.Entity<Fred>()
+                .Property(e => e.Id)
+                .HasDefaultValueSql("2")
+                .UseHiLo();
+
+            // Assert - this does not throw
+            Validate(modelBuilder);
+
+            var logEntry = Fixture.ListLoggerFactory.Log.Single(
+                l => l.Level == LogLevel.Warning && l.Id == SqlServerEventId.ConflictingValueGenerationStrategiesWarning);
+            Assert.Equal(SqlServerResources.LogConflictingValueGenerationStrategies(
+                        new TestLogger<SqlServerLoggingDefinitions>())
+                        .GenerateMessage(SqlServerValueGenerationStrategy.SequenceHiLo.ToString(), "DefaultValueSql", "Id", nameof(Fred)),
+                        logEntry.Message);
+        }
+
+        public class NoThrowContext : DbContext
+        {
+            public NoThrowContext(DbContextOptions options) : base(options)
+            {
+            }
+
+            public virtual DbSet<Fred> Freds { get; set; }
+
+            // override the normal behavior of ConflictingValueGenerationStrategiesWarning
+            protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
+                => optionsBuilder
+                    .UseSqlServer()
+                    .ConfigureWarnings(
+                        b => b.Log(SqlServerEventId.ConflictingValueGenerationStrategiesWarning));
+        }
+    }
+
+    public class SqlServerValueGenerationConflictTest<TContext>
+        : IClassFixture<SqlServerValueGenerationStrategyFixture<TContext>>
+        where TContext : DbContext
+    {
+        public SqlServerValueGenerationConflictTest(SqlServerValueGenerationStrategyFixture<TContext> fixture)
+            => Fixture = fixture;
+
+        protected SqlServerValueGenerationStrategyFixture<TContext> Fixture { get; }
+
+        public TContext CreateContext() => (TContext)Fixture.CreateContext();
+
+        protected virtual ModelBuilder CreateModelBuilder()
+        {
+            var conventionSet = CreateConventionSetBuilder(CreateContext()).CreateConventionSet();
+
+            return new ModelBuilder(conventionSet);
+        }
+
+        protected virtual IConventionSetBuilder CreateConventionSetBuilder(DbContext context)
+            => context.GetService<IConventionSetBuilder>();
+
+        protected virtual IModel Validate(ModelBuilder modelBuilder)
+            => modelBuilder.FinalizeModel();
+
+        public class Fred
+        {
+            public int Id { get; set; }
+        }
+    }
+
+    public class SqlServerValueGenerationStrategyFixture<TContext> : SharedStoreFixtureBase<DbContext>
+        where TContext : DbContext
+    {
+        protected override string StoreName { get; } = "SqlServerValueGenerationStrategy";
+        protected override Type ContextType { get; } = typeof(TContext);
+        protected override ITestStoreFactory TestStoreFactory => SqlServerTestStoreFactory.Instance;
+        protected override bool ShouldLogCategory(string logCategory)
+            => logCategory == DbLoggerCategory.Model.Validation.Name;
+        protected override bool UsePooling => false;
+    }
+}