Skip to content
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: Include and ThenInclude should support collection navigation properties that are just IEnumerable<T> #1481

Closed
divega opened this issue Jan 27, 2015 · 3 comments
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@divega
Copy link
Contributor

divega commented Jan 27, 2015

We learned yesterday while discussing #1463 with @anpete and @ajcvickers that the EF7 runtime does not constrain collection navigation properties to be of types that can be assigned to ICollection<T> but that IEnumerable<T> is generally sufficient.

The eager loading API should relax to support the same navigation property types.

@divega
Copy link
Contributor Author

divega commented Oct 19, 2015

HasMany() already supports IEnumerable<TRelatedEntity> so this seems to be the only issue blocking having navigation properties that are statically of that type.

@ajcvickers @rowanmiller @anpete note that this is currently pri1 for RTM but I wonder if it is just a matter of adding some tests and changing the signature of this overload of ThenInclude() from:

public static IIncludableQueryable<TEntity, TProperty> 
    ThenInclude<TEntity, TPreviousProperty, TProperty>( 
        [NotNull] this IIncludableQueryable<TEntity, ICollection<TPreviousProperty>> source, 
        [NotNull] Expression<Func<TPreviousProperty, TProperty>> navigationPropertyPath) 

To

public static IIncludableQueryable<TEntity, TProperty> 
    ThenInclude<TEntity, TPreviousProperty, TProperty>( 
        [NotNull] this IIncludableQueryable<TEntity, IEnumerable<TPreviousProperty>> source, 
        [NotNull] Expression<Func<TPreviousProperty, TProperty>> navigationPropertyPath) 

@cbaxter
Copy link

cbaxter commented Jun 12, 2016

@divega Based on the current state of dev branch, making the proposed change seems to work as you expected with no obvious side-effects.

I modified the overload of ThenInclude in a local build and verified the existing tests pass via the official build script (although Azure tests skipped in my environment). I also ran through several ThenInclude examples in my test project and everything seems to be working like a charm.

Although a small change, this would be a nice addition to help keep entities from having to expose a public ICollection<T> member for use with Include references.

@divega divega removed this from the Backlog milestone Jun 12, 2016
@rowanmiller rowanmiller added this to the 1.0.0 milestone Jun 13, 2016
@ajcvickers
Copy link
Contributor

Talked to @divega @rowanmiller and decided to take this post RTM, probably in first point release. We should also make navigations exposed as IEnumerables but backed by ICollections work at that time--they currently don't work.

@ajcvickers ajcvickers modified the milestones: 1.0.1, 1.0.0 Jun 13, 2016
ajcvickers added a commit that referenced this issue Jun 14, 2016
ajcvickers added a commit that referenced this issue Jun 17, 2016
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jul 6, 2016
@ajcvickers ajcvickers modified the milestones: 1.1.0-preview1, 1.1.0 Oct 15, 2022
@ajcvickers ajcvickers removed their assignment Sep 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
Development

No branches or pull requests

5 participants