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

CSHARP-5396: Add support for arbitrary join inner expression arguments in LINQ #1531

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kerem-acer
Copy link

@kerem-acer kerem-acer commented Nov 4, 2024

This PR add support for arbitrary join inner expression arguments in LINQ.

Problem

The MongoDB LINQ translator currently throws an exception when attempting to join a foreign collection queryable that includes additional query operations (like .Where()).

Example of the Issue

When trying to join two collections with filtering conditions, the following code throws an exception:

// Collection setup and mapping code...

var pagesWithContainers = await (from page in pages.AsQueryable().Where(x => x.TenantId == 1)
    join container in pageContainers.AsQueryable().Where(x => x.TenantId == 1) 
    on page.Id equals container.PageId into containers
    select new
    {
        page.Id,
        page.Title,
        Containers = containers
    }).ToListAsync();

The code only works if you remove the .Where() clause from pageContainers. This PR adds support for such filtering operations on joined collections.

Current Error

Without this PR, the above code throws the following exception:

MongoDB.Driver.Linq.ExpressionNotSupportedException: Expression not supported: [...] 
because inner expression must be a MongoDB IQueryable against a collection.

Todos

  • Write comprehensive tests

@kerem-acer kerem-acer requested a review from a team as a code owner November 4, 2024 20:35
@kerem-acer kerem-acer requested review from JamesKovacs and removed request for a team November 4, 2024 20:35
@kerem-acer
Copy link
Author

Even though now it doesn't throw exceptions, Where()'s of foreign collection queryable are ignored. Probably more work is needed to get this job done.

As I understand, in JoinMethodToPipelineTranslator.Translate method line 66, innerExpression(foreign queryable expression) is used only for collectionName. So JoinMethodToPipelineTranslator.Translate does not respect the content of innerExpression.

@rstam
Copy link
Contributor

rstam commented Nov 5, 2024

You are correct, we expect the inner argument to Join to be just a way to specify the inner collection, so we expect this:

Join(inner.AsQueryable(), ...)

and not:

Join(inner.AsQueryable().Where(...), ...)

This is by design.

I'm not sure how much work it would be, or if it is even possible, to support arbitrary inner arguments. In any case the first step would be to figure out if there even is an MQL translation that would return the desired results.

@kerem-acer
Copy link
Author

Okay. Now, this this is a new feature rather than a bug. I will investigate and try to implement this feature this week. I will update you here.

Also, should I update the title and description of this PR and Jira issue? The scope of this PR has changed.

@BorisDog BorisDog requested review from rstam and removed request for JamesKovacs November 7, 2024 00:30
@kerem-acer kerem-acer changed the title CSHARP-5396: Fix LINQ join error when add .Where to foreign collectio… CSHARP-5396: Add support for arbitrary join inner expression arguments in LINQ Nov 11, 2024
@kerem-acer
Copy link
Author

I implemented this feature and changed content of this PR.

fyi @rstam

}
else
{
var lookupPipeline = ExpressionToPipelineTranslator.Translate(context, innerExpression);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we create a new TranslationContext for innerExpression?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants