Add dynamic single-valued property support for filter expressions #1330
Conversation
1ff9838 to
3a14c42
Compare
3a14c42 to
810cfba
Compare
| Expression getDeclaredPropertyValueExpr = Expression.Convert( | ||
| Expression.Call(getPropertyExpr, "GetValue", Type.EmptyTypes, sourceExpr), | ||
| typeof(object)); |
There was a problem hiding this comment.
Does this convert the result of PropertyInfo.GetValue() into an object? Isn't the return type of GetValue() already object?
There was a problem hiding this comment.
@habbes I'll check if the Convert is redundant here
| if (openNode.Source is SingleValueOpenPropertyAccessNode openNodeParent) | ||
| { | ||
| sourceExpr = BindNestedDynamicPropertyAccessExpression(openNodeParent, context); | ||
| } | ||
|
|
||
| if (!(openNode.Source is SingleValueOpenPropertyAccessNode)) | ||
| { | ||
| return BindDynamicPropertyAccessExpression(openNode, context); | ||
| } | ||
| else | ||
| { |
There was a problem hiding this comment.
I think this if statements are a bit confusing or partially redundant. The second if statement seems to be the opposite of the first, and so could probably be handled by an else instead of writing the negated if statement. The else block part of the second if statement could be moved back indentation-wise and the else wrapper removed since the if statement has a return statement:
| if (openNode.Source is SingleValueOpenPropertyAccessNode openNodeParent) | |
| { | |
| sourceExpr = BindNestedDynamicPropertyAccessExpression(openNodeParent, context); | |
| } | |
| if (!(openNode.Source is SingleValueOpenPropertyAccessNode)) | |
| { | |
| return BindDynamicPropertyAccessExpression(openNode, context); | |
| } | |
| else | |
| { | |
| if (openNode.Source is SingleValueOpenPropertyAccessNode openNodeParent) | |
| { | |
| sourceExpr = BindNestedDynamicPropertyAccessExpression(openNodeParent, context); | |
| } | |
| else | |
| { | |
| return BindDynamicPropertyAccessExpression(openNode, context); | |
| } | |
| // rest of the code... |
There was a problem hiding this comment.
@habbes I'll try the refactor that you suggested but let me first explain the logic in this method.
Consider either of these two expressions:
Products?$filter=DynamicSegment/NestedDynamicSegment1/NestedDynamicSegment2/NestedDynamicSegment3Products?$filter=DeclaredSegment/NestedDynamicSegment1/NestedDynamicSegment2/NestedDynamicSegment3
In both cases, the query binder will first attempt to bind NestedDynamicSegment3 to it's source (i.e. NestedDynamicSegment2). But we don't know the type for NestedDynamicSegment2 because it's also an open node. It's the same challenge if we attempt to bind NestedDynamicSegment2 to it's source (i.e. NestedDynamicSegment3).
For this reason, what this method first does is recursively call itself until we find a node whose source is not an open node.
Meaning recursion will stop if we find a node whose source is not SingleValueOpenPropertyAccessNode.
When we come across such a node, we bind it to its source which would either be a SingleValueNode or a ResourceRangeVariableReferenceNode.
Binding happens from the BindDynamicPropertyAccessExpression method that returns to us the expression that retrieves the dynamic property at the base of the path expression. The expression is the equivalent of:
{productInstance}.containerDict.Item["DynamicSegment"]for the first expression, and
{productInstance}.DeclaredSegment.containerDict.Item["NestedDynamicSegment"]for the second expression.
We then extend that expressions such that it can retrieve NestedDynamicSegment2 and NestedDynamicSegment2 as either declared or dynamic properties of the subsequent segment.
|
|
||
| if (!(openNode.Source is SingleValueOpenPropertyAccessNode)) | ||
| { | ||
| return BindDynamicPropertyAccessExpression(openNode, context); |
There was a problem hiding this comment.
this is a DeclaredProperty/DynamicProperty scenario?
There was a problem hiding this comment.
No. This is DynamicProperty/NestedDeclaredProperty and DynamicProperty/NestedDynamicProperty scenarios.
Note however that based on the output of the parser, both NestedDeclaredProperty and NestedDynamicProperty will be dynamic! To the parser, everything that follows a dynamic segment is a dynamic segment. We can't know at parse time that nested segment might resolve into a declared segment.
| Expression.Constant(openNode.Name)); | ||
|
|
||
| // Get declared property value | ||
| Expression getDeclaredPropertyValueExpr = Expression.Convert( |
There was a problem hiding this comment.
Just for my own sanity check, this expression is the equivalent of (object)DynamicParentProperty.GetType().GetProperty("DeclareProperty").GetValue(DynamicParentPropertyInstance)?
There was a problem hiding this comment.
@gathogojr Think of it this way...
The call to BindDynamicPropertyAccessExpression resolves into something like:
var dynamicPropertyValue = containerDict.Item["{dynamicPropertyName}"]Then, the statements in this else block do something like:
Type dynamicPropertyType = dynamicPropertyValue.GetType();
PropertyInfo propertyInfo = dynamicPropertyType.GetProperty("{nestedPropertyName}");
/* ... Check that propertyInfo is not null ... */
object nestedDeclaredPropertyValue = propertyInfo.GetValue(dynamicPropertyValue);The nestedDeclaredPropertyValue is returned if indeed the nested property is a declared property.
The logic in scenario 2 on the other hand does the equivalent of:
IEdmTypeReference edmTypeReference = context.Model.GetEdmTypeReference(dynamicPropertyType);
PropertyInfo containerPropertyInfo = GetDynamicPropertyContainer(edmTypeReference, nodeTypeKind, context.Model);
/* ... Check that containerPropertyInfo is not null ... */
Dictionary<string, object> nestedContainerDict = containerPropertyInfo.GetValue(dynamicPropertyValue);
object nestedDynamicPropertyValue = nestedContainerDict.Item["{nestedPropertyName}"]The nestedDynamicPropertyValue is returned if indeed the nested property is a dynamic property property.
cd9ecae to
bef2002
Compare
bef2002 to
17fc5d6
Compare
Add dynamic single-valued property support for filter expressions
Scenarios: