Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow index IsDescending to get empty array for all-descending #28489

Merged
merged 2 commits into from
Jul 26, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -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;
}
}

/// <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
29 changes: 18 additions & 11 deletions test/EFCore.Design.Tests/Migrations/ModelSnapshotSqlServerTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ private abstract class AbstractBase
{
public int Id { get; set; }
}

private class BaseEntity : AbstractBase
{
public string Discriminator { get; set; }
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -4881,7 +4881,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 @@ -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<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