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-4684: Make part of IMongoQueryProvider public. #1325

Merged
merged 1 commit into from
May 23, 2024

Conversation

rstam
Copy link
Contributor

@rstam rstam commented May 14, 2024

This PR is to solve an issue Damien was having getting the LoggedStages in the EF Core Provider in some cases.

Our earlier work made LoggedStage available via the IMongoQueryable interface. But when executing "scalar" (single valued) LINQ queries the EF Core Provider calls Execute directly on the internal IMongoQueryProvider.

Damien was having to use reflection to call the LoggedStages property on the IMongoQueryProvider.

This PR is a proposal to make part of the IMongoQueryProvider interface public by splitting the interface into public and internal parts.

I also made the ExecuteAsync method public. Damien, you are already calling the Execute method (defined in IQueryProvider), do you not need to call ExecuteAsync to handle async queries? Or does EF Core not support async queries?

For purposes of looking at this together I just used the existing CSHARP-4684 ticket. We may choose to create a new ticket.

@rstam rstam requested review from JamesKovacs and damieng May 14, 2024 16:30
@rstam rstam requested a review from a team as a code owner May 14, 2024 16:30
Copy link
Contributor

@JamesKovacs JamesKovacs left a comment

Choose a reason for hiding this comment

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

LGTM. This feels like a good solution to avoid reflection in our EF provider. Splitting IMongoQueryProvider into public and internal parts is a good design choice.

@damieng
Copy link
Member

damieng commented May 15, 2024

Right now I only use Execute but want to support ExecuteAsync soon.

@damieng
Copy link
Member

damieng commented May 16, 2024

Tested this branch with the EF provider and it's working nicely without reflection. I switched to using Provider.LoggedStages for both single/scalar and enumerable so keep the code simpler my end - we could I guess remove it entirely though that would be a breaking change if anyone else has adopted it yet.

Copy link
Member

@damieng damieng left a comment

Choose a reason for hiding this comment

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

Looks great, nice to see cleaner tests as a result too.

@rstam rstam merged commit a5bcbd1 into mongodb:master May 23, 2024
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants