Fix index creation with legacy filter

Fixes #24640
dotnet · Apr 13, 2021 · c1fde00 · c1fde00
1 parent 7af5e17
commit c1fde00
Show file tree

Hide file tree

Showing 3 changed files with 66 additions and 36 deletions.
diff --git a/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs b/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs
@@ -1952,7 +1952,7 @@ protected virtual bool HasLegacyRenameOperations(IModel? model)
         /// <param name="model"> The target model. </param>
         /// <param name="version"> The version. </param>
         /// <returns> <see langword="true" /> if the version could be retrieved. </returns>
-        protected virtual bool TryGetVersion(IModel? model, [NotNullWhen(true)] out string? version)
+        protected virtual bool TryGetVersion([NotNullWhen(true)] IModel? model, [NotNullWhen(true)] out string? version)
         {
             if (!(model?.GetProductVersion() is string versionString))
             {

diff --git a/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs b/src/EFCore.SqlServer/Migrations/SqlServerMigrationsSqlGenerator.cs
@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
 using System.Globalization;
 using System.IO;
 using System.Linq;
@@ -663,9 +664,7 @@ protected override void Generate(
             Check.NotNull(builder, nameof(builder));
 
             var table = model?.GetRelationalModel().FindTable(operation.Table, operation.Schema);
-            var nullableColumns = operation.Columns
-                .Where(c => table?.FindColumn(c)?.IsNullable != false)
-                .ToList();
+            var hasNullableColumns = operation.Columns.Any(c => table?.FindColumn(c)?.IsNullable != false);
 
             var memoryOptimized = IsMemoryOptimized(operation, model, operation.Schema, operation.Table);
             if (memoryOptimized)
@@ -676,8 +675,7 @@ protected override void Generate(
                     .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Name))
                     .Append(" ");
 
-                if (operation.IsUnique
-                    && nullableColumns.Count == 0)
+                if (operation.IsUnique && !hasNullableColumns)
                 {
                     builder.Append("UNIQUE ");
                 }
@@ -691,19 +689,7 @@ protected override void Generate(
             }
             else
             {
-                var needsLegacyFilter = false;
-                if (operation.Filter == null
-                    && UseLegacyIndexFilters(model))
-                {
-                    var clustered = operation[SqlServerAnnotationNames.Clustered] as bool?;
-                    if (operation.IsUnique
-                        && (clustered != true)
-                        && nullableColumns.Count != 0)
-                    {
-                        needsLegacyFilter = true;
-                    }
-                }
-
+                var needsLegacyFilter = UseLegacyIndexFilters(operation, model);
                 var needsExec = Options.HasFlag(MigrationsSqlGenerationOptions.Idempotent)
                     && (operation.Filter != null
                         || needsLegacyFilter);
@@ -713,22 +699,6 @@ protected override void Generate(
 
                 base.Generate(operation, model, subBuilder, terminate: false);
 
-                if (needsLegacyFilter)
-                {
-                    subBuilder.Append(" WHERE ");
-                    for (var i = 0; i < nullableColumns.Count; i++)
-                    {
-                        if (i != 0)
-                        {
-                            subBuilder.Append(" AND ");
-                        }
-
-                        subBuilder
-                            .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(nullableColumns[i]))
-                            .Append(" IS NOT NULL");
-                    }
-                }
-
                 if (needsExec)
                 {
                     subBuilder
@@ -1694,7 +1664,32 @@ protected override void IndexOptions(CreateIndexOperation operation, IModel? mod
                 builder.Append(")");
             }
 
-            base.IndexOptions(operation, model, builder);
+            if (!string.IsNullOrEmpty(operation.Filter))
+            {
+                builder
+                    .Append(" WHERE ")
+                    .Append(operation.Filter);
+            }
+            else if (UseLegacyIndexFilters(operation, model))
+            {
+                var table = model?.GetRelationalModel().FindTable(operation.Table, operation.Schema);
+                var nullableColumns = operation.Columns
+                    .Where(c => table?.FindColumn(c)?.IsNullable != false)
+                    .ToList();
+
+                builder.Append(" WHERE ");
+                for (var i = 0; i < nullableColumns.Count; i++)
+                {
+                    if (i != 0)
+                    {
+                        builder.Append(" AND ");
+                    }
+
+                    builder
+                        .Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(nullableColumns[i]))
+                        .Append(" IS NOT NULL");
+                }
+            }
 
             IndexWithOptions(operation, builder);
         }
@@ -2015,12 +2010,28 @@ string Literal(string s)
                 => stringTypeMapping.GenerateSqlLiteral(s);
         }
 
+        /// <summary>
+        ///     Checks whether or not <see cref="CreateIndexOperation" /> should have a filter generated for it by
+        ///     Migrations.
+        /// </summary>
+        /// <param name="operation"> The index creation operation. </param>
+        /// <param name="model"> The target model. </param>
+        /// <returns> <see langword="true" /> if a filter should be generated. </returns>
+        protected virtual bool UseLegacyIndexFilters(CreateIndexOperation operation, IModel? model)
+            => (!TryGetVersion(model, out var version) || VersionComparer.Compare(version, "2.0.0") < 0)
+                && operation.Filter is null
+                && operation.IsUnique
+                && operation[SqlServerAnnotationNames.Clustered] is null or false
+                && model?.GetRelationalModel().FindTable(operation.Table, operation.Schema) is var table
+                && operation.Columns.Any(c => table?.FindColumn(c)?.IsNullable != false);
+
         /// <summary>
         ///     Checks whether or not <see cref="CreateIndexOperation" /> should have a filter generated for it by
         ///     Migrations.
         /// </summary>
         /// <param name="model"> The target model. </param>
         /// <returns> <see langword="true" /> if a filter should be generated. </returns>
+        [Obsolete("Use UseLegacyIndexFilters which accepts a CreateIndexOperation")]
         protected virtual bool UseLegacyIndexFilters(IModel? model)
             => !TryGetVersion(model, out var version) || VersionComparer.Compare(version, "2.0.0") < 0;
 

diff --git a/test/EFCore.SqlServer.FunctionalTests/Migrations/SqlServerMigrationsSqlGeneratorTest.cs b/test/EFCore.SqlServer.FunctionalTests/Migrations/SqlServerMigrationsSqlGeneratorTest.cs
@@ -17,6 +17,25 @@ namespace Microsoft.EntityFrameworkCore.Migrations
 {
     public class SqlServerMigrationsSqlGeneratorTest : MigrationsSqlGeneratorTestBase
     {
+        [ConditionalFact]
+        public void CreateIndexOperation_unique_online()
+        {
+            Generate(
+                new CreateIndexOperation
+                {
+                    Name = "IX_People_Name",
+                    Table = "People",
+                    Schema = "dbo",
+                    Columns = new[] { "FirstName", "LastName" },
+                    IsUnique = true,
+                    [SqlServerAnnotationNames.CreatedOnline] = true
+                });
+
+            AssertSql(
+                @"CREATE UNIQUE INDEX [IX_People_Name] ON [dbo].[People] ([FirstName], [LastName]) WHERE [FirstName] IS NOT NULL AND [LastName] IS NOT NULL WITH (ONLINE = ON);
+");
+        }
+
         [ConditionalFact]
         public virtual void AddColumnOperation_identity_legacy()
         {