Issue with Dynamic Sort column In KeySet for Value Type

[Xor-el]

First of all, I would like to thank you for this very useful package that abstracts away the complexity of Keyset Pagination.
So to the issue I seem to have encountered, I have a requirement to use a dynamic sort column in keyset.
in cases where the sort column is a reference type like string everything works Ok, but when the sort column is a value type like int, DateTime, or Guid, it fails with either of the following exceptions depending on the combination used.

System.InvalidOperationException : The binary operator LessThan is not defined for the types 'System.Object' and 'System.Object'.

or

System.InvalidOperationException : The binary operator GreaterThan is not defined for the types 'System.Object' and 'System.Object'.

a simple reproduction can be done by adding the code below to KeysetPaginationTest.cs and running the newly added test methods (Ascending_HasPreviousAsync_Buggy, Descending_HasPreviousAsync_Buggy, Ascending_HasNextAsync_Buggy, Descending_HasNextAsync_Buggy)

[Theory]
[InlineData("Id")]
[InlineData("String")]
[InlineData("Guid")]
[InlineData("IsDone")]
[InlineData("Created")]
[InlineData("CreatedNullable")]
public async Task Ascending_HasPreviousAsync_Buggy(string sortColumn)
{
  var keysetContext = DbContext.MainModels.KeysetPaginate(
    b => b.Ascending(GetSortColumn<MainModel>(sortColumn)));
  var items = await keysetContext.Query
    .Take(20)
    .ToListAsync();
  keysetContext.EnsureCorrectOrder(items);

  var dtos = items.Select(x => new
  {
    x.Id, x.String, x.Guid, x.IsDone, x.Created, x.CreatedNullable
  }).ToList();

  // exception is thrown when this line executes
  await keysetContext.HasPreviousAsync(dtos);
}

[Theory]
[InlineData("Id")]
[InlineData("String")]
[InlineData("Guid")]
[InlineData("IsDone")]
[InlineData("Created")]
[InlineData("CreatedNullable")]
public async Task Descending_HasPreviousAsync_Buggy(string sortColumn)
{
  var keysetContext = DbContext.MainModels.KeysetPaginate(
    b => b.Descending(GetSortColumn<MainModel>(sortColumn)));
  var items = await keysetContext.Query
    .Take(20)
    .ToListAsync();
  keysetContext.EnsureCorrectOrder(items);

  var dtos = items.Select(x => new
  {
    x.Id, x.String, x.Guid, x.IsDone, x.Created, x.CreatedNullable
  }).ToList();

  // exception is thrown when this line executes
  await keysetContext.HasPreviousAsync(dtos);
}

[Theory]
[InlineData("Id")]
[InlineData("String")]
[InlineData("Guid")]
[InlineData("IsDone")]
[InlineData("Created")]
[InlineData("CreatedNullable")]
public async Task Ascending_HasNextAsync_Buggy(string sortColumn)
{
  var keysetContext = DbContext.MainModels.KeysetPaginate(
    b => b.Ascending(GetSortColumn<MainModel>(sortColumn)));
  var items = await keysetContext.Query
    .Take(20)
    .ToListAsync();
  keysetContext.EnsureCorrectOrder(items);

  var dtos = items.Select(x => new
  {
    x.Id, x.String, x.Guid, x.IsDone, x.Created, x.CreatedNullable
  }).ToList();

  // exception is thrown when this line executes
  await keysetContext.HasNextAsync(dtos);
}

[Theory]
[InlineData("Id")]
[InlineData("String")]
[InlineData("Guid")]
[InlineData("IsDone")]
[InlineData("Created")]
[InlineData("CreatedNullable")]
public async Task Descending_HasNextAsync_Buggy(string sortColumn)
{
  var keysetContext = DbContext.MainModels.KeysetPaginate(
    b => b.Descending(GetSortColumn<MainModel>(sortColumn)));
  var items = await keysetContext.Query
    .Take(20)
    .ToListAsync();
  keysetContext.EnsureCorrectOrder(items);

  var dtos = items.Select(x => new
  {
    x.Id, x.String, x.Guid, x.IsDone, x.Created, x.CreatedNullable
  }).ToList();

  // exception is thrown when this line executes
  await keysetContext.HasNextAsync(dtos);
}

private static Expression<Func<TEntity, object>> GetSortColumn<TEntity>(string sortColumn) where TEntity: MainModel
{
  return sortColumn switch
  {
    _ when string.Equals(sortColumn, "Id", StringComparison.OrdinalIgnoreCase) => x => x.Id,
      _ when string.Equals(sortColumn, "String", StringComparison.OrdinalIgnoreCase) => x => x.String,
      _ when string.Equals(sortColumn, "Guid", StringComparison.OrdinalIgnoreCase) => x => x.Guid,
      _ when string.Equals(sortColumn, "IsDone", StringComparison.OrdinalIgnoreCase) => x => x.IsDone,
      _ when string.Equals(sortColumn, "Created", StringComparison.OrdinalIgnoreCase) => x => x.Created,
      _ when string.Equals(sortColumn, "CreatedNullable", StringComparison.OrdinalIgnoreCase) => x => x.CreatedNullable,
      _ =>
      throw new NotImplementedException($ "Unsupported {nameof(sortColumn)} {sortColumn}")
  };
}

from my little investigation, it seems that in order to coerce an expression returning a value type into Func<TEntity,object> the compiler needs to insert a Convert(expr, typeof(object)), a UnaryExpression.
For strings and other reference types, there is no need to box, so a "straight" member expression is returned.

[mrahhal]

Hello. Thanks for providing a repro. So, this is related to how column configuration is dependent on the generic type inferred inside Ascending(...) and Descending(...). The problem in your exmaple is that the inferred column type is always object because that's what you coerce GetSortColumn's result to, which is the wrong inferrance.

I don't have a solution to make your particular method work (might be resolvable if I look into the insides if I can resolve the type in a better more reliable way), but right now I don't recommend calling methods that coerce column types in Ascending and Descending. But there's an easy alternative, we need to allow Ascending to infer the right column type, and so just do the branching on a higher level:

[Theory]
[InlineData("Id")]
[InlineData("String")]
[InlineData("Guid")]
[InlineData("IsDone")]
[InlineData("Created")]
[InlineData("CreatedNullable")]
public async Task Ascending_HasPreviousAsync_Buggy(string sortColumn)
{
	var keysetContext = DbContext.MainModels.KeysetPaginate(
		b => ConfigureAscendingForColumn(b, sortColumn));
	var items = await keysetContext.Query
		.Take(20)
		.ToListAsync();
	keysetContext.EnsureCorrectOrder(items);

	var dtos = items.Select(x => new
	{
		x.Id,
		x.String,
		x.Guid,
		x.IsDone,
		x.Created,
		x.CreatedNullable
	}).ToList();

	// exception is thrown when this line executes
	await keysetContext.HasPreviousAsync(dtos);
}

// Just to illustrate.
private void ConfigureAscendingForColumn(KeysetPaginationBuilder<MainModel> b, string sortColumn)
{
	switch (sortColumn)
	{
		case "Id":
			b.Ascending(x => x.Id);
			break;
		case "String":
			b.Ascending(x => x.String);
			break;
		case "Guid":
			b.Ascending(x => x.Guid);
			break;
		case "IsDone":
			b.Ascending(x => x.IsDone);
			break;
		case "Created":
			b.Ascending(x => x.Created);
			break;
		case "CreatedNullable":
			b.Ascending(x => x.CreatedNullable);
			break;
		default:
			throw new NotImplementedException($"Unsupported {nameof(sortColumn)} {sortColumn}");
	}
}

I do something similar in the samples (but on an even higher level). A bit more code maybe, but works.

If you were to prebuild the keyset definitions (which I recommend), then you'll have to do the branching on a higher level anyway.

[Xor-el]

@mrahhal thanks for the response.
Indeed you are right and your approach indeed works but as you are aware, it is indeed a bit more code especially for my use case.
I could use your suggestion as a workaround for now while we maybe investigate if there is a better way to handle the coercion issue.

meanwhile I discovered something that might be of help but unfortunately I am not well versed in C# expressions, maybe you might have an idea.

so if we modify the constructor of KeysetColumn to this

public KeysetColumn(bool isDescending, Expression<Func<T, TColumn>> expression)
  //: base(isDescending, expression)
  : base(isDescending, ProcessExpression(expression))
  {}

private static Expression<Func<T, TColumn>> ProcessExpression(Expression<Func<T, TColumn>> expression)
{
  var body = expression.Body;
  if (body is UnaryExpression unaryExpression && unaryExpression.NodeType == ExpressionType.Convert)
  {
    // Check if it's a unary expression with conversion
    if (unaryExpression.Operand is MemberExpression operand)
    {
      // at this point, the operand is a MemberExpression that has the true type of TColumn
      // The problem now is how to create a lambda expression 'Expression<Func<T, TColumn>>' for 'operand' without the compiler forcing us to explicitly
      // define an Expression.Convert(operand, typeof(object))
      throw new NotImplementedException();
    }
  }
  // If it doesn't match the expected format, return the original expression
  return expression;
}

Please see the comments in the code above.

[mrahhal]

The problem is that after v1.3.0 I support any kind of expression, so I'm hesitant to have this kind of special casing processing specific expressions, as even if it works this would work in certain cases but not others. Forcing no coersion is better than this arbitrary behavior. In any case, I'm going to investigate having better more reliable type inference at some point, but for now the alternative works just fine without major changes.

[Xor-el]

Thanks for your time @mrahhal very much appreciated.

[mrahhal]

No worries. I'm gonna keep this open for now in case I look into it later.