EF-76: More work on projections involving tuple create #242
Conversation
…b#240) * EF-76: Flow EF metadata down through CreateGetValueExpression That is, the IProperty or INavigation associated with the access. Types, requiredness, type-mappings, etc. are then obtained from the property. An alias is passed in if the name in the BSON document is different from the mapped element name for the property. * Updates based on co-pilot review.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances projection handling in the MongoDB Entity Framework Core provider, specifically improving support for tuple creation expressions in projections. The changes address issues with tuple expressions involving complex expressions like concatenation, comparison, and conditional logic.
- Refactors projection binding system to use MongoDB-specific projection classes instead of generic EF Core ones
- Adds support for ordinal-based value retrieval to handle tuple member access
- Updates various visitors and binding mechanisms to properly handle tuple expressions and their components
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/MongoDB.EntityFrameworkCore.UnitTests/Storage/BsonBindingTests.cs | Updates test method calls to include new null parameter for BsonBinding.GetPropertyValue |
| tests/MongoDB.EntityFrameworkCore.SpecificationTests/Query/NorthwindMiscellaneousQueryMongoTest.cs | Updates test expectations and error messages for improved ToString() handling |
| tests/MongoDB.EntityFrameworkCore.FunctionalTests/Query/ProjectionTests.cs | Adds new test for tuple projections with expressions |
| src/MongoDB.EntityFrameworkCore/Storage/MongoUpdate.cs | Updates method call to include null parameter for GetPropertySerializationInfo |
| src/MongoDB.EntityFrameworkCore/Storage/BsonBinding.cs | Major refactor to support alias-based and ordinal-based value retrieval for projections |
| src/MongoDB.EntityFrameworkCore/Serializers/EntitySerializer.cs | Updates method call to include null parameter for GetPropertySerializationInfo |
| src/MongoDB.EntityFrameworkCore/Serializers/BsonSerializerFactory.cs | Adds alias parameter support and new type serialization info method |
| src/MongoDB.EntityFrameworkCore/Query/Visitors/MongoShapedQueryCompilingExpressionVisitor.cs | Removes root entity type parameter and adds logic for handling Select expressions |
| src/MongoDB.EntityFrameworkCore/Query/Visitors/MongoQueryableMethodTranslatingExpressionVisitor.cs | Updates to use MongoDB-specific projection binding expressions |
| src/MongoDB.EntityFrameworkCore/Query/Visitors/MongoProjectionBindingRemovingExpressionVisitor.cs | Major refactor to handle MongoDB-specific projection bindings and ordinal-based access |
| src/MongoDB.EntityFrameworkCore/Query/Visitors/MongoProjectionBindingExpressionVisitor.cs | Adds tuple creation support and ordinal tracking for complex expressions |
| src/MongoDB.EntityFrameworkCore/Query/MongoProjectionMember.cs | New class for MongoDB-specific projection member handling using string-based chains |
| src/MongoDB.EntityFrameworkCore/Query/Expressions/MongoQueryExpression.cs | Updates to use MongoDB-specific projection members |
| src/MongoDB.EntityFrameworkCore/Query/Expressions/MongoProjectionBindingExpression.cs | New MongoDB-specific projection binding expression with ordinal support |
Comments suppressed due to low confidence (1)
src/MongoDB.EntityFrameworkCore/Query/Visitors/MongoProjectionBindingRemovingExpressionVisitor.cs:1
- The comment indicates a known performance issue where the array is retrieved multiple times instead of being cached and reused.
/* Copyright 2023-present MongoDB Inc.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| InternalEntityEntry? entry, | ||
| #pragma warning restore EF1001 // Internal EF Core API usage. | ||
| object entity, | ||
| object? entity, |
There was a problem hiding this comment.
Making these parameters nullable changes the method signature. Ensure all callers are updated to handle nullable parameters appropriately.
Copilot uses AI. Check for mistakes.
| TIncludedEntity relatedEntity, | ||
| INavigation navigation, | ||
| INavigation inverseNavigation, | ||
| INavigation? inverseNavigation, |
There was a problem hiding this comment.
Making these parameters nullable changes the method signature. Ensure all callers are updated to handle nullable parameters appropriately.
Copilot uses AI. Check for mistakes.
|
|
||
| /// <inheritdoc /> | ||
| public override Type Type { get; } | ||
|
|
There was a problem hiding this comment.
The ProjectionType property lacks XML documentation explaining its purpose and how it differs from the Type property.
| /// <summary> | |
| /// Gets the CLR type of the value as projected from the underlying query. | |
| /// This may differ from <see cref="Type"/> if the value is shaped or converted during projection. | |
| /// </summary> |
Copilot uses AI. Check for mistakes.
| var parameterizeArguments = methodCallExpression.Method.IsGenericMethod | ||
| && methodCallExpression.Method.GetGenericMethodDefinition() == | ||
| typeof(Tuple).GetMethods().Single(e => e.Name == nameof(Tuple.Create) | ||
| && e.GetParameters().Length == argumentsCount); |
There was a problem hiding this comment.
The expression typeof(Tuple).GetMethods().Single(...) is executed for every method call. Consider caching this method lookup or using a more efficient comparison.
Copilot uses AI. Check for mistakes.
src/MongoDB.EntityFrameworkCore/Query/Expressions/MongoProjectionBindingExpression.cs
Show resolved
Hide resolved
| : shapedQueryExpression.Type); | ||
|
|
||
| // TODO: Handle select expressions and non-EF shapers more comprehensively | ||
| if (projectedEntityType == null) |
There was a problem hiding this comment.
Ultimately, we hope to remove this entirely, right? Otherwise we may end up having to keep this in sync with what is and isn't supported down the "normal" path. Do you think a TODO is okay for this?
|
Replaced with a more manageable series of PRs. |
2 participants
No description provided.