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

Verify exceptions when Include() is used in invalid or not supported locations in a query #14671

Open
divega opened this issue Feb 11, 2019 · 3 comments

Comments

@divega
Copy link
Contributor

divega commented Feb 11, 2019

Some time ago we discussed in #9030 the possibility of throwing an exception by default when we find an Include() call that has to be ignored.

We decided against switching to throw by default because there are legitimate situations in which after a valid Include() is added to a query, further composition makes the Include() unnecessary, and we definitively don't want to throw in those situations. A common example of this is adding a call to Count() at the end of a query to compute the number of rows the original query will return.

However there is another situations in which Include() is called in a location which we either consider invalid or unsupported in a given release.

I am creating this issue to consider distinguishing between these situations and ensuring that we throw for the later.

I believe this aligns well with the philosophy of our query design in 3.0, in that it can help customers detect things that won't work earlier in development.

It can also help us evolve support for Include() in the new query pipeline more intentionally: we can start with support for basic scenarios in 3.0 and gradually add support for additional scenarios based on customer feedback, and with less risk of introducing breaking changes.

I also think addressing #12737 (revisit interpretation of path passed to Include() to allow partial matches) should help in reducing the number of situations in which Include() appears to be needed in weird locations in a query.

@ajcvickers ajcvickers added this to the 3.0.0 milestone Feb 20, 2019
@ajcvickers ajcvickers changed the title Consider throwing when Include() is used in invalid or not supported locations in a query Verify exceptions when Include() is used in invalid or not supported locations in a query Jul 24, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0, Backlog Jul 24, 2019
@ajcvickers ajcvickers assigned maumar and unassigned smitpatel Jul 24, 2019
@smitpatel
Copy link
Member

https://github.com/aspnet/EntityFrameworkCore/blob/17da6da5edc902d99b835a14bacc444993619a6b/src/EFCore/Query/Internal/NavigationExpandingExpressionVisitor.cs#L424-L476

Above code restricts include to specified only on queryables which represent an entityType except for using Select (or SelectMany in future).

@smitpatel smitpatel added verify-fixed This issue is likely fixed in new query pipeline. and removed Nav-Expansion labels Sep 3, 2019
@ajcvickers ajcvickers modified the milestones: Backlog, 3.1.0 Sep 4, 2019
@maumar
Copy link
Contributor

maumar commented Oct 2, 2019

We don't throw for queries where include comes after identity projection, or simple SelectMany.

ss.Set<Faction>().Select(f => f).Include(h => h.Capital)
ss.Set<Faction>().SelectMany(f => f.Capital.BornGears).Include(g => g.Squad)

@maumar maumar removed this from the 3.1.0 milestone Oct 2, 2019
maumar added a commit that referenced this issue Oct 2, 2019
resolves #14550
resolves #15164
resolves #15994

added regression tests for #14671
added regression test for #17852
converted remaining tests in Gears of War into AssertQuery pattern (part of #12501)
maumar added a commit that referenced this issue Oct 2, 2019
resolves #14550
resolves #15164
resolves #15994

added regression tests for #14671
added regression test for #17852
converted remaining tests in Gears of War into AssertQuery pattern (part of #12501)
maumar added a commit that referenced this issue Oct 2, 2019
resolves #14550
resolves #15164
resolves #15994
resolves #16722

added regression tests for #14671
added regression test for #17852
converted remaining tests in Gears of War into AssertQuery pattern (part of #12501)
@maumar maumar removed the verify-fixed This issue is likely fixed in new query pipeline. label Oct 3, 2019
@ajcvickers ajcvickers added this to the Backlog milestone Oct 7, 2019
@smitpatel
Copy link
Member

There are test disabled with this. If we decide to close this, we should clean up

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

No branches or pull requests

4 participants