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.

Sign up for GitHub

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

Jump to bottom

CSHARP-5450: Verify that all calls to Translate or TranslateEnumerable are correct. #1582

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

CSHARP-5450: Verify that all calls to Translate or TranslateEnumerable are correct. #1582

wants to merge 3 commits into from

Conversation

rstam
Copy link
Contributor

@rstam rstam commented Dec 31, 2024

No description provided.

@rstam rstam requested a review from a team as a code owner December 31, 2024 01:52
@rstam rstam changed the title CSHARP-4248: Verify that all calls to Translate or TranslateEnumerable are correct. CSHARP-5450: Verify that all calls to Translate or TranslateEnumerable are correct. Dec 31, 2024
@rstam rstam requested a review from damieng December 31, 2024 23:20
rstam
rstam commented Dec 31, 2024
View reviewed changes
...xpressionTranslators/MethodTranslators/AsQueryableMethodToAggregationExpressionTranslator.cs Outdated
@@ -32,11 +32,12 @@ public static AggregationExpression Translate(TranslationContext context, Method
{
var sourceExpression = arguments[0];
var sourceTranslation = ExpressionToAggregationExpressionTranslator.TranslateEnumerable(context, sourceExpression);
NestedAsQueryableHelper.EnsureQueryableMethodHasNestedAsQueryableSource(expression, sourceTranslation);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@damieng another example of the complications caused by nested AsQueryable in the driver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops... this is the one place where we do not want to call EnsureQueryableMethodHasNestedAsQueryableSource

@rstam
Copy link
Contributor Author

rstam commented Jan 6, 2025

I'm looking into why the patch build failed.

@rstam
Copy link
Contributor Author

rstam commented Jan 6, 2025

I'm also going to rebase on main to resolve the merge conflicts.

@rstam
Copy link
Contributor Author

rstam commented Jan 6, 2025

I'm looking into why the patch build failed.

I fixed the failing tests.

There are still some open questions:

  1. Can the EnsureQueryableMethodHasNestedAsQueryableSource tests be moved to TranslateEnumerable so we don't forget to do them

  2. What about the existing calls that are not followed by a call to EnsureQueryableMethodHasNestedAsQueryableSource? They aren't method translators so calling EnsureQueryableMethodHasNestedAsQueryableSource would currently be a no-op, but perhaps there is something else that would trigger the check?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sanych-sun sanych-sun Awaiting requested review from sanych-sun

@adelinowona adelinowona Awaiting requested review from adelinowona

@damieng damieng Awaiting requested review from damieng

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@rstam