New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Query: Support Include/ThenInclude for navigation on derived type #3910

Closed
davidroth opened this Issue Nov 27, 2015 · 56 comments

Comments

@davidroth
Contributor

davidroth commented Nov 27, 2015

Is there a solution for using ThenInclude with polymorphic types?

Example model:

public abstract class BasePosition : Entity
{
    public Order Order { get; set; }
}

public class GroupType : Entity
{}

public class GroupPosition : BasePosition
{
      public GroupType GroupType { get; set; }
}

public class SalesPosition : BasePosition
{
    public GroupPosition Group { get; set; }
}

public class Order : Entity
{
    private List<BasePosition> positions;

    public ICollection<BasePosition> Positions 
    => positions ?? (positions = new List<BasePosition>());
}

Example query:

context.Set<Order>()
.Include(x => x.Positions)
    .ThenInclude<SalesPosition>(x => x.Group) // Pseudo-Code, does obviously not work
    .ThenInclude<GroupPosition>(x => x.GroupType) // Pseudo-Code, does obviously not work

AFAIK the only way this currently works is by manually querying the derived types after loading the base shape:

var o = context.Set<Order>()
.Where(o => o.Id == 1234)
.Single();

context.Set<GroupPosition>()
    .Where(x => x.Order == o)
    .Include(x => x.GroupType)
    .Load();

context.Set<SalesPosition>()
    .Where(x => x.Order == o)
    .Include(x => x.Group)
    .Load();

@davidroth davidroth changed the title from "ThenInclude" with polymorphic collections to "ThenInclude" with polymorphic types in a collection Nov 27, 2015

@divega

This comment has been minimized.

Show comment
Hide comment
@divega

divega Nov 27, 2015

Member

@davidroth As far as I know EF Core currently doesn't support any pattern you could use to express that you want query results to include navigations that only exist in derived types. Cc @anpete in case I am wrong.

As for how such pattern could look like, here is something we thought about in the past applied to the new ThenInclude method:

context.Set<Order>()
    .Include(x => x.Positions)
    .ThenInclude(x => (x as SalesPosition).Group) 
    .Include(x => x.Positions)
    .ThenInclude(x => (x as GroupPosition).GroupType) 

Of course, there are probably other things that could work and for instance the null-conditional operator would be better than this if it was supported in LINQ expressions.

Member

divega commented Nov 27, 2015

@davidroth As far as I know EF Core currently doesn't support any pattern you could use to express that you want query results to include navigations that only exist in derived types. Cc @anpete in case I am wrong.

As for how such pattern could look like, here is something we thought about in the past applied to the new ThenInclude method:

context.Set<Order>()
    .Include(x => x.Positions)
    .ThenInclude(x => (x as SalesPosition).Group) 
    .Include(x => x.Positions)
    .ThenInclude(x => (x as GroupPosition).GroupType) 

Of course, there are probably other things that could work and for instance the null-conditional operator would be better than this if it was supported in LINQ expressions.

@rowanmiller rowanmiller added this to the Backlog milestone Dec 1, 2015

@rowanmiller

This comment has been minimized.

Show comment
Hide comment
@rowanmiller

rowanmiller Dec 23, 2015

Member

@divega @davidroth I think this same issue applies to Include as well as ThenInclude right? If the type you are querying is a base type then there is no way to Include navigations defined on derived types in the hierarchy.

Member

rowanmiller commented Dec 23, 2015

@divega @davidroth I think this same issue applies to Include as well as ThenInclude right? If the type you are querying is a base type then there is no way to Include navigations defined on derived types in the hierarchy.

@divega

This comment has been minimized.

Show comment
Hide comment
@divega

divega Dec 24, 2015

Member

Yes, it would apply to both. Only this particular example had it in ThenInclude().

Member

divega commented Dec 24, 2015

Yes, it would apply to both. Only this particular example had it in ThenInclude().

@rowanmiller rowanmiller changed the title from "ThenInclude" with polymorphic types in a collection to Query: Support Include/ThenInclude for navigation on derived type Dec 28, 2015

@gregbty

This comment has been minimized.

Show comment
Hide comment
@gregbty

gregbty Jan 7, 2016

Does this also apply to base abstract classes?

    public abstract class Entity
    {
        public DateTime CreatedAt { get; set; }
        public DateTime UpdatedAt { get; set; }
        public Guid? CreatedById { get; set; }
        public Guid? UpdatedById { get; set; }
        public User CreatedBy { get; set; }
        public User UpdatedBy { get; set; }
    }
    public class Company : Entity
    {
        ...
    }
   context.Set<Company>().Include(t => t.CreatedBy);

This results in: ArgumentNullException: Value cannot be null.

gregbty commented Jan 7, 2016

Does this also apply to base abstract classes?

    public abstract class Entity
    {
        public DateTime CreatedAt { get; set; }
        public DateTime UpdatedAt { get; set; }
        public Guid? CreatedById { get; set; }
        public Guid? UpdatedById { get; set; }
        public User CreatedBy { get; set; }
        public User UpdatedBy { get; set; }
    }
    public class Company : Entity
    {
        ...
    }
   context.Set<Company>().Include(t => t.CreatedBy);

This results in: ArgumentNullException: Value cannot be null.

@zedr0n

This comment has been minimized.

Show comment
Hide comment
@zedr0n

zedr0n May 28, 2016

I'm writing an eager loading routine via graph building and expression trees and I'm running into this problem. Although the way I implemented it actually works as long as the actual database value is of the correct derived class

    public static IQueryable<T> ThenIncludeQueryEx<T, TPreviousProperty, TProperty>(IQueryable<T> query, string propertyName)
        where T : InternalObject
    {
        var pe = Expression.Parameter(typeof(TPreviousProperty));
        var me = Expression.Convert(Expression.Property(pe, propertyName),typeof(TProperty));

        var includeQuery = query as IIncludableQueryable<T, TPreviousProperty>;
        query = includeQuery.ThenInclude(Expression.Lambda<Func<TPreviousProperty, TProperty>>(me, pe));

        return query;
    }

but it fails when the cast cannot be performed. Unfortunately, EF doesn't like Expression.TypeAs so I cannot combine it directly with Expression.Conditional to invoke a different path if the cast fails.

So considering this if the explicit type is saved to DB ( which is anyways saved as a Discriminator field ) the initial check could be included in the Where clause? Does that make sense? You would still need to run multiple queries based on each separate derived class possible to eagerly load all possible combinations

zedr0n commented May 28, 2016

I'm writing an eager loading routine via graph building and expression trees and I'm running into this problem. Although the way I implemented it actually works as long as the actual database value is of the correct derived class

    public static IQueryable<T> ThenIncludeQueryEx<T, TPreviousProperty, TProperty>(IQueryable<T> query, string propertyName)
        where T : InternalObject
    {
        var pe = Expression.Parameter(typeof(TPreviousProperty));
        var me = Expression.Convert(Expression.Property(pe, propertyName),typeof(TProperty));

        var includeQuery = query as IIncludableQueryable<T, TPreviousProperty>;
        query = includeQuery.ThenInclude(Expression.Lambda<Func<TPreviousProperty, TProperty>>(me, pe));

        return query;
    }

but it fails when the cast cannot be performed. Unfortunately, EF doesn't like Expression.TypeAs so I cannot combine it directly with Expression.Conditional to invoke a different path if the cast fails.

So considering this if the explicit type is saved to DB ( which is anyways saved as a Discriminator field ) the initial check could be included in the Where clause? Does that make sense? You would still need to run multiple queries based on each separate derived class possible to eagerly load all possible combinations

@HappyNomad

This comment has been minimized.

Show comment
Hide comment
@HappyNomad

HappyNomad Jan 10, 2017

The Include/ThenInclude overloads that accept LINQ Expression trees like those @divega describe would help in some scenarios. What I need is reflection-friendly overloads that I call like this:

.Include( "Positions" )
.ThenInclude( typeof(SalesPosition), "Group" )
.Include( "Positions" )
.ThenInclude( typeof(GroupPosition), "GroupType" )

Currently, there's no ThenInclude that's tacked onto the reflection-friendly Include. But it would be useful here. Either that, or have Include search derived classes for chained properties specified in its string navigationPropertyPath parameter.

Why do I want this? I already defined a [Part] attribute that I use in OnModelCreating to enable cascade delete. Now I want to use this attribute to create an IncludeParts method.

I don't think the current workaround will work well for me since, after #1833 is implemented, I'll be filtering the upstream results before including the derived properties downstream.

The Include/ThenInclude overloads that accept LINQ Expression trees like those @divega describe would help in some scenarios. What I need is reflection-friendly overloads that I call like this:

.Include( "Positions" )
.ThenInclude( typeof(SalesPosition), "Group" )
.Include( "Positions" )
.ThenInclude( typeof(GroupPosition), "GroupType" )

Currently, there's no ThenInclude that's tacked onto the reflection-friendly Include. But it would be useful here. Either that, or have Include search derived classes for chained properties specified in its string navigationPropertyPath parameter.

Why do I want this? I already defined a [Part] attribute that I use in OnModelCreating to enable cascade delete. Now I want to use this attribute to create an IncludeParts method.

I don't think the current workaround will work well for me since, after #1833 is implemented, I'll be filtering the upstream results before including the derived properties downstream.

@weitzhandler

This comment has been minimized.

Show comment
Hide comment
@weitzhandler

weitzhandler May 22, 2017

Contributor

@divega

context.Set<Order>()
  .Include(x => x.Positions)
    .ThenInclude(x => (x as SalesPosition).Group) 
  .Include(x => x.Positions)
    .ThenInclude(x => (x as GroupPosition).GroupType)

This is feature is very compelling to me, although it may also be achieved in a shorter way, something like:

    context.Set<Order>()
      .Include(x => x.Positions)
        .ThenInclude<SalesPosition>(sp => sp.Group) 
      .Include(x => x.Positions)
        .ThenInclude<GroupPosition>(gp => gp.GroupType)
          .ThenInclude(gt => gt.TypeName)
      .Include<BusinessOrder>(bo => bo.Customer)

Having the ThenInclude's (and also the Include's - see last line, this is important) generic type argument used as an OfType filter, branching the local IIncludableQueryable to the desired type.

Contributor

weitzhandler commented May 22, 2017

@divega

context.Set<Order>()
  .Include(x => x.Positions)
    .ThenInclude(x => (x as SalesPosition).Group) 
  .Include(x => x.Positions)
    .ThenInclude(x => (x as GroupPosition).GroupType)

This is feature is very compelling to me, although it may also be achieved in a shorter way, something like:

    context.Set<Order>()
      .Include(x => x.Positions)
        .ThenInclude<SalesPosition>(sp => sp.Group) 
      .Include(x => x.Positions)
        .ThenInclude<GroupPosition>(gp => gp.GroupType)
          .ThenInclude(gt => gt.TypeName)
      .Include<BusinessOrder>(bo => bo.Customer)

Having the ThenInclude's (and also the Include's - see last line, this is important) generic type argument used as an OfType filter, branching the local IIncludableQueryable to the desired type.

@maumar

This comment has been minimized.

Show comment
Hide comment
@maumar

maumar Jun 8, 2017

Contributor

@divega @anpete @smitpatel this issue popped up in a conversation today

Contributor

maumar commented Jun 8, 2017

@divega @anpete @smitpatel this issue popped up in a conversation today

@maumar maumar self-assigned this Jun 8, 2017

@maumar maumar modified the milestones: 2.0.0, Backlog Jun 8, 2017

@weitzhandler

This comment has been minimized.

Show comment
Hide comment
@weitzhandler

weitzhandler Jun 9, 2017

Contributor

And also Include(property).Where(predicate).ThenInclude(property).Where(predicate)

@HappyNomad ditto. Constructing lambda expressions could be painful. There should be a reflection friendly version too, with and without a type filter (supporting inheritance).

Contributor

weitzhandler commented Jun 9, 2017

And also Include(property).Where(predicate).ThenInclude(property).Where(predicate)

@HappyNomad ditto. Constructing lambda expressions could be painful. There should be a reflection friendly version too, with and without a type filter (supporting inheritance).

@smitpatel

This comment has been minimized.

Show comment
Hide comment
@smitpatel

smitpatel Jun 9, 2017

Contributor

@maumar - What are we planning to do here for now?

Contributor

smitpatel commented Jun 9, 2017

@maumar - What are we planning to do here for now?

@anpete

This comment has been minimized.

Show comment
Hide comment
@anpete

anpete Jun 9, 2017

We are investigating supporting this via "as" and string based include for Preview2.

anpete commented Jun 9, 2017

We are investigating supporting this via "as" and string based include for Preview2.

@weitzhandler

This comment has been minimized.

Show comment
Hide comment
@weitzhandler

weitzhandler Jun 9, 2017

Contributor

@anpete I'd just like to make sure you saw this, isn't it nicer than as?

Contributor

weitzhandler commented Jun 9, 2017

@anpete I'd just like to make sure you saw this, isn't it nicer than as?

@smitpatel

This comment has been minimized.

Show comment
Hide comment
@smitpatel

smitpatel Jun 9, 2017

Contributor

We have discussed this in brief as a team. Though we haven't finalized exact sketch what it would be since we are bit short on time.

Here are some of the thoughts/experiments I did around the topic.
(I am writing for Include though it is equally expanded for ThenInclude)

String based Include

IQueryable<Base>().Include("NavigationOnBase") //We interpret as navigation defined on base
IQueryable<Base>().Include(typeof(Derived), "NavigationOnDerived") // We interpret as navigation on the type arg.

This API looks clean. We have similar pattern for string based ModelBuilding support (HasForeignKey with one-to-one has such syntax). It will be an overload so there won't be confusion about inferring and I believe it would be visible enough. Also the first one can be re-written into second easily.
This is same as what @HappyNomad suggested though I did not understand what is "reflection friendly". As for PartAttribute we don't have plan to add it unless there is general purpose customer scenario.

Expression based Include

// Options 1
IQueryable<Base>().Include(e => e.NavigationOnBase)
IQueryable<Base>().Include(e => (e as Derived).NavigationOnDerived)

//Option 2
IQueryable<Base>().Include<Base, Base, ReferencedType>(e => e.NavigationOnBase)
IQueryable<Base>().Include<Base, Derived, DerivedReferencedType>(e => e.NavigationOnDerived)

We are looking to support option 1 here (Explained option 2 below why its bit cumbersome for user experience)
The current version of Include already takes expression as parameter. The "as derived" part is just to give intellisense to user. We can start parsing such pattern of expression and generate include accordingly.

Option 2
There are 3 different Typed args to the function here. The incoming IQueryable type, the type used for expression lambda & the return type. Presently there are only 2 type args and they are inferred by compiler but if you want to add another type arg to change type of lambda then everything need to be typed explicitly.
@weitzhandler - Your approach is kind of reaching there by giving typed arg to generic function but the other 2 also needs to be specified explicitly because compiler doesn't do "infer the rest"
This makes option 1 a better choice, (less thing for user to figure out especially providing return type is very awkward).

I am still experimenting with different syntax for expression based include. Our goal is to provide good intellisense to user. We can always add extra overload if we find a good syntax and support the "as" syntax (which is somewhat similar to processing filtered include)

Given the time constraint we have, we are doing the "as" syntax and string based as above.

Include(property).Where(predicate).ThenInclude(property).Where(predicate)

In this syntax, all predicates will be applied to the first IQueryable only and not on the include. If you mean to apply the predicate on the included entity then its filtered include #1833
The issue for not supporting above syntax is
When user writes
context.Blogs.Include(b => b.Posts).Where(a => a.BlogId == 1).ToList()
It gives different meanings, if user wants to just Blog with Id 1 and all its post loaded
or All blogs but load only posts with BlogId=1.
Also for intellisense, what should be the type of "a" Blog or Post. It puts stricter requirement on user to specify include at certain place only (have to put last in most cases sort of). Hence its better not to involve such confusion.

Contributor

smitpatel commented Jun 9, 2017

We have discussed this in brief as a team. Though we haven't finalized exact sketch what it would be since we are bit short on time.

Here are some of the thoughts/experiments I did around the topic.
(I am writing for Include though it is equally expanded for ThenInclude)

String based Include

IQueryable<Base>().Include("NavigationOnBase") //We interpret as navigation defined on base
IQueryable<Base>().Include(typeof(Derived), "NavigationOnDerived") // We interpret as navigation on the type arg.

This API looks clean. We have similar pattern for string based ModelBuilding support (HasForeignKey with one-to-one has such syntax). It will be an overload so there won't be confusion about inferring and I believe it would be visible enough. Also the first one can be re-written into second easily.
This is same as what @HappyNomad suggested though I did not understand what is "reflection friendly". As for PartAttribute we don't have plan to add it unless there is general purpose customer scenario.

Expression based Include

// Options 1
IQueryable<Base>().Include(e => e.NavigationOnBase)
IQueryable<Base>().Include(e => (e as Derived).NavigationOnDerived)

//Option 2
IQueryable<Base>().Include<Base, Base, ReferencedType>(e => e.NavigationOnBase)
IQueryable<Base>().Include<Base, Derived, DerivedReferencedType>(e => e.NavigationOnDerived)

We are looking to support option 1 here (Explained option 2 below why its bit cumbersome for user experience)
The current version of Include already takes expression as parameter. The "as derived" part is just to give intellisense to user. We can start parsing such pattern of expression and generate include accordingly.

Option 2
There are 3 different Typed args to the function here. The incoming IQueryable type, the type used for expression lambda & the return type. Presently there are only 2 type args and they are inferred by compiler but if you want to add another type arg to change type of lambda then everything need to be typed explicitly.
@weitzhandler - Your approach is kind of reaching there by giving typed arg to generic function but the other 2 also needs to be specified explicitly because compiler doesn't do "infer the rest"
This makes option 1 a better choice, (less thing for user to figure out especially providing return type is very awkward).

I am still experimenting with different syntax for expression based include. Our goal is to provide good intellisense to user. We can always add extra overload if we find a good syntax and support the "as" syntax (which is somewhat similar to processing filtered include)

Given the time constraint we have, we are doing the "as" syntax and string based as above.

Include(property).Where(predicate).ThenInclude(property).Where(predicate)

In this syntax, all predicates will be applied to the first IQueryable only and not on the include. If you mean to apply the predicate on the included entity then its filtered include #1833
The issue for not supporting above syntax is
When user writes
context.Blogs.Include(b => b.Posts).Where(a => a.BlogId == 1).ToList()
It gives different meanings, if user wants to just Blog with Id 1 and all its post loaded
or All blogs but load only posts with BlogId=1.
Also for intellisense, what should be the type of "a" Blog or Post. It puts stricter requirement on user to specify include at certain place only (have to put last in most cases sort of). Hence its better not to involve such confusion.

@weitzhandler

This comment has been minimized.

Show comment
Hide comment
@weitzhandler

weitzhandler Jun 9, 2017

Contributor

@smitpatel

suggested though I did not understand what is "reflection friendly".

I believe he was referring to a convenient way of dynamically providing the includes, i.e. non LINQ-expressions. The one you mentioned earlier.

our approach is kind of reaching there by giving typed arg to generic function but the other 2 also needs to be specified explicitly because compiler doesn't do "infer the rest"
This makes option 1 a better choice, (less thing for user to figure out especially providing return type is very awkward).

You're right.
And BTW: Related.

If you mean to apply the predicate on the included entity then its filtered include #1833

Yep, that was it. Was just randomly pseudoing.

When user writes
context.Blogs.Include(b => b.Posts).Where(a => a.BlogId == 1).ToList()
It gives different meanings, if user wants to just Blog with Id 1 and all its post loaded
or All blogs but load only posts with BlogId=1.

That's should be

context.Blogs.Include(b => b.Posts.Where(a => a.BlogId == 1)).ToList()

Notice the Where is inside the Include scope. Again, was pseudoing before but you understood me well anyway.

Thanks!

Contributor

weitzhandler commented Jun 9, 2017

@smitpatel

suggested though I did not understand what is "reflection friendly".

I believe he was referring to a convenient way of dynamically providing the includes, i.e. non LINQ-expressions. The one you mentioned earlier.

our approach is kind of reaching there by giving typed arg to generic function but the other 2 also needs to be specified explicitly because compiler doesn't do "infer the rest"
This makes option 1 a better choice, (less thing for user to figure out especially providing return type is very awkward).

You're right.
And BTW: Related.

If you mean to apply the predicate on the included entity then its filtered include #1833

Yep, that was it. Was just randomly pseudoing.

When user writes
context.Blogs.Include(b => b.Posts).Where(a => a.BlogId == 1).ToList()
It gives different meanings, if user wants to just Blog with Id 1 and all its post loaded
or All blogs but load only posts with BlogId=1.

That's should be

context.Blogs.Include(b => b.Posts.Where(a => a.BlogId == 1)).ToList()

Notice the Where is inside the Include scope. Again, was pseudoing before but you understood me well anyway.

Thanks!

maumar added a commit that referenced this issue Jun 9, 2017

Fix to #3910 - Query: Support Include/ThenInclude for navigation on d…
…erived type

It was not possible to specify include on a derived type. Include was very strict when computing navigation paths from Include method and generated code was accounting for type discrepancies between caller and navigation.

Fix is to allow specifying include on derived types using string overload:
entities.Include("DerivedProperty")

or using lambda with "as" operator:
entities.Include(e => (e as Derived).DerivedProperty)

Made tweaks to the logic that computes navigation paths - now it looks for navigation on derived types also.
Added additional checks to the generated code (_Include method fixup)

old:

    fixup: (QueryContext queryContext | SomeEntity entity | Object[] included) =>
    {
        return !(bool ReferenceEquals(included[0], null))
        {
            return entity.Navigation = (OtherEntity)included[0]
        }
         : default(OtherEntity)
    }

new:

    fixup: (QueryContext queryContext | SomeEntity entity | Object[] included) =>
    {
        return !(bool ReferenceEquals(included[0], null)) && (entity is DerivedEntity) ?
        {
            return ((DerivedEntity)entity).DerivedNavigation = (OtherEntity)included[0]
        }
         : default(OtherEntity)
    }

This also allows to reuse include pipeline to project collection navigations on derived types (before it was producing N+1 queries)

maumar added a commit that referenced this issue Jun 9, 2017

Fix to #3910 - Query: Support Include/ThenInclude for navigation on d…
…erived type

It was not possible to specify include on a derived type. Include was very strict when computing navigation paths from Include method and generated code wasn't accounting for type discrepancies between caller and navigation.

Fix is to allow specifying include on derived types using string overload:
entities.Include("DerivedProperty")

or using lambda with "as" operator:
entities.Include(e => (e as Derived).DerivedProperty)

Made tweaks to the logic that computes navigation paths - now it looks for navigation on derived types also.
Added additional checks to the generated code (_Include method fixup)

old:

    fixup: (QueryContext queryContext | SomeEntity entity | Object[] included) =>
    {
        return !(bool ReferenceEquals(included[0], null))
        {
            return entity.Navigation = (OtherEntity)included[0]
        }
         : default(OtherEntity)
    }

new:

    fixup: (QueryContext queryContext | SomeEntity entity | Object[] included) =>
    {
        return !(bool ReferenceEquals(included[0], null)) && (entity is DerivedEntity) ?
        {
            return ((DerivedEntity)entity).DerivedNavigation = (OtherEntity)included[0]
        }
         : default(OtherEntity)
    }

This also allows to reuse include pipeline to project collection navigations on derived types (before it was producing N+1 queries)
@divega

This comment has been minimized.

Show comment
Hide comment
@divega

divega Jun 9, 2017

Member

@smitpatel @anpete for option 2, did you try writing the lambda like this?

queryOfBase.Include((Derived e) => e.NavigationOnDerived);
Member

divega commented Jun 9, 2017

@smitpatel @anpete for option 2, did you try writing the lambda like this?

queryOfBase.Include((Derived e) => e.NavigationOnDerived);
@smitpatel

This comment has been minimized.

Show comment
Hide comment
@smitpatel

smitpatel Jun 9, 2017

Contributor

@divega - It indeed passes compilation and also provides intellisense. I am investigating into chaining (ThenInclude) in generic complex. 🤞

Contributor

smitpatel commented Jun 9, 2017

@divega - It indeed passes compilation and also provides intellisense. I am investigating into chaining (ThenInclude) in generic complex. 🤞

@smitpatel

This comment has been minimized.

Show comment
Hide comment
@smitpatel

smitpatel Jun 9, 2017

Contributor

Following compiles correctly providing good intellisense (For reference navigation cases, collection navigations & ThenInclude doesn't go well). It contains a good mix of current form of Includes (which applies to base only) as well as new form.

var includeOnDerivedTypes = new List<BasePosition>().AsQueryable()
    .Include(bp => bp.Order) //Include On base
    .Include((GroupPosition gp) => gp.GroupType) //Include on derived
    .Include((SalesPosition sp) => sp.Group); //Include on derived

var thenIncludeOnDerivedTypes = new List<Order>().AsQueryable()
    .Include(o => o.Positions)
        .ThenInclude(p => p.Order) //ThenInclude on base
    .Include(o => o.Positions)
        .ThenInclude((GroupPosition gp) => gp.GroupType) //ThenInclude on derived
    .Include(o => o.Positions)
        .ThenInclude((SalesPosition sp) => sp.Group) //ThenInclude on derived
            .ThenInclude(gp => gp.GroupType);
Contributor

smitpatel commented Jun 9, 2017

Following compiles correctly providing good intellisense (For reference navigation cases, collection navigations & ThenInclude doesn't go well). It contains a good mix of current form of Includes (which applies to base only) as well as new form.

var includeOnDerivedTypes = new List<BasePosition>().AsQueryable()
    .Include(bp => bp.Order) //Include On base
    .Include((GroupPosition gp) => gp.GroupType) //Include on derived
    .Include((SalesPosition sp) => sp.Group); //Include on derived

var thenIncludeOnDerivedTypes = new List<Order>().AsQueryable()
    .Include(o => o.Positions)
        .ThenInclude(p => p.Order) //ThenInclude on base
    .Include(o => o.Positions)
        .ThenInclude((GroupPosition gp) => gp.GroupType) //ThenInclude on derived
    .Include(o => o.Positions)
        .ThenInclude((SalesPosition sp) => sp.Group) //ThenInclude on derived
            .ThenInclude(gp => gp.GroupType);
@ajcvickers

This comment has been minimized.

Show comment
Hide comment
@ajcvickers

ajcvickers Jun 9, 2017

Member

@smitpatel @divega @anpete @maumar I haven't read all of the discussion from last night. How confident are we that we are getting the API right? Are there open questions still?

Member

ajcvickers commented Jun 9, 2017

@smitpatel @divega @anpete @maumar I haven't read all of the discussion from last night. How confident are we that we are getting the API right? Are there open questions still?

@anpete

This comment has been minimized.

Show comment
Hide comment
@anpete

anpete Jun 9, 2017

@ajcvickers The latest proposal looks very nice. If other agree then I think we should go for it - Given the "loose" nature of LINQ expressions here, we are free to support alternative patterns in the future if we choose.

anpete commented Jun 9, 2017

@ajcvickers The latest proposal looks very nice. If other agree then I think we should go for it - Given the "loose" nature of LINQ expressions here, we are free to support alternative patterns in the future if we choose.

@ajcvickers

This comment has been minimized.

Show comment
Hide comment
@ajcvickers

ajcvickers Jun 9, 2017

Member

@anpete I'll let @divega make the call on the API. If he's okay, then let's try to get this in.

Member

ajcvickers commented Jun 9, 2017

@anpete I'll let @divega make the call on the API. If he's okay, then let's try to get this in.

@maumar

This comment has been minimized.

Show comment
Hide comment
@maumar

maumar Jun 9, 2017

Contributor

One drawback of the new API is that you can only include one reference per Include method. With soft casting you can do entities.Include(e => ((e as Foo).Reference1) as Bar).Reference2). I don't know if we care, since this looks quite ugly anyway

Contributor

maumar commented Jun 9, 2017

One drawback of the new API is that you can only include one reference per Include method. With soft casting you can do entities.Include(e => ((e as Foo).Reference1) as Bar).Reference2). I don't know if we care, since this looks quite ugly anyway

@anpete

This comment has been minimized.

Show comment
Hide comment
@anpete

anpete Jun 9, 2017

@maumar We think this is OK provided one can achieve the desire effect using ThenInclude - Agree the nested 'as' syntax is not nice.

anpete commented Jun 9, 2017

@maumar We think this is OK provided one can achieve the desire effect using ThenInclude - Agree the nested 'as' syntax is not nice.

@divega

This comment has been minimized.

Show comment
Hide comment
@divega

divega Jun 9, 2017

Member

@ajcvickers @anpete @maumar @smitpatel Yes, I also like this ((GroupPosition gp) => gp.GroupType pattern.

Member

divega commented Jun 9, 2017

@ajcvickers @anpete @maumar @smitpatel Yes, I also like this ((GroupPosition gp) => gp.GroupType pattern.

maumar added a commit that referenced this issue Jun 9, 2017

Fix to #3910 - Query: Support Include/ThenInclude for navigation on d…
…erived type

It was not possible to specify include on a derived type. Include was very strict when computing navigation paths from Include method and generated code wasn't accounting for type discrepancies between caller and navigation.

Fix is to allow specifying include on derived types using string overload:
entities.Include("DerivedProperty")

or using lambda and specifying parameter type explicitly
entities.Include((Derived e) => e.DerivedProperty)

Made tweaks to the logic that computes navigation paths - now it looks for navigation on derived types also.
Added additional checks to the generated code (_Include method fixup)

old:

    fixup: (QueryContext queryContext | SomeEntity entity | Object[] included) =>
    {
        return !(bool ReferenceEquals(included[0], null))
        {
            return entity.Navigation = (OtherEntity)included[0]
        }
         : default(OtherEntity)
    }

new:

    fixup: (QueryContext queryContext | SomeEntity entity | Object[] included) =>
    {
        return !(bool ReferenceEquals(included[0], null)) && (entity is DerivedEntity) ?
        {
            return ((DerivedEntity)entity).DerivedNavigation = (OtherEntity)included[0]
        }
         : default(OtherEntity)
    }

This also allows to reuse include pipeline to project collection navigations on derived types (before it was producing N+1 queries)

@maumar maumar modified the milestones: Backlog, 2.0.0 Jun 12, 2017

maumar added a commit that referenced this issue Aug 16, 2017

Fix to #3910 - Query: Support Include/ThenInclude for navigation on d…
…erived type

It was not possible to specify include on a derived type. Include was very strict when computing navigation paths from Include method and generated code wasn't accounting for type discrepancies between caller and navigation.

Fix is to allow specifying include on derived types using string overload:
entities.Include("DerivedProperty")

or using lambda and specifying parameter type explicitly
entities.Include((Derived e) => e.DerivedProperty)

Made tweaks to the logic that computes navigation paths - now it looks for navigation on derived types also.
Added additional checks to the generated code (_Include method fixup)

old:

    fixup: (QueryContext queryContext | SomeEntity entity | Object[] included) =>
    {
        return !(bool ReferenceEquals(included[0], null))
        {
            return entity.Navigation = (OtherEntity)included[0]
        }
         : default(OtherEntity)
    }

new:

    fixup: (QueryContext queryContext | SomeEntity entity | Object[] included) =>
    {
        return !(bool ReferenceEquals(included[0], null)) && (entity is DerivedEntity) ?
        {
            return ((DerivedEntity)entity).DerivedNavigation = (OtherEntity)included[0]
        }
         : default(OtherEntity)
    }

This also allows to reuse include pipeline to project collection navigations on derived types (before it was producing N+1 queries)

For string based include, since there is no way to provide the type that the navigation is declared on, we aggressively include properties - if multiple derived types declare a property with the same name, they will all be included.

maumar added a commit that referenced this issue Aug 17, 2017

Fix to #3910 - Query: Support Include/ThenInclude for navigation on d…
…erived type

It was not possible to specify include on a derived type. Include was very strict when computing navigation paths from Include method and generated code wasn't accounting for type discrepancies between caller and navigation.

Fix is to allow specifying include on derived types using string overload:
entities.Include("DerivedProperty")

or using lambda and specifying parameter type explicitly
entities.Include((Derived e) => e.DerivedProperty)

Made tweaks to the logic that computes navigation paths - now it looks for navigation on derived types also.
Added additional checks to the generated code (_Include method fixup)

old:

    fixup: (QueryContext queryContext | SomeEntity entity | Object[] included) =>
    {
        return !(bool ReferenceEquals(included[0], null))
        {
            return entity.Navigation = (OtherEntity)included[0]
        }
         : default(OtherEntity)
    }

new:

    fixup: (QueryContext queryContext | SomeEntity entity | Object[] included) =>
    {
        return !(bool ReferenceEquals(included[0], null)) && (entity is DerivedEntity) ?
        {
            return ((DerivedEntity)entity).DerivedNavigation = (OtherEntity)included[0]
        }
         : default(OtherEntity)
    }

This also allows to reuse include pipeline to project collection navigations on derived types (before it was producing N+1 queries)

For string based include, since there is no way to provide the type that the navigation is declared on, we aggressively include properties - if multiple derived types declare a property with the same name, they will all be included.

maumar added a commit that referenced this issue Aug 17, 2017

Fix to #3910 - Query: Support Include/ThenInclude for navigation on d…
…erived type

It was not possible to specify include on a derived type. Include was very strict when computing navigation paths from Include method and generated code wasn't accounting for type discrepancies between caller and navigation.

Fix is to allow specifying include on derived types using string overload:
entities.Include("DerivedProperty")

or using lambda and specifying parameter type explicitly
entities.Include((Derived e) => e.DerivedProperty)

Made tweaks to the logic that computes navigation paths - now it looks for navigation on derived types also.
Added additional checks to the generated code (_Include method fixup)

old:

    fixup: (QueryContext queryContext | SomeEntity entity | Object[] included) =>
    {
        return !(bool ReferenceEquals(included[0], null))
        {
            return entity.Navigation = (OtherEntity)included[0]
        }
         : default(OtherEntity)
    }

new:

    fixup: (QueryContext queryContext | SomeEntity entity | Object[] included) =>
    {
        return !(bool ReferenceEquals(included[0], null)) && (entity is DerivedEntity) ?
        {
            return ((DerivedEntity)entity).DerivedNavigation = (OtherEntity)included[0]
        }
         : default(OtherEntity)
    }

This also allows to reuse include pipeline to project collection navigations on derived types (before it was producing N+1 queries)

For string based include, since there is no way to provide the type that the navigation is declared on, we aggressively include properties - if multiple derived types declare a property with the same name, they will all be included.

maumar added a commit that referenced this issue Aug 17, 2017

Fix to #3910 - Query: Support Include/ThenInclude for navigation on d…
…erived type

It was not possible to specify include on a derived type. Include was very strict when computing navigation paths from Include method and generated code wasn't accounting for type discrepancies between caller and navigation.

Fix is to allow specifying include on derived types using string overload:
entities.Include("DerivedProperty")

or using lambda and specifying parameter type explicitly
entities.Include((Derived e) => e.DerivedProperty)

Made tweaks to the logic that computes navigation paths - now it looks for navigation on derived types also.
Added additional checks to the generated code (_Include method fixup)

old:

    fixup: (QueryContext queryContext | SomeEntity entity | Object[] included) =>
    {
        return !(bool ReferenceEquals(included[0], null))
        {
            return entity.Navigation = (OtherEntity)included[0]
        }
         : default(OtherEntity)
    }

new:

    fixup: (QueryContext queryContext | SomeEntity entity | Object[] included) =>
    {
        return !(bool ReferenceEquals(included[0], null)) && (entity is DerivedEntity) ?
        {
            return ((DerivedEntity)entity).DerivedNavigation = (OtherEntity)included[0]
        }
         : default(OtherEntity)
    }

This also allows to reuse include pipeline to project collection navigations on derived types (before it was producing N+1 queries)

For string based include, since there is no way to provide the type that the navigation is declared on, we aggressively include properties - if multiple derived types declare a property with the same name, they will all be included.

maumar added a commit that referenced this issue Aug 17, 2017

Fix to #3910 - Query: Support Include/ThenInclude for navigation on d…
…erived type

It was not possible to specify include on a derived type. Include was very strict when computing navigation paths from Include method and generated code wasn't accounting for type discrepancies between caller and navigation.

Fix is to allow specifying include on derived types using string overload:
entities.Include("DerivedProperty")

or using lambda and specifying parameter type explicitly
entities.Include((Derived e) => e.DerivedProperty)

Made tweaks to the logic that computes navigation paths - now it looks for navigation on derived types also.
Added additional checks to the generated code (_Include method fixup)

old:

    fixup: (QueryContext queryContext | SomeEntity entity | Object[] included) =>
    {
        return !(bool ReferenceEquals(included[0], null))
        {
            return entity.Navigation = (OtherEntity)included[0]
        }
         : default(OtherEntity)
    }

new:

    fixup: (QueryContext queryContext | SomeEntity entity | Object[] included) =>
    {
        return !(bool ReferenceEquals(included[0], null)) && (entity is DerivedEntity) ?
        {
            return ((DerivedEntity)entity).DerivedNavigation = (OtherEntity)included[0]
        }
         : default(OtherEntity)
    }

This also allows to reuse include pipeline to project collection navigations on derived types (before it was producing N+1 queries)

For string based include, since there is no way to provide the type that the navigation is declared on, we aggressively include properties - if multiple derived types declare a property with the same name, they will all be included.
@smitpatel

This comment has been minimized.

Show comment
Hide comment
@smitpatel

smitpatel Aug 18, 2017

Contributor

@maumar - is this fixed?

Contributor

smitpatel commented Aug 18, 2017

@maumar - is this fixed?

@maumar

This comment has been minimized.

Show comment
Hide comment
@maumar

maumar Aug 19, 2017

Contributor

fixed in 07afd7a

Contributor

maumar commented Aug 19, 2017

fixed in 07afd7a

@eL-Prova

This comment has been minimized.

Show comment
Hide comment
@eL-Prova

eL-Prova Oct 26, 2017

Is this also available in Entity Framework 6.1? Or easy to implement/extend?

Is this also available in Entity Framework 6.1? Or easy to implement/extend?

@ajcvickers

This comment has been minimized.

Show comment
Hide comment
@ajcvickers

ajcvickers Oct 26, 2017

Member

@eL-Prova Unfortunately, no and no. EF6 is an entirely different code base than EF Core. This could be implemented in EF6, and we would likely take a PR if it was a well-written and robust implementation, but I don't think it's super easy to implement.

Member

ajcvickers commented Oct 26, 2017

@eL-Prova Unfortunately, no and no. EF6 is an entirely different code base than EF Core. This could be implemented in EF6, and we would likely take a PR if it was a well-written and robust implementation, but I don't think it's super easy to implement.

@eL-Prova

This comment has been minimized.

Show comment
Hide comment
@eL-Prova

eL-Prova Oct 27, 2017

@ajcvickers thanks for your reply. I will take a look to create an extension :-)

@ajcvickers thanks for your reply. I will take a look to create an extension :-)

@Tarig0

This comment has been minimized.

Show comment
Hide comment
@Tarig0

Tarig0 Nov 7, 2017

Is there any docs for this? I'm having some trouble getting this to work.

_db.set<Person>().Include((German g) => g.Something)

Error CS1660 Cannot convert lambda expression to type 'string' because it is not a delegate type

Tarig0 commented Nov 7, 2017

Is there any docs for this? I'm having some trouble getting this to work.

_db.set<Person>().Include((German g) => g.Something)

Error CS1660 Cannot convert lambda expression to type 'string' because it is not a delegate type

@Tarig0

This comment has been minimized.

Show comment
Hide comment
@Tarig0

Tarig0 Nov 7, 2017

NVM I realize this is a 2.1 feature not 2.0

Tarig0 commented Nov 7, 2017

NVM I realize this is a 2.1 feature not 2.0

@subprime

This comment has been minimized.

Show comment
Hide comment
@subprime

subprime Nov 16, 2017

For the meanwhile is there any solution for an workaround? I'm also have this problem for an REST API Endpoint.

I have currently the problem that in definiton of an ressouce only one API Endpoint shall be created, called URL:PORT/api/testmeans with optional type as query string

public abstract class TestMean
{
    /// Basic properties in there...
}

public class Vehicle : TestMean
{
   /// Special Vehicle properties
}

public class Airplane : TestMean
{
   /// Special Airplane properties
}

But how i can return a whole list of testmeans with specific characteristics?

For the meanwhile is there any solution for an workaround? I'm also have this problem for an REST API Endpoint.

I have currently the problem that in definiton of an ressouce only one API Endpoint shall be created, called URL:PORT/api/testmeans with optional type as query string

public abstract class TestMean
{
    /// Basic properties in there...
}

public class Vehicle : TestMean
{
   /// Special Vehicle properties
}

public class Airplane : TestMean
{
   /// Special Airplane properties
}

But how i can return a whole list of testmeans with specific characteristics?

@ajcvickers

This comment has been minimized.

Show comment
Hide comment
@ajcvickers

ajcvickers Nov 16, 2017

Member

@subprime One workaround is to use multiple tracking queries with the same context instance and let the context fixup all the nav properties.

Member

ajcvickers commented Nov 16, 2017

@subprime One workaround is to use multiple tracking queries with the same context instance and let the context fixup all the nav properties.

@TsengSR

This comment has been minimized.

Show comment
Hide comment
@TsengSR

TsengSR Nov 17, 2017

@subprime : Dunno how your DbContext is set up, but when you add a property which only defines the base class, i.e.

public MyDbContext
{
    public DbSet<TestMean> TestMeans { get; set; }
}

If it's part of a other model, it works same. But if you only want a subset of it, you can only load the subset data first and then query your parent model. EF Core will connect them when loading

var testMeans = await context.TestMeans.OfType<Airplane>().ToArrayAsync(a => a.ParentId = parentId);
var parent = await context.Parent.FirstOrDefaultAsync(p => p.ParentId = parentId);

// parent.TestMeans will then contain the test means from above

It's well covered by the Loading Related Data docs.

And yes, this and the other listed workarounds in the docs means additional roundtrips to the database.

TsengSR commented Nov 17, 2017

@subprime : Dunno how your DbContext is set up, but when you add a property which only defines the base class, i.e.

public MyDbContext
{
    public DbSet<TestMean> TestMeans { get; set; }
}

If it's part of a other model, it works same. But if you only want a subset of it, you can only load the subset data first and then query your parent model. EF Core will connect them when loading

var testMeans = await context.TestMeans.OfType<Airplane>().ToArrayAsync(a => a.ParentId = parentId);
var parent = await context.Parent.FirstOrDefaultAsync(p => p.ParentId = parentId);

// parent.TestMeans will then contain the test means from above

It's well covered by the Loading Related Data docs.

And yes, this and the other listed workarounds in the docs means additional roundtrips to the database.

@IgorWolbers

This comment has been minimized.

Show comment
Hide comment
@IgorWolbers

IgorWolbers Nov 21, 2017

@eL-Prova - I created a feature request for EF6 although I am not holding my breath :(.

@eL-Prova - I created a feature request for EF6 although I am not holding my breath :(.

@Tarig0

This comment has been minimized.

Show comment
Hide comment
@Tarig0

Tarig0 Nov 21, 2017

@subprime

What I did was create a generic function and use the oftype to get the type and then discover the navigations of the type passed into my generic.

You could use somethings like that and loop through all of your known types to get all of the correct properties

public async Task<IEnumerable<T>> GetAsync<T>(string key)
            where T:TestMeans
        {
            IEnumerable < T> result;            
                var query = _db.Set<TestMeans>().OfType<T>().Where(r => r.Key== key);
                foreach (var nav in _db.Model.FindEntityType(typeof(T)).GetNavigations())
                {
                    query = query.Include(nav.Name);
                }
                return  await query.ToListAsync();
        }

Tarig0 commented Nov 21, 2017

@subprime

What I did was create a generic function and use the oftype to get the type and then discover the navigations of the type passed into my generic.

You could use somethings like that and loop through all of your known types to get all of the correct properties

public async Task<IEnumerable<T>> GetAsync<T>(string key)
            where T:TestMeans
        {
            IEnumerable < T> result;            
                var query = _db.Set<TestMeans>().OfType<T>().Where(r => r.Key== key);
                foreach (var nav in _db.Model.FindEntityType(typeof(T)).GetNavigations())
                {
                    query = query.Include(nav.Name);
                }
                return  await query.ToListAsync();
        }
@heightpi

This comment has been minimized.

Show comment
Hide comment
@heightpi

heightpi Jan 6, 2018

Is there still any work to be done on this?
I have the following classes:
public class Warehouse { public int Id{set;get;} public string Name{set;get;} }
and a child class:
public class SupplierWarehouse:Warehouse { public int SupplierId{set;get;} public Supplier Supplier{set;get;} }
In the DB I have one record of each.
and when I do var list = await _dbContext.Warehouses .Include(e => (e as SupplierWarehouse).Supplier) .ToListAsync (); or var list = await _dbContext.Warehouses .Include("Supplier") .ToListAsync ();
I get only the one SupplierWarehouse, but if I remove the Include, I get both records.

heightpi commented Jan 6, 2018

Is there still any work to be done on this?
I have the following classes:
public class Warehouse { public int Id{set;get;} public string Name{set;get;} }
and a child class:
public class SupplierWarehouse:Warehouse { public int SupplierId{set;get;} public Supplier Supplier{set;get;} }
In the DB I have one record of each.
and when I do var list = await _dbContext.Warehouses .Include(e => (e as SupplierWarehouse).Supplier) .ToListAsync (); or var list = await _dbContext.Warehouses .Include("Supplier") .ToListAsync ();
I get only the one SupplierWarehouse, but if I remove the Include, I get both records.

@heightpi

This comment has been minimized.

Show comment
Hide comment
@heightpi

heightpi Jan 6, 2018

ok, looks like because SupplierId is ForeignKey in one to one realtion, it generated the SQL with Inner Join, changing it to int? fixed the problem

heightpi commented Jan 6, 2018

ok, looks like because SupplierId is ForeignKey in one to one realtion, it generated the SQL with Inner Join, changing it to int? fixed the problem

@iudelsmann

This comment has been minimized.

Show comment
Hide comment
@iudelsmann

iudelsmann Jan 11, 2018

After this is released in 2.1, will I be able to order by derived property? Is there a way to do so now in 2.0?

After this is released in 2.1, will I be able to order by derived property? Is there a way to do so now in 2.0?

@ajcvickers

This comment has been minimized.

Show comment
Hide comment
@ajcvickers

ajcvickers Jan 11, 2018

Member

@maumar @anpete Can you answer the question above from @iudelsmann?

Member

ajcvickers commented Jan 11, 2018

@maumar @anpete Can you answer the question above from @iudelsmann?

@heightpi

This comment has been minimized.

Show comment
Hide comment
@heightpi

heightpi Jan 12, 2018

@iudelsmann as what I've seen from Pull Requests, this issue/feature is related only to the Inlcude/ThenInclude. I'm pretty sure that OrderBy will require a different logic

@iudelsmann as what I've seen from Pull Requests, this issue/feature is related only to the Inlcude/ThenInclude. I'm pretty sure that OrderBy will require a different logic

@iudelsmann

This comment has been minimized.

Show comment
Hide comment
@iudelsmann

iudelsmann Jan 12, 2018

I did some testing and was able to OrderBy derived property by casting (in 2.0).

_context.Persons.OrderBy(person => ((Student)person).Registration);

But when ordering by a derived joined property, it did not return the instances of the parent class (it performed inner join instead of left join)

_context.Persons.OrderBy(person => ((Student)person).Course.Name);

I'm not sure if I'm doing things the right way or if this is still a limitation, I would only like to open an issue if it really is one. But you're right, this issue is only about Include/ThenInclude operations so I'll move this question to stack overflow soon. Thanks.

I did some testing and was able to OrderBy derived property by casting (in 2.0).

_context.Persons.OrderBy(person => ((Student)person).Registration);

But when ordering by a derived joined property, it did not return the instances of the parent class (it performed inner join instead of left join)

_context.Persons.OrderBy(person => ((Student)person).Course.Name);

I'm not sure if I'm doing things the right way or if this is still a limitation, I would only like to open an issue if it really is one. But you're right, this issue is only about Include/ThenInclude operations so I'll move this question to stack overflow soon. Thanks.

@maumar

This comment has been minimized.

Show comment
Hide comment
@maumar

maumar Jan 12, 2018

Contributor

@iudelsmann can you post the model you used to get the INNER JOIN query? (entities and contents of OnModelCreating method on DbContext).

I tried similar query on current bits and got query with LEFT JOIN:

SELECT [f].[Id], [f].[CapitalName], [f].[Discriminator], [f].[Name], [f].[CommanderName], [f].[Eradicated]
FROM [Factions] AS [f]
LEFT JOIN (
    SELECT [f.Commander].*
    FROM [LocustLeaders] AS [f.Commander]
    WHERE [f.Commander].[Discriminator] = N'LocustCommander'
) AS [t] ON CASE
    WHEN [f].[Discriminator] = N'LocustHorde'
    THEN [f].[CommanderName] ELSE NULL
END = [t].[Name]
WHERE [f].[Discriminator] = N'LocustHorde'
ORDER BY [t].[ThreatLevel]
Contributor

maumar commented Jan 12, 2018

@iudelsmann can you post the model you used to get the INNER JOIN query? (entities and contents of OnModelCreating method on DbContext).

I tried similar query on current bits and got query with LEFT JOIN:

SELECT [f].[Id], [f].[CapitalName], [f].[Discriminator], [f].[Name], [f].[CommanderName], [f].[Eradicated]
FROM [Factions] AS [f]
LEFT JOIN (
    SELECT [f.Commander].*
    FROM [LocustLeaders] AS [f.Commander]
    WHERE [f.Commander].[Discriminator] = N'LocustCommander'
) AS [t] ON CASE
    WHEN [f].[Discriminator] = N'LocustHorde'
    THEN [f].[CommanderName] ELSE NULL
END = [t].[Name]
WHERE [f].[Discriminator] = N'LocustHorde'
ORDER BY [t].[ThreatLevel]
@iudelsmann

This comment has been minimized.

Show comment
Hide comment
@iudelsmann

iudelsmann Jan 15, 2018

@maumar Ok, I have omitted some stuff but this is basically it. One "detail" I forgot to mention, I'm using Postgresql and Npgsql (everything is 2.0, will update to 2.0.1 soon).

Models (BaseModel and Model are abstract classes):

public class Service : BaseModel
    {
        [Required]
        public string Name { get; set; }

        public ServiceCategory Category { get; set; }
    }

public class Course : Service
    {
        [Required]
        public CourseModality Modality { get; set; }

        [Required]
        public CourseType Type { get; set; }
    }

public class CourseType : Model
    {
        [Required]
        public string Name { get; set; }
    }

There is nothing related to this tables in OnModelCreating.

When doing this query (paged search):

_context.Services.Include().Include(service => service.Category).OrderBy(service => ((Course)service).Type.Name).Skip((pageNumber- 1) * pageSize).Take(pageSize).ToListAsync();

I got this query (for the first page) (omitted some stuff):

SELECT [...]
      FROM "Service" AS "service"
      LEFT JOIN "ServiceCategory" AS "service.Category" ON "service"."CategoryId" = "service.Category"."Id"
      INNER JOIN "CourseType" AS "service.Type" ON "service"."TypeId" = "service.Type"."Id"
      WHERE "service"."Discriminator" IN ('Course', 'Service')
      ORDER BY "service.Type"."Name"
      LIMIT @__p_1 OFFSET @__p_0

@maumar Ok, I have omitted some stuff but this is basically it. One "detail" I forgot to mention, I'm using Postgresql and Npgsql (everything is 2.0, will update to 2.0.1 soon).

Models (BaseModel and Model are abstract classes):

public class Service : BaseModel
    {
        [Required]
        public string Name { get; set; }

        public ServiceCategory Category { get; set; }
    }

public class Course : Service
    {
        [Required]
        public CourseModality Modality { get; set; }

        [Required]
        public CourseType Type { get; set; }
    }

public class CourseType : Model
    {
        [Required]
        public string Name { get; set; }
    }

There is nothing related to this tables in OnModelCreating.

When doing this query (paged search):

_context.Services.Include().Include(service => service.Category).OrderBy(service => ((Course)service).Type.Name).Skip((pageNumber- 1) * pageSize).Take(pageSize).ToListAsync();

I got this query (for the first page) (omitted some stuff):

SELECT [...]
      FROM "Service" AS "service"
      LEFT JOIN "ServiceCategory" AS "service.Category" ON "service"."CategoryId" = "service.Category"."Id"
      INNER JOIN "CourseType" AS "service.Type" ON "service"."TypeId" = "service.Type"."Id"
      WHERE "service"."Discriminator" IN ('Course', 'Service')
      ORDER BY "service.Type"."Name"
      LIMIT @__p_1 OFFSET @__p_0
@HappyNomad

This comment has been minimized.

Show comment
Hide comment
@HappyNomad

HappyNomad Feb 13, 2018

After this is released in 2.1, will it be safe (i.e. no exceptions thrown) to call the string based include on a collection with items of mixed derived types, such some have the included property and others don't? To be useful, it seems the answer must be 'yes'. I'm just double checking.

After this is released in 2.1, will it be safe (i.e. no exceptions thrown) to call the string based include on a collection with items of mixed derived types, such some have the included property and others don't? To be useful, it seems the answer must be 'yes'. I'm just double checking.

@anpete

This comment has been minimized.

Show comment
Hide comment
@HappyNomad

This comment has been minimized.

Show comment
Hide comment
@HappyNomad

HappyNomad Feb 13, 2018

@anpete Thanks, good to know. Looks like you decided against a string based include overload that takes a System.Type as suggested here and echoed here.

@anpete Thanks, good to know. Looks like you decided against a string based include overload that takes a System.Type as suggested here and echoed here.

@anpete

This comment has been minimized.

Show comment
Hide comment
@anpete

anpete Feb 13, 2018

@HappyNomad Feel free to create a separate issue for this, or send a PR. 😉

anpete commented Feb 13, 2018

@HappyNomad Feel free to create a separate issue for this, or send a PR. 😉

@HappyNomad

This comment has been minimized.

Show comment
Hide comment
@HappyNomad

HappyNomad Feb 13, 2018

@anpete No, that's okay. I was just clarifying. In that same comment, I also suggested the alternative that you did end up implementing.

Either that, or have Include search derived classes for chained properties specified in its string navigationPropertyPath parameter.

I assume that's how it works in 2.1.

@anpete No, that's okay. I was just clarifying. In that same comment, I also suggested the alternative that you did end up implementing.

Either that, or have Include search derived classes for chained properties specified in its string navigationPropertyPath parameter.

I assume that's how it works in 2.1.

@maumar

This comment has been minimized.

Show comment
Hide comment
@maumar

maumar Feb 14, 2018

Contributor

@iudelsmann looks like you are hitting #10101 - try performing Include as the last operation:

_context.Services.OrderBy(service => ((Course)service).Type.Name).Skip((pageNumber- 1) * pageSize).Take(pageSize).Include(service => service.Category).ToListAsync();
Contributor

maumar commented Feb 14, 2018

@iudelsmann looks like you are hitting #10101 - try performing Include as the last operation:

_context.Services.OrderBy(service => ((Course)service).Type.Name).Skip((pageNumber- 1) * pageSize).Take(pageSize).Include(service => service.Category).ToListAsync();
@iudelsmann

This comment has been minimized.

Show comment
Hide comment
@iudelsmann

iudelsmann Feb 15, 2018

@maumar That did not work, it is still performing a inner join. Observe that I'm not Includeing the CourseType from the Course entity, as in 2.0 we can't include derived navigation properties. So I think it might work in 2.1 if I include it, as it should perform the appropriate left join, would you agree?

@maumar That did not work, it is still performing a inner join. Observe that I'm not Includeing the CourseType from the Course entity, as in 2.0 we can't include derived navigation properties. So I think it might work in 2.1 if I include it, as it should perform the appropriate left join, would you agree?

@maumar

This comment has been minimized.

Show comment
Hide comment
@maumar

maumar Feb 16, 2018

Contributor

@iudelsmann ahh, I understand now. In fact INNER JOIN for this scenario is the current behavior even in the current developer bits. Problem here is that navigation "Service.Type" is required and this is how we determine whether INNER JOIN or LEFT JOIN should be produced. (we don't take into account that the navigation is declared on derived). I have filed #10988 to track this issue.

Contributor

maumar commented Feb 16, 2018

@iudelsmann ahh, I understand now. In fact INNER JOIN for this scenario is the current behavior even in the current developer bits. Problem here is that navigation "Service.Type" is required and this is how we determine whether INNER JOIN or LEFT JOIN should be produced. (we don't take into account that the navigation is declared on derived). I have filed #10988 to track this issue.

@SvenEV

This comment has been minimized.

Show comment
Hide comment
@SvenEV

SvenEV Feb 28, 2018

After this is released in 2.1, will it be safe (i.e. no exceptions thrown) to call the string based include on a collection with items of mixed derived types, such some have the included property and others don't? To be useful, it seems the answer must be 'yes'. I'm just double checking.

Yes!

@anpete Should this be supported already in the current preview (2.1 preview 1)?

SvenEV commented Feb 28, 2018

After this is released in 2.1, will it be safe (i.e. no exceptions thrown) to call the string based include on a collection with items of mixed derived types, such some have the included property and others don't? To be useful, it seems the answer must be 'yes'. I'm just double checking.

Yes!

@anpete Should this be supported already in the current preview (2.1 preview 1)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment