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

Cosmos: Properly handle nested owned types on owned collections #18461

Merged
merged 1 commit into from
Oct 23, 2019

Conversation

AndriySvyryd
Copy link
Member

Consolidate Enumerable methods info
Print the declaring type name for static method calls

Fixes #18265

@smitpatel
Copy link
Contributor

smitpatel commented Oct 22, 2019

                shaperExpression = unaryExpression.Operand as EntityShaperExpression;

This should also check NodeType of UnaryExpression. (Assuming only convert is allowed)
#Resolved


Refers to: src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs:193 in 4d082f8. [](commit_id = 4d082f8, deletion_comment = False)

@smitpatel
Copy link
Contributor

smitpatel commented Oct 22, 2019

            case ProjectionBindingExpression innerProjectionBindingExpression:

This assumes that projectionBinding is always mapped by Index. Is that always true? Can it not be projection mapping? #WontFix


Refers to: src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs:195 in 4d082f8. [](commit_id = 4d082f8, deletion_comment = True)

@smitpatel
Copy link
Contributor

smitpatel commented Oct 22, 2019

                innerEntityProjection = (EntityProjectionExpression)((UnaryExpression)unaryExpression.Operand).Operand;

Should there be type safety check? Or we have invariant in pipeline for this? #WontFix


Refers to: src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs:201 in 4d082f8. [](commit_id = 4d082f8, deletion_comment = True)

@smitpatel
Copy link
Contributor

smitpatel commented Oct 22, 2019

                        else if (projectionExpression is UnaryExpression convertExpression)

Also check nodeType?
#Resolved


Refers to: src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.CosmosProjectionBindingRemovingExpressionVisitor.cs:101 in 4d082f8. [](commit_id = 4d082f8, deletion_comment = True)

@smitpatel
Copy link
Contributor

smitpatel commented Oct 22, 2019

                        var projectionExpression = ((UnaryExpression)binaryExpression.Right).Operand;

Another invariant? #Resolved


Refers to: src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.CosmosProjectionBindingRemovingExpressionVisitor.cs:91 in 4d082f8. [](commit_id = 4d082f8, deletion_comment = False)

@AndriySvyryd
Copy link
Member Author

            case ProjectionBindingExpression innerProjectionBindingExpression:

For now Member would only be called in an Index projectionBinding, I think we would only need projection mapping if we allow querying unmapped properties unless you can think of a different case off top of your head.


In reply to: 545169128 [](ancestors = 545169128)


Refers to: src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs:195 in 4d082f8. [](commit_id = 4d082f8, deletion_comment = True)

@AndriySvyryd
Copy link
Member Author

                innerEntityProjection = (EntityProjectionExpression)((UnaryExpression)unaryExpression.Operand).Operand;

This is an invariant - we always need to cast the projection to ValueBuffer and we only support Member on entity projection. I guess we could throw a better exception message if it's not, but I would need to think to come up with a test case.


In reply to: 545171073 [](ancestors = 545171073)


Refers to: src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs:201 in 4d082f8. [](commit_id = 4d082f8, deletion_comment = True)

if (method.DeclaringType == typeof(Enumerable)
|| method.DeclaringType == typeof(EnumerableExtensions))
{
if (method.Name == nameof(Enumerable.Select)
Copy link
Contributor

Choose a reason for hiding this comment

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

Merge Ifs.
Remove EnumerableExtensions

Consolidate Enumerable methods info
Print the declaring type name for static method calls

Fixes #18265
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.

Cosmos: querying an entity with a nested owned collection throws
2 participants