Support value types in complex JSON shaper by roji · Pull Request #36557 · dotnet/efcore

roji · 2025-08-12T12:50:44Z

cincuranet · 2025-08-15T16:23:20Z

@roji looks like you committed also your overwritten versions (I guess it's from that). Either way, the Versions.props, etc. should not be modified.

roji · 2025-08-15T16:33:47Z

@cincuranet thanks, yeah - I keep have to bring that commit in and remove it because of the unsigned SDK on mac issue. Removing.

AndriySvyryd · 2025-08-15T16:37:35Z

...onalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.ClientMethods.cs

            bool nullable,
-            Func<QueryContext, object[]?, JsonReaderData, TEntity> shaper)
-            where TEntity : class
+            Func<QueryContext, object[]?, JsonReaderData, TStructural> shaper)


Shouldn't this have where TStructural : class?

Not currently - this also gets used with non-nullable structs. The other method that this PR introduces is only for use with nullable value types: it accepts a parameter with shaper returning the non-nullable type, and wraps a null check around it to return null.

I do agree that we should probably review the behavior for non-nullable structs... For scalars, if a null is retrieved from the database and the CLR is non-nullable, we throw rather than return default, which seems to be the right thing (/cc @cincuranet for optional table splitting).

For scalars, if a null is retrieved from the database and the CLR is non-nullable, we throw rather than return default, which seems to be the right thing

That's not what we decided for Cosmos #21006

But it makes sense to have a holistic approach and look at this together with #26981

Right, I agree we need to consider all this holistically. But note that above I was specifically mentioning the case of a null value - not a missing value - in the JSON document (combined with a non-nullable CLR property). That case seems very similar to a traditional null value in a relational column, where the user modeled it with a non-nullable property on the .NET side - AFAIK we consider this a configuration error and throw (rather than return the default CLR type).

In other words, we have the following questions:

Null value in JSON, non-nullable .NET value property (the above case; my vote: should throw just like non-JSON relational)

Missing value in JSON (I think here we agreed to return the CLR default, to make schema evolution easier)

Missing value in JSON, HasDefaultValue or similar mechanism (for returning some other non-default CLR value)

Does that make sense?

Discussed offline, we agree that for 10 we should throw if null is present in the database and the property is a non-nullable value type (opened #36587 to track).

AndriySvyryd · 2025-08-15T16:40:35Z

...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs

+                            // we unwrap the lambda and integrate its body directly.
+                            // We should ideally do this for all cases (no need for the extra lambda Invoke), but there are some issues around us writing
+                            // to readonly fields.
+                            if (jsonStructuralTypeVariable.Type.IsValueType /*&& Nullable.GetUnderlyingType(jsonStructuralTypeVariable.Type) is null*/)


Remove commented out code

AndriySvyryd · 2025-08-15T16:45:40Z

src/EFCore/Metadata/Internal/ClrCollectionAccessor.cs

@@ -96,7 +95,7 @@ public virtual object Create()
        {
            throw new InvalidOperationException(


At least file an issue and add a TODO to fix all the exceptions here if this is going to be used for something other than navigations.
Though, I still don't think that makes practical sense.

AndriySvyryd · 2025-08-15T16:48:33Z

test/EFCore.Specification.Tests/Query/Relationships/RelationshipsQueryFixtureBase.cs

+        AssertValueRelatedType(e.RequiredRelated, a.RequiredRelated);
+        NullSafeAssert<ValueRelatedType>(e.OptionalRelated, a.OptionalRelated, AssertValueRelatedType);
+
+        // TODO: Complete for collection, mind ordering (how is this done elsewhere?)


Add Issue #

AndriySvyryd · 2025-08-15T16:48:38Z

test/EFCore.Specification.Tests/Query/Relationships/RelationshipsQueryFixtureBase.cs

+        AssertValueNestedType(e.RequiredNested, a.RequiredNested);
+        NullSafeAssert<ValueNestedType>(e.OptionalNested, a.OptionalNested, AssertValueNestedType);
+
+        // TODO: Complete for collection, mind ordering (how is this done elsewhere?)


Add Issue #

AndriySvyryd · 2025-08-15T16:49:28Z

...ification.Tests/Query/Relationships/ComplexProperties/ComplexPropertiesProjectionTestBase.cs

+            ss => ss.Set<ValueRootEntity>(),
+            queryTrackingBehavior: queryTrackingBehavior);
+
+    #endregion Value types


Add a test for projecting out the outermost value type property

Copilot

Pull Request Overview

This PR adds support for value types (structs) in complex JSON shaper functionality, addressing issue #36552. The implementation enables Entity Framework Core to properly handle value type complex properties when working with JSON columns.

Adds new test entities that mirror existing reference type complex entities but use value types (structs)
Updates internal infrastructure to handle structural types (both reference and value types) instead of just entities
Modifies JSON materialization logic to properly handle nullable value types and fixup operations

Reviewed Changes

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

Show a summary per file

File	Description
`RelationshipsModelValueTypes.cs`	Defines new value type entities (`ValueRootEntity`, `ValueRelatedType`, `ValueNestedType`) for testing
`RelationshipsData.cs`	Adds creation and querying support for value type test entities
`ComplexPropertiesProjectionTestBase.cs`	Adds test case for selecting root entities with value type complex properties
`ComplexJsonProjectionSqlServerTest.cs`	SQL Server-specific test implementation for value type complex properties
`ComplexPropertiesFixtureBase.cs`	Configures model mapping for value type entities
`ComplexJsonRelationalFixtureBase.cs`	Configures JSON column mapping for value type entities
`ComplexTableSplittingRelationalFixtureBase.cs`	Configures table splitting for value type entities
`ClrCollectionAccessorFactory.cs`	Generalizes from `TEntity` to `TStructural` to support both reference and value types
`ClrCollectionAccessor.cs`	Updates generic constraints to support structural types instead of just classes
`SelectExpression.cs`	Fixes type handling for complex properties by unwrapping nullable types
`RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs`	Major updates to handle value type fixups and nullable value type materialization
`RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.ClientMethods.cs`	Adds new materialization methods for structural types and nullable value types

_{You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.}

Copilot · 2025-08-15T16:50:46Z

test/EFCore.Specification.Tests/Query/Relationships/RelationshipsModelValueTypes.cs

+           && Int == other.Int
+           && String == other.String
+           && RequiredNested.Equals(other.RequiredNested)
+           && (OptionalNested is null && other.OptionalNested is null || OptionalNested?.Equals(other.RequiredNested) == true)


The equality comparison is checking OptionalNested?.Equals(other.RequiredNested) but should be checking OptionalNested?.Equals(other.OptionalNested) to compare the same properties.

Suggested change

&& (OptionalNested is null && other.OptionalNested is null || OptionalNested?.Equals(other.RequiredNested) == true)

&& (OptionalNested is null && other.OptionalNested is null || OptionalNested?.Equals(other.OptionalNested) == true)

An actual bug caught by Copilot, amazing

Copilot · 2025-08-15T16:50:47Z

...l/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs

+                            // we unwrap the lambda and integrate its body directly.
+                            // We should ideally do this for all cases (no need for the extra lambda Invoke), but there are some issues around us writing
+                            // to readonly fields.
+                            if (jsonStructuralTypeVariable.Type.IsValueType /*&& Nullable.GetUnderlyingType(jsonStructuralTypeVariable.Type) is null*/)


The commented out condition should either be removed if not needed or uncommented with an explanation if it's intentionally disabled for debugging purposes.

Suggested change

if (jsonStructuralTypeVariable.Type.IsValueType /*&& Nullable.GetUnderlyingType(jsonStructuralTypeVariable.Type) is null*/)

// Only unwrap and integrate for non-nullable value types; for Nullable<T>, handle in the else branch.

if (jsonStructuralTypeVariable.Type.IsValueType && Nullable.GetUnderlyingType(jsonStructuralTypeVariable.Type) is null)

Closes dotnet#36552

Part of dotnet#31376 and dotnet#36296 Continues dotnet#36557

* Fixes to complex values types Part of #31376 and #36296 Continues #36557 * Fixes

roji force-pushed the ValueComplexTypeQueryBug branch from 404f616 to 45cca57 Compare August 15, 2025 15:44

roji marked this pull request as ready for review August 15, 2025 15:44

roji requested review from a team, AndriySvyryd and cincuranet as code owners August 15, 2025 15:44

roji force-pushed the ValueComplexTypeQueryBug branch from 45cca57 to 73650a5 Compare August 15, 2025 16:33

AndriySvyryd reviewed Aug 15, 2025

View reviewed changes

AndriySvyryd requested a review from Copilot August 15, 2025 16:49

AndriySvyryd approved these changes Aug 15, 2025

View reviewed changes

Copilot AI reviewed Aug 15, 2025

View reviewed changes

Support value types in complex JSON shaper

d1fd606

Closes dotnet#36552

roji force-pushed the ValueComplexTypeQueryBug branch from 73650a5 to d1fd606 Compare August 15, 2025 20:42

roji enabled auto-merge (squash) August 15, 2025 20:42

roji mentioned this pull request Aug 15, 2025

Throw when materializating JSON nulls to non-nullable .NET properties #36587

Closed

roji merged commit 5f505fd into dotnet:main Aug 15, 2025
7 checks passed

roji deleted the ValueComplexTypeQueryBug branch August 15, 2025 21:26

roji added a commit to roji/efcore that referenced this pull request Aug 20, 2025

Fixes to complex values types

cd381e7

Part of dotnet#31376 and dotnet#36296 Continues dotnet#36557

roji mentioned this pull request Aug 20, 2025

[RC2] - Fixes to complex values types #36617

Merged

roji added a commit that referenced this pull request Aug 20, 2025

[RC2] - Fixes to complex values types (#36617)

87cf2b6

* Fixes to complex values types Part of #31376 and #36296 Continues #36557 * Fixes

		@@ -96,7 +95,7 @@ public virtual object Create()
		{
		throw new InvalidOperationException(

	&& (OptionalNested is null && other.OptionalNested is null \|\| OptionalNested?.Equals(other.RequiredNested) == true)
	&& (OptionalNested is null && other.OptionalNested is null \|\| OptionalNested?.Equals(other.OptionalNested) == true)

	if (jsonStructuralTypeVariable.Type.IsValueType /&& Nullable.GetUnderlyingType(jsonStructuralTypeVariable.Type) is null/)
	// Only unwrap and integrate for non-nullable value types; for Nullable<T>, handle in the else branch.
	if (jsonStructuralTypeVariable.Type.IsValueType && Nullable.GetUnderlyingType(jsonStructuralTypeVariable.Type) is null)

Conversation

roji commented Aug 12, 2025

Uh oh!

cincuranet commented Aug 15, 2025

Uh oh!

roji commented Aug 15, 2025

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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants