WebApi icon indicating copy to clipboard operation
WebApi copied to clipboard

HttpRequest.GetNextPageLink(pageSize) always returns a next link, even if there are no more items to return.

Open KaterynaKateryna opened this issue 3 years ago • 6 comments

We have migrated from classic Asp.Net to Asp.Net Core 3.1, and so we had to change the OData library we used. We went from Microsoft.AspNet.WebApi.OData 5.7.0 to Microsoft.AspNetCore.Odata 7.4.1.

The issue we have now is that HttpRequest.GetNextPageLink(pageSize) always returns a next page link, even if there are no more items to return. For example, if there are 20 items in the set and the page size is 20, then the request returns a next page link.

This behaviour was different for HttpRequestMessage.ODataProperties().NextLink in the old library. If there were no more items to return, then the next page link wasn't generated.

How can the same behaviour be achieved in the Asp.Net Core 3.1?

Assemblies affected

Microsoft.AspNetCore.Odata 7.4.1 (previously we used Microsoft.AspNet.WebApi.OData 5.7.0)

Reproduce steps

This can be reproduced if the number of items in the set is exactly divisible by page size. For instance, page size is 20 and there are 20, 40, 60 etc items in the set.

KaterynaKateryna avatar Nov 09 '21 08:11 KaterynaKateryna

@KaterynaKateryna That you for reporting this issue. Unfortunately I'm unable to reproduce it. Find here my attempt at reproducing it and paging works just fine. I referenced Microsoft.AspNetCore.OData 7.4.1 specifically.

I have an OrdersController that returns 6 orders in total. When I apply a page size of 2, I get the following results:

GET http://localhost:52769/odata/Orders

{
    "@odata.context": "http://localhost:52769/odata/$metadata#Orders",
    "value": [
        {
            "Id": 1,
            "Amount": 80
        },
        {
            "Id": 2,
            "Amount": 10
        }
    ],
    "@odata.nextLink": "http://localhost:52769/odata/Orders?$skip=2"
}

GET http://localhost:52769/odata/Orders?$skip=2

{
    "@odata.context": "http://localhost:52769/odata/$metadata#Orders",
    "value": [
        {
            "Id": 3,
            "Amount": 40
        },
        {
            "Id": 4,
            "Amount": 80
        }
    ],
    "@odata.nextLink": "http://localhost:52769/odata/Orders?$skip=4"
}

GET http://localhost:52769/odata/Orders?$skip=4

{
    "@odata.context": "http://localhost:52769/odata/$metadata#Orders",
    "value": [
        {
            "Id": 5,
            "Amount": 50
        },
        {
            "Id": 6,
            "Amount": 50
        }
    ]
}

As you can see, the last response doesn't contain a @odata.nextLink property. Kindly let me know if I missed something. Alternatively provide a repro that I can look at

gathogojr avatar Nov 10 '21 17:11 gathogojr

Thank you for your reply @gathogojr. I checked your repo and it does work correctly in your code. However, I can't implement it the same way in our solution, because we return a custom response model. That means that we construct it ourselves. Instead of returning items like this:

public class OrdersController : ODataController
{
    [EnableQuery(PageSize = 2)]
    public ActionResult Get()
    {
        return Ok(orders);
    }
}

we return them like this:

public class OrdersController : ControllerBase
{
    public ActionResult Get(ODataQueryOptions<Order> queryOptions)
    {
        // this is an extension method from AutoMapper.Extensions.OData that basically applies OData query options to IQueryable and maps the result
        // it can be found here:
        // https://github.com/AutoMapper/AutoMapper.Extensions.OData/blob/8786d0c9eddb857cbfcfe0d02732979a88f7e9f2/AutoMapper.AspNetCore.OData.EFCore/QueryableExtensions.cs#L38
        IEnumerable<Order> page = await orders.GetAsync(mapper, queryOptions, querySettings);

        return new PagedResponse<Order>(
              page,
              request.HttpContext.ODataFeature().NextLink
          );
    }
}

where PagedResponse<TModel> is our custom response. It has been used historically in our APIs and so we have to keep it to ensure we don't break our contract

public class PagedResponse<TModel>
{
    public IEnumerable<TModel> Items { get; set; }

    public string NextPageLink { get; set; }

    public PagedResponse(IEnumerable<TModel> results, Uri nextPageLink)
    {
        Items = results;
        NextPageLink = nextPageLink?.AbsoluteUri;
    }
}

I tried doing something similar without AutoMapper extension method, and I got the same result: next page link is not empty even if there are no more items to return. Here is your example modified to match our case:

public class OrdersController : ControllerBase
{
    public ActionResult Get(ODataQueryOptions<Order> queryOptions)
    {
        var querySettings = new ODataQuerySettings()
        {
            PageSize = 2,
            EnsureStableOrdering = true
        };

        IQueryable results = queryOptions.ApplyTo(orders.AsQueryable(), querySettings);

        Uri nextLink = Request.GetNextPageLink(2);

        return Ok(new PagedResponse<Order>(results as IEnumerable<Order>, nextLink));
    }
}

As you can see I don't use [EnableQuery] or ODataController, but I call queryOptions.ApplyTo and Request.GetNextPageLink myself. And in this case nextLink is always returned as not empty, even if there are no more cases. I know that our scenario is not the most common one, but it used to work in the previous version of the library this way. Unfortunately, we can't use [EnableQuery] or ODataController because of our custom response serialization.

Let me know if the case is clear and if you need more details. Thanks again!

KaterynaKateryna avatar Nov 11 '21 08:11 KaterynaKateryna

@KaterynaKateryna The additional information and context helps. Let me try and see if I can identify the problem

gathogojr avatar Nov 11 '21 09:11 gathogojr

@KaterynaKateryna The PagedResponse type you're using is not from the OData library so I really don't have any knowledge of the implementation. Below is how you'd you'd accomplish the same using the features supported by the library:

private const int PageSize = 2;
// ...

[EnableQuery(PageSize=PageSize)]
public ActionResult Get(ODataQueryOptions<Order> queryOptions)
{
    Uri nextPageLink = null;

    if (queryOptions.Skip == null
        || (queryOptions.Skip != null && (queryOptions.Skip.Value + PageSize) < orders.Count))
    {
        nextPageLink = Request.GetNextPageLink(PageSize);
    }

    return Ok(new PageResult<Order>(orders.AsEnumerable(), nextPageLink, null));
}

gathogojr avatar Nov 11 '21 18:11 gathogojr

@KaterynaKateryna A lot of things could have changed between 5.x and 7.x. Breaking changes are usually included in major releases. You may need to debug through the implementation of PagedResponse to identify where you might need to tweak for things to work. Or maybe passing a null as nextLink when there are no additional items might do the trick

gathogojr avatar Nov 11 '21 19:11 gathogojr

@gathogojr PagedResponse is just a DTO, I provided its full code in my comment above. I added it to the example just to illustrate that we need a custom response.

The workaround you suggest will work but since the real solution doesn't use an in-memory collection, but an IQueryable going to a SQL database, orders.Count() will mean an extra database call, which we want to avoid.

We have found a different workaround, where we fetch pagesize+1 items, and then if only pagesize items were returned by the query then we don't return nextLink. But that is quite ugly, and we need to make sure we don't return that one extra item to the caller. It looks something like this:

public ActionResult Get(ODataQueryOptions<Order> queryOptions)
{
    int pageSize = 2;
    var querySettings = new ODataQuerySettings()
    {
        PageSize = pageSize + 1, // fetch one extra item
        EnsureStableOrdering = true
    };

    IEnumerable<Order> results = (IEnumerable<Order>)queryOptions
        .ApplyTo(orders.AsQueryable(), querySettings);

    Uri nextLink = null;
    if (results.Count() > pageSize)
    {
        nextLink = Request.GetNextPageLink(pageSize);
    }

    // call results.Take(pageSize) not to return that one extra item
    return Ok(new PagedResponse<Order>(results.Take(pageSize), nextLink));
}

I know that major releases can contain breaking changes, but this behaviour seems like a bug to me, because the previous behaviour made more sense and didn't require workarounds.

KaterynaKateryna avatar Nov 15 '21 08:11 KaterynaKateryna

Hi @KaterynaKateryna. It has been long and I don't know if you're still tracking this issue. I finally got round to thoroughly investigating the issue. I created sample OData services based on the following:

  • Microsoft.AspNet.WebApi.OData 5.7.0 / Data Source: In-memory collection
  • Microsoft.AspNet.WebApi.OData 5.7.0 / Data Source: Entity Framework & SqlServer
  • Microsoft.AspNetCore.OData 7.4.1 / Data Source: In-memory collection
  • Microsoft.AspNetCore.OData 7.4.1 / Data Source: Entity Framework & SqlServer
  • Microsoft.AspNetCore.OData 8.0.3 / Data Source: In-memory collection

In all projects, I used a custom class when returning the paged response - mirroring the one you shared:

public class PagedResponse<TModel> : IEnumerable<TModel>, IEnumerable
{
    public IEnumerable<TModel> Items { get; set; }

    public string NextPageLink { get; set; }

    public PagedResponse(IEnumerable<TModel> results, Uri nextPageLink)
    {
        Items = results;
        NextPageLink = nextPageLink?.AbsoluteUri;
    }

    public IEnumerator<TModel> GetEnumerator()
    {
        return Items.GetEnumerator();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
         return Items.GetEnumerator();
    }
}

Usage:

public class OrdersController : ODataController
{
    // ...

    public ActionResult Get(ODataQueryOptions<Order> queryOptions)
    {
        var querySettings = new ODataQuerySettings { PageSize = 2 };
        var result = queryOptions.ApplyTo(this.db.Orders, querySettings) as IEnumerable<Order>;

        return Ok(new PagedResponse<Order>(result, Request.ODataFeature().NextLink));
    }
}

In all of my scenarios, at no time is the @odata.nextLink annotation rendered on the last page.

Kindly review the sample OData services and let me know if I covered all the bases in my repros. Feel free to clone the repo and run the projects to verify.

gathogojr avatar May 24 '23 12:05 gathogojr

Hi @KaterynaKateryna. Did you get a chance to review my latest update on this issue?

gathogojr avatar May 26 '23 08:05 gathogojr

I'll go ahead and close this issue. Please go ahead and reopen it if you feel it hasn't been adequately addressed.

gathogojr avatar May 30 '23 06:05 gathogojr