Skip to content

Make orderBy unique for paging. #222

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

Merged
BlaiseD merged 1 commit into from
Dec 7, 2024
Merged

Make orderBy unique for paging. #222

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
67 changes: 47 additions & 20 deletions AutoMapper.AspNetCore.OData.EFCore/LinqExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,30 +123,27 @@ public static Expression<Func<T, bool>> ToFilterExpression<T>(this ODataQueryOpt
/// <returns></returns>
public static Expression<Func<IQueryable<T>, IQueryable<T>>> GetQueryableExpression<T>(this ODataQueryOptions<T> options, ODataSettings oDataSettings = null)
{
if (NoQueryableMethod(options, oDataSettings))
Copy link
Member

@BlaiseD BlaiseD Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic did not create the OrderBy if there were no related methods Top, Skip etc... Why did that have to go?

Copy link
Contributor Author

@NetanelPersik NetanelPersik Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we don't want to return null on NoQueryableMethod we always need to append some orderBy: #221

Copy link
Member

@BlaiseD BlaiseD Nov 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't agree. Microsoft.AspNetCore.OData.Query.EnableQueryAttribute does not sort by default either which leads me think that the logic enforcing the "ThenBy PK" sort really belongs in your own code. We should close this.

Copy link
Contributor Author

@NetanelPersik NetanelPersik Nov 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before closing this, just want to point out few things:

  • Please notice there are 2 parts here:
    1. Applying OrderBy even when it was not requested at all.
    2. When one passes some OrderBy we append ThenBy PK as well
  • Saying that "Microsoft doesn't do it so we shouldn't do it" isn't really a valid argument...
  • It is difficult to do it after GetQueryAsync since EF apply the orderBy after the LIMIT which ruins the point:
SELECT *
FROM (
    SELECT *
    FROM "Cases" AS "c"
    ORDER BY "c"."Type" DESC
    LIMIT @__p_0
) AS "t"
ORDER BY "t"."Id"
  • This is a general//real issue during paging, solving this can help us make paging consistent, it can also opens us to Keyset Pagination https://google.aip.dev/158

Maybe we can settle for 2. ("When one passes some OrderBy we append ThenBy PK as well")

Copy link
Member

@BlaiseD BlaiseD Dec 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the other way round. Changing the code needs justification - leaving it untouched doesn't.

Adding the ThenBy to your request when necessary is better. Then it does not have to run for users who are not requesting it.

Copy link
Contributor Author

@NetanelPersik NetanelPersik Dec 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to add thenBy pk to requests containing $orderBy=notPk

So if the request looks like this:

"/corebuilding?$top=5&$expand=Builder($expand=City),Tenant&$orderby=Name desc"

We want it to look like this:

"/corebuilding?$top=5&$expand=Builder($expand=City),Tenant&$orderby=Name desc,Identity";

Notice we chained Identity to the orderBy to make it unique in case orderBy=Name will return duplicates (since Name is not unique)

That way paging will be consistent...

Databases will return random order for duplicate values...

Copy link
Member

@BlaiseD BlaiseD Dec 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct - just do this: "/corebuilding?$top=5&$expand=Builder($expand=City),Tenant&$orderby=Name desc,Identity"; Otherwise we'd be adding the ThenBy for every user on every request.

Copy link
Contributor Author

@NetanelPersik NetanelPersik Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO adding thenBy pk on every request is best practice.

I don't want the consistency of my paging depending on customers (whether they pass Identity or not) also notice I don't change the request itself, it is immutable, internally im taking care of adding thenBy...

Copy link
Member

@BlaiseD BlaiseD Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should work if we add an opt-in flag to ODataSettings. Something like ODataSettings.AlwaysSortByPrimaryKey then we can do both behaviors when that flag is set (PK sort when NoQueryableMethod plus the ThenBy PK) .

Copy link
Contributor Author

@NetanelPersik NetanelPersik Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks!

return null;

ParameterExpression param = Expression.Parameter(typeof(IQueryable<T>), "q");
if (NoQueryableMethod(options, oDataSettings)) return null;
var param = Expression.Parameter(typeof(IQueryable<T>), "q");

return Expression.Lambda<Func<IQueryable<T>, IQueryable<T>>>
(
param.GetOrderByMethod(options, oDataSettings), param
);
}

public static Expression GetOrderByMethod<T>(this Expression expression,
private static Expression GetOrderByMethod<T>(this Expression expression,
ODataQueryOptions<T> options, ODataSettings oDataSettings = null)
{
if (NoQueryableMethod(options, oDataSettings))
return null;

if (NoQueryableMethod(options, oDataSettings)) return null;
return expression.GetQueryableMethod
(
options.Context,
options.OrderBy?.OrderByClause,
typeof(T),
options.Skip?.Value,
GetPageSize()
GetPageSize(),
oDataSettings
);

int? GetPageSize()
Expand All @@ -166,8 +163,30 @@ public static Expression GetOrderByMethod<T>(this Expression expression,
}

public static Expression GetQueryableMethod(this Expression expression,
ODataQueryContext context, OrderByClause orderByClause, Type type, int? skip, int? top)
ODataQueryContext context, OrderByClause orderByClause, Type type, int? skip, int? top,
ODataSettings? oDataSettings = null)
{
if (oDataSettings?.AlwaysSortByPrimaryKey is true)
{
var orderBySettings = context.FindSortableProperties(type, OrderByDirection.Descending);
if (orderBySettings is null) return null;

return orderByClause switch
{
null when skip is null && top is null => expression
.GetDefaultOrderByCall(orderBySettings),
null => expression
.GetDefaultOrderByCall(orderBySettings)
.GetSkipCall(skip)
.GetTakeCall(top),
_ => expression
.GetOrderByCall(orderByClause, context)
.GetDefaultThenByCall(orderBySettings)
.GetSkipCall(skip)
.GetTakeCall(top)
};
}

if (orderByClause is null && skip is null && top is null)
return null;

Expand All @@ -189,22 +208,26 @@ public static Expression GetQueryableMethod(this Expression expression,
.GetSkipCall(skip)
.GetTakeCall(top);
}

private static bool NoQueryableMethod(ODataQueryOptions options, ODataSettings oDataSettings)
=> options.OrderBy is null
&& options.Top is null
&& options.Skip is null
&& oDataSettings?.PageSize is null;


&& options.Top is null
&& options.Skip is null
&& oDataSettings?.PageSize is null
&& (oDataSettings is null || oDataSettings.AlwaysSortByPrimaryKey is false);
private static Expression GetDefaultThenByCall(this Expression expression, OrderBySetting settings)
{
return settings.ThenBy is null
? GetMethodCall()
: GetMethodCall().GetDefaultThenByCall(settings.ThenBy);

Expression GetMethodCall() =>
expression.GetOrderByCall(settings.Name, nameof(Queryable.ThenBy));
Expression GetMethodCall()
{
return settings.Direction is OrderByDirection.Ascending
? expression.GetOrderByCall(settings.Name, nameof(Queryable.ThenBy))
: expression.GetOrderByCall(settings.Name, nameof(Queryable.ThenByDescending));
}
}

private static Expression GetDefaultOrderByCall(this Expression expression, OrderBySetting settings)
Expand All @@ -213,8 +236,12 @@ private static Expression GetDefaultOrderByCall(this Expression expression, Orde
? GetMethodCall()
: GetMethodCall().GetDefaultThenByCall(settings.ThenBy);

Expression GetMethodCall() =>
expression.GetOrderByCall(settings.Name, nameof(Queryable.OrderBy));
Expression GetMethodCall()
{
return settings.Direction is OrderByDirection.Ascending
? expression.GetOrderByCall(settings.Name, nameof(Queryable.OrderBy))
: expression.GetOrderByCall(settings.Name, nameof(Queryable.OrderByDescending));
}
}

private static Expression GetOrderByCall(this Expression expression, OrderByClause orderByClause, ODataQueryContext context)
Expand Down
11 changes: 7 additions & 4 deletions AutoMapper.AspNetCore.OData.EFCore/ODataQueryContextExtentions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,20 @@
using System;
using System.Collections.Generic;
using System.Linq;
using Microsoft.OData.UriParser;

namespace AutoMapper.AspNet.OData
{
internal static class ODataQueryContextExtentions
{
public static OrderBySetting FindSortableProperties(this ODataQueryContext context, Type type)
public static OrderBySetting FindSortableProperties(this ODataQueryContext context, Type type,
OrderByDirection orderByDirection = OrderByDirection.Ascending)
{
context = context ?? throw new ArgumentNullException(nameof(context));

var entity = GetEntity();
return entity is not null
? FindProperties(entity)
? FindProperties(entity, orderByDirection)
: throw new InvalidOperationException($"The type '{type.FullName}' has not been declared in the entity data model.");

IEdmEntityType GetEntity()
Expand All @@ -26,7 +28,7 @@ IEdmEntityType GetEntity()
return null;
}

static OrderBySetting FindProperties(IEdmEntityType entity)
static OrderBySetting FindProperties(IEdmEntityType entity, OrderByDirection orderByDirection)
{
var propertyNames = entity.Key().Any() switch
{
Expand All @@ -43,9 +45,10 @@ static OrderBySetting FindProperties(IEdmEntityType entity)
if (settings.Name is null)
{
settings.Name = name;
settings.Direction = orderByDirection;
return settings;
}
settings.ThenBy = new() { Name = name };
settings.ThenBy = new() { Name = name, Direction = orderByDirection };
return settings.ThenBy;
});
return orderBySettings.Name is null ? null : orderBySettings;
Expand Down
17 changes: 17 additions & 0 deletions AutoMapper.AspNetCore.OData.EFCore/ODataSettings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,5 +41,22 @@ public class ODataSettings
/// Default is true.
/// </value>
public bool EnableConstantParameterization { get; set; } = true;

/// <summary>
/// If sets to true, orderBy pk desc will always be present on main entity.
/// </summary>
/// <example>
/// SELECT *
/// FROM "TEntitiy" AS "c"
/// ORDER BY "c"."Id" DESC
/// In case some orderBy was passed, additional thenBy pk will be applied
/// SELECT *
/// FROM "TEntitiy" AS "c"
/// ORDER BY "c"."Type" DESC, "c"."Id" DESC
/// </example>
/// <value>
/// Default is false.
/// </value>
public bool AlwaysSortByPrimaryKey { get; set; }
}
}
7 changes: 5 additions & 2 deletions AutoMapper.AspNetCore.OData.EFCore/OrderBySetting.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
namespace AutoMapper.AspNet.OData
using Microsoft.OData.UriParser;

namespace AutoMapper.AspNet.OData
{
internal class OrderBySetting
{
public string Name { get; set; }
public OrderByDirection Direction { get; set; } = OrderByDirection.Ascending;
public OrderBySetting ThenBy { get; set; }
}
}
}
105 changes: 105 additions & 0 deletions AutoMapper.OData.EFCore.Tests/GetQueryTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,84 @@ void Test(ICollection<OpsTenant> collection)
Assert.Equal("Two", collection.First().Name);
}
}

[Theory]
[InlineData(true)]
[InlineData(false)]
public async void OpsTenantNoExpandNoFilterNoOrderByShouldApplyByPk(bool alwaysSortByPk)
{
// Arrange
const string query = "/opstenant";
var querySettings = new QuerySettings
{
ODataSettings = new ODataSettings { AlwaysSortByPrimaryKey = alwaysSortByPk }
};

Test(Get<OpsTenant, TMandator>(query, GetMandators(), querySettings: querySettings));
Test(await GetAsync<OpsTenant, TMandator>(query, GetMandators(), querySettings: querySettings));
Test(await GetUsingCustomNameSpace<OpsTenant, TMandator>(query, GetMandators(), querySettings: querySettings));
return;

void Test(ICollection<OpsTenant> collection)
{
var expected = collection
.Select(x => x.Identity)
.OrderByDescending(identity => identity)
.ToList();

if (alwaysSortByPk)
{
Assert.True(collection
.Select(x => x.Identity)
.SequenceEqual(expected));
}
else
{
Assert.False(collection
.Select(x => x.Identity)
.SequenceEqual(expected));
}
}
}

[Theory]
[InlineData(true)]
[InlineData(false)]
public async Task OpsTenantNoExpandNoFilterWithOrderByShouldApplyByPk(bool alwaysSortByPk)
{
const string query = "/opstenant?$orderby=Name desc";
var querySettings = new QuerySettings
{
ODataSettings = new ODataSettings { AlwaysSortByPrimaryKey = alwaysSortByPk }
};

// Test multiple scenarios
Test(Get<OpsTenant, TMandator>(query, GetMandators(), querySettings: querySettings));
Test(await GetAsync<OpsTenant, TMandator>(query, GetMandators(), querySettings: querySettings));
Test(await GetUsingCustomNameSpace<OpsTenant, TMandator>(query, GetMandators(), querySettings: querySettings));

return;

void Test(ICollection<OpsTenant> collection)
{
// Check if the collection is correctly ordered by Name (desc) and then by Identity (asc)
var expected = collection
.OrderByDescending(x => x.Name)
.ThenByDescending(x => x.Identity)
.ToList();

if (alwaysSortByPk)
{
Assert.True(collection.SequenceEqual(expected),
"Collection is not ordered by Name (desc) and Identity (desc).");
}
else
{
Assert.False(collection.SequenceEqual(expected),
"Collection is ordered by Name (desc) and Identity (desc).");
}
}
}

[Fact]
public async void OpsTenantNoExpandFilterEqAndOrderBy()
Expand Down Expand Up @@ -847,6 +925,33 @@ void Test(ICollection<CoreBuilding> collection)
Assert.Equal("Leeds", collection.Last().Builder.City.Name);
}
}

private IQueryable<TMandator> GetMandators()
{
return new TMandator[]
{
new TMandator
{
Identity = Guid.Empty, // The first guide in order.
Name = "Two", // Duplicate name.
CreatedDate = new DateTime(2011, 12, 12)
},
new TMandator
{
Identity = Guid.NewGuid(),
Name = "One",
CreatedDate = new DateTime(2012, 12, 12),
Buildings = new List<TBuilding>()
},
new TMandator
{
Identity = Guid.NewGuid(),
Name = "Two",
CreatedDate = new DateTime(2012, 12, 12),
Buildings = new List<TBuilding>()
}
}.AsQueryable();
}

private IQueryable<Category> GetCategories()
=> new Category[]
Expand Down