Efcore: OrderBy and ThenBy problem

[justserega]

I know that this came from EF6, but it is ugly construction. It is very difficult to do chain of method with conditions:

var items = db.Items;
if (condition1)
{
    items = items.OrderBy(x => x.Field1);
}
if (condition2)
{
    items = items.ThenBy(x => x.Field2); 
}

if condition1 is false, then ThenBy throw exception. There are no really need to call OrderBy first and ThenBy next. More convinient would be method AddOrderBy instead of that two. Or in addition for them.

[jnm2]

I see what you're saying. You can do that yourself:

c# static class EnumerableExtensions { public static IEnumerable<T> AppendOrderBy<T, TKey>(this IEnumerable<T> source, Func<T, TKey> sortSelector) { return (source as IOrderedEnumerable<T>)?.ThenBy(sortSelector) ?? source.OrderBy(sortSelector); } }

In general it doesn't make a terrible amount of sense to do an AddOrderBy (_meaning ThenBy_) without explicitly tracking which ordering it's coming after, since the two sorts can't be done independently. (I'm not even sure how you got db.Items.ThenBy to compile.)

This is more of a corefx question.

[justserega]

You can do that yourself

I tried it and this is not work. DbQuery<T> realize both IQuerable<T> and IOrderedQuerable<T>. There are no way to realize was OrderBy called or not.

Magic of OrderBy -> ThenBy work on interfaces that they return and extend (as extensition method). It work only if you call them in one chain

var items = db.Items.OrderBy(x => x.Time).ThenBy(x => x.Name)

and broke in this scenario:

var items = db.Items.AsQueryable();
items = items.OrderBy(x => x.Time);
items = items.ThenBy(x => x.Name); // oops, it is not compiled

[jnm2]

Oops, I figured out what was wrong. I ran this code and it works, calling OrderBy the first time and ThenBy the second time:

```c#
static class QueryableExtensions
{
public static IOrderedQueryable AppendOrderBy(this IQueryable source, Expression> sortSelector)
{
return (source as IOrderedQueryable)?.ThenBy(sortSelector)
?? source.OrderBy(sortSelector);
}
}

Your code:
```c#
var items = db.Items.AsQueryable();
if (condition1)
{
    items = items.AppendOrderBy(x => x.Field1);
}
if (condition2)
{
    items = items.AppendOrderBy(x => x.Field2); 
}

[justserega]

@jnm2 it is not work. source realize IOrderedQueryable<T> nevertheless was OrderBy called or not.

[kierenj]

@justserega - I don't think that's the case: see the source for DbSet<>:

https://github.com/aspnet/EntityFramework/blob/dev/src/Microsoft.EntityFrameworkCore/DbSet%60.cs#L44

Maybe you have some other code which is applying a sort beforehand, or are just mistaken?

[jnm2]

@justserega It works with EF Core and the exact code you posted. I've stepped through the program in the debugger and seen that it works.
What version of Entity Framework are you using?

[justserega]

@jnm2 sorry I give wrong example. Just add Where before AppendOrderBy and you see the problem

var items = db.Items.AsQueryable();
items = items.Where(x => x.Field1 == 1);
if (condition1)
{
    items = items.AppendOrderBy(x => x.Field1);
}
if (condition2)
{
    items = items.AppendOrderBy(x => x.Field2); 
}

[jnm2]

Yuck, you're right! Where does return IOrderedQueryable<> instances.
Apparently EntityQueryable<> inherits from Relinq's QueryableBase<> which implements IOrderedQueryable<>.

@rowanmiller @divega this seems wrong. 1) can EntityQueryable either stop inheriting QueryableBase or can Relinq provide a non-ordered queryable base, and 2) what's the best workaround in the meantime?

[jnm2]

@justserega In the meantime this seems to work as intended:

```c#
static class QueryableExtensions
{
public static IOrderedQueryable AppendOrderBy(this IQueryable source, Expression> sortSelector)
{
var ordered = source as IOrderedQueryable;
if (ordered != null)
{
var lastMethod = (source.Expression as MethodCallExpression)?.Method;

        if (lastMethod?.DeclaringType == typeof(Queryable))
            switch (lastMethod.Name)
            {
                case nameof(Queryable.OrderBy):
                case nameof(Queryable.OrderByDescending):
                case nameof(Queryable.ThenBy):
                case nameof(Queryable.ThenByDescending):
                    return ordered.ThenBy(sortSelector);
            }
    }

    return source.OrderBy(sortSelector);
}

}
```

[justserega]

@jnm2 seems like hack, but interesting. It would be great to have this method in EF API without hacks

[jnm2]

@justserega I agree. I think the initial
as IOrderedQueryable version would be perfect if it worked.

[jnm2]

Oh great -_- I forgot that you'd want to check for ThenBy and ThenByDescending too. Updated.

[divega]

@anpete @MichaelKetting reading the whole thread I tend agree that the fact that queries returned from all operators are IOrderedQueryable<T> is less than ideal. Any thoughts on this?

[MichaelKetting]

@divega Sure :)

Historically speaking, yes, EntityFramework 5.0 and 6.0 also implments IOrderedQueryable in addition to IQueryable on the root query source. Linq2SQL on the other hand does not implement IOrderedQueryable on the root query source (Table), but it does so on the DataQuery type returned by every call to IQueryProvider.CreateQuery() so the interface is introduced right with the first query operator. I think that's why we went with the implementation of IOrderedQueryable on QueryableBase - it doesn't really make a difference when compared with EntityFramework and Linq2SQL and the code is simpler with only one implmentation to watch out for. But that was a long time ago, so things are a tad fuzzy.

Technically, it’s probably possible to update the IQueryProvider.CreateQuery() implementation to return an OrderedQueryable based on the expression but it would be a significant breaking change and make implementing Linq providers a tad more complicated. Might be an option for a future major release, though.

Oh, and just a warning:
Simply chaining OrderBy statements instead of OrderBy + ThenBy will lead to a different semantic.
See http://stackoverflow.com/questions/2908029/linq-to-sql-orderby-thenby which references the explanation for Enumerable.ThenBy (https://msdn.microsoft.com/en-us/library/bb534743.aspx) with “[Chaining OrderBy statements] introduces a new primary ordering that ignores the previously established ordering.”

~Michael

[divega]

@MichaelKetting that is super useful information and great perspective, thanks. Then the version of the extension method that @jnm2 wrote that inspects the previousMethodCallExpression is anyway a practical approach to address this requirement for many of the most used LINQ implementations.

I will close as I think the original question was answered by @jnm2.

[MichaelKetting]

@divega Glad to help!