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

nextLink is always populated if PageSize is set, leading to a never ending loop when fetching data #176

Open
leoerlandsson opened this issue Mar 9, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@leoerlandsson
Copy link

leoerlandsson commented Mar 9, 2023

Source/destination types

N/A

Mapping configuration

N/A

Version: 4.0.0

EF Core 7.0.0

Expected behavior

nextLink is set to null when there are no more records to return.

I understand that there will be a need for a count to be included to be able to determine if there is more data to return or not.

Actual behavior

nextLink is always set when PageSize is present, disregarding if there is more data to return or not, leading to a never ending loop when fetching data. The nextLink includes a $skip=, which is correct, but the fetch goes on forever as nextLink is never set to null.

E.g:

"@odata.nextLink": "https://localhost:44334/api/Customer?$count=true&$skip=45500"

The $skip count will go on forever.

Steps to reproduce

// When calling: 
var result = await query.GetQueryAsync(_mapper, options, QuerySettings);

// The code in the following file always adds nextLink, if PageSize is set, disregarding if there is more data to return or not:
// This results in a never ending loop when fetching data from the API
// [https://github.com/AutoMapper/AutoMapper.Extensions.OData/blob/master/AutoMapper.AspNetCore.OData.EFCore/QueryableExtensions.cs](https://github.com/AutoMapper/AutoMapper.Extensions.OData/blob/master/AutoMapper.AspNetCore.OData.EFCore/QueryableExtensions.cs)

private static void ApplyOptions<TModel>(ODataQueryOptions<TModel> options, QuerySettings querySettings)
        {
            options.AddExpandOptionsResult();
            if (querySettings?.ODataSettings?.PageSize.HasValue == true)
                options.AddNextLinkOptionsResult(querySettings.ODataSettings.PageSize.Value);
        }

Workaround

We implemented a workaround to clear the nextLink if there are no more record to return. This obviously is only possible if we have a TotalCount:

var query = await query.GetQueryAsync(_mapper, options, QuerySettings);

// Workaround for nextLink if we have count
                if (options.Request.ODataFeature().TotalCount.HasValue)
                {
                    var skip = options.Skip != null ? options.Skip.Value : 0;

                    if (skip + QuerySettings.ODataSettings.PageSize >= options.Request.ODataFeature().TotalCount) {
                        options.Request.ODataFeature().NextLink = null;
                    }
                }
@BlaiseD BlaiseD added bug Something isn't working and removed bug Something isn't working labels Mar 9, 2023
@BlaiseD
Copy link
Member

BlaiseD commented Mar 9, 2023

What's the recommended alternative logic (other than the workaround)?

@leoerlandsson
Copy link
Author

Hi,

Thanks for the prompt answer.

I guess checking the TotalCount is the only way to solve this. But that logic could perhaps be incorporated in the ApplyOptions method, so that if there is a TotalCount, nextLink is not always added.

I think the count is done after this in the code today, so the call to
options.AddNextLinkOptionsResult(querySettings.ODataSettings.PageSize.Value);

will then need to be moved after the count in QueryableExtensions.cs:

public static async Task ApplyOptionsAsync<TModel, TData>(this IQueryable query, IMapper mapper, Expression<Func<TModel, bool>> filter, ODataQueryOptions options, QuerySettings querySettings)
{
ApplyOptions(options, querySettings); // NextLink is generated here, so this need to be moved below count.
if (options.Count?.Value == true)
options.AddCountOptionsResult(await query.QueryLongCountAsync(mapper, filter, querySettings?.AsyncSettings?.CancellationToken ?? default));
}

Br,
Leo

@BlaiseD
Copy link
Member

BlaiseD commented Mar 9, 2023

PRs are welcome. I suggest making the fix applicable only if a count is also requested i.e. options.Count?.Value == true - plus a section in the ReadMe would help. Otherwise the LongCount() call would be unexpected I think.

@BlaiseD BlaiseD added the enhancement New feature or request label Mar 9, 2023
@jazzmanro
Copy link

Any news on this? I'm wondering how the EnableQueryAttribute works, as if you use that (of course it breaks other things) I don't see this issue.

@leoerlandsson
Copy link
Author

@jazzmanro No progress but you can use the workaround as shown above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants