OData / WebApi

OData Web API: A server library built upon ODataLib and WebApi
https://docs.microsoft.com/odata
Other
856 stars 473 forks source link

$Skiptoken pagination issue when combined with $orderby #1943

Closed ghost closed 1 year ago

ghost commented 5 years ago

Pagination is not working when it is based on the $skiptoken and combined with $orderby on properties of type Byte or nullable integer (int?).

I get this error on OData WebApi lib 7.2.1 and 7.2.2

You can reproduce the issue using the solution available here

The whole error message is:


{
    "error": {
        "code": "",
        "message": "The query specified in the URI is not valid. The binary operator AndAlso is not defined for the types 'System.Nullable`1[System.Boolean]' and 'System.Boolean'.",
        "innererror": {
            "message": "The binary operator AndAlso is not defined for the types 'System.Nullable`1[System.Boolean]' and 'System.Boolean'.",
            "type": "System.InvalidOperationException",
            "stacktrace": "   at System.Linq.Expressions.Expression.AndAlso(Expression left, Expression right, MethodInfo method)\r\n   at Microsoft.AspNet.OData.Query.DefaultSkipTokenHandler.ApplyToCore(IQueryable query, ODataQuerySettings querySettings, IList`1 orderByNodes, ODataQueryContext context, String skipTokenRawValue)\r\n   at Microsoft.AspNet.OData.Query.DefaultSkipTokenHandler.ApplyTo(IQueryable query, SkipTokenQueryOption skipTokenQueryOption)\r\n   at Microsoft.AspNet.OData.Query.SkipTokenQueryOption.ApplyTo(IQueryable query, ODataQuerySettings querySettings, ODataQueryOptions queryOptions)\r\n   at Microsoft.AspNet.OData.Query.ODataQueryOptions.ApplyTo(IQueryable query, ODataQuerySettings querySettings)\r\n   at Microsoft.AspNet.OData.EnableQueryAttribute.ApplyQuery(IQueryable queryable, ODataQueryOptions queryOptions)\r\n   at Microsoft.AspNet.OData.EnableQueryAttribute.ExecuteQuery(Object responseValue, IQueryable singleResultCollection, IWebApiActionDescriptor actionDescriptor, Func`2 modelFunction, IWebApiRequestMessage request, Func`2 createQueryOptionFunction)\r\n   at Microsoft.AspNet.OData.EnableQueryAttribute.OnActionExecuted(Object responseValue, IQueryable singleResultCollection, IWebApiActionDescriptor actionDescriptor, IWebApiRequestMessage request, Func`2 modelFunction, Func`2 createQueryOptionFunction, Action`1 createResponseAction, Action`3 createErrorAction)"
        }
    }
}
TehWardy commented 5 years ago

What query are you giving it? Seems like a standard casting exception you might get from C# code doing something like ...

var x = ((bool?)null) == false;

... since bool is a different type to nullable bool you get implicit failure.

ghost commented 5 years ago

What query are you giving it? Seems like a standard casting exception you might get from C# code doing something like ...

var x = ((bool?)null) == false;

... since bool is a different type to nullable bool you get implicit failure.

Hello

I'm not doing any thing in the c# code. I get the error when i try to access the second page of the payload using the @odata.nextLink

http://localhost:8880/odata/people?$orderby=code&$skiptoken=Code-2,Id-2

For more information please take a look at the example i provided. It's quite simple to reproduce.

TehWardy commented 5 years ago

I don't know enough about $skiptoken to be sure but somethings triggering me about that.

You could use $skip and $take (which as I understand it is the more common approach. For my UI scenarios I combine my webapi OData endpoints with Kendo UI which builds $kip and $take params from a client side data source.

This also works with $orderby parameters.

ghost commented 5 years ago

Thank you for the advice. But, the main reason why we prefer using the $skiptoken is to avoid performance issues related to $skip and $take.

TehWardy commented 5 years ago

According to the examples given here: https://docs.microsoft.com/en-us/odata/webapi/skiptoken-for-server-side-paging ... I think the example you gave might be wrong ... perhaps that's your issue?

$skiptoken=Code-2,Id-2

... should be something like ..,.

$skiptoken=Code:2,Id:2

... surely ???

Or have i misunderstood this ?

Also if you you are using order by it appears that all the fields in your orderby clause must also appear in the skiptoken clause (again I could be missing something but this sounds like a typical querying issue).

For example ...

GET ~/EntitySet?$orderby=Prop1,Prop2&$skiptoken=Prop1:value1,Prop2:value2,Id1:idVal1,Id2:idVal2

ghost commented 5 years ago

The link i provided was generated by odata itself and i confirm you that's a valid url. For information, since the last update of the skiptokenHanlder, they change the format of the skiptoken, they replace the : by a dash

I provided the link just to show you how to reproduce quickly.


{
    "@odata.context": "http://localhost:8880/odata/$metadata#People",
    "value": [
        {
            "Id": 3,
            "Name": "third",
            "Age": 78,
            "Code": 1
        },
        {
            "Id": 2,
            "Name": "second",
            "Age": 45,
            "Code": 2
        }
    ],
    "@odata.nextLink": "http://localhost:8880/odata/people?$orderby=code&$skiptoken=Code-2,Id-2"
}
ghost commented 5 years ago

Hello

I did a quick analysis of this bug and it seems that there are two issues :

  1. Properties of type Byte are not populated correctly from the skiptoken raw value
    
    // 20191031094412
    // http://localhost:1652/odata/people?$orderby=age&$skiptoken=Age-45,Id-2

{ "error": { "code": "", "message": "The query specified in the URI is not valid. Type verification failed. Expected type 'Edm.Byte' but received the value '45'.", "innererror": { "message": "Type verification failed. Expected type 'Edm.Byte' but received the value '45'.", "type": "Microsoft.OData.ODataException", "stacktrace": " at Microsoft.OData.ODataUriConversionUtils.VerifyAndCoerceUriPrimitiveLiteral(Object primitiveValue, String literalValue, IEdmModel model, IEdmTypeReference expectedTypeReference)\r\n at Microsoft.OData.ODataUriUtils.ConvertFromUriLiteral(String value, ODataVersion version, IEdmModel model, IEdmTypeReference typeReference)\r\n at Microsoft.AspNet.OData.Query.DefaultSkipTokenHandler.PopulatePropertyValuePairs(String value, ODataQueryContext context) in C:\enablon\OData\WebApi\src\Microsoft.AspNet.OData.Shared\Query\DefaultSkipTokenHandler.cs:line 312\r\n at Microsoft.AspNet.OData.Query.DefaultSkipTokenHandler.ApplyToCore(IQueryable query, ODataQuerySettings querySettings, IList1 orderByNodes, ODataQueryContext context, String skipTokenRawValue) in C:\\enablon\\OData\\WebApi\\src\\Microsoft.AspNet.OData.Shared\\Query\\DefaultSkipTokenHandler.cs:line 220\r\n at Microsoft.AspNet.OData.Query.DefaultSkipTokenHandler.ApplyTo(IQueryable query, SkipTokenQueryOption skipTokenQueryOption) in C:\\enablon\\OData\\WebApi\\src\\Microsoft.AspNet.OData.Shared\\Query\\DefaultSkipTokenHandler.cs:line 190\r\n at Microsoft.AspNet.OData.Query.SkipTokenQueryOption.ApplyTo(IQueryable query, ODataQuerySettings querySettings, ODataQueryOptions queryOptions) in C:\\enablon\\OData\\WebApi\\src\\Microsoft.AspNet.OData.Shared\\Query\\SkipTokenQueryOption.cs:line 104\r\n at Microsoft.AspNet.OData.Query.ODataQueryOptions.ApplyTo(IQueryable query, ODataQuerySettings querySettings) in C:\\enablon\\OData\\WebApi\\src\\Microsoft.AspNet.OData.Shared\\Query\\ODataQueryOptions.cs:line 389\r\n at Microsoft.AspNet.OData.EnableQueryAttribute.ApplyQuery(IQueryable queryable, ODataQueryOptions queryOptions) in C:\\enablon\\OData\\WebApi\\src\\Microsoft.AspNet.OData.Shared\\EnableQueryAttribute.cs:line 482\r\n at Microsoft.AspNet.OData.EnableQueryAttribute.ExecuteQuery(Object responseValue, IQueryable singleResultCollection, IWebApiActionDescriptor actionDescriptor, Func2 modelFunction, IWebApiRequestMessage request, Func2 createQueryOptionFunction) in C:\\enablon\\OData\\WebApi\\src\\Microsoft.AspNet.OData.Shared\\EnableQueryAttribute.cs:line 621\r\n at Microsoft.AspNet.OData.EnableQueryAttribute.OnActionExecuted(Object responseValue, IQueryable singleResultCollection, IWebApiActionDescriptor actionDescriptor, IWebApiRequestMessage request, Func2 modelFunction, Func2 createQueryOptionFunction, Action1 createResponseAction, Action`3 createErrorAction) in C:\enablon\OData\WebApi\src\Microsoft.AspNet.OData.Shared\EnableQueryAttribute.cs:line 415" } } }


2. The second issue is related to null propagation when generating the comparison expression. In the following example, Id is the primary key of type integer (not nullable) and the Code is just a nullable integer property. Including the Code in the orderby will raise this issue
```json
// 20191031095151
// http://localhost:1652/odata/people?$orderby=code&$skiptoken=Code-2,Id-2

{
  "error": {
    "code": "",
    "message": "The query specified in the URI is not valid. The binary operator AndAlso is not defined for the types 'System.Nullable`1[System.Boolean]' and 'System.Boolean'.",
    "innererror": {
      "message": "The binary operator AndAlso is not defined for the types 'System.Nullable`1[System.Boolean]' and 'System.Boolean'.",
      "type": "System.InvalidOperationException",
      "stacktrace": "   at System.Linq.Expressions.Expression.AndAlso(Expression left, Expression right, MethodInfo method)\r\n   at System.Linq.Expressions.Expression.AndAlso(Expression left, Expression right)\r\n   at Microsoft.AspNet.OData.Query.DefaultSkipTokenHandler.ApplyToCore(IQueryable query, ODataQuerySettings querySettings, IList`1 orderByNodes, ODataQueryContext context, String skipTokenRawValue) in C:\\enablon\\OData\\WebApi\\src\\Microsoft.AspNet.OData.Shared\\Query\\DefaultSkipTokenHandler.cs:line 272\r\n   at Microsoft.AspNet.OData.Query.DefaultSkipTokenHandler.ApplyTo(IQueryable query, SkipTokenQueryOption skipTokenQueryOption) in C:\\enablon\\OData\\WebApi\\src\\Microsoft.AspNet.OData.Shared\\Query\\DefaultSkipTokenHandler.cs:line 190\r\n   at Microsoft.AspNet.OData.Query.SkipTokenQueryOption.ApplyTo(IQueryable query, ODataQuerySettings querySettings, ODataQueryOptions queryOptions) in C:\\enablon\\OData\\WebApi\\src\\Microsoft.AspNet.OData.Shared\\Query\\SkipTokenQueryOption.cs:line 104\r\n   at Microsoft.AspNet.OData.Query.ODataQueryOptions.ApplyTo(IQueryable query, ODataQuerySettings querySettings) in C:\\enablon\\OData\\WebApi\\src\\Microsoft.AspNet.OData.Shared\\Query\\ODataQueryOptions.cs:line 389\r\n   at Microsoft.AspNet.OData.EnableQueryAttribute.ApplyQuery(IQueryable queryable, ODataQueryOptions queryOptions) in C:\\enablon\\OData\\WebApi\\src\\Microsoft.AspNet.OData.Shared\\EnableQueryAttribute.cs:line 482\r\n   at Microsoft.AspNet.OData.EnableQueryAttribute.ExecuteQuery(Object responseValue, IQueryable singleResultCollection, IWebApiActionDescriptor actionDescriptor, Func`2 modelFunction, IWebApiRequestMessage request, Func`2 createQueryOptionFunction) in C:\\enablon\\OData\\WebApi\\src\\Microsoft.AspNet.OData.Shared\\EnableQueryAttribute.cs:line 621\r\n   at Microsoft.AspNet.OData.EnableQueryAttribute.OnActionExecuted(Object responseValue, IQueryable singleResultCollection, IWebApiActionDescriptor actionDescriptor, IWebApiRequestMessage request, Func`2 modelFunction, Func`2 createQueryOptionFunction, Action`1 createResponseAction, Action`3 createErrorAction) in C:\\enablon\\OData\\WebApi\\src\\Microsoft.AspNet.OData.Shared\\EnableQueryAttribute.cs:line 415"
    }
  }
}
kephouzz commented 5 years ago

Hello @msftclas,

it's a blocking issue for our project. Do you have any updates on this ticket?

Thanks

KanishManuja-MS commented 5 years ago

@kephouzz These types are not supported in the default implementation. Perhaps you can inject a custom implementation to handle these types. I can help you to do that.

One other temporary workaround can be to enable skiptoken only for the routes which deal with supported types and fall back to $skip and $top for other routes.

kephouzz commented 5 years ago

But, will you work on it for fixing it?

Thanks

KanishManuja-MS commented 5 years ago

@kephouzz Yes, I will at some point. I will have this added to the backlog. If you feel it is urgent for you to be fixed then let me know.

ghost commented 5 years ago

@KanishManuja-MS Thank you for your answer.

Regarding the workaround based on $skip and $top, we faced performance issues with this solution and this is why we switched to the $skiptoken.

Could we have an estimation on when do you plan to fix this issue?

Thanks

KanishManuja-MS commented 5 years ago

@MYoucef Performance for $skip and $top will be better than skiptoken for paging. Skiptoken definitely does more computation for processing the results but is more robust in case of deletions and additions.

Can you share why you consider $skip and $top has performance issues?

ghost commented 4 years ago

@KanishManuja-MS It seems that in same cases the performance of the $skip and $top is very poor especially where we have a lot of records in the data table.

Here a concrete example where we faced this issue;

-- $skip and $top generated SQL Query
SELECT Id
FROM [TDE_myehs].[dbo].[S_sd__PX_Data]
order by Id
offset 90000000 rows
fetch next 10000 rows only

-- $skiptoken generated SQL Query
SELECT top 10000 Id
FROM [TDE_myehs].[dbo].[S_sd__PX_Data]
where Id > 156924231
order by Id

These two requests generate the following execution plan

execplan

It seems that the "Clustered Index Scan" is slower than the "Clustered Index Seek"

We tried to optimize the SQL query using :

DECLARE @StartingRowNumber int = 1

SELECT Id
  FROM [TDE_myehs].[dbo].[S_sd__PX_Data]
  order by Id
      OFFSET @StartingRowNumber - 1 ROWS  
        FETCH NEXT 10000 ROWS ONLY 
   OPTION ( OPTIMIZE FOR (@StartingRowNumber = 90000000) ); 

This SQL query is fast. Unfortunately, this implementation cannot be generalized for the $skip and $top because it depends on the column data type (in our case a identity clustered)

So, this is why we switched to the $skiptoken. I hope this answer your question. Don't hesitate if you need further information.

TehWardy commented 4 years ago

Presumably that only works if the result set is ordered by that indexes values and they are nicely "sequential", how do you handle this when the ordering is not that way?

ambitiousrahul commented 4 years ago

@KanishManuja-MS It seems that in same cases the performance of the $skip and $top is very poor especially where we have a lot of records in the data table.

Here a concrete example where we faced this issue;

-- $skip and $top generated SQL Query
SELECT Id
FROM [TDE_myehs].[dbo].[S_sd__PX_Data]
order by Id
offset 90000000 rows
fetch next 10000 rows only

-- $skiptoken generated SQL Query
SELECT top 10000 Id
FROM [TDE_myehs].[dbo].[S_sd__PX_Data]
where Id > 156924231
order by Id

These two requests generate the following execution plan

execplan

It seems that the "Clustered Index Scan" is slower than the "Clustered Index Seek"

We tried to optimize the SQL query using :

DECLARE @StartingRowNumber int = 1

SELECT Id
  FROM [TDE_myehs].[dbo].[S_sd__PX_Data]
  order by Id
      OFFSET @StartingRowNumber - 1 ROWS  
        FETCH NEXT 10000 ROWS ONLY 
   OPTION ( OPTIMIZE FOR (@StartingRowNumber = 90000000) ); 

This SQL query is fast. Unfortunately, this implementation cannot be generalized for the $skip and $top because it depends on the column data type (in our case a identity clustered)

So, this is why we switched to the $skiptoken. I hope this answer your question. Don't hesitate if you need further information.

@MYoucef may I know how did you get this sql query from oData request?

ghost commented 4 years ago

@ambitiousrahul I'm not sure I fully got your point. I basically used SQL Server Profiler to track SQL queries triggered by OData to understand what's happening behind the scene. Depending on what database engine you're using, you can use the adequate tool to track all SQL queries.

For the last SQL request, I built it myself based on the original I got from SQL Profiler

ambitiousrahul commented 4 years ago

@KanishManuja-MS

I was going through the blog that you have wrote regarding implementation of $skipToken using oDataController but there you have not explained the usage with aspnet core endpoint routing, and neither have I found anywhere around the web.. Is it even supported with default aspnet core routing or not? because I have been trying to do this for last few days.

I have been trying to use default version of skiptoken , say /endpoints?$select=id&$orderby=recordedAt&$top=10&$skiptoken=5700

which should return top 10 records starting from 5701 row_number. How do I do that..

Also, I am trying to do so because I have found that $top=10&$skip=5700 does not returns consistent result from the database , not in sync with the normal paging request returned by database. I have raised a issue for this

So, also I wanted to know , if I would surely get same results with $skiptoken or not?

ghost commented 4 years ago

@ambitiousrahul

First you need to enable the SkipToken because it's not the default one.

For example:

config.Count().Expand().SkipToken();

Then in you should format your url correctly. You should provide a valuekeypair Attribute-Value:

...$skiptoken=Id-2,Code-'Code2'

ambitiousrahul commented 4 years ago

@ambitiousrahul

First you need to enable the SkipToken because it's not the default one.

For example:

config.Count().Expand().SkipToken();

Then in you should format your url correctly. You should provide a valuekeypair Attribute-Value:

...$skiptoken=Id-2,Code-'Code2'

Apologies for the replying this late.. I have enabled the skiptoken already as you have mentioned but I am not able to understand how to format skipToken for paging.. examples shows that skipToken can too be used as skip which works as OFFSET e..g. endpoints?$select=id&$orderby=recordedAt&$top=10&$skiptoken=5700

But, this certainly does not works. Thanks for your help @MYoucef

wise-tjorvi commented 2 years ago

I'm hitting this same issue. In my case I'm ordering by a nullable DateTime column. Is there a known workaround for this?

Like the original poster, I switched to $skiptoken because of performance issues with $skip on Sql Server.