From b34a195ad19957f8017066a3f2bf13c55e8a279b Mon Sep 17 00:00:00 2001 From: Dean Hunter Date: Tue, 23 Jan 2024 10:21:20 +0000 Subject: [PATCH 1/2] Add FILLFACTOR for keys Fixes #32803 --- .../Migrations/MigrationsSqlGenerator.cs | 16 + .../SqlServerAnnotationCodeGenerator.cs | 17 +- ...verCSharpRuntimeAnnotationCodeGenerator.cs | 2 + .../SqlServerKeyBuilderExtensions.cs | 82 ++++++ .../Extensions/SqlServerKeyExtensions.cs | 81 ++++++ .../SqlServerRuntimeModelConvention.cs | 1 + .../Internal/SqlServerAnnotationProvider.cs | 5 + .../SqlServerMigrationsSqlGenerator.cs | 29 ++ .../Internal/SqlServerDatabaseModelFactory.cs | 20 +- ...rpMigrationsGeneratorTest.ModelSnapshot.cs | 66 +++++ .../Migrations/MigrationsSqlServerTest.cs | 45 +++ .../SqlServerDatabaseModelFactoryTest.cs | 55 ++++ .../SqlServerAnnotationCodeGeneratorTest.cs | 45 +++ .../SqlServerBuilderExtensionsTest.cs | 47 +++ .../Migrations/SqlServerModelDifferTest.cs | 274 ++++++++++++++++++ 15 files changed, 777 insertions(+), 8 deletions(-) diff --git a/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs b/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs index 7547570299e..e1b4c6c0654 100644 --- a/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs +++ b/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs @@ -238,8 +238,11 @@ protected virtual void Generate( .Append("ALTER TABLE ") .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Table, operation.Schema)) .Append(" ADD "); + PrimaryKeyConstraint(operation, model, builder); + KeyOptions(operation, builder); + if (terminate) { builder.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator); @@ -263,7 +266,11 @@ protected virtual void Generate( .Append("ALTER TABLE ") .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Table, operation.Schema)) .Append(" ADD "); + UniqueConstraint(operation, model, builder); + + KeyOptions(operation, builder); + builder.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator); EndStatement(builder); } @@ -1671,6 +1678,15 @@ protected virtual void CheckConstraint( .Append(")"); } + /// + /// Generates a SQL fragment for extras where options of an key from a or . + /// + /// The operation. + /// The command builder to use to add the SQL fragment. + protected virtual void KeyOptions(MigrationOperation operation, MigrationCommandListBuilder builder) + { + } + /// /// Generates a SQL fragment for traits of an index from a , /// , or . diff --git a/src/EFCore.SqlServer/Design/Internal/SqlServerAnnotationCodeGenerator.cs b/src/EFCore.SqlServer/Design/Internal/SqlServerAnnotationCodeGenerator.cs index faca2555765..2c0bebd2ab1 100644 --- a/src/EFCore.SqlServer/Design/Internal/SqlServerAnnotationCodeGenerator.cs +++ b/src/EFCore.SqlServer/Design/Internal/SqlServerAnnotationCodeGenerator.cs @@ -91,6 +91,10 @@ private static readonly MethodInfo KeyIsClusteredMethodInfo = typeof(SqlServerKeyBuilderExtensions).GetRuntimeMethod( nameof(SqlServerKeyBuilderExtensions.IsClustered), [typeof(KeyBuilder), typeof(bool)])!; + private static readonly MethodInfo KeyHasFillFactorMethodInfo + = typeof(SqlServerKeyBuilderExtensions).GetRuntimeMethod( + nameof(SqlServerKeyBuilderExtensions.HasFillFactor), [typeof(KeyBuilder), typeof(int)])!; + private static readonly MethodInfo TableIsTemporalMethodInfo = typeof(SqlServerTableBuilderExtensions).GetRuntimeMethod( nameof(SqlServerTableBuilderExtensions.IsTemporal), [typeof(TableBuilder), typeof(bool)])!; @@ -328,11 +332,16 @@ protected override bool IsHandledByConvention(IProperty property, IAnnotation an /// doing so can result in application failures when updating to a new Entity Framework Core release. /// protected override MethodCallCodeFragment? GenerateFluentApi(IKey key, IAnnotation annotation) - => annotation.Name == SqlServerAnnotationNames.Clustered - ? (bool)annotation.Value! == false + => annotation.Name switch + { + SqlServerAnnotationNames.Clustered => (bool)annotation.Value! == false ? new MethodCallCodeFragment(KeyIsClusteredMethodInfo, false) - : new MethodCallCodeFragment(KeyIsClusteredMethodInfo) - : null; + : new MethodCallCodeFragment(KeyIsClusteredMethodInfo), + + SqlServerAnnotationNames.FillFactor => new MethodCallCodeFragment(KeyHasFillFactorMethodInfo, annotation.Value), + + _ => null + }; /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to diff --git a/src/EFCore.SqlServer/Design/Internal/SqlServerCSharpRuntimeAnnotationCodeGenerator.cs b/src/EFCore.SqlServer/Design/Internal/SqlServerCSharpRuntimeAnnotationCodeGenerator.cs index 54d27476513..49f80dbd033 100644 --- a/src/EFCore.SqlServer/Design/Internal/SqlServerCSharpRuntimeAnnotationCodeGenerator.cs +++ b/src/EFCore.SqlServer/Design/Internal/SqlServerCSharpRuntimeAnnotationCodeGenerator.cs @@ -132,6 +132,7 @@ public override void Generate(IKey key, CSharpRuntimeAnnotationCodeGeneratorPara { var annotations = parameters.Annotations; annotations.Remove(SqlServerAnnotationNames.Clustered); + annotations.Remove(SqlServerAnnotationNames.FillFactor); } base.Generate(key, parameters); @@ -144,6 +145,7 @@ public override void Generate(IUniqueConstraint uniqueConstraint, CSharpRuntimeA { var annotations = parameters.Annotations; annotations.Remove(SqlServerAnnotationNames.Clustered); + annotations.Remove(SqlServerAnnotationNames.FillFactor); } base.Generate(uniqueConstraint, parameters); diff --git a/src/EFCore.SqlServer/Extensions/SqlServerKeyBuilderExtensions.cs b/src/EFCore.SqlServer/Extensions/SqlServerKeyBuilderExtensions.cs index 0deb6c04875..4164b264750 100644 --- a/src/EFCore.SqlServer/Extensions/SqlServerKeyBuilderExtensions.cs +++ b/src/EFCore.SqlServer/Extensions/SqlServerKeyBuilderExtensions.cs @@ -96,4 +96,86 @@ public static bool CanSetIsClustered( bool? clustered, bool fromDataAnnotation = false) => keyBuilder.CanSetAnnotation(SqlServerAnnotationNames.Clustered, clustered, fromDataAnnotation); + + /// + /// Configures whether the key is created with fill factor option when targeting SQL Server. + /// + /// + /// See Modeling entity types and relationships, and + /// Accessing SQL Server and Azure SQL databases with EF Core + /// for more information and examples. + /// + /// The builder for the key being configured. + /// A value indicating whether the key is created with fill factor option. + /// A builder to further configure the key. + public static KeyBuilder HasFillFactor(this KeyBuilder keyBuilder, int fillFactor) + { + keyBuilder.Metadata.SetFillFactor(fillFactor); + + return keyBuilder; + } + + /// + /// Configures whether the key is created with fill factor option when targeting SQL Server. + /// + /// + /// See Modeling entity types and relationships, and + /// Accessing SQL Server and Azure SQL databases with EF Core + /// for more information and examples. + /// + /// The builder for the key being configured. + /// A value indicating whether the key is created with fill factor option. + /// A builder to further configure the key. + public static KeyBuilder HasFillFactor( + this KeyBuilder keyBuilder, + int fillFactor) + => (KeyBuilder)HasFillFactor((KeyBuilder)keyBuilder, fillFactor); + + /// + /// Configures whether the key is created with fill factor option when targeting SQL Server. + /// + /// + /// See Modeling entity types and relationships, and + /// Accessing SQL Server and Azure SQL databases with EF Core + /// for more information and examples. + /// + /// The builder for the key being configured. + /// A value indicating whether the key is created with fill factor option. + /// Indicates whether the configuration was specified using a data annotation. + /// + /// The same builder instance if the configuration was applied, + /// otherwise. + /// + public static IConventionKeyBuilder? HasFillFactor( + this IConventionKeyBuilder keyBuilder, + int? fillFactor, + bool fromDataAnnotation = false) + { + if (keyBuilder.CanSetFillFactor(fillFactor, fromDataAnnotation)) + { + keyBuilder.Metadata.SetFillFactor(fillFactor, fromDataAnnotation); + + return keyBuilder; + } + + return null; + } + + /// + /// Returns a value indicating whether the key can be configured with fill factor option when targeting SQL Server. + /// + /// + /// See Modeling entity types and relationships, and + /// Accessing SQL Server and Azure SQL databases with EF Core + /// for more information and examples. + /// + /// The builder for the key being configured. + /// A value indicating whether the key is created with fill factor option. + /// Indicates whether the configuration was specified using a data annotation. + /// if the key can be configured with fill factor option when targeting SQL Server. + public static bool CanSetFillFactor( + this IConventionKeyBuilder keyBuilder, + int? fillFactor, + bool fromDataAnnotation = false) + => keyBuilder.CanSetAnnotation(SqlServerAnnotationNames.FillFactor, fillFactor, fromDataAnnotation); } diff --git a/src/EFCore.SqlServer/Extensions/SqlServerKeyExtensions.cs b/src/EFCore.SqlServer/Extensions/SqlServerKeyExtensions.cs index 5a5c7d1e1b4..be2a27d2ec0 100644 --- a/src/EFCore.SqlServer/Extensions/SqlServerKeyExtensions.cs +++ b/src/EFCore.SqlServer/Extensions/SqlServerKeyExtensions.cs @@ -82,4 +82,85 @@ public static void SetIsClustered(this IMutableKey key, bool? clustered) /// The for whether the key is clustered. public static ConfigurationSource? GetIsClusteredConfigurationSource(this IConventionKey key) => key.FindAnnotation(SqlServerAnnotationNames.Clustered)?.GetConfigurationSource(); + + /// + /// Returns the fill factor that the key uses. + /// + /// The key. + /// The fill factor that the key uses + public static int? GetFillFactor(this IReadOnlyKey key) + => (key is RuntimeKey) + ? throw new InvalidOperationException(CoreStrings.RuntimeModelMissingData) + : (int?)key[SqlServerAnnotationNames.FillFactor]; + + /// + /// Returns the fill factor that the key uses. + /// + /// The key. + /// The identifier of the store object. + /// The fill factor that the key uses + public static int? GetFillFactor(this IReadOnlyKey key, in StoreObjectIdentifier storeObject) + { + if (key is RuntimeKey) + { + throw new InvalidOperationException(CoreStrings.RuntimeModelMissingData); + } + + var annotation = key.FindAnnotation(SqlServerAnnotationNames.FillFactor); + if (annotation != null) + { + return (int?)annotation.Value; + } + + var sharedTableRootKey = key.FindSharedObjectRootKey(storeObject); + return sharedTableRootKey?.GetFillFactor(storeObject); + } + + /// + /// Sets a value for fill factor the key uses. + /// + /// The key. + /// The value to set. + public static void SetFillFactor(this IMutableKey key, int? fillFactor) + { + if (fillFactor is <= 0 or > 100) + { + throw new ArgumentOutOfRangeException(nameof(fillFactor)); + } + + key.SetAnnotation( + SqlServerAnnotationNames.FillFactor, + fillFactor); + } + + /// + /// Sets a value for fill factor the key uses. + /// + /// The key. + /// The value to set. + /// Indicates whether the configuration was specified using a data annotation. + /// The configured value. + public static int? SetFillFactor( + this IConventionKey key, + int? fillFactor, + bool fromDataAnnotation = false) + { + if (fillFactor is <= 0 or > 100) + { + throw new ArgumentOutOfRangeException(nameof(fillFactor)); + } + + return (int?)key.SetAnnotation( + SqlServerAnnotationNames.FillFactor, + fillFactor, + fromDataAnnotation)?.Value; + } + + /// + /// Returns the for whether the key uses the fill factor. + /// + /// The key. + /// The for whether the key uses the fill factor. + public static ConfigurationSource? GetFillFactorConfigurationSource(this IConventionKey key) + => key.FindAnnotation(SqlServerAnnotationNames.FillFactor)?.GetConfigurationSource(); } diff --git a/src/EFCore.SqlServer/Metadata/Conventions/SqlServerRuntimeModelConvention.cs b/src/EFCore.SqlServer/Metadata/Conventions/SqlServerRuntimeModelConvention.cs index 6a12502aa52..abec9d2859b 100644 --- a/src/EFCore.SqlServer/Metadata/Conventions/SqlServerRuntimeModelConvention.cs +++ b/src/EFCore.SqlServer/Metadata/Conventions/SqlServerRuntimeModelConvention.cs @@ -116,6 +116,7 @@ protected override void ProcessKeyAnnotations( if (!runtime) { annotations.Remove(SqlServerAnnotationNames.Clustered); + annotations.Remove(SqlServerAnnotationNames.FillFactor); } } diff --git a/src/EFCore.SqlServer/Metadata/Internal/SqlServerAnnotationProvider.cs b/src/EFCore.SqlServer/Metadata/Internal/SqlServerAnnotationProvider.cs index 6667cfbb6d3..a6fecde4a4f 100644 --- a/src/EFCore.SqlServer/Metadata/Internal/SqlServerAnnotationProvider.cs +++ b/src/EFCore.SqlServer/Metadata/Internal/SqlServerAnnotationProvider.cs @@ -158,6 +158,11 @@ public override IEnumerable For(IUniqueConstraint constraint, bool { yield return new Annotation(SqlServerAnnotationNames.Clustered, isClustered); } + + if (key.GetFillFactor() is int fillFactor) + { + yield return new Annotation(SqlServerAnnotationNames.FillFactor, fillFactor); + } } /// diff --git a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs index db485e6e018..412d5a555e9 100644 --- a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs +++ b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs @@ -1741,6 +1741,35 @@ protected virtual void Transfer( } } + /// + /// Generates a SQL fragment for extra where options of an key from a + /// , or . + /// + /// The operation. + /// The command builder to use to add the SQL fragment. + protected override void KeyOptions(MigrationOperation operation, MigrationCommandListBuilder builder) + { + KeyWithOptions(operation, builder); + } + + private static void KeyWithOptions(MigrationOperation operation, MigrationCommandListBuilder builder) + { + var options = new List(); + + if (operation[SqlServerAnnotationNames.FillFactor] is int fillFactor) + { + options.Add("FILLFACTOR = " + fillFactor); + } + + if (options.Count > 0) + { + builder + .Append(" WITH (") + .Append(string.Join(", ", options)) + .Append(")"); + } + } + /// /// Generates a SQL fragment for traits of an index from a , /// , or . diff --git a/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs b/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs index d0be82e8608..2cd011ba677 100644 --- a/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs +++ b/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs @@ -1081,7 +1081,8 @@ FROM [sys].[indexes] i .GroupBy( ddr => (Name: ddr.GetFieldValue("index_name"), - TypeDesc: ddr.GetValueOrDefault("type_desc"))) + TypeDesc: ddr.GetValueOrDefault("type_desc"), + FillFactor: ddr.GetValueOrDefault("fill_factor"))) .ToArray(); Check.DebugAssert(primaryKeyGroups.Length is 0 or 1, "Multiple primary keys found"); @@ -1100,7 +1101,8 @@ FROM [sys].[indexes] i .GroupBy( ddr => (Name: ddr.GetValueOrDefault("index_name"), - TypeDesc: ddr.GetValueOrDefault("type_desc"))) + TypeDesc: ddr.GetValueOrDefault("type_desc"), + FillFactor: ddr.GetValueOrDefault("fill_factor"))) .ToArray(); foreach (var uniqueConstraintGroup in uniqueConstraintGroups) @@ -1136,7 +1138,7 @@ FROM [sys].[indexes] i } bool TryGetPrimaryKey( - IGrouping<(string Name, string? TypeDesc), DbDataRecord> primaryKeyGroup, + IGrouping<(string Name, string? TypeDesc, byte FillFactor), DbDataRecord> primaryKeyGroup, [NotNullWhen(true)] out DatabasePrimaryKey? primaryKey) { primaryKey = new DatabasePrimaryKey { Table = table, Name = primaryKeyGroup.Key.Name }; @@ -1146,6 +1148,11 @@ bool TryGetPrimaryKey( primaryKey[SqlServerAnnotationNames.Clustered] = false; } + if (primaryKeyGroup.Key.FillFactor is > 0 and <= 100) + { + primaryKey[SqlServerAnnotationNames.FillFactor] = (int)primaryKeyGroup.Key.FillFactor; + } + foreach (var dataRecord in primaryKeyGroup) { var columnName = dataRecord.GetValueOrDefault("column_name"); @@ -1165,7 +1172,7 @@ bool TryGetPrimaryKey( } bool TryGetUniqueConstraint( - IGrouping<(string? Name, string? TypeDesc), DbDataRecord> uniqueConstraintGroup, + IGrouping<(string? Name, string? TypeDesc, byte FillFactor), DbDataRecord> uniqueConstraintGroup, [NotNullWhen(true)] out DatabaseUniqueConstraint? uniqueConstraint) { uniqueConstraint = new DatabaseUniqueConstraint { Table = table, Name = uniqueConstraintGroup.Key.Name }; @@ -1175,6 +1182,11 @@ bool TryGetUniqueConstraint( uniqueConstraint[SqlServerAnnotationNames.Clustered] = true; } + if (uniqueConstraintGroup.Key.FillFactor is > 0 and <= 100) + { + uniqueConstraint[SqlServerAnnotationNames.FillFactor] = (int)uniqueConstraintGroup.Key.FillFactor; + } + foreach (var dataRecord in uniqueConstraintGroup) { var columnName = dataRecord.GetValueOrDefault("column_name"); diff --git a/test/EFCore.Design.Tests/Migrations/Design/CSharpMigrationsGeneratorTest.ModelSnapshot.cs b/test/EFCore.Design.Tests/Migrations/Design/CSharpMigrationsGeneratorTest.ModelSnapshot.cs index a2752cb916f..cf64b5453e1 100644 --- a/test/EFCore.Design.Tests/Migrations/Design/CSharpMigrationsGeneratorTest.ModelSnapshot.cs +++ b/test/EFCore.Design.Tests/Migrations/Design/CSharpMigrationsGeneratorTest.ModelSnapshot.cs @@ -5916,6 +5916,72 @@ public virtual void Key_multiple_annotations_are_stored_in_snapshot() Assert.Equal("IndexName", key["Relational:Name"]); }); + [ConditionalFact] + public virtual void Key_fill_factor_is_stored_in_snapshot() + => Test( + builder => + { + builder.Entity().HasKey(t => t.Id).HasFillFactor(90); + builder.Ignore(); + }, + AddBoilerPlate( + GetHeading() + + """ + modelBuilder.Entity("Microsoft.EntityFrameworkCore.Migrations.Design.CSharpMigrationsGeneratorTest+EntityWithOneProperty", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("int"); + + SqlServerPropertyBuilderExtensions.UseIdentityColumn(b.Property("Id")); + + b.HasKey("Id"); + + SqlServerKeyBuilderExtensions.HasFillFactor(b.HasKey("Id"), 90); + + b.ToTable("EntityWithOneProperty", "DefaultSchema"); + }); +"""), + o => Assert.Equal(90, o.GetEntityTypes().First().GetKeys().Single(k => k.IsPrimaryKey()).GetFillFactor())); + + [ConditionalFact] + public virtual void Unique_constraint_fill_factor_is_stored_in_snapshot() + => Test( + builder => + { + builder.Entity().HasAlternateKey(t => t.AlternateId).HasName("KeyName").HasFillFactor(90); + builder.Ignore(); + }, + AddBoilerPlate( + GetHeading() + + """ + modelBuilder.Entity("Microsoft.EntityFrameworkCore.Migrations.Design.CSharpMigrationsGeneratorTest+EntityWithTwoProperties", b => + { + b.Property("Id") + .ValueGeneratedOnAdd() + .HasColumnType("int"); + + SqlServerPropertyBuilderExtensions.UseIdentityColumn(b.Property("Id")); + + b.Property("AlternateId") + .HasColumnType("int"); + + b.HasKey("Id"); + + b.HasAlternateKey("AlternateId") + .HasName("KeyName"); + + SqlServerKeyBuilderExtensions.HasFillFactor(b.HasAlternateKey("AlternateId"), 90); + + b.ToTable("EntityWithTwoProperties", "DefaultSchema"); + }); +"""), + model => + { + var key = model.GetEntityTypes().First().GetKeys().First(); + Assert.Equal(90, key.GetFillFactor()); + }); + #endregion #region Index diff --git a/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs index ca99e2aad34..10cdd612d37 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Migrations/MigrationsSqlServerTest.cs @@ -2787,6 +2787,51 @@ await Test( """); } + [ConditionalFact] + public virtual async Task Add_primary_key_with_fill_factor() + { + await Test( + builder => builder.Entity("People").Property("SomeField").IsRequired().HasMaxLength(450), + builder => { }, + builder => builder.Entity("People").HasKey("SomeField").HasFillFactor(80), + model => + { + var table = Assert.Single(model.Tables); + var primaryKey = table.PrimaryKey; + Assert.NotNull(primaryKey); + Assert.Equal(80, primaryKey[SqlServerAnnotationNames.FillFactor]); + }); + + AssertSql( + """ +ALTER TABLE [People] ADD CONSTRAINT [PK_People] PRIMARY KEY ([SomeField]) WITH (FILLFACTOR = 80); +"""); + } + + [ConditionalFact] + public virtual async Task Add_alternate_key_with_fill_factor() + { + await Test( + builder => { + builder.Entity("People").Property("SomeField").IsRequired().HasMaxLength(450); + builder.Entity("People").Property("SomeOtherField").IsRequired().HasMaxLength(450); + }, + builder => { }, + builder => builder.Entity("People").HasAlternateKey(["SomeField", "SomeOtherField"]).HasFillFactor(80), + model => + { + var table = Assert.Single(model.Tables); + var uniqueConstraint = table.UniqueConstraints.FirstOrDefault(); + Assert.NotNull(uniqueConstraint); + Assert.Equal(80, uniqueConstraint[SqlServerAnnotationNames.FillFactor]); + }); + + AssertSql( + """ +ALTER TABLE [People] ADD CONSTRAINT [AK_People_SomeField_SomeOtherField] UNIQUE ([SomeField], [SomeOtherField]) WITH (FILLFACTOR = 80); +"""); + } + public override async Task Drop_primary_key_int() { var exception = await Assert.ThrowsAsync(() => base.Drop_primary_key_int()); diff --git a/test/EFCore.SqlServer.FunctionalTests/Scaffolding/SqlServerDatabaseModelFactoryTest.cs b/test/EFCore.SqlServer.FunctionalTests/Scaffolding/SqlServerDatabaseModelFactoryTest.cs index 850413b4b6c..801e04100b4 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Scaffolding/SqlServerDatabaseModelFactoryTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Scaffolding/SqlServerDatabaseModelFactoryTest.cs @@ -4749,6 +4749,33 @@ CONSTRAINT MyPK PRIMARY KEY ( Id2 ), }, "DROP TABLE PrimaryKeyName;"); + [ConditionalFact] + public void Primary_key_fill_factor() + => Test( + @" +CREATE TABLE PrimaryKeyFillFactor +( + Id INT IDENTITY NOT NULL, + Name NVARCHAR(100), + CONSTRAINT [PK_Id] PRIMARY KEY NONCLUSTERED +( + [Id] ASC +) WITH (FILLFACTOR = 80) ON [PRIMARY] +) ON [PRIMARY];", + Enumerable.Empty(), + Enumerable.Empty(), + (dbModel, scaffoldingFactory) => + { + var pk = dbModel.Tables.Single().PrimaryKey; + Assert.NotNull(pk); + Assert.Equal(["Id"], pk!.Columns.Select(kc => kc.Name).ToList()); + Assert.Equal(80, pk[SqlServerAnnotationNames.FillFactor]); + + var model = scaffoldingFactory.Create(dbModel, new()); + Assert.Equal(1, model.GetEntityTypes().Count()); + }, + "DROP TABLE PrimaryKeyFillFactor;"); + #endregion #region UniqueConstraintFacets @@ -4831,6 +4858,34 @@ CONSTRAINT MyUC UNIQUE ( Id2 ), }, "DROP TABLE UniqueConstraintName;"); + [ConditionalFact] + public void Unique_constraint_fill_factor() + => Test( + @" +CREATE TABLE UniqueConstraintFillFactor +( + Something NVARCHAR(100) NOT NULL, + SomethingElse NVARCHAR(100) NOT NULL, + CONSTRAINT [UC_Something_SomethingElse] UNIQUE NONCLUSTERED +( + [Something] ASC, + [SomethingElse] ASC +) WITH (FILLFACTOR = 80) ON [PRIMARY] +) ON [PRIMARY];", + Enumerable.Empty(), + Enumerable.Empty(), + (dbModel, scaffoldingFactory) => + { + var uniqueConstraint = Assert.Single(dbModel.Tables.Single().UniqueConstraints); + Assert.NotNull(uniqueConstraint); + Assert.Equal(["Something", "SomethingElse"], uniqueConstraint!.Columns.Select(kc => kc.Name).ToList()); + Assert.Equal(80, uniqueConstraint[SqlServerAnnotationNames.FillFactor]); + + var model = scaffoldingFactory.Create(dbModel, new()); + Assert.Equal(1, model.GetEntityTypes().Count()); + }, + "DROP TABLE UniqueConstraintFillFactor;"); + #endregion #region IndexFacets diff --git a/test/EFCore.SqlServer.Tests/Design/Internal/SqlServerAnnotationCodeGeneratorTest.cs b/test/EFCore.SqlServer.Tests/Design/Internal/SqlServerAnnotationCodeGeneratorTest.cs index fd30348ccac..2a3f79be4b5 100644 --- a/test/EFCore.SqlServer.Tests/Design/Internal/SqlServerAnnotationCodeGeneratorTest.cs +++ b/test/EFCore.SqlServer.Tests/Design/Internal/SqlServerAnnotationCodeGeneratorTest.cs @@ -57,6 +57,51 @@ public void GenerateFluentApi_IKey_works_when_nonclustered() Assert.Equal(false, result.Arguments[0]); } + [ConditionalFact] + public void GenerateFluentApi_IKey_works_with_fillfactor() + { + var generator = CreateGenerator(); + var modelBuilder = SqlServerConventionSetBuilder.CreateModelBuilder(); + modelBuilder.Entity( + "Post", + x => + { + x.Property("Id"); + x.HasKey("Id").HasFillFactor(80); + }); + + var key = (IKey)modelBuilder.Model.FindEntityType("Post")!.GetKeys().Single(); + var result = generator.GenerateFluentApiCalls(key, key.GetAnnotations().ToDictionary(a => a.Name, a => a)) + .Single(); + + Assert.Equal("HasFillFactor", result.Method); + Assert.Equal(1, result.Arguments.Count); + Assert.Equal(80, result.Arguments[0]); + } + + [ConditionalFact] + public void GenerateFluentApi_IUniqueConstraint_works_with_fillfactor() + { + var generator = CreateGenerator(); + var modelBuilder = SqlServerConventionSetBuilder.CreateModelBuilder(); + modelBuilder.Entity( + "Post", + x => + { + x.Property("Something"); + x.Property("SomethingElse"); + x.HasAlternateKey(["Something", "SomethingElse"]).HasFillFactor(80); + }); + + var uniqueConstraint = (IKey)modelBuilder.Model.FindEntityType("Post")!.GetKeys().Single(); + var result = generator.GenerateFluentApiCalls(uniqueConstraint, uniqueConstraint.GetAnnotations().ToDictionary(a => a.Name, a => a)) + .Single(); + + Assert.Equal("HasFillFactor", result.Method); + Assert.Equal(1, result.Arguments.Count); + Assert.Equal(80, result.Arguments[0]); + } + [ConditionalFact] public void GenerateFluentApi_IIndex_works_when_clustered() { diff --git a/test/EFCore.SqlServer.Tests/Metadata/SqlServerBuilderExtensionsTest.cs b/test/EFCore.SqlServer.Tests/Metadata/SqlServerBuilderExtensionsTest.cs index 5dc89729e06..ab0d45d7695 100644 --- a/test/EFCore.SqlServer.Tests/Metadata/SqlServerBuilderExtensionsTest.cs +++ b/test/EFCore.SqlServer.Tests/Metadata/SqlServerBuilderExtensionsTest.cs @@ -127,6 +127,36 @@ public void Can_set_key_clustering() Assert.True(key.IsClustered().Value); } + [ConditionalFact] + public void Can_set_key_with_fillfactor() + { + var modelBuilder = CreateConventionModelBuilder(); + + modelBuilder + .Entity() + .HasKey(e => e.Id) + .HasFillFactor(90); + + var key = modelBuilder.Model.FindEntityType(typeof(Customer)).FindPrimaryKey(); + + Assert.Equal(90, key.GetFillFactor()); + } + + [ConditionalFact] + public void Can_set_key_with_fillfactor_non_generic() + { + var modelBuilder = CreateConventionModelBuilder(); + + modelBuilder + .Entity(typeof(Customer)) + .HasKey("Id") + .HasFillFactor(90); + + var key = modelBuilder.Model.FindEntityType(typeof(Customer)).FindPrimaryKey(); + + Assert.Equal(90, key.GetFillFactor()); + } + [ConditionalFact] public void Can_set_index_include() { @@ -1061,6 +1091,23 @@ public void Can_set_index_with_fillfactor_non_generic() Assert.Equal(90, index.GetFillFactor()); } + [ConditionalTheory] + [InlineData(0)] + [InlineData(101)] + public void Throws_if_attempt_to_set_key_fillfactor_with_argument_out_of_range(int fillFactor) + { + var modelBuilder = CreateConventionModelBuilder(); + + Assert.Throws( + () => + { + modelBuilder + .Entity(typeof(Customer)) + .HasKey("Id") + .HasFillFactor(fillFactor); + }); + } + [ConditionalTheory] [InlineData(0)] [InlineData(101)] diff --git a/test/EFCore.SqlServer.Tests/Migrations/SqlServerModelDifferTest.cs b/test/EFCore.SqlServer.Tests/Migrations/SqlServerModelDifferTest.cs index ff9471259fb..0f8dddae5a4 100644 --- a/test/EFCore.SqlServer.Tests/Migrations/SqlServerModelDifferTest.cs +++ b/test/EFCore.SqlServer.Tests/Migrations/SqlServerModelDifferTest.cs @@ -1250,6 +1250,56 @@ protected override MigrationsModelDiffer CreateModelDiffer(DbContextOptions opti private bool? IsMemoryOptimized(Annotatable annotatable) => annotatable[SqlServerAnnotationNames.MemoryOptimized] as bool?; + [ConditionalFact] + public void Dont_rebuild_key_index_with_unchanged_fillfactor_option() + => Execute( + source => source + .Entity( + "Address", + x => + { + x.Property("Id"); + x.HasKey("Id").HasFillFactor(90); + x.Property("Zip"); + x.Property("City"); + }), + target => target + .Entity( + "Address", + x => + { + x.Property("Id"); + x.HasKey("Id").HasFillFactor(90); + x.Property("Zip"); + x.Property("City"); + }), + operations => Assert.Equal(0, operations.Count)); + + [ConditionalFact] + public void Dont_rebuild_composite_key_index_with_unchanged_fillfactor_option() + => Execute( + source => source + .Entity( + "Address", + x => + { + x.Property("Id"); + x.Property("Zip"); + x.Property("City"); + x.HasAlternateKey("Zip", "City").HasFillFactor(90); + }), + target => target + .Entity( + "Address", + x => + { + x.Property("Id"); + x.Property("Zip"); + x.Property("City"); + x.HasAlternateKey("Zip", "City").HasFillFactor(90); + }), + operations => Assert.Equal(0, operations.Count)); + [ConditionalFact] public void Dont_rebuild_index_with_unchanged_fillfactor_option() => Execute( @@ -1277,6 +1327,230 @@ public void Dont_rebuild_index_with_unchanged_fillfactor_option() }), operations => Assert.Equal(0, operations.Count)); + [ConditionalFact] + public void Rebuild_key_index_when_adding_fillfactor_option() + => Execute( + _ => { }, + source => source + .Entity( + "Address", + x => + { + x.Property("Id"); + x.HasKey("Id"); + x.Property("Zip"); + x.Property("City"); + x.Property("Street"); + x.HasIndex("Zip"); + }), + target => target + .Entity( + "Address", + x => + { + x.Property("Id"); + x.HasKey("Id").HasFillFactor(90); + x.Property("Zip"); + x.Property("City"); + x.Property("Street"); + x.HasIndex("Zip"); + }), + upOps => + { + Assert.Equal(2, upOps.Count); + + var operation1 = Assert.IsType(upOps[0]); + Assert.Equal("Address", operation1.Table); + Assert.Equal("PK_Address", operation1.Name); + + Assert.Empty(operation1.GetAnnotations()); + + var operation2 = Assert.IsType(upOps[1]); + Assert.Equal("Address", operation1.Table); + Assert.Equal("PK_Address", operation1.Name); + + var annotation = operation2.GetAnnotation(SqlServerAnnotationNames.FillFactor); + Assert.NotNull(annotation); + + var annotationValue = Assert.IsType(annotation.Value); + Assert.Equal(90, annotationValue); + }, + downOps => + { + Assert.Equal(2, downOps.Count); + + var operation1 = Assert.IsType(downOps[0]); + Assert.Equal("Address", operation1.Table); + Assert.Equal("PK_Address", operation1.Name); + + Assert.Empty(operation1.GetAnnotations()); + + var operation2 = Assert.IsType(downOps[1]); + Assert.Equal("Address", operation1.Table); + Assert.Equal("PK_Address", operation1.Name); + + Assert.Empty(operation2.GetAnnotations()); + }); + + [ConditionalFact] + public void Rebuild_key_index_with_different_fillfactor_value() + => Execute( + source => source + .Entity( + "Address", + x => + { + x.Property("Id"); + x.HasKey("Id").HasFillFactor(50); + x.Property("Zip"); + x.Property("City"); + x.Property("Street"); + }), + target => target + .Entity( + "Address", + x => + { + x.Property("Id"); + x.HasKey("Id").HasFillFactor(90); + x.Property("Zip"); + x.Property("City"); + x.Property("Street"); + }), + operations => + { + Assert.Equal(2, operations.Count); + + var operation1 = Assert.IsType(operations[0]); + Assert.Equal("Address", operation1.Table); + Assert.Equal("PK_Address", operation1.Name); + + Assert.Empty(operation1.GetAnnotations()); + + var operation2 = Assert.IsType(operations[1]); + Assert.Equal("Address", operation1.Table); + Assert.Equal("PK_Address", operation1.Name); + + var annotation = operation2.GetAnnotation(SqlServerAnnotationNames.FillFactor); + Assert.NotNull(annotation); + + var annotationValue = Assert.IsType(annotation.Value); + + Assert.Equal(90, annotationValue); + }); + + [ConditionalFact] + public void Rebuild_composite_key_index_when_adding_fillfactor_option() + => Execute( + _ => { }, + source => source + .Entity( + "Address", + x => + { + x.Property("Id"); + x.Property("Zip"); + x.Property("City"); + x.Property("Street"); + x.HasAlternateKey("Zip", "City"); + }), + target => target + .Entity( + "Address", + x => + { + x.Property("Id"); + x.Property("Zip"); + x.Property("City"); + x.Property("Street"); + x.HasAlternateKey("Zip", "City").HasFillFactor(90); + }), + upOps => + { + Assert.Equal(2, upOps.Count); + + var operation1 = Assert.IsType(upOps[0]); + Assert.Equal("Address", operation1.Table); + Assert.Equal("AK_Address_Zip_City", operation1.Name); + + Assert.Empty(operation1.GetAnnotations()); + + var operation2 = Assert.IsType(upOps[1]); + Assert.Equal("Address", operation1.Table); + Assert.Equal("AK_Address_Zip_City", operation1.Name); + + var annotation = operation2.GetAnnotation(SqlServerAnnotationNames.FillFactor); + Assert.NotNull(annotation); + + var annotationValue = Assert.IsType(annotation.Value); + Assert.Equal(90, annotationValue); + }, + downOps => + { + Assert.Equal(2, downOps.Count); + + var operation1 = Assert.IsType(downOps[0]); + Assert.Equal("Address", operation1.Table); + Assert.Equal("AK_Address_Zip_City", operation1.Name); + + Assert.Empty(operation1.GetAnnotations()); + + var operation2 = Assert.IsType(downOps[1]); + Assert.Equal("Address", operation1.Table); + Assert.Equal("AK_Address_Zip_City", operation1.Name); + + Assert.Empty(operation2.GetAnnotations()); + }); + + [ConditionalFact] + public void Rebuild_composite_key_index_with_different_fillfactor_value() + => Execute( + source => source + .Entity( + "Address", + x => + { + x.Property("Id"); + x.Property("Zip"); + x.Property("City"); + x.Property("Street"); + x.HasIndex("Zip"); + x.HasAlternateKey("Zip", "City").HasFillFactor(50); + }), + target => target + .Entity( + "Address", + x => + { + x.Property("Id"); + x.Property("Zip"); + x.Property("City"); + x.Property("Street"); + x.HasIndex("Zip"); + x.HasAlternateKey("Zip", "City").HasFillFactor(90); + }), + operations => + { + Assert.Equal(2, operations.Count); + + var operation1 = Assert.IsType(operations[0]); + Assert.Equal("Address", operation1.Table); + Assert.Equal("AK_Address_Zip_City", operation1.Name); + + Assert.Empty(operation1.GetAnnotations()); + + var operation2 = Assert.IsType(operations[1]); + Assert.Equal("Address", operation1.Table); + Assert.Equal("AK_Address_Zip_City", operation1.Name); + + var annotation = operation2.GetAnnotation(SqlServerAnnotationNames.FillFactor); + Assert.NotNull(annotation); + + var annotationValue = Assert.IsType(annotation.Value); + + Assert.Equal(90, annotationValue); + }); + [ConditionalFact] public void Rebuild_index_when_adding_fillfactor_option() => Execute( From 2dd7aecf79cd481cf3413aa6e80669badfb61a44 Mon Sep 17 00:00:00 2001 From: Dean Hunter Date: Tue, 23 Jan 2024 14:55:00 +0000 Subject: [PATCH 2/2] Tidy up KeyWithOptions method and summary test --- .../Migrations/MigrationsSqlGenerator.cs | 9 +++++---- .../Migrations/SqlServerMigrationsSqlGenerator.cs | 9 ++------- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs b/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs index e1b4c6c0654..82517c47938 100644 --- a/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs +++ b/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs @@ -241,7 +241,7 @@ protected virtual void Generate( PrimaryKeyConstraint(operation, model, builder); - KeyOptions(operation, builder); + KeyWithOptions(operation, builder); if (terminate) { @@ -269,7 +269,7 @@ protected virtual void Generate( UniqueConstraint(operation, model, builder); - KeyOptions(operation, builder); + KeyWithOptions(operation, builder); builder.AppendLine(Dependencies.SqlGenerationHelper.StatementTerminator); EndStatement(builder); @@ -1679,11 +1679,12 @@ protected virtual void CheckConstraint( } /// - /// Generates a SQL fragment for extras where options of an key from a or . + /// Generates a SQL fragment for extra with options of a key from a + /// or . /// /// The operation. /// The command builder to use to add the SQL fragment. - protected virtual void KeyOptions(MigrationOperation operation, MigrationCommandListBuilder builder) + protected virtual void KeyWithOptions(MigrationOperation operation, MigrationCommandListBuilder builder) { } diff --git a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs index 412d5a555e9..53e157a715c 100644 --- a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs +++ b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs @@ -1742,17 +1742,12 @@ protected virtual void Transfer( } /// - /// Generates a SQL fragment for extra where options of an key from a + /// Generates a SQL fragment for extra with options of a key from a /// , or . /// /// The operation. /// The command builder to use to add the SQL fragment. - protected override void KeyOptions(MigrationOperation operation, MigrationCommandListBuilder builder) - { - KeyWithOptions(operation, builder); - } - - private static void KeyWithOptions(MigrationOperation operation, MigrationCommandListBuilder builder) + protected override void KeyWithOptions(MigrationOperation operation, MigrationCommandListBuilder builder) { var options = new List();