Simplify pattern for Include around collections/references

[rowanmiller]

Currently you can only use ThenInclude after landing on a collection nav but you can't use it after landing on a reference in the previous Include call. This is super hard to describe/document... and that's a good sign it will be confusing.

Historically, trying to use overload resolution to detect collection/reference has never worked. So the proposal is to do the same Reference/Collection named pairs of methods we use elsewhere in the stack:

query.IncludeCollection(blog => blog.Posts).ThenIncludeReference(post => post.Author)...

We would still allow the dotted syntax but it wouldn't be the primary API we push folks too.

When discussing this we should also consider using the Reference/Collection names in the relationship API (rather than One/Many).

Once decided on this we need to add API docs. I removed them for the moment because the current pattern is hard to describe and if we are going to change it I don't want to waste time working out how to word it 😄

[divega]

Historically, trying to use overload resolution to detect collection/reference has never worked

We actually had it working in this case. We just removed the extra overload because it seemed redundant to me.

Reference/Collection named pairs of methods we use elsewhere in the stack

It is not nice in this case it is not really needed.

[divega]

We discussed this and we will try IncludeCollection/IncludeReference (or is it IncludeMany/IncludeOne for consistency?) because that accounts for the corner cases in which the CLR type does not match the conceptual cardinality of the navigation property. We should consider doing #1481 at once since we are touching these methods and it might get harder to do it later.

[divega]

I would like to bring this back to discussion. The more I think about it the more I believe this is going to complicate much more than simplify things.

[divega]

I confirmed that this won't be necessary and that all we need to do is bring back the following "reference" overload of ThenInclude() which was removed in #1491:

public static IIncludableQueryable<TEntity, TProperty> ThenInclude<TEntity, TPreviousProperty, TProperty>(
    this IIncludableQueryable<TEntity, TPreviousProperty> source,
    Expression<Func<TPreviousProperty, TProperty>> navigationPropertyPath)
    where TEntity : class
{

This is in addition to the existing "collection" overload:

public static IIncludableQueryable<TEntity, TProperty> ThenInclude<TEntity, TPreviousProperty, TProperty>(
    this IIncludableQueryable<TEntity, ICollection<TPreviousProperty>> source,
    Expression<Func<TPreviousProperty, TProperty>> navigationPropertyPath)
    where TEntity : class
{

I confirmed that the latter handles different collection types correctly because as part of #1491 we already made IIncludableQueryable<TEntity, TProperty> covariant with respect to TProperty. This means that then the type of the this argument IIncludableQueryable<TEntity, ICollection<TPreviousProperty>> will match any IIncludableQueryable<TEntity, SomeCollection<TPreviousProperty>> for any SomeCollection<T> implements ICollection<T>.

I also played with the corner case in which an entity type implements ICollection<T> and it seem things work pretty well:

1. The compiler won't fail to resolve an overload for this case. Instead it will automatically select the correct overload for the lambda expression passed, i.e. it is possible for the lambda to refer to either properties directly on the entity type or on the collection's element type.
2. If there is an ambiguity, e.g. the same property name could refer to a property directly on type of the entity or a property of the element type of the collection, the compiler will give preference to the "reference" overload.
3. It is possible to explicitly switch to the collection overload by introducing a simple cast in the lambda, which should be handy if in the future we want to enable entity types that are at the same time collections of other types. E.g. given a ProductPresentation entity type that has a Description property and a Product entity that has a Description property and also implements ICollection it is possible to get to ProductPresentation.Description like this:

ctx.Customers
    .Include(c => c.Orders)
    .ThenInclude(o => o.OrderLines)
    .ThenInclude(ol => (ICollection<ProductPresentation>)ol.Product)
    .ThenInclude(pp => pp.Description);

Finally, Intellisense inside ThenInclude() offers the union of all the possible lambda expressions that would make sense for either the reference or the collection overload.

[rowanmiller]

Sweet, yeah the implementing ICollection is a corner case so needing to a bit of casting etc. is fine I think 😄.

This work item was about the more general need to simplify the pattern... so it can now just track the pattern listed above.

We should reconsider this when we get to the EntityEntry API for navigation properties... I think we should keep the Collection/Reference API we had in EF6 there... but we should at least have the discussion.

[rowanmiller]

BTW we should verify that this correctly resolves on non-Roslyn compilers too

[divega]

I verified on both.

[maumar]

Fixed with d271361