Make orderBy unique for paging.

[NetanelPersik]

OrderBy is not fully unique #221

[BlaiseD]

Pre-existing unrelated logic should not change. Neither should any of the existing tests.

[BlaiseD]

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

[NetanelPersik]

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

[BlaiseD]

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.

[NetanelPersik]

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")

[BlaiseD]

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.

[NetanelPersik]

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...

[BlaiseD]

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.

[NetanelPersik]

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...

[BlaiseD]

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) .

[NetanelPersik]

Done, thanks!

[BlaiseD]

Thanks