Change value generation defaults for numeric keys when using TPC (#28529

)

src/EFCore.Relational/Metadata/Conventions/RelationalValueGenerationConvention.cs

@@ -112,6 +112,19 @@ public virtual void ProcessEntityTypeAnnotationChanged(
                 tableName,
                 entityTypeBuilder.Metadata.GetSchema());
         }
+        else if (name == RelationalAnnotationNames.MappingStrategy)
+        {
+            var primaryKey = entityTypeBuilder.Metadata.FindPrimaryKey();
+            if (primaryKey == null)
+            {
+                return;
+            }
+
+            foreach (var property in primaryKey.Properties)
+            {
+                property.Builder.ValueGenerated(GetValueGenerated(property));
+            }
+        }
     }
 
     private static void ProcessTableChanged(
@@ -123,14 +136,18 @@ private static void ProcessTableChanged(
     {
         if (newTable == null)
         {
-            foreach (var property in entityTypeBuilder.Metadata.GetProperties())
+            if (entityTypeBuilder.Metadata.GetDerivedTypes().All(e => e.GetTableName() == null))
             {
-                property.Builder.ValueGenerated(null);
+                foreach (var property in entityTypeBuilder.Metadata.GetProperties())
+                {
+                    property.Builder.ValueGenerated(null);
+                }
             }
 
             return;
         }
-        else if (oldTable == null)
+
+        if (oldTable == null)
         {
             foreach (var property in entityTypeBuilder.Metadata.GetProperties())
             {
@@ -172,7 +189,33 @@ private static void ProcessTableChanged(
 
         return tableName == null
             ? null
-            : GetValueGenerated(property, StoreObjectIdentifier.Table(tableName, property.DeclaringEntityType.GetSchema()));
+            : !MappingStrategyAllowsValueGeneration(property, property.DeclaringEntityType.GetMappingStrategy())
+                ? null
+                : GetValueGenerated(property, StoreObjectIdentifier.Table(tableName, property.DeclaringEntityType.GetSchema()));
+    }
+
+    /// <summary>
+    ///     Checks whether or not the mapping strategy and property allow value generation by convention.
+    /// </summary>
+    /// <param name="property">The property for which value generation is being considered.</param>
+    /// <param name="mappingStrategy">The current mapping strategy.</param>
+    /// <returns><see langword="true" /> if value generation is allowed; <see langword="false" /> otherwise.</returns>
+    protected virtual bool MappingStrategyAllowsValueGeneration(
+        IConventionProperty property,
+        string? mappingStrategy)
+    {
+        if (mappingStrategy == RelationalAnnotationNames.TpcMappingStrategy)
+        {
+            var propertyType = property.ClrType.UnwrapNullableType();
+            if (property.IsPrimaryKey()
+                && propertyType.IsInteger()
+                && propertyType != typeof(byte))
+            {
+                return false;
+            }
+        }
+
+        return true;
     }
 
     /// <summary>

src/EFCore.SqlServer/Extensions/SqlServerModelBuilderExtensions.cs

@@ -150,63 +150,6 @@ public static ModelBuilder UseKeySequences(
         return modelBuilder;
     }
 
-    /// <summary>
-    ///     Configures the database sequence to generate values for key properties
-    ///     marked as <see cref="ValueGenerated.OnAdd" />, when targeting SQL Server.
-    /// </summary>
-    /// <remarks>
-    ///     See <see href="https://aka.ms/efcore-docs-modeling">Modeling entity types and relationships</see>, and
-    ///     <see href="https://aka.ms/efcore-docs-sqlserver">Accessing SQL Server and SQL Azure databases with EF Core</see>
-    ///     for more information and examples.
-    /// </remarks>
-    /// <param name="modelBuilder">The model builder.</param>
-    /// <param name="name">The name of the sequence.</param>
-    /// <param name="schema">The schema of the sequence.</param>
-    /// <param name="fromDataAnnotation">Indicates whether the configuration was specified using a data annotation.</param>
-    /// <returns>A builder to further configure the sequence.</returns>
-    public static IConventionSequenceBuilder? HasKeySequences(
-        this IConventionModelBuilder modelBuilder,
-        string? name,
-        string? schema,
-        bool fromDataAnnotation = false)
-    {
-        if (!modelBuilder.CanSetKeySequences(name, schema))
-        {
-            return null;
-        }
-
-        modelBuilder.Metadata.SetSequenceNameSuffix(name, fromDataAnnotation);
-        modelBuilder.Metadata.SetSequenceSchema(schema, fromDataAnnotation);
-
-        return null;
-    }
-
-    /// <summary>
-    ///     Returns a value indicating whether the given name and schema can be set for the key value generation sequence.
-    /// </summary>
-    /// <remarks>
-    ///     See <see href="https://aka.ms/efcore-docs-modeling">Modeling entity types and relationships</see>, and
-    ///     <see href="https://aka.ms/efcore-docs-sqlserver">Accessing SQL Server and SQL Azure databases with EF Core</see>
-    ///     for more information and examples.
-    /// </remarks>
-    /// <param name="modelBuilder">The model builder.</param>
-    /// <param name="nameSuffix">The name of the sequence.</param>
-    /// <param name="schema">The schema of the sequence.</param>
-    /// <param name="fromDataAnnotation">Indicates whether the configuration was specified using a data annotation.</param>
-    /// <returns><see langword="true" /> if the given name and schema can be set for the hi-lo sequence.</returns>
-    public static bool CanSetKeySequences(
-        this IConventionModelBuilder modelBuilder,
-        string? nameSuffix,
-        string? schema,
-        bool fromDataAnnotation = false)
-    {
-        Check.NullButNotEmpty(nameSuffix, nameof(nameSuffix));
-        Check.NullButNotEmpty(schema, nameof(schema));
-
-        return modelBuilder.CanSetAnnotation(SqlServerAnnotationNames.SequenceNameSuffix, nameSuffix, fromDataAnnotation)
-            && modelBuilder.CanSetAnnotation(SqlServerAnnotationNames.SequenceSchema, schema, fromDataAnnotation);
-    }
-
     /// <summary>
     ///     Configures the model to use the SQL Server IDENTITY feature to generate values for key properties
     ///     marked as <see cref="ValueGenerated.OnAdd" />, when targeting SQL Server. This is the default
@@ -381,13 +324,13 @@ public static bool CanSetIdentityColumnIncrement(
             {
                 modelBuilder.HasIdentityColumnSeed(null, fromDataAnnotation);
                 modelBuilder.HasIdentityColumnIncrement(null, fromDataAnnotation);
-                modelBuilder.HasKeySequences(null, null, fromDataAnnotation);
+                RemoveKeySequenceAnnotations();
             }
 
             if (valueGenerationStrategy != SqlServerValueGenerationStrategy.SequenceHiLo)
             {
                 modelBuilder.HasHiLoSequence(null, null, fromDataAnnotation);
-                modelBuilder.HasKeySequences(null, null, fromDataAnnotation);
+                RemoveKeySequenceAnnotations();
             }
 
             if (valueGenerationStrategy != SqlServerValueGenerationStrategy.Sequence)
@@ -401,6 +344,16 @@ public static bool CanSetIdentityColumnIncrement(
         }
 
         return null;
+
+        void RemoveKeySequenceAnnotations()
+        {
+            if (modelBuilder.CanSetAnnotation(SqlServerAnnotationNames.SequenceNameSuffix, null)
+                && modelBuilder.CanSetAnnotation(SqlServerAnnotationNames.SequenceSchema, null))
+            {
+                modelBuilder.Metadata.SetSequenceNameSuffix(null, fromDataAnnotation);
+                modelBuilder.Metadata.SetSequenceSchema(null, fromDataAnnotation);
+            }
+        }
     }
 
     /// <summary>

src/EFCore.SqlServer/Extensions/SqlServerPropertyExtensions.cs

@@ -858,7 +858,9 @@ private static SqlServerValueGenerationStrategy GetDefaultValueGenerationStrateg
 
         return modelStrategy == SqlServerValueGenerationStrategy.IdentityColumn
             && IsCompatibleWithValueGeneration(property, storeObject, typeMappingSource)
-                ? SqlServerValueGenerationStrategy.IdentityColumn
+                ? property.DeclaringEntityType.GetMappingStrategy() == RelationalAnnotationNames.TpcMappingStrategy
+                    ? SqlServerValueGenerationStrategy.Sequence
+                    : SqlServerValueGenerationStrategy.IdentityColumn
                 : SqlServerValueGenerationStrategy.None;
     }
 

test/EFCore.Design.Tests/Migrations/ModelSnapshotSqlServerTest.cs

@@ -679,13 +679,16 @@ public virtual void Entities_are_stored_in_model_snapshot_for_TPC()
             AddBoilerPlate(
                 GetHeading()
                 + @"
+            modelBuilder.HasSequence(""AbstractBaseSequence"");
+
             modelBuilder.Entity(""Microsoft.EntityFrameworkCore.Migrations.ModelSnapshotSqlServerTest+AbstractBase"", b =>
                 {
                     b.Property<int>(""Id"")
                         .ValueGeneratedOnAdd()
-                        .HasColumnType(""int"");
+                        .HasColumnType(""int"")
+                        .HasDefaultValueSql(""NEXT VALUE FOR [AbstractBaseSequence]"");
 
-                    SqlServerPropertyBuilderExtensions.UseIdentityColumn(b.Property<int>(""Id""));
+                    SqlServerPropertyBuilderExtensions.UseSequence(b.Property<int>(""Id""));
 
                     b.HasKey(""Id"");
 
@@ -714,7 +717,7 @@ public virtual void Entities_are_stored_in_model_snapshot_for_TPC()
                 });"),
             model =>
             {
-                Assert.Equal(4, model.GetAnnotations().Count());
+                Assert.Equal(5, model.GetAnnotations().Count());
                 Assert.Equal(3, model.GetEntityTypes().Count());
 
                 var abstractBase = model.FindEntityType("Microsoft.EntityFrameworkCore.Migrations.ModelSnapshotSqlServerTest+AbstractBase");

test/EFCore.Design.Tests/Scaffolding/Internal/CSharpRuntimeModelCodeGeneratorTest.cs

@@ -41,7 +41,6 @@ namespace Microsoft.EntityFrameworkCore.Scaffolding.Internal
 {
     public class CSharpRuntimeModelCodeGeneratorTest
     {
-
         [ConditionalFact]
         public void Self_referential_property()
             => Test(
@@ -2348,6 +2347,7 @@ static TpcContextModel()
                             "TpcContextModelBuilder.cs",
                             @"// <auto-generated />
 using System;
+using System.Collections.Generic;
 using Microsoft.EntityFrameworkCore.Infrastructure;
 using Microsoft.EntityFrameworkCore.Metadata;
 
@@ -2372,6 +2372,16 @@ partial void Initialize()
             PrincipalBaseEntityType.CreateAnnotations(principalBase);
             PrincipalDerivedEntityType.CreateAnnotations(principalDerived);
 
+            var sequences = new SortedDictionary<(string, string), ISequence>();
+            var principalBaseSequence = new RuntimeSequence(
+                ""PrincipalBaseSequence"",
+                this,
+                typeof(long),
+                schema: ""TPC"");
+
+            sequences[(""PrincipalBaseSequence"", ""TPC"")] = principalBaseSequence;
+
+            AddAnnotation(""Relational:Sequences"", sequences);
             AddAnnotation(""Relational:DefaultSchema"", ""TPC"");
             AddAnnotation(""Relational:MaxIdentifierLength"", 128);
             AddAnnotation(""SqlServer:ValueGenerationStrategy"", SqlServerValueGenerationStrategy.IdentityColumn);
@@ -2525,7 +2535,8 @@ public static RuntimeEntityType Create(RuntimeModel model, RuntimeEntityType? ba
             overrides.Add(StoreObjectIdentifier.InsertStoredProcedure(""PrincipalBase_Insert"", ""TPC""), idPrincipalBaseInsert);
             id.AddAnnotation(""Relational:RelationalOverrides"", overrides);
 
-            id.AddAnnotation(""SqlServer:ValueGenerationStrategy"", SqlServerValueGenerationStrategy.IdentityColumn);
+            id.AddAnnotation(""Relational:DefaultValueSql"", ""NEXT VALUE FOR [TPC].[PrincipalBaseSequence]"");
+            id.AddAnnotation(""SqlServer:ValueGenerationStrategy"", SqlServerValueGenerationStrategy.Sequence);
 
             var principalBaseId = runtimeEntityType.AddProperty(
                 ""PrincipalBaseId"",
@@ -2788,7 +2799,7 @@ public static void CreateAnnotations(RuntimeEntityType runtimeEntityType)
                     Assert.Equal("DerivedId", id.GetColumnName(StoreObjectIdentifier.Create(principalDerived, StoreObjectType.InsertStoredProcedure).Value));
                     Assert.Equal("bar3",
                         id.FindOverrides(StoreObjectIdentifier.Create(principalDerived, StoreObjectType.InsertStoredProcedure).Value)["foo"]);
-                    
+
                     updateSproc = principalDerived.GetUpdateStoredProcedure()!;
                     Assert.Equal("Derived_Update", updateSproc.Name);
                     Assert.Equal("Derived", updateSproc.Schema);
@@ -2809,7 +2820,7 @@ public static void CreateAnnotations(RuntimeEntityType runtimeEntityType)
                     Assert.Same(principalDerived, deleteSproc.EntityType);
                     Assert.Equal("Id", id.GetColumnName(StoreObjectIdentifier.Create(principalDerived, StoreObjectType.DeleteStoredProcedure).Value));
                     Assert.Null(id.FindOverrides(StoreObjectIdentifier.Create(principalDerived, StoreObjectType.DeleteStoredProcedure).Value));
-                    
+
                     Assert.Equal("PrincipalDerived<DependentBase<byte?>>", principalDerived.GetDiscriminatorValue());
                     Assert.Null(principalDerived.FindDiscriminatorProperty());
                     Assert.Equal("TPC", principalDerived.GetMappingStrategy());
@@ -2903,7 +2914,7 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
 
                         eb.ToTable("PrincipalDerived");
                         eb.ToView("PrincipalDerivedView");
-                        
+
                         eb.InsertUsingStoredProcedure("Derived_Insert", s => s
                             .HasParameter("PrincipalBaseId")
                             .HasParameter("PrincipalDerivedId")

test/EFCore.Relational.Tests/Infrastructure/RelationalModelValidatorTest.cs

@@ -2138,7 +2138,14 @@ public virtual void Detects_view_TPC_with_discriminator()
     public virtual void Detects_store_generated_PK_in_TPC()
     {
         var modelBuilder = CreateConventionModelBuilder();
-        modelBuilder.Entity<Animal>().UseTpcMappingStrategy();
+
+        modelBuilder.Entity<Animal>(
+            b =>
+            {
+                b.UseTpcMappingStrategy();
+                b.Property(e => e.Id).ValueGeneratedOnAdd();
+            });
+
         modelBuilder.Entity<Cat>();
 
         var definition =
@@ -2356,7 +2363,7 @@ public virtual void Detects_invalid_sql_query_overrides()
             RelationalStrings.SqlQueryOverrideMismatch("Animal.Name", "Dog"),
             modelBuilder);
     }
-    
+
     [ConditionalFact]
     public virtual void Detects_invalid_stored_procedure_overrides()
     {
@@ -2595,7 +2602,7 @@ public void Detects_multiple_entity_types_mapped_to_the_same_stored_procedure()
                 "Delete"),
             modelBuilder);
     }
-    
+
     [ConditionalFact]
     public virtual void Detects_keyless_entity_type_mapped_to_a_stored_procedure()
     {
@@ -2818,7 +2825,7 @@ public virtual void Passes_on_derived_entity_type_not_mapped_to_a_stored_procedu
 
         Validate(modelBuilder);
     }
-    
+
     [ConditionalFact]
     public virtual void Detects_missing_stored_procedure_parameters_in_TPT()
     {

test/EFCore.Specification.Tests/Query/InheritanceQueryFixtureBase.cs

@@ -374,17 +374,6 @@ protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext con
         modelBuilder.Entity<Lilt>();
         modelBuilder.Entity<Coke>();
 
-        if (UseGeneratedKeys)
-        {
-            modelBuilder.Entity<Animal>().Property(e => e.Id).ValueGeneratedOnAdd();
-            modelBuilder.Entity<Drink>().Property(e => e.Id).ValueGeneratedOnAdd();
-        }
-        else
-        {
-            modelBuilder.Entity<Animal>().Property(e => e.Id).ValueGeneratedNever();
-            modelBuilder.Entity<Drink>().Property(e => e.Id).ValueGeneratedNever();
-        }
-
         if (HasDiscriminator)
         {
             modelBuilder.Entity<Bird>().HasDiscriminator<string>("Discriminator").IsComplete(IsDiscriminatorMappingComplete);

test/EFCore.SqlServer.FunctionalTests/Query/TPCFiltersInheritanceQuerySqlServerFixture.cs

@@ -7,11 +7,4 @@ public class TPCFiltersInheritanceQuerySqlServerFixture : TPCInheritanceQuerySql
 {
     protected override bool EnableFilters
         => true;
-
-    protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext context)
-    {
-        modelBuilder.UseKeySequences();
-
-        base.OnModelCreating(modelBuilder, context);
-    }
 }

test/EFCore.SqlServer.FunctionalTests/Query/TPCInheritanceQuerySqlServerFixture.cs

@@ -7,11 +7,4 @@ public class TPCInheritanceQuerySqlServerFixture : TPCInheritanceQueryFixture
 {
     protected override ITestStoreFactory TestStoreFactory
         => SqlServerTestStoreFactory.Instance;
-
-    protected override void OnModelCreating(ModelBuilder modelBuilder, DbContext context)
-    {
-        modelBuilder.UseKeySequences();
-
-        base.OnModelCreating(modelBuilder, context);
-    }
 }

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

@@ -52,6 +52,26 @@ public override void Passes_for_ForeignKey_on_inherited_generated_composite_key_
         Validate(modelBuilder);
     }
 
+    public override void Detects_store_generated_PK_in_TPC()
+    {
+        var modelBuilder = CreateConventionModelBuilder();
+
+        modelBuilder.Entity<Animal>(
+            b =>
+            {
+                b.UseTpcMappingStrategy();
+                b.Property(e => e.Id).ValueGeneratedOnAdd();
+            });
+
+        modelBuilder.Entity<Cat>();
+
+        Validate(modelBuilder);
+
+        var keyProperty = modelBuilder.Model.FindEntityType(typeof(Animal))!.FindProperty(nameof(Animal.Id))!;
+        Assert.Equal(ValueGenerated.OnAdd, keyProperty.ValueGenerated);
+        Assert.Equal(SqlServerValueGenerationStrategy.Sequence, keyProperty.GetValueGenerationStrategy());
+    }
+
     [ConditionalFact]
     public virtual void Passes_for_duplicate_column_names_within_hierarchy_with_identity()
     {