[release/8.0] Fix to #32217 - Nav expansion visitor does not visit the Contains item argument (and possibly other non-lambda arguments) #32317
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…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
roji
approved these changes
Nov 16, 2023
|
Note that the branch is closed for 8.0.1 |
|
@maumar Can we cherry-pick these patches on to main, since I'm not sure how long release/8.0 will be closed for, and it would be good to get these fixes into the daily builds so people can try them? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #32217
Fixes #32312
Description
We were not visiting non-lambda members of some Queryable operators. This was always a problem but was uncovered by change in how we process Contains.
Customer impact
Some queries using Contains, specifically ones that use navigations inside the argument to the Contains method now throw, where they used to work.
How found
Found by a customer as well as EF Core team member independently.
Regression
Yes.
Testing
Added tests.
Risk
Low: We add visitation step for the arguments of some methods. This is effectively what we used to do in 7.0 (before 8.0 changes, Contains used to be handled by
ProcessUnknownMethodwhich visits all its arguments). For extra safety, added two separate quirks: one for Contains, which is a reported regression, and one for other methods that have exactly the same bug (Skip, Take, ElementAt) for which don't have a clear regression scenario yet.