Support ternary expression in sorting or better error message

[FlorianArnould]

Hi, I tried something similar to this

queryable.KeysetPaginate(e => e.Ascending(e => e.Which == "A" ? e.A : e.B));

But it doesn't work due to A limitation in the code. I'm trying to find my way in there and maybe will create a pull request with an implementation.

But at least it could be better to have a specific error message here https://github.com/mrahhal/MR.EntityFrameworkCore.KeysetPagination/blob/main/src/MR.EntityFrameworkCore.KeysetPagination/KeysetPaginationBuilder.cs#L36 if you get to the else without a MemberExpression you could get in this state :

var properties = ExpressionHelper.GetNestedPropertiesFromMemberAccess(unwrapped); // -> empty list
_columns.Add(new KeysetColumnNested<T, TProp>(
  properties,
  isDescending));

// Then in the KeysetColumnNested contructor

Properties = properties;
Property = properties[^1]; // Error with an empty list

[mrahhal]

Hi. This is similar to how only a limited set of expressions are supported in EF's linq (although, in this particular case ternary operations are supported in EF). Here, only properties (including nested access) is supported, so the way to handle this is through switching on cases when building the keyset itself (similar to what I do in the samples and the pagination package):

// Might not compile, just to illustrate the point:
queryable.KeysetPaginate(b =>
{
    switch (...)
    {
        case "A": b.Ascending(e => e.A); break;
        case "B": b.Ascending(e => e.B); break;
    }
});

Support for ternary would need a non trivial amount of changes I feel, so I'm not sure it's worth the effort or the complexity. If there's no more convincing example, I think the solution above is the best way to go. I'm willing to look over a PR anyway though, I would take it if it doesn't overcomplicate the keyset building logic.

But at least it could be better to have a specific error message here

That's right, a more helpful message and better error detection would be good here. Would also gladly accept a PR for the time being.

[d00ML0rDz]

@mrahhal correct me if I am wrong, but using the switch method you mentioned above doesn't allow you to change the property you are sorting on per each entity instance?

An example we are facing is with properties that may be null. Take this example for some posts that make up a social media newsfeed:

public class Post
{
    public int Id { get; set; }
    public DateTime Created { get; set; }
    public DateTime? ScheduledRelease { get; set; }
    ...
}

In the above example, users can create Posts that by default have a created date, but they may also have a scheduled time for that post to be published and visible in a newsfeed.

When trying to order someone's chronological newsfeed, we would want to sort the posts by their Created time, unless they have a ScheduledRelease date which we would then use instead. This would translate to:

queryable.KeysetPaginate(p => p.Descending(p => p.ScheduledRelease == null ? p.Created : p.ScheduledRelease));

But as far as I can see, there is no way to go about that using this method?

[mrahhal]

@d00ML0rDz That's right, but this is a completely different problem. Nullable columns are not supported in the keyset. Non-nullable stable columns in the keyset are required to produce consistent results. The way you would deal with the case you showed is through a computed column (at least for now. I'm investigating ways to make this easier in https://github.com/mrahhal/MR.EntityFrameworkCore.KeysetPagination/issues/37).

Check this caveat doc for more information about the null problem and for more about the computed column workaround.

[d00ML0rDz]

If you're able to get what you've suggested in #37 working, that looks like a perfect solution for what we are trying to achieve. I did see the suggestion of computed columns, but we have quite a few places where this would be needed and in a lot of cases the situations are more complex with multiple levels of nested objects that may be null.

Very happy to test any changes you're able to come up with if needed 😊 Thank you!

[mrahhal]

Working on #37, and a few things came to mind.

It's important to note that any solution that doesn't use a computed column might sacrifice performance. This is because you could index a computed column, whereas something like a COALESCE("x"."Created", '0001-01-01 00:00:00') (which is the translated x.Created ?? DateTime.MinValue) will not be properly indexed and so you end up with a table scan instead of a seek. While I was able to, in theory, get x.Created ?? DateTime.MinValue to work, this is especially problematic in this library because this library only exists to be a faster method of pagination. So now I'm doubting the benefit of allowing such dynamic expressions.

At the same time, even if I do end up merging the expression improvements, the original case in this issue, e.Ascending(e => e.Which == "A" ? e.A : e.B) will remain the wrong way to do this. I'll keep this issue open because of the better error reporting suggestion, but as for the original case, my original response stands (lift the ternary condition out of the keyset building and build different keysets for each case).

[d00ML0rDz]

@mrahhal Good to hear you've had some success with this. I was having a little look at our project where we use MR.EntityFrameworkCore.KeysetPagination in, and we have situations where we need to use a ternary expression where I don't think using a computed column is the right thing to do performance wise.

An example is where we use nested objects. Here we have two types of profiles, user profiles and business profiles. Posts can have both a UserProfile and a BusinessProfile associated with them. If we want to sort all our profiles by the amount of posts they have, it would look something like this:

queryable.KeysetPaginate(p => p.Descending(profile => profile.Type == EProfileType.Business ? profile.BusinessProfile.Posts.Count : profile.UserProfile.Posts.Count));

However, we don't really want a computed column getting a profile's post count every time we load a profile as it would only be necessary for this one situation. Maybe you know of a better way to handle this, but the solution you've proposed in #37 looks like it would solve this?

[mrahhal]

@d00ML0rDz I would like it if you can move this comment to #37, would like to track this there instead. We can continue the discussion there.

[d00ML0rDz]

@mrahhal Of course, will do that now

[mrahhal]

I'm closing this for now as the original case is resolved using the solution I described above. Other cases and better error reporting will be handled in #37 as I'm changing the whole expression parsing strategy.