Cosmos: Complex properties binding #37400

JoasE · 2025-12-18T20:21:29Z

Adds a CosmosStructuralTypeMaterializerSource and overrides AddStructuralTypeInitialization in CosmosShapedQueryCompilingExpressionVisitor to generate materialization expressions for complex properties
Leverages Nullable<>.HasValue for nullable value types in to support value types without equals operator
Part of: #31253

Copilot

Pull request overview

This pull request implements binding logic for complex properties in the Cosmos DB provider, enabling proper materialization of complex types from JSON documents. The implementation introduces a custom CosmosStructuralTypeMaterializerSource that defers complex property handling to allow nested materialization expressions to be generated during query compilation.

Key changes:

Introduces CosmosStructuralTypeMaterializerSource to control when complex properties are materialized during shaping
Implements nested complex property and collection binding in CosmosShapedQueryCompilingExpressionVisitor
Enhances null-handling logic in CosmosProjectionBindingExpressionVisitor for nullable value types

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
`src/EFCore.Cosmos/Query/Internal/CosmosStructuralTypeMaterializerSource.cs`	New class that overrides complex type materialization behavior to enable deferred binding
`src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.cs`	Adds logic to generate materialization expressions for complex properties and collections from JObject/JArray
`src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.CosmosProjectionBindingRemovingExpressionVisitorBase.cs`	Exposes methods and fields needed for complex property binding; adds handling for ComplexPropertyBindingExpression
`src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs`	Improves null-safety checks for nullable value types and refactors member expression updates
`src/EFCore.Cosmos/Extensions/CosmosServiceCollectionExtensions.cs`	Registers the new CosmosStructuralTypeMaterializerSource service
`test/EFCore.Cosmos.FunctionalTests/CosmosComplexTypesTrackingTest.cs`	Uncomments assertions that now pass with complex property binding implemented
`test/EFCore.Cosmos.FunctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesProjectionCosmosTest.cs`	New test class with Cosmos-specific overrides for complex property projection tests
`test/EFCore.Cosmos.FunctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesCosmosFixture.cs`	Test fixture configuration for complex properties tests in Cosmos

...unctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesProjectionCosmosTest.cs

src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs

src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.cs

...unctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesProjectionCosmosTest.cs

src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.cs

...unctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesProjectionCosmosTest.cs

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

...unctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesProjectionCosmosTest.cs

src/EFCore.Cosmos/Query/Internal/CosmosStructuralTypeMaterializerSource.cs

...hapedQueryCompilingExpressionVisitor.CosmosProjectionBindingRemovingExpressionVisitorBase.cs

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.

roji

Hey @JoasE, thanks for submitting this and sorry I haven't gotten around to reviewing it earlier.

I admit that this part of the Cosmos query pipeline is generally unfamiliar to me, and also represents an older/legacy part of the EF codebase in general. I'll do my best to fully understand everything and provide meaningful feedback. As we add more test coverage I'm sure issues will be flushed out.

For now, here's a first round of review comments - most are nits but see some more important things as well.

roji · 2026-01-06T18:04:05Z

src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs

        {
-            Expression updatedMemberExpression = memberExpression.Update(
-                expression != null ? MatchTypes(expression, memberExpression.Expression!.Type) : expression);
+            if (expression is null)


Can we please rename expression to innerExpression here, to make it clear what it's referring to? Otherwise the code below is somewhat confusing (I initially thought it's referring to the member expression)

roji · 2026-01-06T18:14:19Z

src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs

+            var expressionValue = Expression.Parameter(expression.Type);
+            var assignment = Expression.Assign(expressionValue, expression);
+
+            if (expression.Type.IsNullableType() == true


Small nit (also below):

Suggested change

if (expression.Type.IsNullableType() == true

if (expression.Type.IsNullableType()

roji · 2026-01-06T18:16:19Z

src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs

+            var expressionValue = Expression.Parameter(expression.Type);
+            var assignment = Expression.Assign(expressionValue, expression);
+
+            if (expression.Type.IsNullableType() == true


If I understand this correctly, when the inner expression is a Nullable<> and the outer isn't nullable, this adds "safe null" compensation logic, checking inner.HasValue() and then calling inner.Value.Foo only if true. Also, you're using a block with a variable and an assignment to make sure that inner only gets evaluated once (nice idea).

What I'm not clear on, is why exactly we need both this block and the one below (where we again check and compensate)...

Also, can you please add a short comment before the block explaining this (basically my explanation above, assuming it's correct), as it's really non-obvious?

roji · 2026-01-06T18:19:18Z

...unctionalTests/Query/Associations/ComplexProperties/ComplexPropertiesProjectionCosmosTest.cs

+
+        AssertSql(
+            """
+SELECT VALUE c


This should be projecting out only c["RequiredAssociate"] rather than all of c, right? I'm assuming this is because query support isn't yet fully done (i.e. full translation capabilities), but will be done later?

Note that the same seems true for various other tests on this class.

roji · 2026-01-06T18:22:19Z

src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.cs

+        }
+    }
+
+    private int _currentComplexIndex;


nit: please move this to the top of the class, where we have fields.

roji · 2026-01-06T18:23:54Z

src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.cs

+                    Constant(complexProperty.Name)
+                )
+            )
+        );


Nit: we generally don't put closing parentheses on their own lines, but simply stack them on the last line of the expression. Can you please do a pass and ensure it's this way everywhere?

Suggested change

Constant(complexProperty.Name)

)

)

);

Constant(complexProperty.Name))));

roji · 2026-01-06T18:25:36Z

src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.cs

+            if (complexProperty.IsCollection)
+            {
+                expressions.Add(CreateComplexCollectionAssignmentBlock(member, complexProperty));
+            }
+            else
+            {
+                expressions.Add(CreateComplexPropertyAssignmentBlock(member, complexProperty));
+            }


Suggested change

if (complexProperty.IsCollection)

{

expressions.Add(CreateComplexCollectionAssignmentBlock(member, complexProperty));

}

else

{

expressions.Add(CreateComplexPropertyAssignmentBlock(member, complexProperty));

}

expressions.Add(complexProperty.IsCollection

? CreateComplexCollectionAssignmentBlock(member, complexProperty)

: CreateComplexPropertyAssignmentBlock(member, complexProperty));

roji · 2026-01-06T18:31:17Z

src/EFCore.Cosmos/Query/Internal/CosmosShapedQueryCompilingExpressionVisitor.cs

+
+    private sealed class ComplexPropertyBindingExpression : Expression
+    {
+        public ComplexPropertyBindingExpression(IComplexProperty complexProperty, ParameterExpression jObjectParameter)


nit: use primary constructor

roji · 2026-01-06T18:42:14Z

...hapedQueryCompilingExpressionVisitor.CosmosProjectionBindingRemovingExpressionVisitorBase.cs

-        private static readonly MethodInfo GetItemMethodInfo
-            = typeof(JObject).GetRuntimeProperties()
-                .Single(pi => pi.Name == "Item" && pi.GetIndexParameters()[0].ParameterType == typeof(string))
+        public static readonly MethodInfo GetItemMethodInfo


Is this actually used anywhere not on a JObject, passing a string property name as an argument?

roji · 2026-01-06T18:44:04Z

...hapedQueryCompilingExpressionVisitor.CosmosProjectionBindingRemovingExpressionVisitorBase.cs

+                            case MethodCallExpression jObjectMethodCallExpression
+                                when jObjectMethodCallExpression.Method.IsGenericMethod && jObjectMethodCallExpression.Method.GetGenericMethodDefinition() == ToObjectWithSerializerMethodInfo:


Suggested change

case MethodCallExpression jObjectMethodCallExpression

when jObjectMethodCallExpression.Method.IsGenericMethod && jObjectMethodCallExpression.Method.GetGenericMethodDefinition() == ToObjectWithSerializerMethodInfo:

case MethodCallExpression { Method.IsGenericMethod: true } jObjectMethodCallExpression

when jObjectMethodCallExpression.Method.GetGenericMethodDefinition() == ToObjectWithSerializerMethodInfo:

JoasE added 13 commits December 18, 2025 21:19

WIP

601adaa

WIP

072889d

WIP

35ef5c7

Move back to old way

221d5a7

Remove todo (works)

8143650

WIP

b2d51dd

WIP: Firsts things working

1a116bc

Add skip to projection based tests

e5dcfe1

Fix constructor binding cases

81428b1

Cleanup

0e45c86

Cleanup

cdf1861

Remove todo in tests

b69203b

Cleanup

b66d913

This comment was marked as resolved.

Sign in to view

Move to CosmosStructuralTypeMaterializerSource

699da56

JoasE marked this pull request as ready for review December 19, 2025 11:53

JoasE requested a review from a team as a code owner December 19, 2025 11:53

Copilot AI review requested due to automatic review settings December 19, 2025 11:53

Copilot started reviewing on behalf of JoasE December 19, 2025 11:53 View session

Copilot AI reviewed Dec 19, 2025

View reviewed changes

Fix formatting and typo

900f117

JoasE requested a review from Copilot December 19, 2025 12:12

Copilot started reviewing on behalf of JoasE December 19, 2025 12:12 View session

Copilot AI reviewed Dec 19, 2025

View reviewed changes

JoasE marked this pull request as draft December 19, 2025 12:33

Fix typo and formatting

004d37a

JoasE requested a review from Copilot December 19, 2025 16:32

Copilot started reviewing on behalf of JoasE December 19, 2025 16:33 View session

Copilot AI reviewed Dec 19, 2025

View reviewed changes

JoasE marked this pull request as ready for review December 19, 2025 16:48

AndriySvyryd added the community-contribution label Dec 22, 2025

AndriySvyryd assigned roji Dec 22, 2025

JoasE mentioned this pull request Dec 31, 2025

Add Cosmos support for complex types #31253

Open

3 tasks

roji reviewed Jan 6, 2026

View reviewed changes

	if (expression.Type.IsNullableType() == true
	if (expression.Type.IsNullableType()

		case MethodCallExpression jObjectMethodCallExpression
		when jObjectMethodCallExpression.Method.IsGenericMethod && jObjectMethodCallExpression.Method.GetGenericMethodDefinition() == ToObjectWithSerializerMethodInfo:

Cosmos: Complex properties binding #37400

Are you sure you want to change the base?

Cosmos: Complex properties binding #37400

Conversation

JoasE commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

roji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JoasE commented Dec 18, 2025 •

edited

Loading