Skip to content
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.

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

@odata.nextLink doesn't work when query contains $orderby=parentEntity/childEntity #1147

Closed
AndriiLesiuk opened this issue Dec 27, 2023 · 11 comments · Fixed by #1164
Closed
Assignees
Labels
bug Something isn't working P2

Comments

@AndriiLesiuk
Copy link

OData 8.2.3

My Odata query:
https://localhost:44325/odata/Mas?$expand=product&$select=product&$orderby=product/name

It returns me such responce:
image

with current @odata.nextLink:
"@odata.nextLink": "https://localhost:44325/odata/Mas?$expand=product&$select=product&$orderby=product%2Fname&$skiptoken=name-null,source_id-%2711514%27"

When I follow this link I get the following error:
image

For some reason, the functionality is trying to find the "name" field in the parentEntity.
If it's not a bug, then tell me how to make my query work.
Thank you for your advice.

Project to reproduce

@AndriiLesiuk AndriiLesiuk added the bug Something isn't working label Dec 27, 2023
@aboryczko
Copy link

I understand that this might be some simplification of your issue but expanding, selecting and ordering inside of a single property would probably behave better with this query https://localhost:44325/odata/Mas/product&$orderby=name

@aboryczko
Copy link

also after checking the implementation of DefaultSkipTokenHandler I don't see any handling of nested properties.

@AndriiLesiuk
Copy link
Author

I understand that this might be some simplification of your issue but expanding, selecting and ordering inside of a single property would probably behave better with this query https://localhost:44325/odata/Mas/product&$orderby=name

Thanks for the answer.
But your example doesn't work for my controller:

        [HttpGet, EnableQuery(PageSize = 1000)]
        [ProducesResponseType(typeof(Domain.Entities.Mas.Mas), 200)]
        public IActionResult Get()
        {
            var result = _service.Get(); //  IQueryable<Mas>
            return Ok(result);
        }

Although it seemed to me that such functionality should be out of the box.
Anyway, thanks for your help.

@aboryczko
Copy link

I think you would need to implement a GetProduct() method in the controller for that. Unfortunately there is no OOB support for this feature.

@AndriiLesiuk
Copy link
Author

@aboryczko thanks.
@julealgon, @xuzhg could you pls confirm that is no OOB support for this feature?
Cheers

@senioroman4uk
Copy link
Contributor

I was able to catch the same issue with a complex property as well. https://github.com/senioroman4uk/odata-skip-token-exception
Whenever OData tries to generate a skip token with orderby for a complex property DefaultSkipTokenHandler will throw a null reference exception.

Any suggestions to mitigate this would be appreciated.

System.NullReferenceException: Object reference not set to an instance of an object.
         at Microsoft.AspNetCore.OData.Query.DefaultSkipTokenHandler.GenerateSkipTokenValue(Object lastMember, IEdmModel model, IList`1 orderByNodes, TimeZoneInfo timeZoneInfo)
         at Microsoft.AspNetCore.OData.Query.DefaultSkipTokenHandler.<>c__DisplayClass3_0.<GenerateNextPageLink>b__1(Object obj)
         at Microsoft.AspNetCore.OData.Extensions.GetNextPageHelper.GetNextPageLink(Uri requestUri, IEnumerable`1 queryParameters, Int32 pageSize, Object instance, Func`2 objectToSkipTokenValue)
         at Microsoft.AspNetCore.OData.Extensions.GetNextPageHelper.GetNextPageLink(Uri requestUri, Int32 pageSize, Object instance, Func`2 objectToSkipTokenValue)
         at Microsoft.AspNetCore.OData.Query.DefaultSkipTokenHandler.GenerateNextPageLink(Uri baseUri, Int32 pageSize, Object instance, ODataSerializerContext context)
         at Microsoft.AspNetCore.OData.Formatter.Serialization.ODataResourceSetSerializer.<>c__DisplayClass17_2.<GetNextLinkGenerator>b__2(Object obj)
         at Microsoft.AspNetCore.OData.Formatter.Serialization.ODataResourceSetSerializer.WriteResourceSetAsync(IEnumerable enumerable, IEdmTypeReference resourceSetType, ODataWriter writer, ODataSerializerContext writeContext)
         at Microsoft.AspNetCore.OData.Formatter.Serialization.ODataResourceSetSerializer.WriteObjectInlineAsync(Object graph, IEdmTypeReference expectedType, ODataWriter writer, ODataSerializerContext writeContext)
         at Microsoft.AspNetCore.OData.Formatter.Serialization.ODataResourceSetSerializer.WriteObjectAsync(Object graph, Type type, ODataMessageWriter messageWriter, ODataSerializerContext writeContext)
         at Microsoft.AspNetCore.OData.Formatter.ODataOutputFormatterHelper.WriteToStreamAsync(Type type, Object value, IEdmModel model, ODataVersion version, Uri baseAddress, MediaTypeHeaderValue contentType, HttpRequest request, IHeaderDictionary requestHeaders, IODataSerializerProvider serializerProvider)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeResultAsync>g__Logged|22_0(ResourceInvoker invoker, IActionResult result)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResultFilterAsync>g__Awaited|30_0[TFilter,TFilterAsync](ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResultExecutedContextSealed context)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.ResultNext[TFilter,TFilterAsync](State& next, Scope& scope, Object& state, Boolean& isCompleted)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeResultFilters()
      --- End of stack trace from previous location ---
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeFilterPipelineAsync>g__Awaited|20_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Logged|17_1(ResourceInvoker invoker)
         at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Logged|17_1(ResourceInvoker invoker)
         at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
         at Microsoft.AspNetCore.OData.Query.ODataQueryRequestMiddleware.Invoke(HttpContext context)
         at Microsoft.AspNetCore.OData.Routing.ODataRouteDebugMiddleware.Invoke(HttpContext context)
         at Swashbuckle.AspNetCore.SwaggerUI.SwaggerUIMiddleware.Invoke(HttpContext httpContext)
         at Swashbuckle.AspNetCore.Swagger.SwaggerMiddleware.Invoke(HttpContext httpContext, ISwaggerProvider swaggerProvider)
         at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)
         at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)
         at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ProcessRequests[TContext](IHttpApplication`1 application)

@julealgon
Copy link
Contributor

@julealgon, @xuzhg could you pls confirm that is no OOB support for this feature?

I don't know @AndriiLesiuk but it feels like a nasty bug to me for sure... especially with it generating "garbage" instead of just throwing an exception or something to signal it wouldn't be supported (if it really was supposed to not be supported in the first place, which I don't think is the case here).

This type of issue makes the library look very unprofessional IMHO. Would be nice if it could be prioritized @habbes .

@habbes habbes added the P2 label Jan 17, 2024
@habbes
Copy link
Contributor

habbes commented Jan 17, 2024

@julealgon @senioroman4uk @AndriiLesiuk thanks. We're currently working on fixing this issue cc @xuzhg

@xuzhg
Copy link
Member

xuzhg commented Jan 17, 2024

@AndriiLesiuk @senioroman4uk With the latest 8.2.4, it should work using $orderby=complex/property without enable skiptoken.
if you do enable skiptoken, customize the SkipTokenHandler to return null for this scenario and we'd switch to use $skip.

as @habbes mentioned, we are fixing the issue now. But, it's related to the following discussion:

What skiptoken pattern by default should take for advanced $orderby?

For example, here's a complex $orderby:
$orderby=tolower(substring(product/name,1,2))

What skiptoken should look like:

  1. $skiptoken=tolower(substring(product/name,1,2))-'ab',source_id-%2711514%27

or
2) $skiptoken='ab',%2711514%27 (only contains the value)

or

  1. $skiptoken=source_id-%2711514%27 (only contains the key name and value, since we can calculate tolower(substring(product/name,1,2)) at server side)

or

  1. $skiptoken=%2711514%27 (only contains the key value, since we can calculate tolower(substring(product/name,1,2)) at server side)

@julealgon @senioroman4uk @AndriiLesiuk any thoughts?

@AndriiLesiuk
Copy link
Author

@xuzhg
My opinion is that the entire skiptoken in the end should look like in your third example:
$skiptoken=source_id-%2711514%27 (only contains the key name and value, since we can calculate tolower(substring(product/name,1,2)) at server side)

@mikepizzo
Copy link
Member

@AndriiLesiuk -- the issue with including only the id in the $skiptoken is that we need to know the last read values of all orderby properties in order to generate the query to read the next page. We already have those values when generating the skiptoken, so encoding them in the skiptoken saves us from having to calculate them when generating the query for the next page. If we don't serialize them as part of the skiptoken, then when we parse the skiptoken we either need to make a separate request to get the values to use to generate the query for the next page, or we need to create a potentially very complex query with multiple subselects in order to calculate those values as part of executing the next page query. Our primary concern with either case is performance -- we're adding a lot of server load to recalculate something that we already had and could have just encoded in the skiptoken (which is defined as being opaque). A second concern would be if the last record read happened to be deleted then we wouldn't be able to process the skip token, where-as if we included the values when we generated the skiptoken then it would be resilient to the record being changed or deleted.

xuzhg added a commit that referenced this issue Feb 29, 2024
…use (#1164)

* Fixes #1147: Enable skiptoken handler to handler advanced orderby clause

* Fix the failing test cases

* Address the comments

* Address the comments
1) Can handle the 'asc' suffix
2) Can handle the ',' within expression
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants