CSHARP-6125: Reject non-collection inner sequences in LINQ Join/LeftJoin - #2075
CSHARP-6125: Reject non-collection inner sequences in LINQ Join/LeftJoin#2075damieng wants to merge 2 commits into
Conversation
|
Assigned |
There was a problem hiding this comment.
Pull request overview
This PR restores LINQ3 behavior for Join/LeftJoin by rejecting “non-collection” inner sequences (e.g., filtered/ordered/limited queryables) that cannot be translated with correct semantics, aligning with CSHARP-6125.
Changes:
- Updates the LINQ3 join pipeline translator to only allow a bare collection as the inner sequence (rejecting subqueries on the inner).
- Updates
QueryableLeftJoinTeststo assert translation-time rejection for unsupported inner sequences, including a new ordered+limited case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/MongoDB.Driver.Tests/Linq/Integration/QueryableLeftJoinTests.cs | Updates/extends coverage to assert ExpressionNotSupportedException for unsupported inner sequences in LeftJoin. |
| src/MongoDB.Driver/Linq/Linq3Implementation/Translators/ExpressionToPipelineTranslators/JoinMethodToPipelineTranslator.cs | Removes translation of inner pipelines and restricts inner to a bare collection for join/left-join translation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Only a bare collection is supported as the inner sequence. A non-collection inner sequence | ||
| // (e.g. one with OrderBy/Take/Skip) would be translated into a correlated $lookup pipeline that | ||
| // applies per outer document rather than once globally, producing wrong results. Reject it here; | ||
| // proper support for such subqueries is tracked in CSHARP-6118. | ||
| var (innerCollectionName, innerSerializer) = innerExpression.GetCollectionInfoFromQueryable(containerExpression: expression); |
There was a problem hiding this comment.
I think this copilot comment could be wrong, but can you give it a look?
There was a problem hiding this comment.
In theory if somebody was to build an expression tree by hand and wrap the inner query in a ConstantExpression then they would get the wrong results when that inner query is Join or LeftJoin. That scenario exists prior to 3.10 for Join and we don't provide a way using our LINQ or Builders to do that - the user would have have to have written it using Expression.Constant themselves.
| // A non-collection inner sequence (filtered, ordered, or limited) is not supported: translating it | ||
| // into a correlated $lookup pipeline would apply the operation per outer document rather than once | ||
| // globally, producing wrong results (CSHARP-6125). Such subqueries must be rejected until proper | ||
| // support is added (CSHARP-6118). |
papafe
left a comment
papafe
left a comment
There was a problem hiding this comment.
Overall it looks good. Just a couple of comments.
| // support is added (CSHARP-6118). | ||
| [Fact] | ||
| public void LeftJoin_with_filtered_inner_queryable_should_apply_filter() | ||
| public void LeftJoin_with_filtered_inner_queryable_should_throw() |
There was a problem hiding this comment.
Do we need similar tests for Join?
| // Only a bare collection is supported as the inner sequence. A non-collection inner sequence | ||
| // (e.g. one with OrderBy/Take/Skip) would be translated into a correlated $lookup pipeline that | ||
| // applies per outer document rather than once globally, producing wrong results. Reject it here; | ||
| // proper support for such subqueries is tracked in CSHARP-6118. | ||
| var (innerCollectionName, innerSerializer) = innerExpression.GetCollectionInfoFromQueryable(containerExpression: expression); |
There was a problem hiding this comment.
I think this copilot comment could be wrong, but can you give it a look?
These kinds of sequences do not translate correctly and CSHARP-6017 inadvertently let them through instead of continuing to throw.