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

Cosmos: Fix reference nested owned entities in queries #16276

Merged
merged 2 commits into from Jun 27, 2019
Merged

Cosmos: Fix reference nested owned entities in queries #16276

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6bfeedc
Fix reference nested owned entities in queries.
AndriySvyryd Jun 24, 2019
95c1c54
asfasf
AndriySvyryd Jun 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 10 additions & 6 deletions src/EFCore.Cosmos/Query/Pipeline/CosmosProjectionBindingExpressionVisitor.cs
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@
using System.Linq.Expressions;
using System.Reflection;
using Microsoft.EntityFrameworkCore.Query;
using Microsoft.EntityFrameworkCore.Query.Internal;
using Microsoft.EntityFrameworkCore.Query.Pipeline;
using Microsoft.EntityFrameworkCore.Storage;

Expand Down Expand Up @@ -135,25 +136,28 @@ protected override Expression VisitExtension(Expression extensionExpression)
{
if (extensionExpression is EntityShaperExpression entityShaperExpression)
{
VerifySelectExpression(entityShaperExpression.ValueBufferExpression);
var projectionBindingExpression = (ProjectionBindingExpression)entityShaperExpression.ValueBufferExpression;
VerifySelectExpression(projectionBindingExpression);

if (_clientEval)
{
var entityProjection = (EntityProjectionExpression)_selectExpression.GetMappedProjection(
entityShaperExpression.ValueBufferExpression.ProjectionMember);
projectionBindingExpression.ProjectionMember);

return entityShaperExpression.Update(
new ProjectionBindingExpression(
_selectExpression, _selectExpression.AddToProjection(entityProjection), typeof(ValueBuffer)));
_selectExpression, _selectExpression.AddToProjection(entityProjection), typeof(ValueBuffer)),
entityShaperExpression.NestedEntities);
}
else
{
_projectionMapping[_projectionMembers.Peek()]
= _selectExpression.GetMappedProjection(
entityShaperExpression.ValueBufferExpression.ProjectionMember);
projectionBindingExpression.ProjectionMember);

return entityShaperExpression.Update(
new ProjectionBindingExpression(_selectExpression, _projectionMembers.Peek(), typeof(ValueBuffer)));
new ProjectionBindingExpression(_selectExpression, _projectionMembers.Peek(), typeof(ValueBuffer)),
entityShaperExpression.NestedEntities);
}
}

Expand All @@ -162,7 +166,7 @@ protected override Expression VisitExtension(Expression extensionExpression)
// return _clientEval ? base.VisitExtension(includeExpression) : null;
//}

throw new InvalidOperationException();
throw new InvalidOperationException(new ExpressionPrinter().Print(extensionExpression));
}

protected override Expression VisitNew(NewExpression newExpression)
Expand Down
124 changes: 109 additions & 15 deletions src/EFCore.Cosmos/Query/Pipeline/CosmosShapedQueryCompilingExpressionVisitor.cs
View file
Open in desktop
Expand Up @@ -4,12 +4,14 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Linq.Expressions;
using System.Reflection;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.EntityFrameworkCore.Cosmos.Metadata.Conventions;
using Microsoft.EntityFrameworkCore.Cosmos.Metadata.Internal;
using Microsoft.EntityFrameworkCore.Diagnostics;
using Microsoft.EntityFrameworkCore.Internal;
using Microsoft.EntityFrameworkCore.Metadata;
Expand Down Expand Up @@ -47,13 +49,12 @@ public class CosmosShapedQueryCompilingExpressionVisitor : ShapedQueryCompilingE

protected override Expression VisitShapedQueryExpression(ShapedQueryExpression shapedQueryExpression)
{
var shaperBody = InjectEntityMaterializer(shapedQueryExpression.ShaperExpression);
var selectExpression = (SelectExpression)shapedQueryExpression.QueryExpression;
selectExpression.ApplyProjection();
var jObjectParameter = Expression.Parameter(typeof(JObject), "jObject");

shaperBody = new CosmosProjectionBindingRemovingExpressionVisitor(selectExpression, jObjectParameter)
.Visit(shaperBody);
var jObjectParameter = Expression.Parameter(typeof(JObject), "jObject");
var shaperBody = new CosmosProjectionBindingRemovingExpressionVisitor(selectExpression, jObjectParameter, this)
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep injecting materializer and binding removal separate?
We can have another visitor which inject nested entityshapers

Copy link
Member Author

Choose a reason for hiding this comment

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

That was my original design, but we need to add the jObject variable before each materializer is injected.

.Visit(shapedQueryExpression.ShaperExpression);

var shaperLambda = Expression.Lambda(
shaperBody,
Expand All @@ -75,8 +76,6 @@ protected override Expression VisitShapedQueryExpression(ShapedQueryExpression s

private class CosmosProjectionBindingRemovingExpressionVisitor : ExpressionVisitor
{
private SelectExpression _selectExpression;
private readonly ParameterExpression _jObjectParameter;
private static readonly MethodInfo _getItemMethodInfo
= typeof(JObject).GetTypeInfo().GetRuntimeProperties()
.Single(pi => pi.Name == "Item" && pi.GetIndexParameters()[0].ParameterType == typeof(string))
Expand All @@ -88,14 +87,24 @@ private class CosmosProjectionBindingRemovingExpressionVisitor : ExpressionVisit
= typeof(CosmosProjectionBindingRemovingExpressionVisitor).GetTypeInfo().GetRuntimeMethods()
.Single(mi => mi.Name == nameof(IsNull));

private readonly SelectExpression _selectExpression;
private ParameterExpression _jObjectParameter;
private readonly CosmosShapedQueryCompilingExpressionVisitor _shapedQueryCompilingExpressionVisitor;

private readonly IDictionary<ProjectionBindingExpression, Expression> _valueBufferBindings
= new Dictionary<ProjectionBindingExpression, Expression>();
private readonly IDictionary<ParameterExpression, Expression> _materializationContextBindings
= new Dictionary<ParameterExpression, Expression>();
private int _currentEntityIndex;

public CosmosProjectionBindingRemovingExpressionVisitor(
SelectExpression selectExpression, ParameterExpression jObjectParameter)
SelectExpression selectExpression,
ParameterExpression jObjectParameter,
CosmosShapedQueryCompilingExpressionVisitor shapedQueryCompilingExpressionVisitor)
{
_selectExpression = selectExpression;
_jObjectParameter = jObjectParameter;
_shapedQueryCompilingExpressionVisitor = shapedQueryCompilingExpressionVisitor;
}

protected override Expression VisitBinary(BinaryExpression binaryExpression)
Expand All @@ -105,13 +114,8 @@ protected override Expression VisitBinary(BinaryExpression binaryExpression)
&& parameterExpression.Type == typeof(MaterializationContext))
{
var newExpression = (NewExpression)binaryExpression.Right;
var projectionBindingExpression = (ProjectionBindingExpression)newExpression.Arguments[0];
var projectionIndex = (int)GetProjectionIndex(projectionBindingExpression);
var projection = _selectExpression.Projection[projectionIndex];

_materializationContextBindings[parameterExpression] = Expression.Convert(
CreateReadJTokenExpression(_jObjectParameter, projection.Alias),
typeof(JObject));
_materializationContextBindings[parameterExpression] = _jObjectParameter;
Copy link
Member

Choose a reason for hiding this comment

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

Does this work when you have projected out multiple entities in projection

Copy link
Member Author

Choose a reason for hiding this comment

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

It should. Is there a test for that?


var updatedExpression = Expression.New(newExpression.Constructor,
Expression.Constant(ValueBuffer.Empty),
Expand All @@ -128,7 +132,6 @@ protected override Expression VisitBinary(BinaryExpression binaryExpression)
return memberExpression.Assign(Visit(binaryExpression.Right));
}


return base.VisitBinary(binaryExpression);
}

Expand Down Expand Up @@ -181,9 +184,100 @@ protected override Expression VisitExtension(Expression extensionExpression)
projectionBindingExpression.Type);
}

if (extensionExpression is EntityShaperExpression shaperExpression)
{
_currentEntityIndex++;

var jObjectVariable = Expression.Variable(typeof(JObject),
"jObject" + _currentEntityIndex);
var variables = new List<ParameterExpression> { jObjectVariable };

var expressions = new List<Expression>();

if (shaperExpression.ParentNavigation == null)
{
var projectionIndex = (int)GetProjectionIndex((ProjectionBindingExpression)shaperExpression.ValueBufferExpression);
var projection = _selectExpression.Projection[projectionIndex];

expressions.Add(
Expression.Assign(
jObjectVariable,
Expression.TypeAs(
CreateReadJTokenExpression(_jObjectParameter, projection.Alias),
typeof(JObject))));

shaperExpression = shaperExpression.Update(
shaperExpression.ValueBufferExpression,
GetNestedShapers(shaperExpression.EntityType, shaperExpression.ValueBufferExpression));
}
else
{
var methodCallExpression = (MethodCallExpression)shaperExpression.ValueBufferExpression;
Debug.Assert(methodCallExpression.Method.IsGenericMethod
&& methodCallExpression.Method.GetGenericMethodDefinition() == EntityMaterializerSource.TryReadValueMethod);

var navigation = (INavigation)((ConstantExpression)methodCallExpression.Arguments[2]).Value;

expressions.Add(
Expression.Assign(
jObjectVariable,
Expression.TypeAs(
CreateReadJTokenExpression(_jObjectParameter, navigation.GetTargetType().GetCosmosContainingPropertyName()),
typeof(JObject))));
}

var parentJObject = _jObjectParameter;
_jObjectParameter = jObjectVariable;
expressions.Add(Expression.Condition(
Expression.Equal(jObjectVariable, Expression.Constant(null, jObjectVariable.Type)),
Expression.Constant(null, shaperExpression.Type),
Visit(_shapedQueryCompilingExpressionVisitor.InjectEntityMaterializer(shaperExpression))));
_jObjectParameter = parentJObject;

return Expression.Block(
shaperExpression.Type,
variables,
expressions);
}

if (extensionExpression is CollectionShaperExpression collectionShaperExpression)
{
throw new NotImplementedException();
}

return base.VisitExtension(extensionExpression);
}

private List<EntityShaperExpression> GetNestedShapers(IEntityType entityType, Expression valueBufferExpression)
{
var nestedEntities = new List<EntityShaperExpression>();
foreach (var ownedNavigation in entityType.GetNavigations().Concat(entityType.GetDerivedNavigations()))
{
var fk = ownedNavigation.ForeignKey;
if (!fk.IsOwnership
|| ownedNavigation.IsDependentToPrincipal()
|| fk.DeclaringEntityType.IsDocumentRoot())
{
continue;
}

var targetType = ownedNavigation.GetTargetType();

var nestedValueBufferExpression = _shapedQueryCompilingExpressionVisitor.CreateReadValueExpression(
valueBufferExpression,
valueBufferExpression.Type,
ownedNavigation.GetIndex(),
ownedNavigation);

var nestedShaper = new EntityShaperExpression(
targetType, nestedValueBufferExpression, nullable: true, ownedNavigation,
GetNestedShapers(targetType, nestedValueBufferExpression));
nestedEntities.Add(nestedShaper);
}

return nestedEntities.Count == 0 ? null : nestedEntities;
}

private object GetProjectionIndex(ProjectionBindingExpression projectionBindingExpression)
{
return projectionBindingExpression.ProjectionMember != null
Expand Down Expand Up @@ -216,7 +310,7 @@ private static Expression CreateReadJTokenExpression(Expression jObjectExpressio
return CreateGetStoreValueExpression(jObjectExpression, storeName, property.GetTypeMapping(), property.ClrType);
}

public static Expression CreateGetStoreValueExpression(
private static Expression CreateGetStoreValueExpression(
Expression jObjectExpression,
string storeName,
CoreTypeMapping typeMapping,
Expand Down
7 changes: 4 additions & 3 deletions src/EFCore.Cosmos/Query/Pipeline/CosmosSqlTranslatingExpressionVisitor.cs
View file
Open in desktop
Expand Up @@ -93,10 +93,11 @@ private SqlExpression BindProperty(Expression source, string propertyName)
throw new InvalidOperationException();
}

private SqlExpression BindProperty(EntityShaperExpression entityShaper, IProperty property)
private SqlExpression BindProperty(EntityShaperExpression entityShaperExpression, IProperty property)
{
return ((SelectExpression)entityShaper.ValueBufferExpression.QueryExpression)
.BindProperty(entityShaper.ValueBufferExpression, property);
var projectionBindingExpression = (ProjectionBindingExpression)entityShaperExpression.ValueBufferExpression;
return ((SelectExpression)projectionBindingExpression.QueryExpression)
.BindProperty(projectionBindingExpression, property);
}

protected override Expression VisitMethodCall(MethodCallExpression methodCallExpression)
Expand Down
17 changes: 9 additions & 8 deletions src/EFCore.InMemory/Query/Pipeline/InMemoryProjectionBindingExpressionVisitor.cs
View file
Open in desktop
Expand Up @@ -67,13 +67,13 @@ public override Expression Visit(Expression expression)
&& source is EntityShaperExpression entityShaperExpression
&& entityShaperExpression.EntityType.GetProperties().Count() == newArrayExpression.Expressions.Count)
{
VerifyQueryExpression(entityShaperExpression.ValueBufferExpression);
var projectionBindingExpression = (ProjectionBindingExpression)entityShaperExpression.ValueBufferExpression;
VerifyQueryExpression(projectionBindingExpression);

_projectionMapping[_projectionMembers.Peek()]
= _queryExpression.GetMappedProjection(
entityShaperExpression.ValueBufferExpression.ProjectionMember);
= _queryExpression.GetMappedProjection(projectionBindingExpression.ProjectionMember);

return new EntityValuesExpression(entityShaperExpression.EntityType, entityShaperExpression.ValueBufferExpression);
return new EntityValuesExpression(entityShaperExpression.EntityType, projectionBindingExpression);
}

var translation = _expressionTranslatingExpressionVisitor.Translate(_queryExpression, expression);
Expand All @@ -90,14 +90,15 @@ protected override Expression VisitExtension(Expression extensionExpression)
{
if (extensionExpression is EntityShaperExpression entityShaperExpression)
{
VerifyQueryExpression(entityShaperExpression.ValueBufferExpression);
var projectionBindingExpression = (ProjectionBindingExpression)entityShaperExpression.ValueBufferExpression;
VerifyQueryExpression(projectionBindingExpression);

_projectionMapping[_projectionMembers.Peek()]
= _queryExpression.GetMappedProjection(
entityShaperExpression.ValueBufferExpression.ProjectionMember);
= _queryExpression.GetMappedProjection(projectionBindingExpression.ProjectionMember);

return entityShaperExpression.Update(
new ProjectionBindingExpression(_queryExpression, _projectionMembers.Peek(), typeof(ValueBuffer)));
new ProjectionBindingExpression(_queryExpression, _projectionMembers.Peek(), typeof(ValueBuffer)),
entityShaperExpression.NestedEntities);
}

throw new InvalidOperationException();
Expand Down
1 change: 0 additions & 1 deletion src/EFCore.InMemory/Query/Pipeline/InMemoryShapedQueryExpressionVisitor.cs
View file
Open in desktop
Expand Up @@ -12,7 +12,6 @@
using Microsoft.EntityFrameworkCore.Diagnostics;
using Microsoft.EntityFrameworkCore.InMemory.Query.Internal;
using Microsoft.EntityFrameworkCore.Metadata;
using Microsoft.EntityFrameworkCore.Metadata.Internal;
using Microsoft.EntityFrameworkCore.Query;
using Microsoft.EntityFrameworkCore.Query.Internal;
using Microsoft.EntityFrameworkCore.Query.Pipeline;
Expand Down
14 changes: 8 additions & 6 deletions src/EFCore.Relational/Query/Pipeline/RelationalProjectionBindingExpressionVisitor.cs
View file
Open in desktop
Expand Up @@ -143,24 +143,26 @@ protected override Expression VisitExtension(Expression extensionExpression)
{
if (extensionExpression is EntityShaperExpression entityShaperExpression)
{
VerifySelectExpression(entityShaperExpression.ValueBufferExpression);
var projectionBindingExpression = (ProjectionBindingExpression)entityShaperExpression.ValueBufferExpression;
VerifySelectExpression(projectionBindingExpression);

if (_clientEval)
{
var entityProjection = (EntityProjectionExpression)_selectExpression.GetMappedProjection(
entityShaperExpression.ValueBufferExpression.ProjectionMember);
projectionBindingExpression.ProjectionMember);

return entityShaperExpression.Update(
new ProjectionBindingExpression(_selectExpression, _selectExpression.AddToProjection(entityProjection)));
new ProjectionBindingExpression(_selectExpression, _selectExpression.AddToProjection(entityProjection)),
entityShaperExpression.NestedEntities);
}
else
{
_projectionMapping[_projectionMembers.Peek()]
= _selectExpression.GetMappedProjection(
entityShaperExpression.ValueBufferExpression.ProjectionMember);
= _selectExpression.GetMappedProjection(projectionBindingExpression.ProjectionMember);

return entityShaperExpression.Update(
new ProjectionBindingExpression(_selectExpression, _projectionMembers.Peek(), typeof(ValueBuffer)));
new ProjectionBindingExpression(_selectExpression, _projectionMembers.Peek(), typeof(ValueBuffer)),
entityShaperExpression.NestedEntities);
}
}

Expand Down