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: Interpret path passed to Include() and allow partial matches #12737

Closed
divega opened this issue Jul 20, 2018 · 3 comments
Closed

Query: Interpret path passed to Include() and allow partial matches #12737

divega opened this issue Jul 20, 2018 · 3 comments
Assignees
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 Jul 20, 2018

The currently accepted design is that for an Include call to be valid (i.e. not ignored) the starting type of the include path has to appear in the results of the query.

The "accidental design" in 2.0 (and what this issue argues for restoring) is that an Include can still be valid if the starting point ends up not being in the results, as long as other entities reachable through the include path are in the results.

In #12181 (comment) we said:

Notes from triage:

  • It was a bug in 2.0 that the Includes were not ignored. We don't want to make this the accepted behavior going forward because it would further complicate the story around Include, which is already hard for people to understand.
  • @smitpatel and @maumar to see if there are other reasonable ways to write this query that will work with 2.1.
  • Since we already decided on a pattern that uses Include inside a projection (see Query: correlated collection optimization doesn't get applied for subqueries with AsQueryable #12195) then we should try to use that kind of pattern. However, there are API issues extending this to reference navigations.
  • Another way to tackle this longer term is with rule-based eager loading (load patterns). See

However after this was reported again in #12678, @smitpatel, @maumar and I discussed and I believe there is enough new data to re-discuss this decision.

Example query (based on #12678):

var data = 
    from c in db.Checks
        join ct in db.CheckTemplates.Include(a => a.Implementers).ThenInclude(i => i.User)
            on c.CheckTemplateId equals ct.CheckTemplateId
    where c.CheckId == checkId
    select new
        {
	    CheckId = c.CheckId,
	    Description = ct.Description,
	    Implementers = ct.Implementers,
	    NumExaminers = ct.Examiners.Count
        };

Note that Include() is called on ChekTemplates, and CheckTemplates (i.e. ct) ends up not being in the results. However, ct.Implementers, which was in the include path, is projected. In EF Core 2.0 the results would include Users (mentioned in the call to ThenInclude). In 2.1 they don't.

Some of the arguments in favor of restoring the 2.0 behavior:

  1. I believe it is intuitive for customers: Even if a formal explanation for the 2.0 behavior requires more words and concepts than the currently accepted design (i.e. it is harder for us to analyze and describe), in practice it can save us from having to explain customers why it does not work in this way... I.e. our currently accepted design has proven not be capable of realizing users' intent in a number of cases.

  2. It allows writing queries that are hard to write in any other way: We have some ideas around how we can enable users to call Include in other parts of the query or even to use some version of Include that allows specifying an arbitrary starting type at any point in the query. But none of these work, and even if we make them work at some point, they will still look more complicated and harder to discover than this behavior. Also, rewriting queries so that you don't use query roots that you are not going to include in the results can also be very difficult.

  3. It shouldn't be much more expensive at runtime: worse case scenario, some Include() calls that are now being ignored (and causing warnings) will stop being ignored and some additional entities that the user intended to load (like User in the example) will be loaded.

  4. From a brief chat with @smitpatel and @maumar this wouldn't be to hard to do, e.g. in 3.0.

@ajcvickers
Copy link
Member

See also #12802

@smitpatel
Copy link
Member

Design meeting decisions:

  • We will load related data based on Includes even if the root is not in results as long as any entity along the path from root to leaf is in results.
  • If an include chain from root to leaf is specified and no entity along the path is in result then we generate ignored include warning.
  • We will disallow specifying include anywhere in the tree. Applying Include on results of SelectMany or on the collection in anonymous type projection (which requires using AsQueryable to get Include) would be disallowed. You can include the chain from root before applying such operators and they will be loaded appropriately.
  • The only places where Include can be used will be query root followed by any operator which does not change the shape of the result. (e.g. Where/OrderBy/Skip/Take etc) Once the shape changed, includes would be disallowed. When shape changed to known EntityType we allowed Include but when it is changed to nominal type, we disallowed it, even though path was identifiable. This change removes the confusion.

@smitpatel smitpatel changed the title Query: Consider revisiting interpretation of path passed to Include() to allow partial matches Query: Interprete of path passed to Include() and allow partial matches Apr 4, 2019
@ajcvickers ajcvickers changed the title Query: Interprete of path passed to Include() and allow partial matches Query: Interpret of path passed to Include() and allow partial matches May 10, 2019
@ajcvickers ajcvickers changed the title Query: Interpret of path passed to Include() and allow partial matches Query: Interpret path passed to Include() and allow partial matches May 10, 2019
@smitpatel
Copy link
Member

The product change has been made for this. Proper test coverage is being tracked by #15368

@smitpatel smitpatel added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Jun 5, 2019
@smitpatel smitpatel modified the milestones: 3.0.0, 3.0.0-preview6 Jun 5, 2019
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

4 participants