Skip to content

Commit

Permalink
Allow index IsDescending to get empty array for all-descending
Browse files Browse the repository at this point in the history
  • Loading branch information
roji committed Jul 21, 2022
1 parent bf7c303 commit a6d14d9
Show file tree
Hide file tree
Showing 14 changed files with 133 additions and 46 deletions.
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
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 @@ -1226,7 +1226,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 @@ -4880,7 +4880,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 @@ -4911,32 +4913,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
Original file line number Diff line number Diff line change
Expand Up @@ -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] }
});

Expand Down Expand Up @@ -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<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 @@ -1080,7 +1080,7 @@ public virtual Task Create_index_descending()
e.Property<int>("X");
}),
builder => { },
builder => builder.Entity("People").HasIndex("X").IsDescending(true),
builder => builder.Entity("People").HasIndex("X").IsDescending(),
model =>
{
var table = Assert.Single(model.Tables);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,16 @@ public void IsDescending_count_matches_column_count()
operation.Columns = new[] { "X", "Y" };
Assert.Throws<ArgumentException>(() => operation.IsDescending = new[] { true });
}

[ConditionalFact]
public void IsDescending_accepts_empty_array()
{
var operation = new CreateIndexOperation();

operation.IsDescending = Array.Empty<bool>();
operation.Columns = new[] { "X", "Y" };

operation.IsDescending = null;
operation.IsDescending = Array.Empty<bool>();
}
}
42 changes: 42 additions & 0 deletions test/EFCore.Tests/Metadata/Internal/IndexTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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<bool>(), 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<ArgumentException>(() => index.IsDescending = new[] { true });
Assert.Equal(
CoreStrings.InvalidNumberOfIndexSortOrderValues("{'Id', 'Name'}", 1, 2) + " (Parameter 'descending')",
exception.Message);
}

private static IMutableModel CreateModel()
=> new Model();

Expand Down
12 changes: 6 additions & 6 deletions test/EFCore.Tests/ModelBuilding/NonRelationshipTestBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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<ValueComparer.DefaultValueComparer<string>>(down.GetValueComparer());
Assert.IsType<ValueComparer<byte[]>>(down.GetProviderValueComparer());

var charm = entityType.FindProperty("Charm");
Assert.Same(intConverter, charm.GetValueConverter());
Assert.IsType<ValueComparer.DefaultValueComparer<int>>(charm.GetValueComparer());
Expand Down Expand Up @@ -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());
Expand All @@ -937,7 +937,7 @@ public virtual void Properties_can_have_value_converter_set_inline()
Assert.IsType<CustomValueComparer<float>>(strange.GetValueComparer());
Assert.IsType<CustomValueComparer<double>>(strange.GetProviderValueComparer());
}

[ConditionalFact]
public virtual void Properties_can_have_value_converter_set()
{
Expand Down Expand Up @@ -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();

Expand All @@ -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<bool>(), descendingIndex.IsDescending);
}

[ConditionalFact]
Expand Down

0 comments on commit a6d14d9

Please sign in to comment.