Skip to content

Make EnsureStableSortOrderBy respect the ordering direction for added fields #1560

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.

Sign up for GitHub

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

Open
jbaehr wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Make EnsureStableSortOrderBy respect the ordering direction for added fields #1560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 10 additions & 2 deletions src/Microsoft.AspNetCore.OData/Query/ODataQueryOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -748,7 +748,8 @@ private OrderByQueryOption EnsureStableSortOrderBy(OrderByQueryOption orderBy, O
var propertyPathsToAdd = applySortOptions.Where(p => !usedPropertyNames.Contains(p)).OrderBy(p => p);
if (propertyPathsToAdd.Any())
{
var orderByRaw = orderBy.RawValue + "," + String.Join(",", propertyPathsToAdd);
var suffix = ExtractDirectionSuffix(orderBy);
var orderByRaw = orderBy.RawValue + "," + String.Join(",", propertyPathsToAdd.Select(p => p + suffix));
orderBy = new OrderByQueryOption(orderByRaw, context, Apply.RawValue, Compute?.RawValue);
}
}
Expand All @@ -761,12 +762,19 @@ private OrderByQueryOption EnsureStableSortOrderBy(OrderByQueryOption orderBy, O
// Clone the given one and add the remaining properties to end, thereby making
// the sort stable but preserving the user's original intent for the major
// sort order.
var orderByRaw = orderBy.RawValue + "," + string.Join(",", propertiesToAdd.Select(p => p.Name));
var suffix = ExtractDirectionSuffix(orderBy);
var orderByRaw = orderBy.RawValue + "," + string.Join(",", propertiesToAdd.Select(p => p.Name + suffix));
orderBy = new OrderByQueryOption(orderByRaw, context, null, Compute?.RawValue);
}
}

return orderBy;

static string ExtractDirectionSuffix(OrderByQueryOption orderBy)
{
// with Ascending being the default order, we just need to care about deviations from the default
return orderBy.OrderByNodes.LastOrDefault()?.Direction == OrderByDirection.Descending ? " desc" : string.Empty;
}
}

internal static IQueryable LimitResults(IQueryable queryable, int limit, bool parameterize, out bool resultsLimited)
Expand Down
40 changes: 24 additions & 16 deletions test/Microsoft.AspNetCore.OData.E2E.Tests/ServerSidePaging/ServerSidePagingTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -652,20 +652,22 @@ public async Task VerifySkipTokenPagingOrderedByNullablePropertyDescending()
// NOTE: Using a loop in this test (as opposed to parameterized tests using xunit Theory attribute)
// is intentional. The next-link in one response is used in the next request
// so we need to control the execution order (unlike Theory attribute where order is random)
var skipTokenTestData = new List<(int, decimal?)>
var skipTokenTestData = new List<(int, decimal?, int, decimal?)>
{
(6, 35),
(9, 25),
(2, 2),
(3, null)
(8, 50, 6, 35),
(4, 30, 9, 25),
(7, 5, 2, 2),
(5, null, 3, null)
};

string requestUri = "/prefix/SkipTokenPagingS1Customers?$orderby=CreditLimit desc";

foreach (var testData in skipTokenTestData)
{
int idAt1 = testData.Item1;
decimal? creditLimitAt1 = testData.Item2;
int idAt0 = testData.Item1;
decimal? creditLimitAt0 = testData.Item2;
int idAt1 = testData.Item3;
decimal? creditLimitAt1 = testData.Item4;

// Arrange
request = new HttpRequestMessage(HttpMethod.Get, requestUri);
Expand All @@ -681,6 +683,8 @@ public async Task VerifySkipTokenPagingOrderedByNullablePropertyDescending()
pageResult = content["value"] as JArray;
Assert.NotNull(pageResult);
Assert.Equal(2, pageResult.Count);
Assert.Equal(idAt0, (pageResult[0] as JObject)["Id"].ToObject<int>());
Assert.Equal(creditLimitAt0, (pageResult[0] as JObject)["CreditLimit"].ToObject<decimal?>());
Assert.Equal(idAt1, (pageResult[1] as JObject)["Id"].ToObject<int>());
Assert.Equal(creditLimitAt1, (pageResult[1] as JObject)["CreditLimit"].ToObject<decimal?>());

Expand All @@ -703,7 +707,7 @@ public async Task VerifySkipTokenPagingOrderedByNullablePropertyDescending()
pageResult = content["value"] as JArray;
Assert.NotNull(pageResult);
Assert.Single(pageResult);
Assert.Equal(5, (pageResult[0] as JObject)["Id"].ToObject<int>());
Assert.Equal(1, (pageResult[0] as JObject)["Id"].ToObject<int>());
Assert.Null((pageResult[0] as JObject)["CreditLimit"].ToObject<decimal?>());
Assert.Null(content.GetValue("@odata.nextLink"));
}
Expand Down Expand Up @@ -936,20 +940,22 @@ public async Task VerifySkipTokenPagingOrderedByNullableDateTimePropertyDescendi
// NOTE: Using a loop in this test (as opposed to parameterized tests using xunit Theory attribute)
// is intentional. The next-link in one response is used in the next request
// so we need to control the execution order (unlike Theory attribute where order is random)
var skipTokenTestData = new List<(int, DateTime?)>
var skipTokenTestData = new List<(int, DateTime?, int, DateTime?)>
{
(6, new DateTime(2023, 2, 4)),
(9, new DateTime(2023, 1, 25)),
(2, new DateTime(2023, 1, 2)),
(3, null)
(8, new DateTime(2023, 2, 19), 6, new DateTime(2023, 2, 4)),
(4, new DateTime(2023, 1, 30), 9, new DateTime(2023, 1, 25)),
(7, new DateTime(2023, 1, 5), 2, new DateTime(2023, 1, 2)),
(5, null, 3, null)
};

string requestUri = "/prefix/SkipTokenPagingS3Customers?$orderby=CustomerSince desc";

foreach (var testData in skipTokenTestData)
{
int idAt1 = testData.Item1;
DateTime? customerSinceAt1 = testData.Item2;
int idAt0 = testData.Item1;
DateTime? customerSinceAt0 = testData.Item2;
int idAt1 = testData.Item3;
DateTime? customerSinceAt1 = testData.Item4;

// Arrange
request = new HttpRequestMessage(HttpMethod.Get, requestUri);
Expand All @@ -968,6 +974,8 @@ public async Task VerifySkipTokenPagingOrderedByNullableDateTimePropertyDescendi
pageResult = content["value"] as JArray;
Assert.NotNull(pageResult);
Assert.Equal(2, pageResult.Count);
Assert.Equal(idAt0, (pageResult[0] as JObject)["Id"].ToObject<int>());
Assert.Equal(customerSinceAt0, (pageResult[0] as JObject)["CustomerSince"].ToObject<DateTime?>());
Assert.Equal(idAt1, (pageResult[1] as JObject)["Id"].ToObject<int>());
Assert.Equal(customerSinceAt1, (pageResult[1] as JObject)["CustomerSince"].ToObject<DateTime?>());

Expand All @@ -991,7 +999,7 @@ public async Task VerifySkipTokenPagingOrderedByNullableDateTimePropertyDescendi
pageResult = content["value"] as JArray;
Assert.NotNull(pageResult);
Assert.Single(pageResult);
Assert.Equal(5, (pageResult[0] as JObject)["Id"].ToObject<int>());
Assert.Equal(1, (pageResult[0] as JObject)["Id"].ToObject<int>());
Assert.Null((pageResult[0] as JObject)["CustomerSince"].ToObject<DateTime?>());
Assert.Null(content.GetValue("@odata.nextLink"));
}
Expand Down
8 changes: 4 additions & 4 deletions test/Microsoft.AspNetCore.OData.E2E.Tests/SkipToken/SkipTokenQueryTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,12 +177,12 @@ public static TheoryDataSet<string, string[], string> Orders_SkipTokenGeneratorC
},
{
"odata/orders?$orderby=RegId desc",
new string[] { "A9|5", "A9|11" },
"http://localhost/odata/orders?$orderby=RegId%20desc&$skiptoken=RegId-%27A9%27,Id-11"
new string[] { "A9|83", "A9|65" },
"http://localhost/odata/orders?$orderby=RegId%20desc&$skiptoken=RegId-%27A9%27,Id-65"
},
{
"odata/orders?$orderby=Location/city desc",
new string[] { "A9|5", "A9|11"},
new string[] { "A9|65", "A9|11"},
"http://localhost/odata/orders?$orderby=Location%2Fcity%20desc&$skiptoken=%27Settle%27,Id-11,RegId-%27A9%27"
},
{
Expand Down Expand Up @@ -213,7 +213,7 @@ public async Task EnableSkipToken_GenerateNextLinkWithSkipToken_ForCompositeKey(
JObject payloadBody = await response.Content.ReadAsObject<JObject>();
(string[] actualIds, string actualNextLink) = GetOrderActual(payloadBody);

Assert.True(keys.SequenceEqual(actualIds));
Assert.Equal(keys, actualIds);
Assert.Equal(nextLink, actualNextLink);
}

Expand Down
18 changes: 14 additions & 4 deletions test/Microsoft.AspNetCore.OData.Tests/Query/Query/ODataQueryOptionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@

namespace Microsoft.AspNetCore.OData.Tests.Query;

using Microsoft.OData.UriParser;

public class ODataQueryOptionTests
{
internal static IQueryable Customers = new List<Customer>().AsQueryable();
Expand Down Expand Up @@ -221,7 +223,7 @@ public static TheoryDataSet<string, bool, string> SkipTopOrderByUsingKeysTestDat
{ "$orderby=Name asc&$skip=1", true, "OrderBy($it => $it.Name).ThenBy($it => $it.CustomerId).Skip(1)" },

// Second key plus 'desc' suffix, adds 1st key and preserves suffix
{ "$orderby=Name desc&$skip=1", true, "OrderByDescending($it => $it.Name).ThenBy($it => $it.CustomerId).Skip(1)" },
{ "$orderby=Name desc&$skip=1", true, "OrderByDescending($it => $it.Name).ThenByDescending($it => $it.CustomerId).Skip(1)" },

// All keys present, no modification
{ "$orderby=CustomerId,Name&$skip=1", true, "OrderBy($it => $it.CustomerId).ThenBy($it => $it.Name).Skip(1)" },
Expand Down Expand Up @@ -284,6 +286,9 @@ public static TheoryDataSet<string, bool, string> SkipTopOrderByWithNoKeysTestDa
// Single property present with $skip and $top, adds all remaining in alphabetic order
{ "$orderby=CustomerId&$skip=1&$top=2", true, "OrderBy($it => $it.CustomerId).ThenBy($it => $it.Name).ThenBy($it => $it.SharePrice).ThenBy($it => $it.ShareSymbol).ThenBy($it => $it.Website).Skip(1).Take(2)" },

// Single property present plus 'desc' suffix, with $skip and $top, adds all remaining in alphabetic order and preserves suffix
{ "$orderby=CustomerId desc&$skip=1&$top=2", true, "OrderByDescending($it => $it.CustomerId).ThenByDescending($it => $it.Name).ThenByDescending($it => $it.SharePrice).ThenByDescending($it => $it.ShareSymbol).ThenByDescending($it => $it.Website).Skip(1).Take(2)" },

// Single property present, no $skip or $top, no modification
{ "$orderby=SharePrice", false, "OrderBy($it => $it.SharePrice)" },

Expand Down Expand Up @@ -793,12 +798,15 @@ public void IsSystemQueryOption_ThrowsArgumentNull_QueryOptionName()
ExceptionAssert.ThrowsArgumentNullOrEmpty(() => ODataQueryOptions.IsSystemQueryOption(string.Empty), "queryOptionName");
}

[Fact]
public void GenerateStableOrder_Works_WithGroupbyApplyClause()
[Theory]
[InlineData(OrderByDirection.Ascending)]
[InlineData(OrderByDirection.Descending)]
public void GenerateStableOrder_Works_WithGroupbyApplyClause(OrderByDirection direction)
{
// Arrange
IEdmModel model = GetEdmModel(c => c.CustomerId);
HttpRequest request = RequestFactory.Create(HttpMethods.Get, "http://localhost/Customers?$apply=groupby((CustomerId, Name))&$orderby=Name");
String directionSuffix = direction == OrderByDirection.Descending ? " desc" : String.Empty; // "asc" begin the default, so we omit it
HttpRequest request = RequestFactory.Create(HttpMethods.Get, "http://localhost/Customers?$apply=groupby((CustomerId, Name))&$orderby=Name" + directionSuffix);

ODataQueryContext context = new ODataQueryContext(model, typeof(Customer));
ODataQueryOptions option = new ODataQueryOptions(context, request);
Expand All @@ -814,11 +822,13 @@ public void GenerateStableOrder_Works_WithGroupbyApplyClause()
{
OrderByPropertyNode node = Assert.IsType<OrderByPropertyNode>(e);
Assert.Equal("Name", node.Property.Name);
Assert.Equal(direction, node.Direction);
},
e =>
{
OrderByPropertyNode node = Assert.IsType<OrderByPropertyNode>(e);
Assert.Equal("CustomerId", node.Property.Name);
Assert.Equal(direction, node.Direction);
});
}

Expand Down