AutoMapper / AutoMapper.Extensions.OData

Creates LINQ expressions from ODataQueryOptions and executes the query.
MIT License
140 stars 38 forks source link

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

Open leoerlandsson opened 1 year ago

leoerlandsson commented 1 year ago

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 commented 1 year ago

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

leoerlandsson commented 1 year ago

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 commented 1 year ago

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.

jazzmanro commented 7 months ago

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 commented 7 months ago

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