OData / AspNetCoreOData

ASP.NET Core OData: A server library built upon ODataLib and ASP.NET Core
Other
457 stars 158 forks source link

SkipToken returning invalid next link when combined with select #616

Open karayanni opened 2 years ago

karayanni commented 2 years ago

Hi team, we are using AspNetCore.Odata 8.0.8 for a simple get api with paging. the Default paging uses Skip - which is causing us regressions with EFC (inefficient query)

in order to improve the performance, we are trying to use SkipToken for generating the next page link instead.

we added the skip token this way:

AddOData(opt =>
                {
                    var edmModel = GetEdmModel();
                    opt.AddRouteComponents("api", edmModel, ConfigureODataServices).SkipToken();
                    opt.RouteOptions.EnableNonParenthesisForEmptyParameterFunction = true;
                });

this works perfectly when sending a simple get request, however, when the request has a select filter for example: http://localhost:1059/api/Machines?$select=id

the next link is: "@odata.nextLink": "https://localhost:1059/api/Machines?$select=id&$skiptoken=id-null"

can you help in investigating this bug?

Best, Nader.

xuzhg commented 2 years ago

@karayanni the skiptoken token is calculated using the last 'key' value. The last 'key' value is 'null'? What is your response payload look like?

Would you please share your repro?

By the way, we support to customize/inject SkipTokenHandler, if the default one can't meet your requirement, maybe consider to create your handler.

karayanni commented 2 years ago

Hi @xuzhg sorry for the late response,

The last key is not null, all entities have a Key (ID in our case)

we can see that when we don't use the select=id the skiptoken works perfectly, yet when we add the selet filter we get the provided above next link - "https://localhost:1059/api/Machines?$select=id&$skiptoken=id-null"

karayanni commented 2 years ago

@xuzhg friendly ping

karayanni commented 2 years ago

@xuzhg

Hi Sam, I'm debugging the Odata SkipToken flow locally, And I found an interesting behavior you might want to hear about

specifically, in the GenerateSkipTokenValue funciton -

in the code, we see this line

(1)       IEdmStructuredObject obj = lastMember as IEdmStructuredObject; 

and if we are able to parse the object as EDM structured (obj != null), we read the properties by:

(2)       obj.TryGetPropertyValue(clrPropertyName, out value)

if we are not (obj == null) we read the property from the original object with reflection:

(3)       value = lastMember.GetType().GetProperty(clrPropertyName).GetValue(lastMember);

how ever this causes us an issue of inconsistency.

when we try to a request with no select (we have no structured EDM object and hence we read according to the (3) and it works great.

however when we send a request with a select query (doesn't matter what properties we select) we have an edm structured object, and so we read the property's value according to (2). and this in our case scenario fails.

the clrPropertyName returns Id in our case (with capital I), however in the Edm model the property name is id (small i) as we want it. and so (2) return value == null as it can't find Id in the edm model.

I understand that I can solve the issue by assigning the same exact naming for the EDM and the internal property (atleast for the keys and the sortable properties) however we prefer not to do so.

what can we do to solve this?

Also, I noticed I can't inject IPropertyMapper to solve the issue, since SelectExpandWrapper only uses the Default = IdentityPropertyMapper

alex-zak commented 2 years ago

Please check if you have the skiptoken() option enabled. e.g. configuration.MaxTop(100).Expand().Filter().OrderBy().SkipToken(); if you remove it the lib will use a skip instead of a skiptoken for the next link. This should solve your issue. Anyway I thing this is a breaking bug for the skiptoken feature implementation and should be fixed. More information on the topic. https://docs.microsoft.com/en-us/odata/webapi/skiptoken-for-server-side-paging

karayanni commented 2 years ago

Hi @alex-zak, thank you for your response

Ofc I have the skiptoken enabled, otherwise the above code wouldn’t be relevant for my case - meanwhile I disabled it in production since it is a breaking bug (yet the next link is causing us regressions as it is extremely inefficient for high volume paging)

orty commented 1 year ago

We are experiencing the exact same issue. Here follows our EDM configuration :

var odataBuilder = new ODataConventionModelBuilder();
...
odataBuilder.EntitySet<MyObject>("myobjects");
...
odataBuilder.EnableLowerCamelCase();

We need to enable the lower camel case JSON serialization to preserve compliancy with the previous versions of our API.

Then, our entites use the standard notation for auto-properties :

public class MyObject
{
    public Guid Id { get; set; }

    public DateTime Created { get; set;}
}

When using the $select query option, the IEdmStructuredObject representing the Microsoft.AspNetCore.OData.Query.Wrapper.SelectSome<MyObject> holds the properties in its internal dictionary using the lowercase camel notation, as stated by @karayanni.

The GenerateSkipTokenValue method is then unable to retrieve the value for a property which it is given in a Pascal case notation (the result of this call model.GetClrPropertyName(edmProperty)).

Is there anything wrong with our configuration ?

Thanks