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.

Sign up for GitHub

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

Jump to bottom

Fix to #1833 - Support filtered Include #20279

Merged
merged 1 commit into from
Mar 24, 2020
Merged

Fix to #1833 - Support filtered Include #20279

merged 1 commit into from
Mar 24, 2020

Conversation

maumar
Copy link
Contributor

@maumar maumar commented Mar 14, 2020 •
edited
Loading

Allows for additional operations to be specified inside Include/ThenInclude expression when the navigation is a collection:

  • Where,
  • OrderBy(Descending)/ThenBy(Descending),
  • Skip,
  • Take.

Those additional operations are treated like any other within the query, so translation restrictions apply.

Collections included using new filter operations are considered to be loaded.

Only one filter is allowed per navigation. In cases where same navigation is included multiple times (e.g. Include(A).ThenInclude(A_B).Include(A).ThenInclude(A_C)) filter should only be applied once.
Alternatively the same exact filter should be applied to all.

@maumar maumar requested review from smitpatel, roji and ajcvickers March 14, 2020 00:04
@maumar maumar force-pushed the filtered_includes branch from bf58c4a to 59e2c7e Compare March 14, 2020 00:09
@maumar maumar force-pushed the filtered_includes branch from 59e2c7e to 8beca75 Compare March 18, 2020 19:52
@maumar
Copy link
Contributor Author

maumar commented Mar 18, 2020

new version up @smitpatel

@smitpatel
Copy link
Contributor

and all checks failed 😄

smitpatel
smitpatel reviewed Mar 18, 2020
View reviewed changes
src/EFCore/Properties/CoreStrings.resx Outdated
@@ -1290,6 +1290,12 @@
<data name="IncludeOnNonEntity" xml:space="preserve">
<value>Include has been used on non entity queryable.</value>
</data>
<data name="MultipleFilteredIncludesOnSameNavigation" xml:space="preserve">
<value>Multiple filtered include operations on the same navigation '{navigationName}' are not supported.</value>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are supporting more than just where then we need to create a new term rather than filtered include
@dotnet/efcore

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"the spirit" of the feature is still about filter. Ordering is only added for suppose of Skip/Take, so maybe keeping the name is not so bad. We will never have projections in there, that's for sure so its not like we are future-proofing with the less specific name.

However I'm fine if ppl think otherwise. Something like "modified include", "altered include", "composable include"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditional include.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Extended include", "augmented include"
But taking a step back we don't support multiple normal includes on the same navigation either, so perhaps just make the message more generic

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works for me. lets see what others say

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we support it in a sense that it doesn't throw :)

Copy link
Contributor Author

@maumar maumar Mar 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new message:

Different filters: 'navigation
    .Where(x => x.Name != "Bar")
    .OrderByDescending(x => x.Name)
    .Take(3)' and 'navigation
    .Where(x => x.Name != "Foo")
    .OrderBy(x => x.Id)
    .Take(3)' have been applied on the same included navigation. Only one unique filter per navigation is allowed. For more information on including related data, see http://go.microsoft.com/fwlink/?LinkID=746393.

@maumar maumar force-pushed the filtered_includes branch from 8beca75 to 0b276c5 Compare March 18, 2020 20:39
@maumar
Copy link
Contributor Author

maumar commented Mar 18, 2020

fixed again, missed one of the sqlite tests - they all need to be skipped cause lack of apply

@maumar maumar force-pushed the filtered_includes branch from 0b276c5 to e737094 Compare March 19, 2020 01:15
@maumar
Copy link
Contributor Author

maumar commented Mar 19, 2020

another version up @smitpatel

@maumar maumar force-pushed the filtered_includes branch from e737094 to 44bd275 Compare March 19, 2020 18:40
smitpatel
smitpatel reviewed Mar 19, 2020
View reviewed changes
test/EFCore.Specification.Tests/Query/ComplexNavigationsQueryTestBase.cs

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public virtual async Task Filtered_include_with_Distinct_throws(bool async)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a test where navigation is registered as reference navigation in EF Core metadata but implements IEnumerable<> and will allow you to write filtered include syntax. (Essentially filterExpression when put on non collection navigation should throw.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do people actually do that? Seems like very contrived scenario.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's negative case example. Rather than ignoring (which what we are doing currently when we see non-navigation in Include, we should change that), we can throw a better error message.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On note of reference nav which implements IEnumerable, it is not impossible.

smitpatel
smitpatel suggested changes Mar 19, 2020
View reviewed changes
Copy link
Contributor

@smitpatel smitpatel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More tests on further nesting
Nested filtered includes e.g. filter on c.Orders & then include filter on o.OrderDetails

@maumar maumar force-pushed the filtered_includes branch 3 times, most recently from d9f78c4 to 48dfd16 Compare March 23, 2020 22:52
maumar
maumar commented Mar 23, 2020
View reviewed changes
src/EFCore/Properties/CoreStrings.resx
@@ -747,8 +747,8 @@
<data name="AmbiguousForeignKeyPropertyCandidates" xml:space="preserve">
<value>Both relationships between '{firstDependentToPrincipalNavigationSpecification}' and '{firstPrincipalToDependentNavigationSpecification}' and between '{secondDependentToPrincipalNavigationSpecification}' and '{secondPrincipalToDependentNavigationSpecification}' could use {foreignKeyProperties} as the foreign key. To resolve this configure the foreign key properties explicitly on at least one of the relationships.</value>
</data>
<data name="InvalidIncludeLambdaExpression" xml:space="preserve">
<value>The {methodName} property lambda expression '{includeLambdaExpression}' is invalid. The expression should represent a property access: 't =&gt; t.MyProperty'. To target navigations declared on derived types, specify an explicitly typed lambda parameter of the target type, E.g. '(Derived d) =&gt; d.MyProperty'. For more information on including related data, see http://go.microsoft.com/fwlink/?LinkID=746393.</value>
<data name="InvalidIncludeExpression" xml:space="preserve">
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TMI?

@maumar maumar force-pushed the filtered_includes branch from 48dfd16 to 6317688 Compare March 23, 2020 22:55
@maumar
Copy link
Contributor Author

maumar commented Mar 23, 2020

new version up @smitpatel

@maumar maumar force-pushed the filtered_includes branch 2 times, most recently from b662b2f to bb11d83 Compare March 24, 2020 19:50
@maumar
Fix to #1833 - Support filtered Include
21b9a35 
Allows for additional operations to be specified inside Include/ThenInclude expression when the navigation is a collection:
- Where,
- OrderBy(Descending)/ThenBy(Descending),
- Skip,
- Take.

Those additional operations are treated like any other within the query, so translation restrictions apply.

Collections included using new filter operations are considered to be loaded.

Only one filter is allowed per navigation. In cases where same navigation is included multiple times (e.g. Include(A).ThenInclude(A_B).Include(A).ThenInclude(A_C)) filter should only be applied once.
Alternatively the same exact filter should be applied to all.
@maumar maumar force-pushed the filtered_includes branch from bb11d83 to 21b9a35 Compare March 24, 2020 20:50
@maumar maumar merged commit 21b9a35 into master Mar 24, 2020
@maumar maumar deleted the filtered_includes branch March 24, 2020 22:31
@rbhanda rbhanda mentioned this pull request Mar 25, 2020
Closed
1 task
@ajcvickers ajcvickers mentioned this pull request Mar 27, 2020
Closed
@onurh
Copy link

onurh commented May 4, 2020

Are you planning to put this any 3.x version?

@roji
Copy link
Member

roji commented May 4, 2020

@onurh no, this new feature will not be backported to any 3.x version. However, consider trying out the 5.0 previews - they are generally well-tested and work well.

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

@smitpatel smitpatel smitpatel approved these changes

@AndriySvyryd AndriySvyryd AndriySvyryd left review comments

@roji roji Awaiting requested review from roji

@ajcvickers ajcvickers Awaiting requested review from ajcvickers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@maumar @smitpatel @onurh @roji @Moumit @ErikEJ @AndriySvyryd @cgountanis