Skip to content

Commit

Permalink
Allow index IsDescending to get empty array for all-descending (#28489)
Browse files Browse the repository at this point in the history
And add AllDescending to IndexAttribute
  • Loading branch information
roji authored Jul 26, 2022
1 parent 4801ae9 commit 129bacc
Show file tree
Hide file tree
Showing 22 changed files with 271 additions and 63 deletions.
32 changes: 29 additions & 3 deletions src/EFCore.Abstractions/IndexAttribute.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public sealed class IndexAttribute : Attribute
private string? _name;
private bool? _isUnique;
private bool[]? _isDescending;
private bool _allDescending;

/// <summary>
/// Initializes a new instance of the <see cref="IndexAttribute" /> class.
Expand Down Expand Up @@ -78,16 +79,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;
}
}

/// <summary>
/// Whether all index columns have descending sort order.
/// </summary>
public bool AllDescending
{
get => _allDescending;
set
{
if (IsDescending is not null)
{
throw new ArgumentException(AbstractionsStrings.CannotSpecifyBothIsDescendingAndAllDescending);
}

_allDescending = value;
}
}

/// <summary>
/// Checks whether <see cref="IsUnique" /> has been explicitly set to a value.
/// </summary>
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions src/EFCore.Abstractions/Properties/AbstractionsStrings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,9 @@
<data name="ArgumentIsNegativeNumber" xml:space="preserve">
<value>The number argument '{argumentName}' cannot be negative number.</value>
</data>
<data name="CannotSpecifyBothIsDescendingAndAllDescending" xml:space="preserve">
<value>IsDescending and AllDescending cannot both be specified on the [Index] attribute.</value>
</data>
<data name="CollectionArgumentHasEmptyElements" xml:space="preserve">
<value>The collection argument '{argumentName}' must not contain any empty elements.</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
5 changes: 2 additions & 3 deletions src/EFCore.Relational/Metadata/ITableIndex.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions src/EFCore.Relational/Migrations/MigrationBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -604,6 +604,7 @@ public virtual AlterOperationBuilder<AlterTableOperation> AlterTable(
/// <param name="descending">
/// A set of values indicating whether each corresponding index column has descending sort order.
/// If <see langword="null" />, all columns will have ascending order.
/// If an empty array, all columns will have descending order.
/// </param>
/// <returns>A builder to allow annotations to be added to the operation.</returns>
public virtual OperationBuilder<CreateIndexOperation> CreateIndex(
Expand Down
2 changes: 1 addition & 1 deletion src/EFCore.Relational/Migrations/MigrationsSqlGenerator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand All @@ -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));
}
Expand Down
5 changes: 4 additions & 1 deletion src/EFCore/Metadata/Builders/IndexBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,10 @@ public virtual IndexBuilder IsUnique(bool unique = true)
/// <summary>
/// Configures the sort order(s) for the columns of this index (ascending or descending).
/// </summary>
/// <param name="descending">A set of values indicating whether each corresponding index column has descending sort order.</param>
/// <param name="descending">
/// 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.
/// </param>
/// <returns>The same builder instance so that multiple configuration calls can be chained.</returns>
public virtual IndexBuilder IsDescending(params bool[] descending)
{
Expand Down
5 changes: 4 additions & 1 deletion src/EFCore/Metadata/Builders/IndexBuilder`.cs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,10 @@ public IndexBuilder(IMutableIndex index)
/// <summary>
/// Configures the sort order(s) for the columns of this index (ascending or descending).
/// </summary>
/// <param name="descending">A set of values indicating whether each corresponding index column has descending sort order.</param>
/// <param name="descending">
/// 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.
/// </param>
/// <returns>The same builder instance so that multiple configuration calls can be chained.</returns>
public new virtual IndexBuilder<T> IsDescending(params bool[] descending)
=> (IndexBuilder<T>)base.IsDescending(descending);
Expand Down
11 changes: 9 additions & 2 deletions src/EFCore/Metadata/Conventions/IndexAttributeConvention.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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<bool>(), fromDataAnnotation: true);
}
else if (indexAttribute.IsDescending is not null)
{
indexBuilder.IsDescending(indexAttribute.IsDescending, fromDataAnnotation: true);
}
}
}
}
Expand Down
27 changes: 20 additions & 7 deletions src/EFCore/Metadata/Internal/Index.cs
Original file line number Diff line number Diff line change
Expand Up @@ -218,17 +218,30 @@ public virtual IReadOnlyList<bool>? 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<bool>();
}
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)
Expand Down
25 changes: 16 additions & 9 deletions test/EFCore.Design.Tests/Migrations/ModelSnapshotSqlServerTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4919,7 +4919,9 @@ public virtual void Index_IsDescending_is_stored_in_snapshot()
builder.Entity<EntityWithThreeProperties>(
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")
Expand Down Expand Up @@ -4950,32 +4952,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<bool>(), 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<bool>(), allDescendingIndex.IsDescending);
var mixedIndex = Assert.Single(entityType.GetIndexes(), i => i.Name == "IX_mixed");
Assert.Equal(new[] { false, true, false }, mixedIndex.IsDescending);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -714,7 +714,9 @@ public void Indexes_with_descending()
x.Property<int>("Y");
x.Property<int>("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")
Expand Down Expand Up @@ -761,17 +763,19 @@ protected override void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder.Entity<EntityWithIndexes>(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();
});
Expand All @@ -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<bool>(), 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<bool>(), allDescendingIndex.IsDescending);
var mixedIndex = Assert.Single(entityType.GetIndexes(), i => i.Name == "IX_mixed");
Assert.Equal(new[] { false, true, false }, mixedIndex.IsDescending);
Expand Down
Loading

0 comments on commit 129bacc

Please sign in to comment.