From 15800e30ca6796268eb325cf8f84288ac594999b Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Thu, 21 Jul 2022 18:44:18 +0200 Subject: [PATCH 1/2] Allow index IsDescending to get empty array for all-descending --- src/EFCore.Relational/Metadata/ITableIndex.cs | 5 +-- .../Migrations/MigrationBuilder.cs | 1 + .../Migrations/MigrationsSqlGenerator.cs | 2 +- .../Operations/CreateIndexOperation.cs | 4 +- src/EFCore/Metadata/Builders/IndexBuilder.cs | 5 ++- src/EFCore/Metadata/Builders/IndexBuilder`.cs | 5 ++- src/EFCore/Metadata/Internal/Index.cs | 27 ++++++++---- .../Migrations/ModelSnapshotSqlServerTest.cs | 29 ++++++++----- .../Internal/CSharpDbContextGeneratorTest.cs | 25 +++++++---- .../RelationalScaffoldingModelFactoryTest.cs | 8 ++-- .../Migrations/MigrationsTestBase.cs | 2 +- .../Operations/CreateIndexOperationTest.cs | 12 ++++++ .../Conventions/ConventionDispatcherTest.cs | 24 +++++------ .../Metadata/Internal/IndexTest.cs | 42 +++++++++++++++++++ .../ModelBuilding/NonRelationshipTestBase.cs | 12 +++--- 15 files changed, 145 insertions(+), 58 deletions(-) diff --git a/src/EFCore.Relational/Metadata/ITableIndex.cs b/src/EFCore.Relational/Metadata/ITableIndex.cs index 86ab50b304b..90976233f51 100644 --- a/src/EFCore.Relational/Metadata/ITableIndex.cs +++ b/src/EFCore.Relational/Metadata/ITableIndex.cs @@ -84,11 +84,10 @@ string ToDebugString(MetadataDebugStringOptions options = MetadataDebugStringOpt $@"'{Columns[i].Name}'{( MappedIndexes.First() is not RuntimeIndex && IsDescending is not null - && i < IsDescending.Count - && IsDescending[i] + && (IsDescending.Count == 0 || IsDescending[i]) ? " Desc" : "" - )}")) + )}")) .Append('}'); if (IsUnique) diff --git a/src/EFCore.Relational/Migrations/MigrationBuilder.cs b/src/EFCore.Relational/Migrations/MigrationBuilder.cs index d5c37ae52d2..a22d60f3403 100644 --- a/src/EFCore.Relational/Migrations/MigrationBuilder.cs +++ b/src/EFCore.Relational/Migrations/MigrationBuilder.cs @@ -604,6 +604,7 @@ public virtual AlterOperationBuilder AlterTable( /// /// A set of values indicating whether each corresponding index column has descending sort order. /// If , all columns will have ascending order. + /// If an empty array, all columns will have descending order. /// /// A builder to allow annotations to be added to the operation. public virtual OperationBuilder CreateIndex( diff --git a/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs b/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs index 6057a7deeb3..36b7fdbebd6 100644 --- a/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs +++ b/src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs @@ -1688,7 +1688,7 @@ protected virtual void GenerateIndexColumnList(CreateIndexOperation operation, I builder.Append(Dependencies.SqlGenerationHelper.DelimitIdentifier(operation.Columns[i])); - if (operation.IsDescending is not null && i < operation.IsDescending.Length && operation.IsDescending[i]) + if (operation.IsDescending is not null && (operation.IsDescending.Length == 0 || operation.IsDescending[i])) { builder.Append(" DESC"); } diff --git a/src/EFCore.Relational/Migrations/Operations/CreateIndexOperation.cs b/src/EFCore.Relational/Migrations/Operations/CreateIndexOperation.cs index a156281704e..95d4c343fc9 100644 --- a/src/EFCore.Relational/Migrations/Operations/CreateIndexOperation.cs +++ b/src/EFCore.Relational/Migrations/Operations/CreateIndexOperation.cs @@ -38,7 +38,7 @@ public virtual string[] Columns get => _columns!; set { - if (_isDescending is not null && value.Length != _isDescending.Length) + if (_isDescending is not null && _isDescending.Length > 0 && value.Length != _isDescending.Length) { throw new ArgumentException(RelationalStrings.CreateIndexOperationWithInvalidSortOrder(_isDescending.Length, value.Length)); } @@ -60,7 +60,7 @@ public virtual bool[]? IsDescending get => _isDescending; set { - if (value is not null && _columns is not null && value.Length != _columns.Length) + if (value is not null && value.Length > 0 && _columns is not null && value.Length != _columns.Length) { throw new ArgumentException(RelationalStrings.CreateIndexOperationWithInvalidSortOrder(value.Length, _columns.Length)); } diff --git a/src/EFCore/Metadata/Builders/IndexBuilder.cs b/src/EFCore/Metadata/Builders/IndexBuilder.cs index 71779f59636..2000e3bfd8f 100644 --- a/src/EFCore/Metadata/Builders/IndexBuilder.cs +++ b/src/EFCore/Metadata/Builders/IndexBuilder.cs @@ -80,7 +80,10 @@ public virtual IndexBuilder IsUnique(bool unique = true) /// /// Configures the sort order(s) for the columns of this index (ascending or descending). /// - /// A set of values indicating whether each corresponding index column has descending sort order. + /// + /// A set of values indicating whether each corresponding index column has descending sort order. + /// An empty list indicates that all index columns will have descending sort order. + /// /// The same builder instance so that multiple configuration calls can be chained. public virtual IndexBuilder IsDescending(params bool[] descending) { diff --git a/src/EFCore/Metadata/Builders/IndexBuilder`.cs b/src/EFCore/Metadata/Builders/IndexBuilder`.cs index e8f36809e9e..71040c89941 100644 --- a/src/EFCore/Metadata/Builders/IndexBuilder`.cs +++ b/src/EFCore/Metadata/Builders/IndexBuilder`.cs @@ -53,7 +53,10 @@ public IndexBuilder(IMutableIndex index) /// /// Configures the sort order(s) for the columns of this index (ascending or descending). /// - /// A set of values indicating whether each corresponding index column has descending sort order. + /// + /// A set of values indicating whether each corresponding index column has descending sort order. + /// An empty list indicates that all index columns will have descending sort order. + /// /// The same builder instance so that multiple configuration calls can be chained. public new virtual IndexBuilder IsDescending(params bool[] descending) => (IndexBuilder)base.IsDescending(descending); diff --git a/src/EFCore/Metadata/Internal/Index.cs b/src/EFCore/Metadata/Internal/Index.cs index 43c887c43b4..23a785661ad 100644 --- a/src/EFCore/Metadata/Internal/Index.cs +++ b/src/EFCore/Metadata/Internal/Index.cs @@ -218,17 +218,30 @@ public virtual IReadOnlyList? IsDescending { EnsureMutable(); - if (descending is not null && descending.Count != Properties.Count) + if (descending is not null) { - throw new ArgumentException( - CoreStrings.InvalidNumberOfIndexSortOrderValues(DisplayName(), descending.Count, Properties.Count), nameof(descending)); + if (descending.Count == Properties.Count) + { + // Normalize all-ascending/descending to null/empty respectively. + if (descending.All(desc => desc)) + { + descending = Array.Empty(); + } + else if (descending.All(desc => !desc)) + { + descending = null; + } + } + else if (descending.Count > 0) + { + throw new ArgumentException( + CoreStrings.InvalidNumberOfIndexSortOrderValues(DisplayName(), descending.Count, Properties.Count), nameof(descending)); + } } var oldIsDescending = IsDescending; - var isChanging = - (_isDescending is null && descending is not null && descending.Any(desc => desc)) - || (descending is null && _isDescending is not null && _isDescending.Any(desc => desc)) - || (descending is not null && oldIsDescending is not null && !oldIsDescending.SequenceEqual(descending)); + var isChanging = descending is null != _isDescending is null + || (descending is not null && _isDescending is not null && !descending.SequenceEqual(_isDescending)); _isDescending = descending; if (descending == null) diff --git a/test/EFCore.Design.Tests/Migrations/ModelSnapshotSqlServerTest.cs b/test/EFCore.Design.Tests/Migrations/ModelSnapshotSqlServerTest.cs index f816e4b7468..8f03964d34c 100644 --- a/test/EFCore.Design.Tests/Migrations/ModelSnapshotSqlServerTest.cs +++ b/test/EFCore.Design.Tests/Migrations/ModelSnapshotSqlServerTest.cs @@ -219,7 +219,7 @@ private abstract class AbstractBase { public int Id { get; set; } } - + private class BaseEntity : AbstractBase { public string Discriminator { get; set; } @@ -1227,7 +1227,7 @@ public virtual void Sequence_is_stored_in_snapshot_as_fluent_api() model => { Assert.Equal(5, model.GetAnnotations().Count()); - + var sequence = model.GetSequences().Single(); Assert.Equal(2, sequence.StartValue); Assert.Equal(1, sequence.MinValue); @@ -4881,7 +4881,9 @@ public virtual void Index_IsDescending_is_stored_in_snapshot() builder.Entity( e => { - e.HasIndex(t => new { t.X, t.Y, t.Z }, "IX_empty"); + e.HasIndex(t => new { t.X, t.Y, t.Z }, "IX_unspecified"); + e.HasIndex(t => new { t.X, t.Y, t.Z }, "IX_empty") + .IsDescending(); e.HasIndex(t => new { t.X, t.Y, t.Z }, "IX_all_ascending") .IsDescending(false, false, false); e.HasIndex(t => new { t.X, t.Y, t.Z }, "IX_all_descending") @@ -4912,32 +4914,37 @@ public virtual void Index_IsDescending_is_stored_in_snapshot() b.HasKey(""Id""); - b.HasIndex(new[] { ""X"", ""Y"", ""Z"" }, ""IX_all_ascending"") - .IsDescending(false, false, false); + b.HasIndex(new[] { ""X"", ""Y"", ""Z"" }, ""IX_all_ascending""); b.HasIndex(new[] { ""X"", ""Y"", ""Z"" }, ""IX_all_descending"") - .IsDescending(true, true, true); + .IsDescending(); - b.HasIndex(new[] { ""X"", ""Y"", ""Z"" }, ""IX_empty""); + b.HasIndex(new[] { ""X"", ""Y"", ""Z"" }, ""IX_empty"") + .IsDescending(); b.HasIndex(new[] { ""X"", ""Y"", ""Z"" }, ""IX_mixed"") .IsDescending(false, true, false); + b.HasIndex(new[] { ""X"", ""Y"", ""Z"" }, ""IX_unspecified""); + b.ToTable(""EntityWithThreeProperties""); });"), o => { var entityType = o.GetEntityTypes().Single(); - Assert.Equal(4, entityType.GetIndexes().Count()); + Assert.Equal(5, entityType.GetIndexes().Count()); + + var unspecifiedIndex = Assert.Single(entityType.GetIndexes(), i => i.Name == "IX_unspecified"); + Assert.Null(unspecifiedIndex.IsDescending); var emptyIndex = Assert.Single(entityType.GetIndexes(), i => i.Name == "IX_empty"); - Assert.Null(emptyIndex.IsDescending); + Assert.Equal(Array.Empty(), emptyIndex.IsDescending); var allAscendingIndex = Assert.Single(entityType.GetIndexes(), i => i.Name == "IX_all_ascending"); - Assert.Equal(new[] { false, false, false}, allAscendingIndex.IsDescending); + Assert.Null(allAscendingIndex.IsDescending); var allDescendingIndex = Assert.Single(entityType.GetIndexes(), i => i.Name == "IX_all_descending"); - Assert.Equal(new[] { true, true, true }, allDescendingIndex.IsDescending); + Assert.Equal(Array.Empty(), allDescendingIndex.IsDescending); var mixedIndex = Assert.Single(entityType.GetIndexes(), i => i.Name == "IX_mixed"); Assert.Equal(new[] { false, true, false }, mixedIndex.IsDescending); diff --git a/test/EFCore.Design.Tests/Scaffolding/Internal/CSharpDbContextGeneratorTest.cs b/test/EFCore.Design.Tests/Scaffolding/Internal/CSharpDbContextGeneratorTest.cs index 93f4b47f03d..70c3919df2d 100644 --- a/test/EFCore.Design.Tests/Scaffolding/Internal/CSharpDbContextGeneratorTest.cs +++ b/test/EFCore.Design.Tests/Scaffolding/Internal/CSharpDbContextGeneratorTest.cs @@ -714,7 +714,9 @@ public void Indexes_with_descending() x.Property("Y"); x.Property("Z"); x.HasKey("Id"); - x.HasIndex(new[] { "X", "Y", "Z" }, "IX_empty"); + x.HasIndex(new[] { "X", "Y", "Z" }, "IX_unspecified"); + x.HasIndex(new[] { "X", "Y", "Z" }, "IX_empty") + .IsDescending(); x.HasIndex(new[] { "X", "Y", "Z" }, "IX_all_ascending") .IsDescending(false, false, false); x.HasIndex(new[] { "X", "Y", "Z" }, "IX_all_descending") @@ -761,17 +763,19 @@ protected override void OnModelCreating(ModelBuilder modelBuilder) { modelBuilder.Entity(entity => { - entity.HasIndex(e => new { e.X, e.Y, e.Z }, ""IX_all_ascending"") - .IsDescending(false, false, false); + entity.HasIndex(e => new { e.X, e.Y, e.Z }, ""IX_all_ascending""); entity.HasIndex(e => new { e.X, e.Y, e.Z }, ""IX_all_descending"") - .IsDescending(true, true, true); + .IsDescending(); - entity.HasIndex(e => new { e.X, e.Y, e.Z }, ""IX_empty""); + entity.HasIndex(e => new { e.X, e.Y, e.Z }, ""IX_empty"") + .IsDescending(); entity.HasIndex(e => new { e.X, e.Y, e.Z }, ""IX_mixed"") .IsDescending(false, true, false); + entity.HasIndex(e => new { e.X, e.Y, e.Z }, ""IX_unspecified""); + entity.Property(e => e.Id).UseIdentityColumn(); }); @@ -787,16 +791,19 @@ protected override void OnModelCreating(ModelBuilder modelBuilder) model => { var entityType = model.FindEntityType("TestNamespace.EntityWithIndexes")!; - Assert.Equal(4, entityType.GetIndexes().Count()); + Assert.Equal(5, entityType.GetIndexes().Count()); + + var unspecifiedIndex = Assert.Single(entityType.GetIndexes(), i => i.Name == "IX_unspecified"); + Assert.Null(unspecifiedIndex.IsDescending); var emptyIndex = Assert.Single(entityType.GetIndexes(), i => i.Name == "IX_empty"); - Assert.Null(emptyIndex.IsDescending); + Assert.Equal(Array.Empty(), emptyIndex.IsDescending); var allAscendingIndex = Assert.Single(entityType.GetIndexes(), i => i.Name == "IX_all_ascending"); - Assert.Equal(new[] { false, false, false }, allAscendingIndex.IsDescending); + Assert.Null(allAscendingIndex.IsDescending); var allDescendingIndex = Assert.Single(entityType.GetIndexes(), i => i.Name == "IX_all_descending"); - Assert.Equal(new[] { true, true, true }, allDescendingIndex.IsDescending); + Assert.Equal(Array.Empty(), allDescendingIndex.IsDescending); var mixedIndex = Assert.Single(entityType.GetIndexes(), i => i.Name == "IX_mixed"); Assert.Equal(new[] { false, true, false }, mixedIndex.IsDescending); diff --git a/test/EFCore.Design.Tests/Scaffolding/Internal/RelationalScaffoldingModelFactoryTest.cs b/test/EFCore.Design.Tests/Scaffolding/Internal/RelationalScaffoldingModelFactoryTest.cs index 70496fc7542..fa8785b269d 100644 --- a/test/EFCore.Design.Tests/Scaffolding/Internal/RelationalScaffoldingModelFactoryTest.cs +++ b/test/EFCore.Design.Tests/Scaffolding/Internal/RelationalScaffoldingModelFactoryTest.cs @@ -1439,7 +1439,7 @@ public void Index_descending() new DatabaseIndex { Table = Table, - Name = "IX_empty", + Name = "IX_unspecified", Columns = { table.Columns[0], table.Columns[1], table.Columns[2] } }); @@ -1476,14 +1476,14 @@ public void Index_descending() var entityType = model.FindEntityType("SomeTable")!; - var emptyIndex = Assert.Single(entityType.GetIndexes(), i => i.Name == "IX_empty"); - Assert.Null(emptyIndex.IsDescending); + var unspecifiedIndex = Assert.Single(entityType.GetIndexes(), i => i.Name == "IX_unspecified"); + Assert.Null(unspecifiedIndex.IsDescending); var allAscendingIndex = Assert.Single(entityType.GetIndexes(), i => i.Name == "IX_all_ascending"); Assert.Null(allAscendingIndex.IsDescending); var allDescendingIndex = Assert.Single(entityType.GetIndexes(), i => i.Name == "IX_all_descending"); - Assert.Equal(new[] { true, true, true }, allDescendingIndex.IsDescending); + Assert.Equal(Array.Empty(), allDescendingIndex.IsDescending); var mixedIndex = Assert.Single(entityType.GetIndexes(), i => i.Name == "IX_mixed"); Assert.Equal(new[] { false, true, false }, mixedIndex.IsDescending); diff --git a/test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs b/test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs index 6b1797563f7..e9c185dc0ad 100644 --- a/test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs +++ b/test/EFCore.Relational.Specification.Tests/Migrations/MigrationsTestBase.cs @@ -1080,7 +1080,7 @@ public virtual Task Create_index_descending() e.Property("X"); }), builder => { }, - builder => builder.Entity("People").HasIndex("X").IsDescending(true), + builder => builder.Entity("People").HasIndex("X").IsDescending(), model => { var table = Assert.Single(model.Tables); diff --git a/test/EFCore.Relational.Tests/Migrations/Operations/CreateIndexOperationTest.cs b/test/EFCore.Relational.Tests/Migrations/Operations/CreateIndexOperationTest.cs index 61e3afdf453..897229de195 100644 --- a/test/EFCore.Relational.Tests/Migrations/Operations/CreateIndexOperationTest.cs +++ b/test/EFCore.Relational.Tests/Migrations/Operations/CreateIndexOperationTest.cs @@ -18,4 +18,16 @@ public void IsDescending_count_matches_column_count() operation.Columns = new[] { "X", "Y" }; Assert.Throws(() => operation.IsDescending = new[] { true }); } + + [ConditionalFact] + public void IsDescending_accepts_empty_array() + { + var operation = new CreateIndexOperation(); + + operation.IsDescending = Array.Empty(); + operation.Columns = new[] { "X", "Y" }; + + operation.IsDescending = null; + operation.IsDescending = Array.Empty(); + } } diff --git a/test/EFCore.Tests/Metadata/Conventions/ConventionDispatcherTest.cs b/test/EFCore.Tests/Metadata/Conventions/ConventionDispatcherTest.cs index 2e83815ef7f..5df55c3decd 100644 --- a/test/EFCore.Tests/Metadata/Conventions/ConventionDispatcherTest.cs +++ b/test/EFCore.Tests/Metadata/Conventions/ConventionDispatcherTest.cs @@ -3008,11 +3008,11 @@ public void OnIndexSortOrderChanged_calls_conventions_in_order(bool useBuilder, if (useBuilder) { - index.Builder.IsDescending(new[] { true }, ConfigurationSource.Convention); + index.Builder.IsDescending(Array.Empty(), ConfigurationSource.Convention); } else { - index.IsDescending = new[] { true }; + index.IsDescending = Array.Empty(); } if (useScope) @@ -3022,34 +3022,34 @@ public void OnIndexSortOrderChanged_calls_conventions_in_order(bool useBuilder, scope!.Dispose(); } - Assert.Equal(new[] { new[] { true } }, convention1.Calls); - Assert.Equal(new[] { new[] { true } }, convention2.Calls); + Assert.Equal(new[] { Array.Empty() }, convention1.Calls); + Assert.Equal(new[] { Array.Empty() }, convention2.Calls); Assert.Empty(convention3.Calls); if (useBuilder) { - index.Builder.IsDescending(new[] { true }, ConfigurationSource.Convention); + index.Builder.IsDescending(Array.Empty(), ConfigurationSource.Convention); } else { - index.IsDescending = new[] { true }; + index.IsDescending = Array.Empty(); } - Assert.Equal(new[] { new[] { true } }, convention1.Calls); - Assert.Equal(new[] { new[] { true } }, convention2.Calls); + Assert.Equal(new[] { Array.Empty() }, convention1.Calls); + Assert.Equal(new[] { Array.Empty() }, convention2.Calls); Assert.Empty(convention3.Calls); if (useBuilder) { - index.Builder.IsDescending(new[] { false }, ConfigurationSource.Convention); + index.Builder.IsDescending(null, ConfigurationSource.Convention); } else { - index.IsDescending = new[] { false }; + index.IsDescending = null; } - Assert.Equal(new[] { new[] { true }, new[] { false } }, convention1.Calls); - Assert.Equal(new[] { new[] { true }, new[] { false } }, convention2.Calls); + Assert.Equal(new[] { Array.Empty(), null }, convention1.Calls); + Assert.Equal(new[] { Array.Empty(), null }, convention2.Calls); Assert.Empty(convention3.Calls); Assert.Same(index, entityBuilder.Metadata.RemoveIndex(index.Properties)); diff --git a/test/EFCore.Tests/Metadata/Internal/IndexTest.cs b/test/EFCore.Tests/Metadata/Internal/IndexTest.cs index b267985a411..5a0922b237a 100644 --- a/test/EFCore.Tests/Metadata/Internal/IndexTest.cs +++ b/test/EFCore.Tests/Metadata/Internal/IndexTest.cs @@ -60,6 +60,48 @@ public void Can_set_unique() Assert.True(index.IsUnique); } + [ConditionalFact] + public void IsDescending_all_ascending_is_normalized_to_null() + { + var entityType = CreateModel().AddEntityType(typeof(Customer)); + var property1 = entityType.AddProperty(Customer.IdProperty); + var property2 = entityType.AddProperty(Customer.NameProperty); + + var index = entityType.AddIndex(new[] { property1, property2 }); + index.IsDescending = new[] { false, false }; + + Assert.True(new[] { property1, property2 }.SequenceEqual(index.Properties)); + Assert.Null(index.IsDescending); + } + + [ConditionalFact] + public void IsDescending_all_descending_is_normalized_to_empty() + { + var entityType = CreateModel().AddEntityType(typeof(Customer)); + var property1 = entityType.AddProperty(Customer.IdProperty); + var property2 = entityType.AddProperty(Customer.NameProperty); + + var index = entityType.AddIndex(new[] { property1, property2 }); + index.IsDescending = new[] { true, true }; + + Assert.True(new[] { property1, property2 }.SequenceEqual(index.Properties)); + Assert.Equal(Array.Empty(), index.IsDescending); + } + + [ConditionalFact] + public void IsDescending_invalid_number_of_columns_throws() + { + var entityType = CreateModel().AddEntityType(typeof(Customer)); + var property1 = entityType.AddProperty(Customer.IdProperty); + var property2 = entityType.AddProperty(Customer.NameProperty); + + var index = entityType.AddIndex(new[] { property1, property2 }); + var exception = Assert.Throws(() => index.IsDescending = new[] { true }); + Assert.Equal( + CoreStrings.InvalidNumberOfIndexSortOrderValues("{'Id', 'Name'}", 1, 2) + " (Parameter 'descending')", + exception.Message); + } + private static IMutableModel CreateModel() => new Model(); diff --git a/test/EFCore.Tests/ModelBuilding/NonRelationshipTestBase.cs b/test/EFCore.Tests/ModelBuilding/NonRelationshipTestBase.cs index 099584ddbbe..99abb9588ac 100644 --- a/test/EFCore.Tests/ModelBuilding/NonRelationshipTestBase.cs +++ b/test/EFCore.Tests/ModelBuilding/NonRelationshipTestBase.cs @@ -829,12 +829,12 @@ public virtual void Properties_can_have_non_generic_value_converter_set() var entityType = (IReadOnlyEntityType)model.FindEntityType(typeof(Quarks)); Assert.Null(entityType.FindProperty("Up").GetValueConverter()); - + var down = entityType.FindProperty("Down"); Assert.Same(stringConverter, down.GetValueConverter()); Assert.IsType>(down.GetValueComparer()); Assert.IsType>(down.GetProviderValueComparer()); - + var charm = entityType.FindProperty("Charm"); Assert.Same(intConverter, charm.GetValueConverter()); Assert.IsType>(charm.GetValueComparer()); @@ -915,7 +915,7 @@ public virtual void Properties_can_have_value_converter_set_inline() var model = modelBuilder.FinalizeModel(); var entityType = model.FindEntityType(typeof(Quarks)); - + var up = entityType.FindProperty("Up"); Assert.Null(up.GetProviderClrType()); Assert.Null(up.GetValueConverter()); @@ -937,7 +937,7 @@ public virtual void Properties_can_have_value_converter_set_inline() Assert.IsType>(strange.GetValueComparer()); Assert.IsType>(strange.GetProviderValueComparer()); } - + [ConditionalFact] public virtual void Properties_can_have_value_converter_set() { @@ -1809,7 +1809,7 @@ public virtual void Can_add_multiple_indexes() entityBuilder.HasIndex(ix => ix.Id).IsUnique(); entityBuilder.HasIndex(ix => ix.Name).HasAnnotation("A1", "V1"); entityBuilder.HasIndex(ix => ix.Id, "Named"); - entityBuilder.HasIndex(ix => ix.Id, "Descending").IsDescending(true); + entityBuilder.HasIndex(ix => ix.Id, "Descending").IsDescending(); var model = modelBuilder.FinalizeModel(); @@ -1826,7 +1826,7 @@ public virtual void Can_add_multiple_indexes() var namedIndex = entityType.FindIndex("Named"); Assert.False(namedIndex.IsUnique); var descendingIndex = entityType.FindIndex("Descending"); - Assert.Equal(new[] { true }, descendingIndex.IsDescending); + Assert.Equal(Array.Empty(), descendingIndex.IsDescending); } [ConditionalFact] From e29487ec995c81d76d9099f737ae6ecfc4be8c9e Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Tue, 26 Jul 2022 11:49:09 +0200 Subject: [PATCH 2/2] Add AllDescending to IndexAttribute --- src/EFCore.Abstractions/IndexAttribute.cs | 32 ++++++++- .../AbstractionsStrings.Designer.cs | 6 ++ .../Properties/AbstractionsStrings.resx | 3 + .../Internal/CSharpEntityTypeGenerator.cs | 5 +- .../Conventions/IndexAttributeConvention.cs | 11 +++- .../Internal/CSharpEntityTypeGeneratorTest.cs | 65 +++++++++++++++++++ .../IndexAttributeConventionTest.cs | 13 +++- 7 files changed, 128 insertions(+), 7 deletions(-) diff --git a/src/EFCore.Abstractions/IndexAttribute.cs b/src/EFCore.Abstractions/IndexAttribute.cs index 4b93178c115..ca310d0244d 100644 --- a/src/EFCore.Abstractions/IndexAttribute.cs +++ b/src/EFCore.Abstractions/IndexAttribute.cs @@ -18,6 +18,7 @@ public sealed class IndexAttribute : Attribute private string? _name; private bool? _isUnique; private bool[]? _isDescending; + private bool _allDescending; /// /// Initializes a new instance of the class. @@ -63,16 +64,41 @@ public bool[]? IsDescending get => _isDescending; set { - if (value is not null && value.Length != PropertyNames.Count) + if (value is not null) { - throw new ArgumentException( - AbstractionsStrings.InvalidNumberOfIndexSortOrderValues(value.Length, PropertyNames.Count), nameof(IsDescending)); + if (value.Length != PropertyNames.Count) + { + throw new ArgumentException( + AbstractionsStrings.InvalidNumberOfIndexSortOrderValues(value.Length, PropertyNames.Count), nameof(IsDescending)); + } + + if (_allDescending) + { + throw new ArgumentException(AbstractionsStrings.CannotSpecifyBothIsDescendingAndAllDescending); + } } _isDescending = value; } } + /// + /// Whether all index columns have descending sort order. + /// + public bool AllDescending + { + get => _allDescending; + set + { + if (IsDescending is not null) + { + throw new ArgumentException(AbstractionsStrings.CannotSpecifyBothIsDescendingAndAllDescending); + } + + _allDescending = value; + } + } + /// /// Checks whether has been explicitly set to a value. /// diff --git a/src/EFCore.Abstractions/Properties/AbstractionsStrings.Designer.cs b/src/EFCore.Abstractions/Properties/AbstractionsStrings.Designer.cs index 09b286557ff..75da7d5589c 100644 --- a/src/EFCore.Abstractions/Properties/AbstractionsStrings.Designer.cs +++ b/src/EFCore.Abstractions/Properties/AbstractionsStrings.Designer.cs @@ -38,6 +38,12 @@ public static string ArgumentIsNegativeNumber(object? argumentName) GetString("ArgumentIsNegativeNumber", nameof(argumentName)), argumentName); + /// + /// IsDescending and AllDescending cannot both be specified on the [Index] attribute. + /// + public static string CannotSpecifyBothIsDescendingAndAllDescending + => GetString("CannotSpecifyBothIsDescendingAndAllDescending"); + /// /// The collection argument '{argumentName}' must not contain any empty elements. /// diff --git a/src/EFCore.Abstractions/Properties/AbstractionsStrings.resx b/src/EFCore.Abstractions/Properties/AbstractionsStrings.resx index 568f47d97cd..416598e4dd4 100644 --- a/src/EFCore.Abstractions/Properties/AbstractionsStrings.resx +++ b/src/EFCore.Abstractions/Properties/AbstractionsStrings.resx @@ -123,6 +123,9 @@ The number argument '{argumentName}' cannot be negative number. + + IsDescending and AllDescending cannot both be specified on the [Index] attribute. + The collection argument '{argumentName}' must not contain any empty elements. diff --git a/src/EFCore.Design/Scaffolding/Internal/CSharpEntityTypeGenerator.cs b/src/EFCore.Design/Scaffolding/Internal/CSharpEntityTypeGenerator.cs index 0ccca2d1ba8..78019ac5182 100644 --- a/src/EFCore.Design/Scaffolding/Internal/CSharpEntityTypeGenerator.cs +++ b/src/EFCore.Design/Scaffolding/Internal/CSharpEntityTypeGenerator.cs @@ -215,7 +215,10 @@ private void GenerateIndexAttributes(IEntityType entityType) if (index.IsDescending is not null) { - indexAttribute.AddParameter($"{nameof(IndexAttribute.IsDescending)} = {_code.UnknownLiteral(index.IsDescending)}"); + indexAttribute.AddParameter( + index.IsDescending.Count == 0 + ? $"{nameof(IndexAttribute.AllDescending)} = true" + : $"{nameof(IndexAttribute.IsDescending)} = {_code.UnknownLiteral(index.IsDescending)}"); } _sb.AppendLine(indexAttribute.ToString()); diff --git a/src/EFCore/Metadata/Conventions/IndexAttributeConvention.cs b/src/EFCore/Metadata/Conventions/IndexAttributeConvention.cs index c125b974b61..e1acf29c34a 100644 --- a/src/EFCore/Metadata/Conventions/IndexAttributeConvention.cs +++ b/src/EFCore/Metadata/Conventions/IndexAttributeConvention.cs @@ -120,9 +120,16 @@ private static void CheckIndexAttributesAndEnsureIndex( indexBuilder = indexBuilder.IsUnique(indexAttribute.IsUnique, fromDataAnnotation: true); } - if (indexBuilder is not null && indexAttribute.IsDescending is not null) + if (indexBuilder is not null) { - indexBuilder.IsDescending(indexAttribute.IsDescending, fromDataAnnotation: true); + if (indexAttribute.AllDescending) + { + indexBuilder.IsDescending(Array.Empty(), fromDataAnnotation: true); + } + else if (indexAttribute.IsDescending is not null) + { + indexBuilder.IsDescending(indexAttribute.IsDescending, fromDataAnnotation: true); + } } } } diff --git a/test/EFCore.Design.Tests/Scaffolding/Internal/CSharpEntityTypeGeneratorTest.cs b/test/EFCore.Design.Tests/Scaffolding/Internal/CSharpEntityTypeGeneratorTest.cs index 3310b6f2641..1d1ecdabe84 100644 --- a/test/EFCore.Design.Tests/Scaffolding/Internal/CSharpEntityTypeGeneratorTest.cs +++ b/test/EFCore.Design.Tests/Scaffolding/Internal/CSharpEntityTypeGeneratorTest.cs @@ -303,6 +303,71 @@ public partial class EntityWithIndexes t => Assert.Equal("IndexOnBAndC", t.Name)); }); + [ConditionalFact] + public void IndexAttribute_is_generated_with_ascending_descending() + => Test( + modelBuilder => modelBuilder + .Entity( + "EntityWithAscendingDescendingIndexes", + x => + { + x.Property("Id"); + x.Property("A"); + x.Property("B"); + x.HasKey("Id"); + x.HasIndex(new[] { "A", "B" }, "AllAscending"); + x.HasIndex(new[] { "A", "B" }, "PartiallyDescending").IsDescending(true, false); + x.HasIndex(new[] { "A", "B" }, "AllDescending").IsDescending(); + }), + new ModelCodeGenerationOptions { UseDataAnnotations = true }, + code => + { + AssertFileContents( + @"using System; +using System.Collections.Generic; +using System.ComponentModel.DataAnnotations; +using System.ComponentModel.DataAnnotations.Schema; +using Microsoft.EntityFrameworkCore; + +namespace TestNamespace +{ + [Index(""A"", ""B"", Name = ""AllAscending"")] + [Index(""A"", ""B"", Name = ""AllDescending"", AllDescending = true)] + [Index(""A"", ""B"", Name = ""PartiallyDescending"", IsDescending = new[] { true, false })] + public partial class EntityWithAscendingDescendingIndexes + { + [Key] + public int Id { get; set; } + public int A { get; set; } + public int B { get; set; } + } +} +", + code.AdditionalFiles.Single(f => f.Path == "EntityWithAscendingDescendingIndexes.cs")); + }, + model => + { + var entityType = model.FindEntityType("TestNamespace.EntityWithAscendingDescendingIndexes"); + var indexes = entityType.GetIndexes(); + Assert.Collection( + indexes, + i => + { + Assert.Equal("AllAscending", i.Name); + Assert.Null(i.IsDescending); + }, + i => + { + Assert.Equal("AllDescending", i.Name); + Assert.Equal(Array.Empty(), i.IsDescending); + }, + i => + { + Assert.Equal("PartiallyDescending", i.Name); + Assert.Equal(new[] { true, false }, i.IsDescending); + }); + }); + [ConditionalFact] public void Entity_with_indexes_generates_IndexAttribute_only_for_indexes_without_annotations() => Test( diff --git a/test/EFCore.Tests/Metadata/Conventions/IndexAttributeConventionTest.cs b/test/EFCore.Tests/Metadata/Conventions/IndexAttributeConventionTest.cs index e989e1dfc84..1c72c6771e9 100644 --- a/test/EFCore.Tests/Metadata/Conventions/IndexAttributeConventionTest.cs +++ b/test/EFCore.Tests/Metadata/Conventions/IndexAttributeConventionTest.cs @@ -97,6 +97,17 @@ public void IndexAttribute_properties_cannot_include_whitespace(Type entityTypeW () => modelBuilder.Entity(entityTypeWithInvalidIndex)).Message); } + [ConditionalFact] + public void IndexAttribute_AllDescending_is_applied() + { + var modelBuilder = InMemoryTestHelpers.Instance.CreateConventionBuilder(); + var entityBuilder = modelBuilder.Entity(); + modelBuilder.Model.FinalizeModel(); + + var allDescendingIndex = entityBuilder.Metadata.FindIndex("IndexOnBAndC")!; + Assert.Equal(Array.Empty(), allDescendingIndex.IsDescending); + } + [ConditionalFact] public void IndexAttribute_can_be_applied_more_than_once_per_entity_type() { @@ -332,7 +343,7 @@ private class EntityWithIndex } [Index(nameof(A), nameof(B), Name = "IndexOnAAndB", IsUnique = true)] - [Index(nameof(B), nameof(C), Name = "IndexOnBAndC", IsUnique = false)] + [Index(nameof(B), nameof(C), Name = "IndexOnBAndC", IsUnique = false, AllDescending = true)] private class EntityWithTwoIndexes { public int Id { get; set; }