Add dynamic properties support for any and all operators #1322

gathogojr · 2024-10-07T13:09:00Z

Description

Add dynamic properties support for any and all operators.

Fixes OData/odata.net#3059
Fixes OData/WebApi#770
Fixes partially #1325

The any and all operators appear after a collection-valued property in a $filter expression as follows:

$filter=...CollectionValuedProperty/any(...)
$filter=...CollectionValuedProperty/any(...)

If the collection-valued property is a dynamic property, the parser in ODL defers the actual binding to the upstream libraries. The late binding logic needs to do the following:

Verify that the dynamic property is indeed a collection-based property
Verify that the actual type is of the dynamic property in the Edm model, throw exception if it is not.
Verify that we can bind the any/all delegate to the property
If the parent node of the open property node is dynamic, determine if the property is declared or dynamic on the open type
If the open property is indeed dynamic, check if the parent node's type is open, extract the property from the dynamic properties dictionary with the property name as the key, determine if the property value is IEnumerable and cast it to IEnumerable, or throw an exception if it's not IEnumerable

This pull request implements support the following expressions among many others:

{DeclaredSingleValuedProperty}/{DeclaredCollectionValuedPrimitiveProperty}/any(d:d {op} {value})
{DeclaredSingleValuedProperty}/{DynamicCollectionValuedPrimitiveProperty}/any(d:d {op} {value})
{DeclaredSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/any(d:d/{DeclaredSingleValuedPrimitiveProperty} {op} {value})
{DeclaredSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/any(d:d/{DynamicSingleValuedPrimitiveProperty} {op} {value})
{DeclaredSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/any(d:d/{DeclaredCollectionValuedComplexProperty}/any(e:e/{DeclaredSingleValuedPrimitiveProperty} {op} {value}))
{DeclaredSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/any(d:d/{DeclaredCollectionValuedComplexProperty}/any(e:e/{DynamicSingleValuedPrimitiveProperty} {op} {value}))
{DeclaredSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/any(d:d/{DynamicCollectionValuedComplexProperty}/any(e:e/{DeclaredSingleValuedPrimitiveProperty} {op} {value}))
{DeclaredSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/any(d:d/{DynamicCollectionValuedComplexProperty}/any(e:e/{DynamicSingleValuedPrimitiveProperty} {op} {value}))
{DeclaredSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/any(d:d/{DeclaredSingleValuedPrimitiveProperty} {op} {value})
{DeclaredSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/any(d:d/{DynamicSingleValuedPrimitiveProperty} {op} {value})
{DeclaredSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/any(d:d/{DeclaredCollectionValuedComplexProperty}/any(e:e/{DeclaredSingleValuedPrimitiveProperty} {op} {value}))
{DeclaredSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/any(d:d/{DeclaredCollectionValuedComplexProperty}/any(e:e/{DynamicSingleValuedPrimitiveProperty} {op} {value}))
{DeclaredSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/any(d:d/{DynamicCollectionValuedComplexProperty}/any(e:e/{DeclaredSingleValuedPrimitiveProperty} {op} {value}))
{DeclaredSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/any(d:d/{DynamicCollectionValuedComplexProperty}/any(e:e/{DynamicSingleValuedPrimitiveProperty} {op} {value}))
{DynamicSingleValuedProperty}/{DeclaredCollectionValuedPrimitiveProperty}/any(d:d {op} {value})
{DynamicSingleValuedProperty}/{DynamicCollectionValuedPrimitiveProperty}/any(d:d {op} {value})
{DynamicSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/any(d:d/{DeclaredSingleValuedPrimitiveProperty} {op} {value})
{DynamicSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/any(d:d/{DynamicSingleValuedPrimitiveProperty} {op} {value})
{DynamicSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/any(d:d/{DeclaredCollectionValuedComplexProperty}/any(e:e/{DeclaredSingleValuedPrimitiveProperty} {op} {value}))
{DynamicSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/any(d:d/{DeclaredCollectionValuedComplexProperty}/any(e:e/{DynamicSingleValuedPrimitiveProperty} {op} {value}))
{DynamicSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/any(d:d/{DynamicCollectionValuedComplexProperty}/any(e:e/{DeclaredSingleValuedPrimitiveProperty} {op} {value}))
{DynamicSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/any(d:d/{DynamicCollectionValuedComplexProperty}/any(e:e/{DynamicSingleValuedPrimitiveProperty} {op} {value}))
{DynamicSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/any(d:d/{DeclaredSingleValuedPrimitiveProperty} {op} {value})
{DynamicSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/any(d:d/{DynamicSingleValuedPrimitiveProperty} {op} {value})
{DynamicSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/any(d:d/{DeclaredCollectionValuedComplexProperty}/any(e:e/{DeclaredSingleValuedPrimitiveProperty} {op} {value}))
{DynamicSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/any(d:d/{DeclaredCollectionValuedComplexProperty}/any(e:e/{DynamicSingleValuedPrimitiveProperty} {op} {value}))
{DynamicSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/any(d:d/{DynamicCollectionValuedComplexProperty}/any(e:e/{DeclaredSingleValuedPrimitiveProperty} {op} {value}))
{DynamicSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/any(d:d/{DynamicCollectionValuedComplexProperty}/any(e:e/{DynamicSingleValuedPrimitiveProperty} {op} {value}))
{DeclaredSingleValuedProperty}/{DeclaredCollectionValuedPrimitiveProperty}/all(d:d {op} {value})
{DeclaredSingleValuedProperty}/{DynamicCollectionValuedPrimitiveProperty}/all(d:d {op} {value})
{DeclaredSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/all(d:d/{DeclaredSingleValuedPrimitiveProperty} {op} {value})
{DeclaredSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/all(d:d/{DynamicSingleValuedPrimitiveProperty} {op} {value})
{DeclaredSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/all(d:d/{DeclaredCollectionValuedComplexProperty}/all(e:e/{DeclaredSingleValuedPrimitiveProperty} {op} {value}))
{DeclaredSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/all(d:d/{DeclaredCollectionValuedComplexProperty}/all(e:e/{DynamicSingleValuedPrimitiveProperty} {op} {value}))
{DeclaredSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/all(d:d/{DynamicCollectionValuedComplexProperty}/all(e:e/{DeclaredSingleValuedPrimitiveProperty} {op} {value}))
{DeclaredSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/all(d:d/{DynamicCollectionValuedComplexProperty}/all(e:e/{DynamicSingleValuedPrimitiveProperty} {op} {value}))
{DeclaredSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/all(d:d/{DeclaredSingleValuedPrimitiveProperty} {op} {value})
{DeclaredSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/all(d:d/{DynamicSingleValuedPrimitiveProperty} {op} {value})
{DeclaredSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/all(d:d/{DeclaredCollectionValuedComplexProperty}/all(e:e/{DeclaredSingleValuedPrimitiveProperty} {op} {value}))
{DeclaredSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/all(d:d/{DeclaredCollectionValuedComplexProperty}/all(e:e/{DynamicSingleValuedPrimitiveProperty} {op} {value}))
{DeclaredSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/all(d:d/{DynamicCollectionValuedComplexProperty}/all(e:e/{DeclaredSingleValuedPrimitiveProperty} {op} {value}))
{DeclaredSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/all(d:d/{DynamicCollectionValuedComplexProperty}/all(e:e/{DynamicSingleValuedPrimitiveProperty} {op} {value}))
{DynamicSingleValuedProperty}/{DeclaredCollectionValuedPrimitiveProperty}/all(d:d {op} {value})
{DynamicSingleValuedProperty}/{DynamicCollectionValuedPrimitiveProperty}/all(d:d {op} {value})
{DynamicSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/all(d:d/{DeclaredSingleValuedPrimitiveProperty} {op} {value})
{DynamicSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/all(d:d/{DynamicSingleValuedPrimitiveProperty} {op} {value})
{DynamicSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/all(d:d/{DeclaredCollectionValuedComplexProperty}/all(e:e/{DeclaredSingleValuedPrimitiveProperty} {op} {value}))
{DynamicSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/all(d:d/{DeclaredCollectionValuedComplexProperty}/all(e:e/{DynamicSingleValuedPrimitiveProperty} {op} {value}))
{DynamicSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/all(d:d/{DynamicCollectionValuedComplexProperty}/all(e:e/{DeclaredSingleValuedPrimitiveProperty} {op} {value}))
{DynamicSingleValuedProperty}/{DeclaredCollectionValuedComplexProperty}/all(d:d/{DynamicCollectionValuedComplexProperty}/all(e:e/{DynamicSingleValuedPrimitiveProperty} {op} {value}))
{DynamicSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/all(d:d/{DeclaredSingleValuedPrimitiveProperty} {op} {value})
{DynamicSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/all(d:d/{DynamicSingleValuedPrimitiveProperty} {op} {value})
{DynamicSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/all(d:d/{DeclaredCollectionValuedComplexProperty}/all(e:e/{DeclaredSingleValuedPrimitiveProperty} {op} {value}))
{DynamicSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/all(d:d/{DeclaredCollectionValuedComplexProperty}/all(e:e/{DynamicSingleValuedPrimitiveProperty} {op} {value}))
{DynamicSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/all(d:d/{DynamicCollectionValuedComplexProperty}/all(e:e/{DeclaredSingleValuedPrimitiveProperty} {op} {value}))
{DynamicSingleValuedProperty}/{DynamicCollectionValuedComplexProperty}/all(d:d/{DynamicCollectionValuedComplexProperty}/all(e:e/{DynamicSingleValuedPrimitiveProperty} {op} {value}))

Additional Details

This pull request shares some of the logic introduced in Add dynamic single-valued property support for filter expressions #1330. Where necessary, refactor was done to make sharing of code possible.
The tests are deliberated "shaped" to test out the many possible and diverse scenarios, including non-happy cases. The test data has a deeply nested structure. This is deliberate. It helps guarantee that the expected scenarios are anticipated and handled.
The tests also include particular scenarios described by the customer to confirm that they achieve the state objectives.

Port change to 8.x and 7.x

src/Microsoft.AspNetCore.OData/Query/Expressions/QueryBinderContext.cs

test/Microsoft.AspNetCore.OData.E2E.Tests/DollarFilter/DollarFilterDataSource.cs

habbes · 2024-11-04T14:44:45Z

src/Microsoft.AspNetCore.OData/Query/Expressions/QueryBinder.cs

@@ -697,7 +735,7 @@ public virtual Expression BindAllNode(AllNode allNode, QueryBinderContext contex
    {
        CheckArgumentNull(allNode, context);

-       // context.EnterLambdaScope();
+        // context.EnterLambdaScope();


Do we still need this?

@habbes I didn't add the line/comment. It existed. It was only formatted since it had an unnecessary leading space. I don't know if I should remove it altogether...

WanjohiSammy · 2024-11-07T06:07:35Z

src/Microsoft.AspNetCore.OData/Query/Expressions/QueryBinder.cs

@@ -1487,42 +1546,42 @@ private Expression BindDynamicPropertyAccessExpression(SingleValueOpenPropertyAc
    /// <summary>
    /// Creates an expression for retrieving a dynamic property from the dynamic properties container property.
    /// </summary>
-    /// <param name="dynamicPropertiesContainerExpr">The dynamic properties container property access expression.</param>
+    /// <param name="containerPropertyAccessExpr">The dynamic properties container property access expression.</param>


Since you are passing containerPropertyAccessExpr and not dynamicPropertiesContainerExpr, should you also change the doc string for this parameter as well

Suggested change

/// <param name="containerPropertyAccessExpr">The dynamic properties container property access expression.</param>

/// <param name="containerPropertyAccessExpr">The properties container property access expression.</param>

Maybe The container property access expression. Otherwise, The properties container property access expression would sound inaccurate given the explanation I have provided here #1322 (comment)
"The dynamic properties container property access expression" is captures the definition accurately too

WanjohiSammy · 2024-11-07T06:09:43Z

src/Microsoft.AspNetCore.OData/Query/Expressions/QueryBinder.cs

            Expression.Constant(propertyName));

        // ContainsKey method
        MethodCallExpression containsKeyExpression = Expression.Call(
-            dynamicPropertiesContainerExpr,
-            dynamicPropertiesContainerExpr.Type.GetMethod("ContainsKey"),
+            containerPropertyAccessExpr,


What is the different between dynamicPropertiesContainerExpr and containerPropertyAccessExpr. Or do they mean the same and only the name had changed.

@WanjohiSammy I just thought I should harmonize the name with how it's named elsewhere. Otherwise, both can be used to refer to the property of type IDictionary<string, object> (or any of it's implementing types) that we use to store dynamic properties. We can refer to it as the "dynamic properties container property" (mouthful/with 'property' repeated) or just "container property" (shorter).

gathogojr requested review from marabooy, KenitoInc, habbes, mikepizzo, xuzhg, WanjohiSammy, corranrogue9, emmanuel-karanja and DButoyez October 8, 2024 08:48

marabooy previously approved these changes Oct 8, 2024

View reviewed changes

src/Microsoft.AspNetCore.OData/Query/Expressions/QueryBinderContext.cs Show resolved Hide resolved

gathogojr dismissed marabooy’s stale review via c6a3c87 October 11, 2024 06:30

gathogojr force-pushed the feature/dynamic-property-support-for-any-all-in branch 4 times, most recently from a6c74da to f32d433 Compare November 4, 2024 07:58

gathogojr changed the title ~~Add dynamic properties support for any, all and in operators~~ Add dynamic properties support for any and all operators Nov 4, 2024

gathogojr commented Nov 4, 2024

View reviewed changes

test/Microsoft.AspNetCore.OData.E2E.Tests/DollarFilter/DollarFilterDataSource.cs Outdated Show resolved Hide resolved

habbes reviewed Nov 4, 2024

View reviewed changes

gathogojr force-pushed the feature/dynamic-property-support-for-any-all-in branch from f32d433 to 2400b3c Compare November 5, 2024 11:37

Add dynamic properties support for any, all and in operators

be7b47d

gathogojr force-pushed the feature/dynamic-property-support-for-any-all-in branch from 2400b3c to be7b47d Compare November 5, 2024 11:42

marabooy previously approved these changes Nov 5, 2024

View reviewed changes

WanjohiSammy reviewed Nov 7, 2024

View reviewed changes

habbes previously approved these changes Nov 7, 2024

View reviewed changes

WanjohiSammy previously approved these changes Nov 7, 2024

View reviewed changes

Address review feedback

287b8e0

gathogojr dismissed stale reviews from WanjohiSammy, habbes, and marabooy via 287b8e0 November 7, 2024 12:14

habbes approved these changes Nov 7, 2024

View reviewed changes

marabooy approved these changes Nov 7, 2024

View reviewed changes

WanjohiSammy approved these changes Nov 7, 2024

View reviewed changes

gathogojr merged commit de1cdd2 into OData:main Nov 7, 2024
4 checks passed

gathogojr deleted the feature/dynamic-property-support-for-any-all-in branch November 7, 2024 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dynamic properties support for any and all operators #1322

Add dynamic properties support for any and all operators #1322

gathogojr commented Oct 7, 2024 •

edited

Loading

habbes Nov 4, 2024

gathogojr Nov 4, 2024

WanjohiSammy Nov 7, 2024

gathogojr Nov 7, 2024 •

edited

Loading

WanjohiSammy Nov 7, 2024

gathogojr Nov 7, 2024

	/// <param name="containerPropertyAccessExpr">The dynamic properties container property access expression.</param>
	/// <param name="containerPropertyAccessExpr">The properties container property access expression.</param>

Add dynamic properties support for any and all operators #1322

Add dynamic properties support for any and all operators #1322

Conversation

gathogojr commented Oct 7, 2024 • edited Loading

Description

Additional Details

habbes Nov 4, 2024

Choose a reason for hiding this comment

gathogojr Nov 4, 2024

Choose a reason for hiding this comment

WanjohiSammy Nov 7, 2024

Choose a reason for hiding this comment

gathogojr Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

WanjohiSammy Nov 7, 2024

Choose a reason for hiding this comment

gathogojr Nov 7, 2024

Choose a reason for hiding this comment

gathogojr commented Oct 7, 2024 •

edited

Loading

gathogojr Nov 7, 2024 •

edited

Loading