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

Nav expansion visitor does not visit the Contains item argument #32217

Closed
roji opened this issue Nov 2, 2023 · 1 comment
Closed

Nav expansion visitor does not visit the Contains item argument #32217

roji opened this issue Nov 2, 2023 · 1 comment
Assignees
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. regression Servicing-approved type-bug
Milestone

Comments

@roji
Copy link
Member

roji commented Nov 2, 2023

Note: this issue tracks only fixing Contains. Other non-lambda operators (Skip, Take, ElementAt...) are tracked by #32312, to have two quirks for Contains and non-Contains.

When running test Nested_contains_with_Lists_and_no_inferred_type_mapping (introduced in #32214), which has the following code:

var ints = new List<int> { 1, 2, 3 };
var strings = new List<string> { "one", "two", "three" };

return AssertQuery(
    async,
    ss => ss.Set<PrimitiveCollectionsEntity>().Where(e => strings.Contains(ints.Contains(e.Int) ? "one" : "two")));

... the query tree going into nav expansion is:

DbSet<PrimitiveCollectionsEntity>()
    .Where(e => __strings_0
        .AsQueryable()
        .Contains(__ints_1
            .AsQueryable()
            .Contains(e.Int) ? "one" : "two"))

... whereas the tree coming out is:

DbSet<PrimitiveCollectionsEntity>()
    .Where(p => __strings_0
        .Contains(__ints_1
            .AsQueryable()
            .Contains(p.Int) ? "one" : "two"))

Note that the outer AsQueryable (over __strings_0) is gone, while the inner one (over __ints_1) remains.

What's happening here is this... NavigationExpandingEV works recursively on lambda arguments of operators; ProcessWhere calls into ProcessLambdaExpression, which calls into ExpandNavigationsForSource which visits the the Where predicate. This causes the Where lambda to be fully processed by the visitor, and the AsQueryable wrapping __strings_0 is removed.

However, ProcessContains does not similarly recurse into the item argument of Contains; so the AsQueryable there isn't removed.

Note that there may be other important functionality of nav expansion which is not performed here; and there are other operator aside from Contains which likely don't visit their non-lambda arguments. So there are probably other bugs hiding in here.

When this is fixed, we can remove the unwrapping of AsQueryable() from the fallback Contains translation (see #32218).

@roji
Copy link
Member Author

roji commented Nov 14, 2023

Note: #32291 was filed by a user, showing a regression because of this bug. Visiting the item in ProcessContains fixes the problem (#32291 (comment)).

maumar added a commit that referenced this issue Nov 15, 2023
…m argument (and possibly other non-lambda arguments)

This was always a problem but was uncovered by change in how we process Contains. We were going through the full process of nav expansion on the argument (only on the source), which could lead to untranslatable expression tree in some cases.

Fixes #32217
@maumar maumar self-assigned this Nov 15, 2023
@roji roji changed the title Nav expansion visitor does not visit the Contains item argument (and possibly other non-lambda arguments) Nav expansion visitor does not visit the Contains item argument Nov 15, 2023
maumar added a commit that referenced this issue Nov 16, 2023
…m argument (and possibly other non-lambda arguments)

This was always a problem but was uncovered by change in how we process Contains. We were going through the full process of nav expansion on the argument (only on the source), which could lead to untranslatable expression tree in some cases.

Fixes #32217
Fixes #32312
maumar added a commit that referenced this issue Nov 16, 2023
…m argument (and possibly other non-lambda arguments)

This was always a problem but was uncovered by change in how we process Contains. We were going through the full process of nav expansion on the argument (only on the source), which could lead to untranslatable expression tree in some cases.

Fixes #32217
Fixes #32312
maumar added a commit that referenced this issue Nov 16, 2023
…m argument (and possibly other non-lambda arguments)

This was always a problem but was uncovered by change in how we process Contains. We were going through the full process of nav expansion on the argument (only on the source), which could lead to untranslatable expression tree in some cases.

Fixes #32217
Fixes #32312
@ajcvickers ajcvickers modified the milestones: 8.0.x, 8.0.2 Nov 16, 2023
@ajcvickers ajcvickers added Servicing-approved closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. and removed Servicing-consider labels Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. regression Servicing-approved type-bug
Projects
None yet
Development

No branches or pull requests

3 participants