tsharp/fix next page issue

[tsharp]

There is an issue with client specified top sizes. When it is explicitly specified and meets the top > pageSize condition, a null url will be returned which is invalid. This code seems to have a purpose to right size the top statement.

[tsharp]

@microsoft-github-policy-service agree company="Microsoft"

[gathogojr]

@tsharp Thank you for your pull request contribution. Could you please provide details of the issue you're trying to fix? In the PR description you say When it is explicitly specified and meets the top > pageSize condition, a null url will be returned which is invalid, but I can't quite reproduce the issue. I created a simple service based on the following logic:

// Data model
public class Customer
{
    public int Id { get; set; }
    public string? Name { get; set; }
}


// Edm model and service configuration
var builder = WebApplication.CreateBuilder(args);

var modelBuilder = new ODataConventionModelBuilder();
modelBuilder.EntitySet<Customer>("Customers");

builder.Services.AddControllers().AddOData(
    options => options.Select().Filter().OrderBy().Count().Expand().SetMaxTop(null).AddRouteComponents(
        model: modelBuilder.GetEdmModel()));

var app = builder.Build();

app.UseRouting();
app.UseEndpoints(endpoints => endpoints.MapControllers());

app.Run();


// Controller
public class CustomersController : ODataController
{
    private static readonly List<Customer> customers = new List<Customer>(Enumerable.Range(1, 13).Select(
        idx => new Customer
        {
            Id = idx,
            Name = $"Customer {idx}"
        }));

    [EnableQuery (PageSize = 5)]
    public ActionResult<IEnumerable<Customer>> Get()
    {
        return customers;
    }
}

Note that I have configured a page size of 5. If I request for customers and specify a $top value of 7, I get the following response:

{
    "@odata.context": "http://localhost:5167/$metadata#Customers",
    "value": [
        {
            "Id": 1,
            "Name": "Customer 1"
        },
        {
            "Id": 2,
            "Name": "Customer 2"
        },
        {
            "Id": 3,
            "Name": "Customer 3"
        },
        {
            "Id": 4,
            "Name": "Customer 4"
        },
        {
            "Id": 5,
            "Name": "Customer 5"
        }
    ],
    "@odata.nextLink": "http://localhost:5167/Customers?$top=2&$skip=5"
}

Kindly share the steps to reproduce the issue so we are able to relate. Please also familiarize with the documented behaviour here https://learn.microsoft.com/en-us/odata/webapi-8/fundamentals/server-driven-paging#combining-pagesize-and-top

[tsharp]

@gathogojr Right. so if you specify a $top in the request url the @odata.nextLink will be null instead of using the reduced page size. You can reproduce this issue by reverting the change I made and running my updated unit tests.

For example, http://localhost/odata/example would produce a next link but http://localhost/odata/example?$top=5 will not.

[tsharp]

Using your example above, if you specify the $top=3 parameter, the following is produced:

{
    "@odata.context": "https://localhost:7280/$metadata#Customers",
    "value": [
        {
            "Id": 1,
            "Name": "Customer 1"
        },
        {
            "Id": 2,
            "Name": "Customer 2"
        },
        {
            "Id": 3,
            "Name": "Customer 3"
        }
    ]
}

[gathogojr]

Using your example above, if you specify the $top=3 parameter, the following is produced:

{
    "@odata.context": "https://localhost:7280/$metadata#Customers",
    "value": [
        {
            "Id": 1,
            "Name": "Customer 1"
        },
        {
            "Id": 2,
            "Name": "Customer 2"
        },
        {
            "Id": 3,
            "Name": "Customer 3"
        }
    ]
}

@tsharp That is by design

AspNetCoreOData/src/Microsoft.AspNetCore.OData/Extensions/GetNextPageHelper.cs

Line 51 in 9f1547e

// If the $top query option's value is less than or equal to the page size, there is no next page.

[tsharp]

@gathogojr According to the specification - this is not expected behavior and has regressed from v7 to v8 version of the OData Library.

https://docs.oasis-open.org/odata/odata/v4.01/odata-v4.01-part1-protocol.html#sec_SystemQueryOptiontop

The $top system query option specifies a non-negative integer n that limits the number of items returned from a collection. The service returns the number of available items up to but not greater than the specified value n.

https://docs.oasis-open.org/odata/odata-json-format/v4.0/cs01/odata-json-format-v4.0-cs01.html#_Toc365464689

The odata.nextLink annotation indicates that a response is only a subset of the requested collection of entities or collection of entity references. It contains a URL that allows retrieving the next subset of the requested collection.

The specification does not explicitly state that the nextLink may not be or must not* be present when combined with $top. This leads to undesired work arounds for expected behavior.

For further context - the official Microsoft Graph also behaves in this way too:

[gathogojr]

@tsharp

this is not expected behavior and has regressed from v7 to v8 version of the OData Library.

Can you confirm that this is a regression... Because from where I sit both v7 and v8 behave the same way. You can fire up a quick repro using Microsoft.AspNetCore.OData 7.6.5 version to confirm.

[tsharp]

@tsharp

this is not expected behavior and has regressed from v7 to v8 version of the OData Library.

Can you confirm that this is a regression... Because from where I sit both v7 and v8 behave the same way. You can fire up a quick repro using Microsoft.AspNetCore.OData 7.6.5 version to confirm.

Fair point - I am unable to reproduce the issue as well in 7.4., 7.6., etc - but the other statements still align with the expected behavior of the $top query. See the documentation links to the OData spec in the previous comments.

Are we saying that the page size must not be controlled by the client or that the implementation details have some long-standing nuance that sets this precedence? It seems kind of random to not return a next link if the $top is specified in a query, no? maybe those implementation details might be better placed in a different spot? This would allow the developer to still call the Request.GetNextPageLink and have a desired output with supported code. I could even add a parameter to the method to 'allowClientSideTop'.

Either way, what do you suggest we do next here?

[gathogojr]

@tsharp

About the regression, I just wanted us to start from correct premise.

Regarding the way forward, we have both server-controlled (via PageSize) and client-controlled (via $top) at play here. I want to understand well the reasoning behind the current implementation and the proposed fix to be sure we're covering as many angles as we can think of.

For instance, let's say the service is configured to return pages of 50 items, but the client would like to retrieve pages of 3 items at a time. Would it be right to fulfil the client's expectations in all scenarios? If we fulfil the client's expectations and in addition return the next-link, the server has to handle 17 roundtrips (17*3=51) to return 50 items that it had no problem returning in one go. What if the 50 items configuration was the server-side balance between too little and too much?

I don't think we can just consider $top without taking server-controlled paging configuration into account.
Allow me time to review the spec and also discuss it with different people.

We could also move the conversation to the Discussions page to get views from the OData community.

cc. @xuzhg @julealgon @mikepizzo

[tsharp]

That is a fair point as well - let's move this to discussions for now.

[tsharp]

Based on the feedback and discussion it's unlikely that this will become a thing - closing the pr.