Fix to #30575 - Multiple LeftJoins (GroupJoins) lead to GroupJoin Exception when the same where is used twice #30594
Conversation
| @@ -37,6 +37,10 @@ public QueryableMethodNormalizingExpressionVisitor(QueryCompilationContext query | |||
| /// </summary> | |||
| public virtual Expression Normalize(Expression expression) | |||
| { | |||
| var externalParametersDetector = new ExternalParametersDetector(); | |||
| externalParametersDetector.Visit(expression); | |||
| _selectManyVerifyingExpressionVisitor = new(externalParametersDetector.ExternalParameters); | |||
There was a problem hiding this comment.
nit: instead of reinstantiating SelectManyVerifyingExpressionVisitor every time (allocations etc.), we can just pass the allowed parameters to VerifyCollectionSelector
There was a problem hiding this comment.
If I understand correctly, the problem here is that a captured lambda variable is mistakenly identified as a sever-correlated ParameterExpression, and so instead of LEFT JOIN we get OUTER APPLY, right?
If so, and we need to identify captured variables to avoid marking the tree as correlated, then I think the standard way of doing that is checking if the parameter name starts with QueryCompilationContext.QueryParameterPrefix; we already do that in various other contexts.
If that's all correct, then instead of an additional visitation for all queries in order to identify captured variables, I think we can just check the name in SelectManyVerifyingExpressionVisitor.VisitParameter:
if (_allowedParameters.Contains(parameterExpression)
|| parameterExpression.Name?.StartsWith(QueryCompilationContext.QueryParameterPrefix, StringComparison.Ordinal) == true)
{
return parameterExpression;
}In confirmed that just doing this makes your new tests pass.
src/EFCore/Query/Internal/QueryableMethodNormalizingExpressionVisitor.cs
Outdated
Show resolved
Hide resolved
| { | ||
| if (!ExternalParameters.Contains(parameterExpression)) | ||
| { | ||
| ExternalParameters.Add(parameterExpression); |
There was a problem hiding this comment.
Note that all lambda parameters are returned in the allowed, and their scope is "valid" throughout the entire query tree; this is unlike the current implementation, where we remove the lambda parameters from the allow list when we exit the lambda. I don't think it's a problem, since such a parameter could probably only appear outside of its lambda's scope in malformed expression tree.
Another point is that we add the lambda parameters here, but then there's still the code in SelectManyVerifyingExpressionVisitor which adds and removes them again...
To make this simpler/safer, we could have this visitor only add non-lambda parameters, and leave the lambda parameter management to SelectManyVerifyingExpressionVisitor as today. So we'd just provide an externally-provided list of non-lambda parameters.
…eption when the same where is used twice Problem is in QueryableMethodNormalizingExpressionVisitor and specifically part where we convert from GroupJoin-SelectMany-DefaultIfEmpty into left join (SelectManyVerifyingExpressionVisitor). We check if the collection selector is correlated, and we do that by looking at parameters in that lambda. Problem is that the affected queries reference outside variable that gets parameterized and that breaks the correlation finding logic. Fix is to check if a parameter has a prefix associated with external parameter when we count correlated parameters. Fixes #30575
|
Should consider for patching... |
Problem is in QueryableMethodNormalizingExpressionVisitor and specifically part where we convert from GroupJoin-SelectMany-DefaultIfEmpty into left join (SelectManyVerifyingExpressionVisitor). We check if the collection selector is correlated, and we do that by looking at parameters in that lambda. Problem is that the affected queries reference outside variable that gets parameterized and that breaks the correlation finding logic. Fix is to add a step that scans entire query and identifies external parameters before we try to normalize GJSMDIE into LeftJoins, so that those external parameters are not counted as correlated.
Fixes #30575