dncuug / X.PagedList

Library for easily paging through any IEnumerable/IQueryable in ASP.NET
https://nuget.org/packages/X.PagedList
MIT License
886 stars 213 forks source link

ambiguous method call: PagedListExtensions.ToPagedListAsync() #242

Closed Yaevh closed 5 months ago

Yaevh commented 1 year ago

Describe the bug In 8.4.7, when trying to call PagedListExtensions.ToPagedListAsync(this IQueryable superset, int? pageNumber, int pageSize, CancellationToken cancellationToken), the compiler returns the following error: CS0121 The call is ambiguous between the following methods or properties: 'PagedListExtensions.ToPagedListAsync<T>(IEnumerable<T>, int, int, CancellationToken, int?)' and 'PagedListExtensions.ToPagedListAsync<T>(IQueryable<T>, int?, int, CancellationToken, int?)'

To Reproduce Consider the following code snippet:

using Microsoft.EntityFrameworkCore;
using X.PagedList;
using XPagedListAmbiguousMatchError.Data;

var dbContext = new MyDbContext();

var myQuery = dbContext.MyEntities.AsNoTracking()
    .OrderBy(x => x.Id);

var results = await myQuery.ToPagedListAsync(1, 100, CancellationToken.None); // error CS0121: ambiguous call

Additional context PagedListExtensions provide methods for both IEnumerable and IQueryable; however, in this case myQuery is of type IOrderedQueryable, which implements both IQueryable and IEnumerable. It is possible to cast myQuery to either IEnumerable or IQueryable, but it's pretty awkward, especially when ToPagedList() is used multiple times in the codebase.

The error itself was introduced in commit 8278c93c (#225), but it seems to stem from an earlier decision to make nullability of pageNumber parameter inconsistent between various ToPagedList() signatures.

Note that the two ambiguous methods in this case are https://github.com/dncuug/X.PagedList/blob/f67e889bad5271e21db09bb04cd1d6b397eac662/src/X.PagedList/PagedListExtensions.cs#L302 and https://github.com/dncuug/X.PagedList/blob/f67e889bad5271e21db09bb04cd1d6b397eac662/src/X.PagedList/PagedListExtensions.cs#L348 The subtle difference being that pageNumber parameter is optional in the second one. If I modify the signature to make pageNumber non-nullable, the error goes away.

Yaevh commented 1 year ago

The immediate cause seems to boil down to the rules of overload resolution, specifically selecting a better function member.

In my case, the interesting parameters are superset and pageNumber. For the method ToPagedListAsync<T>(this IQueryable<T> superset, int? pageNumber, ...), the parameter superset is of a more-derived type (IQueryable<T> derives from IEnumerable<T>), and thus considered a better match. However, the parameter pageNumber of type int (not int?) seems to be better matched in the method ToPagedListAsync<T>(this IEnumerable<T> superset, int pageNumber, ...). Thus, each method trumps the other one in some respect, so according to the rules of the language, the compiler cannot reliably pick either one.

To test my theory, I tried two solutions:

  1. removing nullability of pageNumber in ToPagedListAsync<T>(this IQueryable<T> superset, int? pageNumber, ...)
  2. adding another overload to PagedListExtensions, namely Task<IPagedList<T>> ToPagedListAsync<T>(this IQueryable<T> superset, int pageNumber, int pageSize, CancellationToken cancellationToken, int? totalSetCount = null) In both cases the error seems to go away.

So, the two possible solutions seem to be:

  1. remove the overloads for IQueryable accepting int? pageNumber and just stick with non-nullable versions
  2. add another set of overloads with non-nullable int pageNumber (and possibly nullable versions of ToPagedListAsync<T>(this IEnumerable<T> superset, int? pageNumber, ...) to match)

The first one seems easier and cleaner, but is a breaking change. The second one is backward-compatible, but leads to more code bloat with six new methods.

I could push a fix with either one of those solutions, but please let me know which one aligns better with your architectural guidelines.

VictorioBerra commented 7 months ago

I just ran into this 😔

VictorioBerra commented 5 months ago

@ernado-x Thanks! When will this be out on NuGet?

ernado-x commented 5 months ago

@VictorioBerra I just made NuGet release!

Also you can find here list of changes https://github.com/dncuug/X.PagedList/releases/tag/v9.1.2