Skip to content

[release/8.0] Respect the difference between null/empty for owned collections mapped to JSON #31831

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ajcvickers merged 1 commit into from
Oct 3, 2023
Merged

[release/8.0] Respect the difference between null/empty for owned collections mapped to JSON #31831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions ...lShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.ClientMethods.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -992,6 +992,7 @@ private static void IncludeJsonEntityCollection<TIncludingEntity, TIncludedColle
JsonReaderData? jsonReaderData,
TIncludingEntity entity,
Func<QueryContext, object[], JsonReaderData, TIncludedCollectionElement> innerShaper,
Action<TIncludingEntity> getOrCreateCollectionObject,
Action<TIncludingEntity, TIncludedCollectionElement> fixup,
bool trackingQuery)
where TIncludingEntity : class
Expand All @@ -1011,6 +1012,8 @@ private static void IncludeJsonEntityCollection<TIncludingEntity, TIncludedColle
RelationalStrings.JsonReaderInvalidTokenType(tokenType.ToString()));
}

getOrCreateCollectionObject(entity);

var newKeyPropertyValues = new object[keyPropertyValues.Length + 1];
Array.Copy(keyPropertyValues, newKeyPropertyValues, keyPropertyValues.Length);

Expand Down
91 changes: 71 additions & 20 deletions ...uery/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ private static readonly MemberInfo ResultContextValuesMemberInfo
private static readonly MemberInfo SingleQueryResultCoordinatorResultReadyMemberInfo
= typeof(SingleQueryResultCoordinator).GetMember(nameof(SingleQueryResultCoordinator.ResultReady))[0];

private static readonly MethodInfo CollectionAccessorGetOrCreateMethodInfo
= typeof(IClrCollectionAccessor).GetTypeInfo().GetDeclaredMethod(nameof(IClrCollectionAccessor.GetOrCreate))!;

private static readonly MethodInfo CollectionAccessorAddMethodInfo
= typeof(IClrCollectionAccessor).GetTypeInfo().GetDeclaredMethod(nameof(IClrCollectionAccessor.Add))!;

Expand Down Expand Up @@ -1281,6 +1284,7 @@ private Expression CreateJsonShapers(

var innerShapersMap = new Dictionary<string, Expression>();
var innerFixupMap = new Dictionary<string, LambdaExpression>();
var trackingInnerFixupMap = new Dictionary<string, LambdaExpression>();
foreach (var ownedNavigation in entityType.GetNavigations().Where(
n => n.TargetEntityType.IsMappedToJson() && n.ForeignKey.IsOwnership && n == n.ForeignKey.PrincipalToDependent))
{
Expand All @@ -1303,21 +1307,30 @@ private Expression CreateJsonShapers(
var shaperEntityParameter = Parameter(ownedNavigation.DeclaringEntityType.ClrType);
var shaperCollectionParameter = Parameter(ownedNavigation.ClrType);
var expressions = new List<Expression>();
var expressionsForTracking = new List<Expression>();

if (!ownedNavigation.IsShadowProperty())
{
expressions.Add(
shaperEntityParameter.MakeMemberAccess(ownedNavigation.GetMemberInfo(forMaterialization: true, forSet: true))
.Assign(shaperCollectionParameter));

expressionsForTracking.Add(
IfThen(
OrElse(
ReferenceEqual(Constant(null), shaperCollectionParameter),
IsFalse(
Call(
typeof(EnumerableExtensions).GetMethod(nameof(EnumerableExtensions.Any))!,
shaperCollectionParameter))),
shaperEntityParameter
.MakeMemberAccess(ownedNavigation.GetMemberInfo(forMaterialization: true, forSet: true))
.Assign(shaperCollectionParameter)));
}

if (ownedNavigation.Inverse is INavigation inverseNavigation
&& !inverseNavigation.IsShadowProperty())
{
//for (var i = 0; i < prm.Count; i++)
//{
// prm[i].Parent = instance
//}
var innerFixupCollectionElementParameter = Parameter(inverseNavigation.DeclaringEntityType.ClrType);
var innerFixupParentParameter = Parameter(inverseNavigation.TargetEntityType.ClrType);

Expand Down Expand Up @@ -1347,6 +1360,14 @@ private Expression CreateJsonShapers(
shaperCollectionParameter);

innerFixupMap[navigationJsonPropertyName] = fixup;

var trackedFixup = Lambda(
Block(typeof(void), expressionsForTracking),
shaperEntityParameter,
shaperCollectionParameter);

innerFixupMap[navigationJsonPropertyName] = fixup;
trackingInnerFixupMap[navigationJsonPropertyName] = trackedFixup;
}
else
{
Expand All @@ -1366,6 +1387,7 @@ private Expression CreateJsonShapers(
jsonReaderDataShaperLambdaParameter,
innerShapersMap,
innerFixupMap,
trackingInnerFixupMap,
_queryLogger).Rewrite(entityShaperMaterializer);

var entityShaperMaterializerVariable = Variable(
Expand Down Expand Up @@ -1416,6 +1438,9 @@ private Expression CreateJsonShapers(
jsonReaderDataParameter,
includingEntityExpression,
shaperLambda,
GetOrCreateCollectionObjectLambda(
navigation.DeclaringEntityType.ClrType,
navigation),
fixup,
Constant(_isTracking));

Expand Down Expand Up @@ -1484,6 +1509,7 @@ private sealed class JsonEntityMaterializerRewriter : ExpressionVisitor
private readonly ParameterExpression _jsonReaderDataParameter;
private readonly IDictionary<string, Expression> _innerShapersMap;
private readonly IDictionary<string, LambdaExpression> _innerFixupMap;
private readonly IDictionary<string, LambdaExpression> _trackingInnerFixupMap;
private readonly IDiagnosticsLogger<DbLoggerCategory.Query> _queryLogger;

private static readonly PropertyInfo JsonEncodedTextEncodedUtf8BytesProperty
Expand All @@ -1499,13 +1525,15 @@ public JsonEntityMaterializerRewriter(
ParameterExpression jsonReaderDataParameter,
IDictionary<string, Expression> innerShapersMap,
IDictionary<string, LambdaExpression> innerFixupMap,
IDictionary<string, LambdaExpression> trackingInnerFixupMap,
IDiagnosticsLogger<DbLoggerCategory.Query> queryLogger)
{
_entityType = entityType;
_isTracking = isTracking;
_jsonReaderDataParameter = jsonReaderDataParameter;
_innerShapersMap = innerShapersMap;
_innerFixupMap = innerFixupMap;
_trackingInnerFixupMap = trackingInnerFixupMap;
_queryLogger = queryLogger;
}

Expand Down Expand Up @@ -1662,10 +1690,26 @@ protected override Expression VisitSwitch(SwitchExpression switchExpression)
finalBlockExpressions.Add(propertyAssignmentReplacer.Visit(jsonEntityTypeInitializerBlockExpression));
}

// fixup is only needed for non-tracking queries, in case of tracking - ChangeTracker does the job
if (!_isTracking)
// Fixup is only needed for non-tracking queries, in case of tracking - ChangeTracker does the job
// or for empty/null collections of a tracking queries.
if (_isTracking)
{
ProcessFixup(_trackingInnerFixupMap);
}
else
{
ProcessFixup(_innerFixupMap);
}

finalBlockExpressions.Add(jsonEntityTypeVariable);

return Block(
finalBlockVariables,
finalBlockExpressions);

void ProcessFixup(IDictionary<string, LambdaExpression> fixupMap)
{
foreach (var fixup in _innerFixupMap)
foreach (var fixup in fixupMap)
{
var navigationEntityParameter = _navigationVariableMap[fixup.Key];

Expand All @@ -1674,25 +1718,15 @@ protected override Expression VisitSwitch(SwitchExpression switchExpression)
// but in this case fixups are standalone, so the null safety must be added by us directly
finalBlockExpressions.Add(
IfThen(
AndAlso(
NotEqual(
jsonEntityTypeVariable,
Constant(null, jsonEntityTypeVariable.Type)),
NotEqual(
navigationEntityParameter,
Constant(null, navigationEntityParameter.Type))),
NotEqual(
jsonEntityTypeVariable,
Constant(null, jsonEntityTypeVariable.Type)),
Invoke(
fixup.Value,
jsonEntityTypeVariable,
_navigationVariableMap[fixup.Key])));
}
}

finalBlockExpressions.Add(jsonEntityTypeVariable);

return Block(
finalBlockVariables,
finalBlockExpressions);
}

return base.VisitSwitch(switchExpression);
Expand Down Expand Up @@ -2369,6 +2403,23 @@ private static Expression AssignReferenceNavigation(
INavigationBase navigation)
=> entity.MakeMemberAccess(navigation.GetMemberInfo(forMaterialization: true, forSet: true)).Assign(relatedEntity);

private static Expression GetOrCreateCollectionObjectLambda(
Type entityType,
INavigationBase navigation)
{
var prm = Parameter(entityType);

return Lambda(
Block(
typeof(void),
Call(
Constant(navigation.GetCollectionAccessor()),
CollectionAccessorGetOrCreateMethodInfo,
prm,
Constant(true))),
prm);
}

private static Expression AddToCollectionNavigation(
ParameterExpression entity,
ParameterExpression relatedEntity,
Expand Down
33 changes: 30 additions & 3 deletions src/EFCore.Relational/Update/ModificationCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,8 @@ private List<IColumnModification> GenerateColumnModifications()
if (_entries.Count > 1
|| _entries is [var singleEntry]
&& (singleEntry.SharedIdentityEntry is not null
|| singleEntry.EntityType.GetComplexProperties().Any()))
|| singleEntry.EntityType.GetComplexProperties().Any()
|| singleEntry.EntityType.GetNavigations().Any(e => e.IsCollection && e.TargetEntityType.IsMappedToJson())))
{
Check.DebugAssert(StoreStoredProcedure is null, "Multiple entries/shared identity not supported with stored procedures");

Expand Down Expand Up @@ -293,9 +294,13 @@ private List<IColumnModification> GenerateColumnModifications()

HandleSharedColumns(entry.EntityType, entry, tableMapping, deleting, sharedTableColumnMap);

if (!jsonEntry && entry.EntityType.IsMappedToJson())
if (!jsonEntry)
{
jsonEntry = true;
if (entry.EntityType.IsMappedToJson()
|| entry.EntityType.GetNavigations().Any(e => e.IsCollection && e.TargetEntityType.IsMappedToJson()))
{
jsonEntry = true;
}
}
}
}
Expand Down Expand Up @@ -684,6 +689,28 @@ void HandleJson(List<IColumnModification> columnModifications)
jsonColumnsUpdateMap[jsonColumn] = jsonPartialUpdateInfo;
}

foreach (var entry in _entries.Where(e => !e.EntityType.IsMappedToJson()))
{
foreach (var jsonCollectionNavigation in entry.EntityType.GetNavigations()
.Where(n => n.IsCollection
&& n.TargetEntityType.IsMappedToJson()
&& (entry.GetCurrentValue(n) as IEnumerable)?.Any() == false))
{
var jsonCollectionEntityType = jsonCollectionNavigation.TargetEntityType;
var jsonCollectionColumn =
GetTableMapping(jsonCollectionEntityType)!.Table.FindColumn(
jsonCollectionEntityType.GetContainerColumnName()!)!;

if (!jsonColumnsUpdateMap.ContainsKey(jsonCollectionColumn))
{
var jsonPartialUpdateInfo = new JsonPartialUpdateInfo();
jsonPartialUpdateInfo.Path.Insert(0, new JsonPartialUpdatePathEntry("$", null, entry, jsonCollectionNavigation));
jsonPartialUpdateInfo.PropertyValue = entry.GetCurrentValue(jsonCollectionNavigation);
jsonColumnsUpdateMap[jsonCollectionColumn] = jsonPartialUpdateInfo;
}
}
}

foreach (var (jsonColumn, updateInfo) in jsonColumnsUpdateMap)
{
var finalUpdatePathElement = updateInfo.Path.Last();
Expand Down
2 changes: 1 addition & 1 deletion test/EFCore.Relational.Specification.Tests/Query/JsonQueryAdHocTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -823,7 +823,7 @@ public class MyJsonEntityLazyLoadingProxies
}

#endregion

#region NotICollection

[ConditionalTheory]
Expand Down
Loading