Closed darjanbogdan closed 2 years ago
It's by design. I think EF6 would throw an exception using Skip
without OrderBy
. It's also recommended with EF Core.
Maybe it's a good idea to throw an InvalidOperationException
if Skip
is provided without OrderBy
- so it's clear the logic is intended.
I agree with your point, however call to GenerateStableOrder
is also skipped, even with EnableStableOrdering=true
which is default. The reason for that is because ApplyTo
is called only if FilterQueryOption
is defined.
default behavior of MS OData lib: https://github.com/OData/WebApi/blob/master/src/Microsoft.AspNet.OData.Shared/Query/ODataQueryOptions.cs#L380
This basically means breaking change from the client's perspective, any thoughts?
Note: OData spec says that service MUST impose a stable sort ordering, if $skip is included. https://docs.oasis-open.org/odata/odata/v4.01/odata-v4.01-part1-protocol.html#_Toc31358954
Note: OData spec says that service MUST impose a stable sort ordering, if $skip is included.
I think that is what we're doing by not allowing $skip
without $orderby
. I won't characterize the non-call to GenerateStableOrder
as being skipped. It's a feature this library does not have.
At some point we should get rid of the ApplyTo here is favor of just return (Expression<Func<T, bool>>)filterOption.FilterClause.GetFilterExpression(typeof(T));
. I don't think it's quite ready.
That way we're generating all our own expressions.
We can add new features like GenerateStableOrder
to QuerySettings and implement them.
Where do you see breaking changes?
Since the library depends on Microsoft.AspNetCore.OData package and uses it to build part of the expression tree I was under the impression that stable ordering behavior will remain unchanged (i.e. follow the base library; non-breaking behavior).
The complete paragraph in the spec states the following:
If no unique ordering is imposed through an $orderby query option, the service MUST impose a stable ordering across requests that include $skip.
So, current behavior as well as throwing an exception could be considered a violation of the spec. But I guess we just have a different view on the topic.
To conclude, I'm happy to help to implement that "feature", but to avoid a lengthy PR process it would be awesome if you can provide some implementation guidelines. For example, I was thinking to expose EnableStableOrdering
prop on ODataOptions
, and still rely on options.GenerateStableOrder
method to generate OrderByClause
if not present already.
EnableStableOrdering
is set on the server correct (using EnableQuery
)? Did you mean ODataSettings?
Getting the OrderByQueryOption
from options.GenerateStableOrder()
works.
EnableStableOrdering
is set on the server via ODataQuerySettings it is true
by default (I'm not sure if it's affected by EnableQuery
attribute, but that's not that important).
So, I was just thinking to reuse the same terminology and indeed extend ODataSettings
with our own EnableStableOrdering
property.
sounds good.
Hi,
I came across the issue just recently and I just want you to note that in case $skip cannot be used without $orderby, the generated paging links returned in @odata.nextLink won't be usable when the client sends a request without initially providing an $oderby field.
Hello,
same situation as @jacquesroesen mentioned @odata.nextLink is not usable with current implementation which definitely is spec violation. Is anyone working on this at the moment?
I know I'm not. PRs are welcome.
Hi @DanielGlos, I also don't work on it, didn't have time to properly implement it back then.
What I did instead is a hack which is acceptable in my case, snippet example:
private static readonly PropertyInfo _orderByPropertyInfo = typeof(ODataQueryOptions).GetProperty("OrderBy");
public static void EnsureStableOrdering(this ODataQueryOptions queryOptions)
{
if (queryOptions.OrderBy is null)
{
_orderByPropertyInfo.SetValue(queryOptions, queryOptions.GenerateStableOrder());
}
}
GenerateStableOrder()
is a method from the Microsoft.AspNet.OData.Query namespace and similarly should be used here in my opinion - just without reflection trickery :)
@darjanbogdan neat, thank you! :)
I'm assuming that skip
without a corresponding orderby
should also work for navigation collections as well?
So, a query such as /CategoryModel?$expand=Products($skip=1;$select=ProductName)
should return an ordered collection of product name's minus the first result?
I'm trying to determine if this is a valid use case.
Yes - I don't see why not - if we're going to allow it at the root then it should be ok for the nested collections also.
I'm also curious about top
. Currently, if an order was not specified but top
was, then the following method call expression is created:
return Expression.Call
(
expression.Type.IsIQueryable() ? typeof(Queryable) : typeof(Enumerable),
"Take",
new[] { type },
expression,
Expression.Constant(top.Value)
);
I believe in this case, a default order should be created for this case as well.
So far, implementing this at root is trivial, but for nested collections it's a bit more involved as there are no ODataQueryOptions
on which to call GenerateStableOrder
.
My thought here is to mimic GenerateStableOrder
, which means determining which properties can be used to generate a stable order. Right now I'm just taking a look at what's being done here.
That's a good point point regarding Top
. Do we not want to apply the same rules? Maybe we should.
With regard to generating an OrdeyBy
, the algorithm itself does not matter - better to use the same one in both cases.
Two ways to go:
Here's an example of some code getting the first literal type.
I like the 2nd option better. You'd be calling MemberInfo.GetCustomAttributes()
to see if System.ComponentModel.DataAnnotations.KeyAttribute
has been applied (if there is no Id property).
So if GenerateStableOrder
is not available for both cases I would just create a new method which does the same thing.
Yeah, so currently I'm generating a default order for the following cases:
orderby
is null
but skip
is not null
orderby
is null
but top
is not null
orderby
is null
but both top
and skip
are not null
I agree with you about using the same method for generating an order, keep things consistent. I'll start on creating such a method and go from there.
OK, just need to write a bunch of unit tests. Hoping to have a PR in for this before the end of the week (assuming I don't run in to a show stopper of course)
I've encountered issue when using standalone $skip parameter, for example
corebuilding?$skip=2
I think problem lies in
GetQueryableMethod
method:So, if
OrderByClause
is not defined, onlyTake
expression is going to be returned, without applyingSkip
expression.Before starting with the fix, could you tell me if this is actually a bug, or intended behavior. To me, it looks like the bug.