Support value types in complex JSON shaper #36557
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -1672,7 +1672,7 @@ private Expression CreateJsonShapers( | |||||||
| var elementFixup = Lambda( | ||||||||
| Block( | ||||||||
| typeof(void), | ||||||||
| AssignStructuralProperty( | ||||||||
| innerFixupCollectionElementParameter, | ||||||||
| innerFixupParentParameter, | ||||||||
| inverseNavigation)), | ||||||||
|
|
@@ -1709,7 +1709,7 @@ private Expression CreateJsonShapers( | |||||||
| { | ||||||||
| var fixup = GenerateReferenceFixupForJson( | ||||||||
| structuralType.ClrType, | ||||||||
| nestedRelationship.ClrType, | ||||||||
| nestedRelationship, | ||||||||
| inverseNavigation); | ||||||||
|
|
||||||||
|
|
@@ -1847,8 +1847,27 @@ private Expression CreateJsonShapers( | |||||||
| return materializeJsonEntityCollectionMethodCall; | ||||||||
| } | ||||||||
|
|
||||||||
|
|
||||||||
| // Return the materializer for this JSON object, including null checks which would return null. | ||||||||
| MethodInfo method; | ||||||||
|
|
||||||||
| if (relationship is not null && Nullable.GetUnderlyingType(relationship.ClrType) is { } underlyingType) | ||||||||
| { | ||||||||
| // The association property into which we're assigning has a nullable value type, so generate | ||||||||
| // a materializer that returns that nullable value type (note that the shaperLambda that | ||||||||
| // we pass itself always returns a non-nullable value (the null checks are outside of it.)) | ||||||||
| Check.DebugAssert(nullable, "On non-nullable relationship but the relationship's ClrType is Nullable<T>"); | ||||||||
| Check.DebugAssert(underlyingType == structuralType.ClrType); | ||||||||
|
|
||||||||
| method = MaterializeJsonNullableValueStructuralTypeMethodInfo.MakeGenericMethod(structuralType.ClrType); | ||||||||
| } | ||||||||
| else | ||||||||
| { | ||||||||
| method = MaterializeJsonStructuralTypeMethodInfo.MakeGenericMethod(structuralType.ClrType); | ||||||||
| } | ||||||||
|
|
||||||||
| var materializedRootJsonEntity = Call( | ||||||||
| method, | ||||||||
| QueryCompilationContext.QueryContextParameter, | ||||||||
| keyValuesParameter, | ||||||||
| jsonReaderDataParameter, | ||||||||
|
|
@@ -1969,9 +1988,9 @@ protected override Expression VisitSwitch(SwitchExpression switchExpression) | |||||||
|
|
||||||||
| var managerVariable = Variable(typeof(Utf8JsonReaderManager), "jsonReaderManager"); | ||||||||
| var tokenTypeVariable = Variable(typeof(JsonTokenType), "tokenType"); | ||||||||
| var jsonStructuralTypeVariable = (ParameterExpression)jsonEntityTypeInitializerBlock.Expressions[^1]; | ||||||||
|
|
||||||||
| Debug.Assert(jsonStructuralTypeVariable.Type == structuralType.ClrType); | ||||||||
|
|
||||||||
| var finalBlockVariables = new List<ParameterExpression> | ||||||||
| { | ||||||||
|
|
@@ -2024,7 +2043,7 @@ protected override Expression VisitSwitch(SwitchExpression switchExpression) | |||||||
| // - navigation fixups | ||||||||
| // - entity instance variable that is returned as end result | ||||||||
| var propertyAssignmentReplacer = new ValueBufferTryReadValueMethodsReplacer( | ||||||||
| jsonStructuralTypeVariable, propertyAssignmentMap); | ||||||||
|
|
||||||||
| if (body.Expressions[0] is BinaryExpression | ||||||||
| { | ||||||||
|
|
@@ -2051,7 +2070,7 @@ protected override Expression VisitSwitch(SwitchExpression switchExpression) | |||||||
| // or for empty/null collections of a tracking queries. | ||||||||
| ProcessFixup(queryStateManager ? trackingInnerFixupMap : innerFixupMap); | ||||||||
|
|
||||||||
| finalBlockExpressions.Add(jsonStructuralTypeVariable); | ||||||||
|
|
||||||||
| return Block( | ||||||||
| finalBlockVariables, | ||||||||
|
|
@@ -2063,18 +2082,35 @@ void ProcessFixup(IDictionary<string, LambdaExpression> fixupMap) | |||||||
| { | ||||||||
| var navigationEntityParameter = _navigationVariableMap[fixup.Key]; | ||||||||
|
|
||||||||
| // Inject the fixup code for each property; we have this as a set of lambdas in the fixup map. | ||||||||
| // In the normal case, simply Invoke the lambda, passing it the structural type to be fixed up as a parameter. | ||||||||
| // This unfortunately doesn't work on value types (where a copy would be mutated), so for them, | ||||||||
| // 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*/) | ||||||||
|
There was a problem hiding this comment. 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
Copilot uses AI. Check for mistakes. |
||||||||
| { | ||||||||
| var fixupBody = ReplacingExpressionVisitor.Replace( | ||||||||
| originals: [fixup.Value.Parameters[0], fixup.Value.Parameters[1]], | ||||||||
| replacements: [jsonStructuralTypeVariable, _navigationVariableMap[fixup.Key]], | ||||||||
| fixup.Value.Body); | ||||||||
|
|
||||||||
| finalBlockExpressions.Add(fixupBody); | ||||||||
| } | ||||||||
| else | ||||||||
| { | ||||||||
| // If the structural type being fixed up is nullable, then we need to add null checks before we run fixup logic. | ||||||||
| // For regular entities, whose fixup is done as part of the "Materialize*" method, the checks are done there | ||||||||
| // (the same will be done for the "optimized" scenario, where we populate properties directly rather than store in variables). | ||||||||
| // But in this case fixups are standalone, so the null safety must be added here. | ||||||||
| finalBlockExpressions.Add( | ||||||||
| IfThen( | ||||||||
| NotEqual(jsonStructuralTypeVariable, Constant(null, jsonStructuralTypeVariable.Type)), | ||||||||
| Invoke( | ||||||||
| fixup.Value, | ||||||||
| jsonStructuralTypeVariable, | ||||||||
| _navigationVariableMap[fixup.Key]))); | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
|
|
@@ -2756,7 +2792,7 @@ private LambdaExpression GenerateFixup( | |||||||
| expressions.Add( | ||||||||
| relationship.IsCollection | ||||||||
| ? AddToCollectionRelationship(entityParameter, relatedEntityParameter, relationship) | ||||||||
| : AssignStructuralProperty(entityParameter, relatedEntityParameter, relationship)); | ||||||||
| } | ||||||||
|
|
||||||||
| if (inverseNavigation != null | ||||||||
|
|
@@ -2765,26 +2801,26 @@ private LambdaExpression GenerateFixup( | |||||||
| expressions.Add( | ||||||||
| inverseNavigation.IsCollection | ||||||||
| ? AddToCollectionRelationship(relatedEntityParameter, entityParameter, inverseNavigation) | ||||||||
| : AssignStructuralProperty(relatedEntityParameter, entityParameter, inverseNavigation)); | ||||||||
| } | ||||||||
|
|
||||||||
| return Lambda(Block(typeof(void), expressions), entityParameter, relatedEntityParameter); | ||||||||
| } | ||||||||
|
|
||||||||
| private static LambdaExpression GenerateReferenceFixupForJson( | ||||||||
| Type clrType, | ||||||||
| Type relatedClrType, | ||||||||
| IPropertyBase relationship, | ||||||||
| INavigationBase? inverseNavigation) | ||||||||
| { | ||||||||
| var entityParameter = Parameter(clrType); | ||||||||
| var relatedEntityParameter = Parameter(relatedClrType); | ||||||||
| var expressions = new List<Expression>(); | ||||||||
|
|
||||||||
| if (!relationship.IsShadowProperty()) | ||||||||
| { | ||||||||
| expressions.Add( | ||||||||
| AssignStructuralProperty( | ||||||||
| entityParameter, | ||||||||
| relatedEntityParameter, | ||||||||
| relationship)); | ||||||||
|
|
@@ -2794,7 +2830,7 @@ private static LambdaExpression GenerateReferenceFixupForJson( | |||||||
| && !inverseNavigation.IsShadowProperty()) | ||||||||
| { | ||||||||
| expressions.Add( | ||||||||
| AssignStructuralProperty( | ||||||||
| relatedEntityParameter, | ||||||||
| entityParameter, | ||||||||
| inverseNavigation)); | ||||||||
|
|
@@ -2821,11 +2857,21 @@ public static void InverseCollectionFixup<TCollectionElement, TEntity>( | |||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| private static Expression AssignStructuralProperty( | ||||||||
| ParameterExpression structuralType, | ||||||||
| ParameterExpression relatedStructuralType, | ||||||||
| IPropertyBase structuralProperty) | ||||||||
| { | ||||||||
| var setter = structuralProperty.GetMemberInfo(forMaterialization: true, forSet: true); | ||||||||
|
|
||||||||
| // If we're assigning a value complex type to a nullable complex property, add an upcast for typing | ||||||||
| var assignee = structuralProperty.ClrType.IsNullableValueType() | ||||||||
| && structuralProperty.ClrType.UnwrapNullableType() == relatedStructuralType.Type | ||||||||
| ? Convert(relatedStructuralType, structuralProperty.ClrType) | ||||||||
| : (Expression)relatedStructuralType; | ||||||||
|
|
||||||||
| return structuralType.MakeMemberAccess(setter).Assign(assignee); | ||||||||
| } | ||||||||
|
|
||||||||
| private Expression GetOrCreateCollectionObjectLambda(Type entityType, IPropertyBase relationship) | ||||||||
| { | ||||||||
|
|
||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this have
where TStructural : class?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).