From 9ceda55c46db143f11e40c58cd5c05542378a632 Mon Sep 17 00:00:00 2001 From: Rafael Almeida Date: Tue, 14 Apr 2020 19:59:01 -0300 Subject: [PATCH 1/4] [WIP] SQLServer: Support fill factor for index --- .../SqlServerAnnotationCodeGenerator.cs | 5 ++ .../SqlServerIndexBuilderExtensions.cs | 71 +++++++++++++++ .../Extensions/SqlServerIndexExtensions.cs | 44 +++++++++ .../Internal/SqlServerAnnotationNames.cs | 8 ++ .../Internal/SqlServerAnnotationProvider.cs | 8 ++ .../Internal/SqlServerIndexExtensions.cs | 17 ++++ .../SqlServerMigrationsSqlGenerator.cs | 22 ++++- .../Properties/SqlServerStrings.Designer.cs | 8 ++ .../Properties/SqlServerStrings.resx | 3 + .../Internal/SqlServerDatabaseModelFactory.cs | 9 +- .../MigrationsSqlServerTest.cs | 90 +++++++++++++++++++ .../SqlServerAnnotationCodeGeneratorTest.cs | 23 +++++ 12 files changed, 306 insertions(+), 2 deletions(-) diff --git a/src/EFCore.SqlServer/Design/Internal/SqlServerAnnotationCodeGenerator.cs b/src/EFCore.SqlServer/Design/Internal/SqlServerAnnotationCodeGenerator.cs index c795b7ecc4e..930e8a7643d 100644 --- a/src/EFCore.SqlServer/Design/Internal/SqlServerAnnotationCodeGenerator.cs +++ b/src/EFCore.SqlServer/Design/Internal/SqlServerAnnotationCodeGenerator.cs @@ -82,6 +82,11 @@ public override MethodCallCodeFragment GenerateFluentApi(IIndex index, IAnnotati return new MethodCallCodeFragment(nameof(SqlServerIndexBuilderExtensions.IncludeProperties), annotation.Value); } + if (annotation.Name == SqlServerAnnotationNames.FillFactor) + { + return new MethodCallCodeFragment(nameof(SqlServerIndexBuilderExtensions.HasFillFactor), annotation.Value); + } + return null; } } diff --git a/src/EFCore.SqlServer/Extensions/SqlServerIndexBuilderExtensions.cs b/src/EFCore.SqlServer/Extensions/SqlServerIndexBuilderExtensions.cs index 1968b83e5fc..056e59c19dd 100644 --- a/src/EFCore.SqlServer/Extensions/SqlServerIndexBuilderExtensions.cs +++ b/src/EFCore.SqlServer/Extensions/SqlServerIndexBuilderExtensions.cs @@ -245,5 +245,76 @@ public static bool CanSetIsCreatedOnline( return indexBuilder.CanSetAnnotation(SqlServerAnnotationNames.CreatedOnline, createdOnline, fromDataAnnotation); } + + /// + /// Configures whether the index is created with fill factor option when targeting SQL Server. + /// + /// The builder for the index being configured. + /// A value indicating whether the index is created with fill factor option. + /// A builder to further configure the index. + public static IndexBuilder HasFillFactor([NotNull] this IndexBuilder indexBuilder, byte fillFactor) + { + Check.NotNull(indexBuilder, nameof(indexBuilder)); + + indexBuilder.Metadata.SetFillFactor(fillFactor); + + return indexBuilder; + } + + /// + /// Configures whether the index is created with fill factor option when targeting SQL Server. + /// + /// The builder for the index being configured. + /// A value indicating whether the index is created with fill factor option. + /// A builder to further configure the index. + public static IndexBuilder HasFillFactor( + [NotNull] this IndexBuilder indexBuilder, byte fillFactor) + => (IndexBuilder)HasFillFactor((IndexBuilder)indexBuilder, fillFactor); + + /// + /// Configures whether the index is created with fill factor option when targeting SQL Server. + /// + /// The builder for the index being configured. + /// A value indicating whether the index 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, + /// null otherwise. + /// + public static IConventionIndexBuilder HasFillFactor( + [NotNull] this IConventionIndexBuilder indexBuilder, + byte? fillFactor, + bool fromDataAnnotation = false) + { + if (indexBuilder.CanSetFillFactor(fillFactor, fromDataAnnotation)) + { + indexBuilder.Metadata.SetFillFactor(fillFactor, fromDataAnnotation); + + return indexBuilder; + } + + return null; + } + + /// + /// Returns a value indicating whether the index can be configured with fill factor option when targeting SQL Server. + /// + /// The builder for the index being configured. + /// A value indicating whether the index 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, + /// null otherwise. + /// + /// true if the index can be configured with fill factor option when targeting SQL Server. + public static bool CanSetFillFactor( + [NotNull] this IConventionIndexBuilder indexBuilder, + byte? fillFactor, + bool fromDataAnnotation = false) + { + Check.NotNull(indexBuilder, nameof(indexBuilder)); + + return indexBuilder.CanSetAnnotation(SqlServerAnnotationNames.FillFactor, fillFactor, fromDataAnnotation); + } } } diff --git a/src/EFCore.SqlServer/Extensions/SqlServerIndexExtensions.cs b/src/EFCore.SqlServer/Extensions/SqlServerIndexExtensions.cs index ba0f1223082..bff713d6e3b 100644 --- a/src/EFCore.SqlServer/Extensions/SqlServerIndexExtensions.cs +++ b/src/EFCore.SqlServer/Extensions/SqlServerIndexExtensions.cs @@ -145,5 +145,49 @@ public static void SetIsCreatedOnline([NotNull] this IMutableIndex index, bool? /// The for whether the index is online. public static ConfigurationSource? GetIsCreatedOnlineConfigurationSource([NotNull] this IConventionIndex index) => index.FindAnnotation(SqlServerAnnotationNames.CreatedOnline)?.GetConfigurationSource(); + + /// + /// Returns a value indicating whether the index uses the fill factor. + /// + /// The index. + /// true if the index is online. + public static byte? GetFillFactor([NotNull] this IIndex index) + => (byte?)index[SqlServerAnnotationNames.FillFactor]; + + /// + /// Sets a value indicating whether the index uses the fill factor. + /// + /// The index. + /// The value to set. + public static void SetFillFactor([NotNull] this IMutableIndex index, byte? fillFactor) + => index.SetOrRemoveAnnotation( + SqlServerAnnotationNames.FillFactor, + fillFactor); + + /// + /// Defines a value indicating whether the index uses the fill factor. + /// + /// The index. + /// The value to set. + /// Indicates whether the configuration was specified using a data annotation. + /// The configured value. + public static byte? SetFillFactor( + [NotNull] this IConventionIndex index, byte? fillFactor, bool fromDataAnnotation = false) + { + index.SetOrRemoveAnnotation( + SqlServerAnnotationNames.CreatedOnline, + fillFactor, + fromDataAnnotation); + + return fillFactor; + } + + /// + /// Returns the for whether the index uses the fill factor. + /// + /// The index. + /// The for whether the index uses the fill factor. + public static ConfigurationSource? GetFillFactorConfigurationSource([NotNull] this IConventionIndex index) + => index.FindAnnotation(SqlServerAnnotationNames.FillFactor)?.GetConfigurationSource(); } } diff --git a/src/EFCore.SqlServer/Metadata/Internal/SqlServerAnnotationNames.cs b/src/EFCore.SqlServer/Metadata/Internal/SqlServerAnnotationNames.cs index ac73917367a..4aec92a48ba 100644 --- a/src/EFCore.SqlServer/Metadata/Internal/SqlServerAnnotationNames.cs +++ b/src/EFCore.SqlServer/Metadata/Internal/SqlServerAnnotationNames.cs @@ -130,5 +130,13 @@ public static class SqlServerAnnotationNames /// doing so can result in application failures when updating to a new Entity Framework Core release. /// public const string PerformanceLevelSql = Prefix + "PerformanceLevelSql"; + + /// + /// 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. + /// + public const string FillFactor = Prefix + "FillFactor"; } } diff --git a/src/EFCore.SqlServer/Metadata/Internal/SqlServerAnnotationProvider.cs b/src/EFCore.SqlServer/Metadata/Internal/SqlServerAnnotationProvider.cs index e2dc0b645ae..d4a15f1c5cf 100644 --- a/src/EFCore.SqlServer/Metadata/Internal/SqlServerAnnotationProvider.cs +++ b/src/EFCore.SqlServer/Metadata/Internal/SqlServerAnnotationProvider.cs @@ -162,6 +162,14 @@ public override IEnumerable For(ITableIndex index) SqlServerAnnotationNames.CreatedOnline, isOnline.Value); } + + var fillFactor = modelIndex.GetFillFactor(); + if (fillFactor.HasValue) + { + yield return new Annotation( + SqlServerAnnotationNames.FillFactor, + fillFactor.Value); + } } /// diff --git a/src/EFCore.SqlServer/Metadata/Internal/SqlServerIndexExtensions.cs b/src/EFCore.SqlServer/Metadata/Internal/SqlServerIndexExtensions.cs index c2a490a166c..dad29905804 100644 --- a/src/EFCore.SqlServer/Metadata/Internal/SqlServerIndexExtensions.cs +++ b/src/EFCore.SqlServer/Metadata/Internal/SqlServerIndexExtensions.cs @@ -80,6 +80,23 @@ public static bool AreCompatibleForSqlServer([NotNull] this IIndex index, [NotNu return false; } + if (index.GetFillFactor() != duplicateIndex.GetFillFactor()) + { + if (shouldThrow) + { + throw new InvalidOperationException( + SqlServerStrings.DuplicateIndexFillFactorMismatch( + index.Properties.Format(), + index.DeclaringEntityType.DisplayName(), + duplicateIndex.Properties.Format(), + duplicateIndex.DeclaringEntityType.DisplayName(), + index.DeclaringEntityType.GetSchemaQualifiedTableName(), + index.GetName())); + } + + return false; + } + return true; } diff --git a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs index aa96594a836..3d9cc7b95a3 100644 --- a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs +++ b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs @@ -1552,9 +1552,29 @@ protected override void IndexOptions(CreateIndexOperation operation, IModel mode base.IndexOptions(operation, model, builder); + IndexWithOptions(operation, builder); + } + + private void IndexWithOptions(CreateIndexOperation operation, MigrationCommandListBuilder builder) + { + var options = new HashSet(); + + if (operation[SqlServerAnnotationNames.FillFactor] is byte fillFactor) + { + options.Add("FILLFACTOR = " + fillFactor); + } + if (operation[SqlServerAnnotationNames.CreatedOnline] is bool isOnline && isOnline) { - builder.Append(" WITH (ONLINE = ON)"); + options.Add("ONLINE = ON"); + } + + if (options.Count > 0) + { + builder + .Append(" WITH (") + .Append(string.Join(", ", options)) + .Append(")"); } } diff --git a/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs b/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs index 95237706db8..274f35ce5e6 100644 --- a/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs +++ b/src/EFCore.SqlServer/Properties/SqlServerStrings.Designer.cs @@ -223,6 +223,14 @@ public static string DuplicateIndexOnlineMismatch([CanBeNull] object index1, [Ca GetString("DuplicateIndexOnlineMismatch", nameof(index1), nameof(entityType1), nameof(index2), nameof(entityType2), nameof(table), nameof(indexName)), index1, entityType1, index2, entityType2, table, indexName); + /// + /// The indexes {index1} on '{entityType1}' and {index2} on '{entityType2}' are both mapped to '{table}.{indexName}' but with different fill factor configuration. + /// + public static string DuplicateIndexFillFactorMismatch([CanBeNull] object index1, [CanBeNull] object entityType1, [CanBeNull] object index2, [CanBeNull] object entityType2, [CanBeNull] object table, [CanBeNull] object indexName) + => string.Format( + GetString("DuplicateIndexFillFactorMismatch", nameof(index1), nameof(entityType1), nameof(index2), nameof(entityType2), nameof(table), nameof(indexName)), + index1, entityType1, index2, entityType2, table, indexName); + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/src/EFCore.SqlServer/Properties/SqlServerStrings.resx b/src/EFCore.SqlServer/Properties/SqlServerStrings.resx index 821be229f8c..054b057c58f 100644 --- a/src/EFCore.SqlServer/Properties/SqlServerStrings.resx +++ b/src/EFCore.SqlServer/Properties/SqlServerStrings.resx @@ -266,4 +266,7 @@ The indexes {index1} on '{entityType1}' and {index2} on '{entityType2}' are both mapped to '{table}.{indexName}' but with different online configuration. + + The indexes {index1} on '{entityType1}' and {index2} on '{entityType2}' are both mapped to '{table}.{indexName}' but with different fill factor configuration. + \ No newline at end of file diff --git a/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs b/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs index b81c671aabf..23ab3ee8117 100644 --- a/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs +++ b/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs @@ -843,6 +843,7 @@ private void GetIndexes(DbConnection connection, IReadOnlyList ta [i].[is_unique], [i].[has_filter], [i].[filter_definition], + [i].[fill_factor], COL_NAME([ic].[object_id], [ic].[column_id]) AS [column_name] FROM [sys].[indexes] AS [i] JOIN [sys].[tables] AS [t] ON [i].[object_id] = [t].[object_id] @@ -965,7 +966,8 @@ FROM [sys].[indexes] i TypeDesc: ddr.GetValueOrDefault("type_desc"), IsUnique: ddr.GetValueOrDefault("is_unique"), HasFilter: ddr.GetValueOrDefault("has_filter"), - FilterDefinition: ddr.GetValueOrDefault("filter_definition"))) + FilterDefinition: ddr.GetValueOrDefault("filter_definition"), + FillFactor: ddr.GetValueOrDefault("fill_factor"))) .ToArray(); foreach (var indexGroup in indexGroups) @@ -983,6 +985,11 @@ FROM [sys].[indexes] i index[SqlServerAnnotationNames.Clustered] = true; } + if (indexGroup.Key.FillFactor > 0 && indexGroup.Key.FillFactor < 100) + { + index[SqlServerAnnotationNames.FillFactor] = indexGroup.Key.FillFactor; + } + foreach (var dataRecord in indexGroup) { var columnName = dataRecord.GetValueOrDefault("column_name"); diff --git a/test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs index ceb2c345b8a..3ac2d0678ca 100644 --- a/test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/MigrationsSqlServerTest.cs @@ -1200,6 +1200,96 @@ FROM [sys].[default_constraints] [d] @"CREATE UNIQUE INDEX [IX_People_Name] ON [People] ([Name]) INCLUDE ([FirstName], [LastName]) WHERE [Name] IS NOT NULL WITH (ONLINE = ON);"); } + [ConditionalFact(Skip = "#19668, Online index operations can only be performed in Enterprise edition of SQL Server")] + [SqlServerCondition(SqlServerCondition.SupportsOnlineIndexes)] + public virtual async Task Create_index_unique_with_include_filter_online_and_fillfactor() + { + await Test( + builder => builder.Entity( + "People", e => + { + e.Property("Id"); + e.Property("FirstName"); + e.Property("LastName"); + e.Property("Name").IsRequired(); + }), + builder => { }, + builder => builder.Entity("People").HasIndex("Name") + .IsUnique() + .IncludeProperties("FirstName", "LastName") + .HasFilter("[Name] IS NOT NULL") + .IsCreatedOnline() + .HasFillFactor(90), + model => + { + var table = Assert.Single(model.Tables); + var index = Assert.Single(table.Indexes); + Assert.True(index.IsUnique); + Assert.Equal("([Name] IS NOT NULL)", index.Filter); + // TODO: This is a scaffolding bug, #17083 + Assert.Equal(3, index.Columns.Count); + Assert.Contains(table.Columns.Single(c => c.Name == "Name"), index.Columns); + Assert.Contains(table.Columns.Single(c => c.Name == "FirstName"), index.Columns); + Assert.Contains(table.Columns.Single(c => c.Name == "LastName"), index.Columns); + // TODO: Online index not scaffolded? + }); + + AssertSql( + @"DECLARE @var0 sysname; +SELECT @var0 = [d].[name] +FROM [sys].[default_constraints] [d] +INNER JOIN [sys].[columns] [c] ON [d].[parent_column_id] = [c].[column_id] AND [d].[parent_object_id] = [c].[object_id] +WHERE ([d].[parent_object_id] = OBJECT_ID(N'[People]') AND [c].[name] = N'Name'); +IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [People] DROP CONSTRAINT [' + @var0 + '];'); +ALTER TABLE [People] ALTER COLUMN [Name] nvarchar(450) NOT NULL;", + // + @"CREATE UNIQUE INDEX [IX_People_Name] ON [People] ([Name]) INCLUDE ([FirstName], [LastName]) WHERE [Name] IS NOT NULL WITH (FILLFACTOR = 90, ONLINE = ON);"); + } + + [ConditionalFact] + public virtual async Task Create_index_unique_with_include_filter_and_fillfactor() + { + await Test( + builder => builder.Entity( + "People", e => + { + e.Property("Id"); + e.Property("FirstName"); + e.Property("LastName"); + e.Property("Name").IsRequired(); + }), + builder => { }, + builder => builder.Entity("People").HasIndex("Name") + .IsUnique() + .IncludeProperties("FirstName", "LastName") + .HasFilter("[Name] IS NOT NULL") + .HasFillFactor(90), + model => + { + var table = Assert.Single(model.Tables); + var index = Assert.Single(table.Indexes); + Assert.True(index.IsUnique); + Assert.Equal("([Name] IS NOT NULL)", index.Filter); + // TODO: This is a scaffolding bug, #17083 + Assert.Equal(3, index.Columns.Count); + Assert.Contains(table.Columns.Single(c => c.Name == "Name"), index.Columns); + Assert.Contains(table.Columns.Single(c => c.Name == "FirstName"), index.Columns); + Assert.Contains(table.Columns.Single(c => c.Name == "LastName"), index.Columns); + // TODO: Online index not scaffolded? + }); + + AssertSql( + @"DECLARE @var0 sysname; +SELECT @var0 = [d].[name] +FROM [sys].[default_constraints] [d] +INNER JOIN [sys].[columns] [c] ON [d].[parent_column_id] = [c].[column_id] AND [d].[parent_object_id] = [c].[object_id] +WHERE ([d].[parent_object_id] = OBJECT_ID(N'[People]') AND [c].[name] = N'Name'); +IF @var0 IS NOT NULL EXEC(N'ALTER TABLE [People] DROP CONSTRAINT [' + @var0 + '];'); +ALTER TABLE [People] ALTER COLUMN [Name] nvarchar(450) NOT NULL;", + // + @"CREATE UNIQUE INDEX [IX_People_Name] ON [People] ([Name]) INCLUDE ([FirstName], [LastName]) WHERE [Name] IS NOT NULL WITH (FILLFACTOR = 90);"); + } + [ConditionalFact] [SqlServerCondition(SqlServerCondition.SupportsMemoryOptimized)] public virtual async Task Create_index_memoryOptimized_unique_nullable() diff --git a/test/EFCore.SqlServer.Tests/Design/Internal/SqlServerAnnotationCodeGeneratorTest.cs b/test/EFCore.SqlServer.Tests/Design/Internal/SqlServerAnnotationCodeGeneratorTest.cs index 84ca03e08f8..5d01b66776b 100644 --- a/test/EFCore.SqlServer.Tests/Design/Internal/SqlServerAnnotationCodeGeneratorTest.cs +++ b/test/EFCore.SqlServer.Tests/Design/Internal/SqlServerAnnotationCodeGeneratorTest.cs @@ -103,6 +103,29 @@ public void GenerateFluentApi_IIndex_works_when_nonclustered() Assert.Equal(false, result.Arguments[0]); } + [ConditionalFact] + public void GenerateFluentApi_IIndex_works_when_fillfactor() + { + var generator = new SqlServerAnnotationCodeGenerator(new AnnotationCodeGeneratorDependencies()); + var modelBuilder = new ModelBuilder(SqlServerConventionSetBuilder.Build()); + modelBuilder.Entity( + "Post", + x => + { + x.Property("Id"); + x.Property("Name"); + x.HasIndex("Name").HasFillFactor(90); + }); + + var index = modelBuilder.Model.FindEntityType("Post").GetIndexes().Single(); + var annotation = index.FindAnnotation(SqlServerAnnotationNames.FillFactor); + var result = generator.GenerateFluentApi(index, annotation); + + Assert.Equal("HasFillFactor", result.Method); + Assert.Equal(1, result.Arguments.Count); + Assert.Equal((byte)90, result.Arguments[0]); + } + [ConditionalFact] public void GenerateFluentApi_IIndex_works_with_includes() { From 21bf555f5c7fc6e0ab09177371cc2a8163e2d6d5 Mon Sep 17 00:00:00 2001 From: Rafael Almeida Date: Fri, 17 Apr 2020 20:23:28 -0300 Subject: [PATCH 2/4] Updating per review feedbacks --- .../SqlServerIndexBuilderExtensions.cs | 8 +- .../Extensions/SqlServerIndexExtensions.cs | 29 ++++-- .../SqlServerMigrationsSqlGenerator.cs | 4 +- .../Internal/SqlServerDatabaseModelFactory.cs | 4 +- .../SqlServerAnnotationCodeGeneratorTest.cs | 4 +- .../SqlServerBuilderExtensionsTest.cs | 49 ++++++++++ .../Migrations/SqlServerModelDifferTest.cs | 95 +++++++++++++++++++ 7 files changed, 176 insertions(+), 17 deletions(-) diff --git a/src/EFCore.SqlServer/Extensions/SqlServerIndexBuilderExtensions.cs b/src/EFCore.SqlServer/Extensions/SqlServerIndexBuilderExtensions.cs index 056e59c19dd..df9aaef168e 100644 --- a/src/EFCore.SqlServer/Extensions/SqlServerIndexBuilderExtensions.cs +++ b/src/EFCore.SqlServer/Extensions/SqlServerIndexBuilderExtensions.cs @@ -252,7 +252,7 @@ public static bool CanSetIsCreatedOnline( /// The builder for the index being configured. /// A value indicating whether the index is created with fill factor option. /// A builder to further configure the index. - public static IndexBuilder HasFillFactor([NotNull] this IndexBuilder indexBuilder, byte fillFactor) + public static IndexBuilder HasFillFactor([NotNull] this IndexBuilder indexBuilder, int fillFactor) { Check.NotNull(indexBuilder, nameof(indexBuilder)); @@ -268,7 +268,7 @@ public static IndexBuilder HasFillFactor([NotNull] this IndexBuilder indexBuilde /// A value indicating whether the index is created with fill factor option. /// A builder to further configure the index. public static IndexBuilder HasFillFactor( - [NotNull] this IndexBuilder indexBuilder, byte fillFactor) + [NotNull] this IndexBuilder indexBuilder, int fillFactor) => (IndexBuilder)HasFillFactor((IndexBuilder)indexBuilder, fillFactor); /// @@ -283,7 +283,7 @@ public static IndexBuilder HasFillFactor( /// public static IConventionIndexBuilder HasFillFactor( [NotNull] this IConventionIndexBuilder indexBuilder, - byte? fillFactor, + int? fillFactor, bool fromDataAnnotation = false) { if (indexBuilder.CanSetFillFactor(fillFactor, fromDataAnnotation)) @@ -309,7 +309,7 @@ public static IConventionIndexBuilder HasFillFactor( /// true if the index can be configured with fill factor option when targeting SQL Server. public static bool CanSetFillFactor( [NotNull] this IConventionIndexBuilder indexBuilder, - byte? fillFactor, + int? fillFactor, bool fromDataAnnotation = false) { Check.NotNull(indexBuilder, nameof(indexBuilder)); diff --git a/src/EFCore.SqlServer/Extensions/SqlServerIndexExtensions.cs b/src/EFCore.SqlServer/Extensions/SqlServerIndexExtensions.cs index bff713d6e3b..4366f718c47 100644 --- a/src/EFCore.SqlServer/Extensions/SqlServerIndexExtensions.cs +++ b/src/EFCore.SqlServer/Extensions/SqlServerIndexExtensions.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 System; using System.Collections.Generic; using JetBrains.Annotations; using Microsoft.EntityFrameworkCore.Metadata; @@ -151,18 +152,25 @@ public static void SetIsCreatedOnline([NotNull] this IMutableIndex index, bool? /// /// The index. /// true if the index is online. - public static byte? GetFillFactor([NotNull] this IIndex index) - => (byte?)index[SqlServerAnnotationNames.FillFactor]; + public static int? GetFillFactor([NotNull] this IIndex index) + => (int?)index[SqlServerAnnotationNames.FillFactor]; /// /// Sets a value indicating whether the index uses the fill factor. /// /// The index. /// The value to set. - public static void SetFillFactor([NotNull] this IMutableIndex index, byte? fillFactor) - => index.SetOrRemoveAnnotation( + public static void SetFillFactor([NotNull] this IMutableIndex index, int? fillFactor) + { + if (fillFactor != null && (fillFactor <= 0 || fillFactor > 100)) + { + throw new ArgumentOutOfRangeException(nameof(fillFactor)); + } + + index.SetOrRemoveAnnotation( SqlServerAnnotationNames.FillFactor, fillFactor); + } /// /// Defines a value indicating whether the index uses the fill factor. @@ -171,11 +179,18 @@ public static void SetFillFactor([NotNull] this IMutableIndex index, byte? fillF /// The value to set. /// Indicates whether the configuration was specified using a data annotation. /// The configured value. - public static byte? SetFillFactor( - [NotNull] this IConventionIndex index, byte? fillFactor, bool fromDataAnnotation = false) + public static int? SetFillFactor( + [NotNull] this IConventionIndex index, + int? fillFactor, + bool fromDataAnnotation = false) { + if (fillFactor != null && (fillFactor <= 0 || fillFactor > 100)) + { + throw new ArgumentOutOfRangeException(nameof(fillFactor)); + } + index.SetOrRemoveAnnotation( - SqlServerAnnotationNames.CreatedOnline, + SqlServerAnnotationNames.FillFactor, fillFactor, fromDataAnnotation); diff --git a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs index 3d9cc7b95a3..7f75332d437 100644 --- a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs +++ b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs @@ -1557,9 +1557,9 @@ protected override void IndexOptions(CreateIndexOperation operation, IModel mode private void IndexWithOptions(CreateIndexOperation operation, MigrationCommandListBuilder builder) { - var options = new HashSet(); + var options = new List(); - if (operation[SqlServerAnnotationNames.FillFactor] is byte fillFactor) + if (operation[SqlServerAnnotationNames.FillFactor] is int fillFactor) { options.Add("FILLFACTOR = " + fillFactor); } diff --git a/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs b/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs index 23ab3ee8117..452d8648994 100644 --- a/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs +++ b/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs @@ -967,7 +967,7 @@ FROM [sys].[indexes] i IsUnique: ddr.GetValueOrDefault("is_unique"), HasFilter: ddr.GetValueOrDefault("has_filter"), FilterDefinition: ddr.GetValueOrDefault("filter_definition"), - FillFactor: ddr.GetValueOrDefault("fill_factor"))) + FillFactor: ddr.GetValueOrDefault("fill_factor"))) .ToArray(); foreach (var indexGroup in indexGroups) @@ -985,7 +985,7 @@ FROM [sys].[indexes] i index[SqlServerAnnotationNames.Clustered] = true; } - if (indexGroup.Key.FillFactor > 0 && indexGroup.Key.FillFactor < 100) + if (indexGroup.Key.FillFactor > 0 && indexGroup.Key.FillFactor <= 100) { index[SqlServerAnnotationNames.FillFactor] = indexGroup.Key.FillFactor; } diff --git a/test/EFCore.SqlServer.Tests/Design/Internal/SqlServerAnnotationCodeGeneratorTest.cs b/test/EFCore.SqlServer.Tests/Design/Internal/SqlServerAnnotationCodeGeneratorTest.cs index 5d01b66776b..6b8edaaff4f 100644 --- a/test/EFCore.SqlServer.Tests/Design/Internal/SqlServerAnnotationCodeGeneratorTest.cs +++ b/test/EFCore.SqlServer.Tests/Design/Internal/SqlServerAnnotationCodeGeneratorTest.cs @@ -104,7 +104,7 @@ public void GenerateFluentApi_IIndex_works_when_nonclustered() } [ConditionalFact] - public void GenerateFluentApi_IIndex_works_when_fillfactor() + public void GenerateFluentApi_IIndex_works_with_fillfactor() { var generator = new SqlServerAnnotationCodeGenerator(new AnnotationCodeGeneratorDependencies()); var modelBuilder = new ModelBuilder(SqlServerConventionSetBuilder.Build()); @@ -123,7 +123,7 @@ public void GenerateFluentApi_IIndex_works_when_fillfactor() Assert.Equal("HasFillFactor", result.Method); Assert.Equal(1, result.Arguments.Count); - Assert.Equal((byte)90, result.Arguments[0]); + Assert.Equal(90, result.Arguments[0]); } [ConditionalFact] diff --git a/test/EFCore.SqlServer.Tests/Metadata/SqlServerBuilderExtensionsTest.cs b/test/EFCore.SqlServer.Tests/Metadata/SqlServerBuilderExtensionsTest.cs index 28909fbaf1e..0603861e723 100644 --- a/test/EFCore.SqlServer.Tests/Metadata/SqlServerBuilderExtensionsTest.cs +++ b/test/EFCore.SqlServer.Tests/Metadata/SqlServerBuilderExtensionsTest.cs @@ -738,6 +738,55 @@ public void Can_write_index_filter_with_where_clauses_generic() Assert.Equal("[Id] % 2 = 0", index.GetFilter()); } + + [ConditionalFact] + public void Can_set_index_with_fillfactor() + { + var modelBuilder = CreateConventionModelBuilder(); + + modelBuilder + .Entity() + .HasIndex(e => e.Name) + .HasFillFactor(90); + + var index = modelBuilder.Model.FindEntityType(typeof(Customer)).GetIndexes().Single(); + + Assert.Equal(90, index.GetFillFactor()); + } + + [ConditionalFact] + public void Can_set_index_with_fillfactor_non_generic() + { + var modelBuilder = CreateConventionModelBuilder(); + + modelBuilder + .Entity(typeof(Customer)) + .HasIndex("Name") + .HasFillFactor(90); + + var index = modelBuilder.Model.FindEntityType(typeof(Customer)).GetIndexes().Single(); + + Assert.Equal(90, index.GetFillFactor()); + } + + [ConditionalTheory] + [InlineData(-1)] + [InlineData(101)] + [InlineData(int.MinValue)] + [InlineData(int.MaxValue)] + public void Can_set_index_with_fillfactor_argument_out_of_range_exception(int fillFactor) + { + var modelBuilder = CreateConventionModelBuilder(); + + Assert.Throws(() => + { + modelBuilder + .Entity(typeof(Customer)) + .HasIndex("Name") + .HasFillFactor(fillFactor); + }); + } + private void AssertIsGeneric(EntityTypeBuilder _) { } diff --git a/test/EFCore.SqlServer.Tests/Migrations/SqlServerModelDifferTest.cs b/test/EFCore.SqlServer.Tests/Migrations/SqlServerModelDifferTest.cs index 72d9c743079..f0d5cb7c07d 100644 --- a/test/EFCore.SqlServer.Tests/Migrations/SqlServerModelDifferTest.cs +++ b/test/EFCore.SqlServer.Tests/Migrations/SqlServerModelDifferTest.cs @@ -908,5 +908,100 @@ protected override MigrationsModelDiffer CreateModelDiffer(DbContextOptions opti private bool? IsMemoryOptimized(Annotatable annotatable) => annotatable[SqlServerAnnotationNames.MemoryOptimized] as bool?; + + [ConditionalFact] + public void Dont_rebuild_index_with_unchanged_fillfactor_option() + { + Execute( + source => source + .Entity( + "Address", + x => + { + x.Property("Id"); + x.Property("Zip"); + x.Property("City"); + x.HasIndex("Zip") + .HasFillFactor(90); + }), + target => target + .Entity( + "Address", + x => + { + x.Property("Id"); + x.Property("Zip"); + x.Property("City"); + x.HasIndex("Zip") + .HasFillFactor(90); + }), + operations => Assert.Equal(0, operations.Count)); + } + + [ConditionalFact] + public void Rebuild_index_when_changing_fillfactor_option() + { + Execute( + _ => { }, + source => source + .Entity( + "Address", + x => + { + x.Property("Id"); + x.Property("Zip"); + x.Property("City"); + x.Property("Street"); + x.HasIndex("Zip"); + }), + target => target + .Entity( + "Address", + x => + { + x.Property("Id"); + x.Property("Zip"); + x.Property("City"); + x.Property("Street"); + x.HasIndex("Zip") + .HasFillFactor(90); + }), + upOps => + { + Assert.Equal(2, upOps.Count); + + var operation1 = Assert.IsType(upOps[0]); + Assert.Equal("Address", operation1.Table); + Assert.Equal("IX_Address_Zip", operation1.Name); + + Assert.Empty(operation1.GetAnnotations()); + + var operation2 = Assert.IsType(upOps[1]); + Assert.Equal("Address", operation1.Table); + Assert.Equal("IX_Address_Zip", 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("IX_Address_Zip", operation1.Name); + + Assert.Empty(operation1.GetAnnotations()); + + var operation2 = Assert.IsType(downOps[1]); + Assert.Equal("Address", operation1.Table); + Assert.Equal("IX_Address_Zip", operation1.Name); + + Assert.Empty(operation2.GetAnnotations()); + }); + } } } From 6718165bf44ee70edb8bd6e0ae6895532d90c7ec Mon Sep 17 00:00:00 2001 From: Rafael Almeida Date: Fri, 17 Apr 2020 22:21:23 -0300 Subject: [PATCH 3/4] Add test and small adjustments --- .../Internal/SqlServerDatabaseModelFactory.cs | 2 +- .../SqlServerBuilderExtensionsTest.cs | 8 ++- .../Migrations/SqlServerModelDifferTest.cs | 51 +++++++++++++++++++ 3 files changed, 55 insertions(+), 6 deletions(-) diff --git a/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs b/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs index 452d8648994..df7caeeb86d 100644 --- a/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs +++ b/src/EFCore.SqlServer/Scaffolding/Internal/SqlServerDatabaseModelFactory.cs @@ -967,7 +967,7 @@ FROM [sys].[indexes] i IsUnique: ddr.GetValueOrDefault("is_unique"), HasFilter: ddr.GetValueOrDefault("has_filter"), FilterDefinition: ddr.GetValueOrDefault("filter_definition"), - FillFactor: ddr.GetValueOrDefault("fill_factor"))) + FillFactor: ddr.GetValueOrDefault("fill_factor"))) .ToArray(); foreach (var indexGroup in indexGroups) diff --git a/test/EFCore.SqlServer.Tests/Metadata/SqlServerBuilderExtensionsTest.cs b/test/EFCore.SqlServer.Tests/Metadata/SqlServerBuilderExtensionsTest.cs index 0603861e723..47b4a655eda 100644 --- a/test/EFCore.SqlServer.Tests/Metadata/SqlServerBuilderExtensionsTest.cs +++ b/test/EFCore.SqlServer.Tests/Metadata/SqlServerBuilderExtensionsTest.cs @@ -234,8 +234,8 @@ public void Can_set_index_online_non_generic() var modelBuilder = CreateConventionModelBuilder(); modelBuilder - .Entity() - .HasIndex(e => e.Name) + .Entity(typeof(Customer)) + .HasIndex("Name") .IsCreatedOnline(); var index = modelBuilder.Model.FindEntityType(typeof(Customer)).GetIndexes().Single(); @@ -770,10 +770,8 @@ public void Can_set_index_with_fillfactor_non_generic() } [ConditionalTheory] - [InlineData(-1)] + [InlineData(0)] [InlineData(101)] - [InlineData(int.MinValue)] - [InlineData(int.MaxValue)] public void Can_set_index_with_fillfactor_argument_out_of_range_exception(int fillFactor) { var modelBuilder = CreateConventionModelBuilder(); diff --git a/test/EFCore.SqlServer.Tests/Migrations/SqlServerModelDifferTest.cs b/test/EFCore.SqlServer.Tests/Migrations/SqlServerModelDifferTest.cs index f0d5cb7c07d..37b735996b4 100644 --- a/test/EFCore.SqlServer.Tests/Migrations/SqlServerModelDifferTest.cs +++ b/test/EFCore.SqlServer.Tests/Migrations/SqlServerModelDifferTest.cs @@ -1003,5 +1003,56 @@ public void Rebuild_index_when_changing_fillfactor_option() Assert.Empty(operation2.GetAnnotations()); }); } + + [ConditionalFact] + public void Rebuild_index_with_different_fillfactor_option() + { + Execute( + source => source + .Entity( + "Address", + x => + { + x.Property("Id"); + x.Property("Zip"); + x.Property("City"); + x.Property("Street"); + x.HasIndex("Zip") + .HasFillFactor(50); + }), + target => target + .Entity( + "Address", + x => + { + x.Property("Id"); + x.Property("Zip"); + x.Property("City"); + x.Property("Street"); + x.HasIndex("Zip") + .HasFillFactor(90); + }), + operations => + { + Assert.Equal(2, operations.Count); + + var operation1 = Assert.IsType(operations[0]); + Assert.Equal("Address", operation1.Table); + Assert.Equal("IX_Address_Zip", operation1.Name); + + Assert.Empty(operation1.GetAnnotations()); + + var operation2 = Assert.IsType(operations[1]); + Assert.Equal("Address", operation1.Table); + Assert.Equal("IX_Address_Zip", operation1.Name); + + var annotation = operation2.GetAnnotation(SqlServerAnnotationNames.FillFactor); + Assert.NotNull(annotation); + + var annotationValue = Assert.IsType(annotation.Value); + + Assert.Equal(90, annotationValue); + }); + } } } From 58f5de74b84db6cc27735e3f459809049dc31075 Mon Sep 17 00:00:00 2001 From: Rafael Almeida Date: Fri, 17 Apr 2020 23:29:46 -0300 Subject: [PATCH 4/4] Rename the name of the test methods. --- .../Extensions/SqlServerIndexBuilderExtensions.cs | 4 ---- .../Metadata/SqlServerBuilderExtensionsTest.cs | 2 +- .../Migrations/SqlServerModelDifferTest.cs | 4 ++-- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/EFCore.SqlServer/Extensions/SqlServerIndexBuilderExtensions.cs b/src/EFCore.SqlServer/Extensions/SqlServerIndexBuilderExtensions.cs index df9aaef168e..ac986c35c13 100644 --- a/src/EFCore.SqlServer/Extensions/SqlServerIndexBuilderExtensions.cs +++ b/src/EFCore.SqlServer/Extensions/SqlServerIndexBuilderExtensions.cs @@ -302,10 +302,6 @@ public static IConventionIndexBuilder HasFillFactor( /// The builder for the index being configured. /// A value indicating whether the index 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, - /// null otherwise. - /// /// true if the index can be configured with fill factor option when targeting SQL Server. public static bool CanSetFillFactor( [NotNull] this IConventionIndexBuilder indexBuilder, diff --git a/test/EFCore.SqlServer.Tests/Metadata/SqlServerBuilderExtensionsTest.cs b/test/EFCore.SqlServer.Tests/Metadata/SqlServerBuilderExtensionsTest.cs index 47b4a655eda..faec607c890 100644 --- a/test/EFCore.SqlServer.Tests/Metadata/SqlServerBuilderExtensionsTest.cs +++ b/test/EFCore.SqlServer.Tests/Metadata/SqlServerBuilderExtensionsTest.cs @@ -772,7 +772,7 @@ public void Can_set_index_with_fillfactor_non_generic() [ConditionalTheory] [InlineData(0)] [InlineData(101)] - public void Can_set_index_with_fillfactor_argument_out_of_range_exception(int fillFactor) + public void Throws_if_attempt_to_set_fillfactor_with_argument_out_of_range(int fillFactor) { var modelBuilder = CreateConventionModelBuilder(); diff --git a/test/EFCore.SqlServer.Tests/Migrations/SqlServerModelDifferTest.cs b/test/EFCore.SqlServer.Tests/Migrations/SqlServerModelDifferTest.cs index 37b735996b4..9d89d8e180c 100644 --- a/test/EFCore.SqlServer.Tests/Migrations/SqlServerModelDifferTest.cs +++ b/test/EFCore.SqlServer.Tests/Migrations/SqlServerModelDifferTest.cs @@ -939,7 +939,7 @@ public void Dont_rebuild_index_with_unchanged_fillfactor_option() } [ConditionalFact] - public void Rebuild_index_when_changing_fillfactor_option() + public void Rebuild_index_when_adding_fillfactor_option() { Execute( _ => { }, @@ -1005,7 +1005,7 @@ public void Rebuild_index_when_changing_fillfactor_option() } [ConditionalFact] - public void Rebuild_index_with_different_fillfactor_option() + public void Rebuild_index_with_different_fillfactor_value() { Execute( source => source