Skip to content

ClrCollectionAccessor cleanup for complex JSON collections #36618

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

Draft
wants to merge 1 commit into
base: release/10.0
Choose a base branch
from
Draft

ClrCollectionAccessor cleanup for complex JSON collections #36618

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
11 changes: 8 additions & 3 deletions ...lShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.ClientMethods.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Runtime.CompilerServices;
using System.Text.Json;
using Microsoft.EntityFrameworkCore.Internal;
using Microsoft.EntityFrameworkCore.Metadata.Internal;
using Microsoft.EntityFrameworkCore.Query.Internal;
using Microsoft.EntityFrameworkCore.Storage.Json;

Expand Down Expand Up @@ -1076,8 +1077,12 @@ static async Task<RelationalDataReader> InitializeReaderAsync(
throw new InvalidOperationException(CoreStrings.JsonReaderInvalidTokenType(tokenType.ToString()));
}

var collectionAccessor = relationship.GetCollectionAccessor();
var result = (TResult)collectionAccessor!.Create();
// var collectionAccessor = relationship.GetCollectionAccessor();
#pragma warning disable EF1001 // Internal EF Core API usage.
var collectionAccessor = ((IRuntimePropertyBase)relationship).GetIndexedCollectionAccessor();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetIndexedCollectionAccessor() is only available on IRuntimePropertyBase (unlike GetCollectionAccessor() which is available on IPropertyBase). I don't have strong feelings here but maybe seems like they should be consistent?

Also note that GetIndexedCollectionAccessor is pubternal (another thing to maybe change).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is pubternal until we are satisfied with the API

#pragma warning restore EF1001 // Internal EF Core API usage.

var result = (TResult)collectionAccessor!.Create(0);

object[]? newKeyPropertyValues = null;

Expand All @@ -1098,7 +1103,7 @@ static async Task<RelationalDataReader> InitializeReaderAsync(
{
manager.CaptureState();
var entity = innerShaper(queryContext, newKeyPropertyValues, jsonReaderData);
collectionAccessor.AddStandalone(result, entity);
// collectionAccessor.AddStandalone(result, entity);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea what this means (yet) but it doesn't exist on ClrIndexedCollectionAccessor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It allows to add an element to the collection without the containing object. We could add SetStandalone and CreateStandalone if necessary

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, Create should just be renamed to CreateStandalone as that would reflect the current behavior. Note that because the containing object could be a value type you would need to either use CreateStandalone with SetStandalone to populate the collection before materializing the containing object. Or use CreateStandalone and Set after materializing it.

manager = new Utf8JsonReaderManager(manager.Data, queryContext.QueryLogger);

if (manager.CurrentReader.TokenType != JsonTokenType.EndObject)
Expand Down
35 changes: 18 additions & 17 deletions src/EFCore/Metadata/Internal/ClrCollectionAccessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,17 @@ namespace Microsoft.EntityFrameworkCore.Metadata.Internal;
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public class ClrCollectionAccessor<TStructural, TCollection, TElement> : IClrCollectionAccessor
public class ClrCollectionAccessor<TEntity, TCollection, TElement> : IClrCollectionAccessor
where TEntity : class
where TCollection : class, IEnumerable<TElement>
where TElement : class
{
private readonly string _propertyName;
private readonly bool _shadow;
private readonly Func<TStructural, TCollection>? _getCollection;
private readonly Action<TStructural, TCollection>? _setCollection;
private readonly Action<TStructural, TCollection>? _setCollectionForMaterialization;
private readonly Func<TStructural, Action<TStructural, TCollection>, TCollection>? _createAndSetCollection;
private readonly Func<TEntity, TCollection>? _getCollection;
private readonly Action<TEntity, TCollection>? _setCollection;
private readonly Action<TEntity, TCollection>? _setCollectionForMaterialization;
private readonly Func<TEntity, Action<TEntity, TCollection>, TCollection>? _createAndSetCollection;
private readonly Func<TCollection>? _createCollection;

/// <summary>
Expand All @@ -41,10 +42,10 @@ public virtual Type CollectionType
public ClrCollectionAccessor(
string propertyName,
bool shadow,
Func<TStructural, TCollection>? getCollection,
Action<TStructural, TCollection>? setCollection,
Action<TStructural, TCollection>? setCollectionForMaterialization,
Func<TStructural, Action<TStructural, TCollection>, TCollection>? createAndSetCollection,
Func<TEntity, TCollection>? getCollection,
Action<TEntity, TCollection>? setCollection,
Action<TEntity, TCollection>? setCollectionForMaterialization,
Func<TEntity, Action<TEntity, TCollection>, TCollection>? createAndSetCollection,
Func<TCollection>? createCollection)
{
_propertyName = propertyName;
Expand Down Expand Up @@ -95,7 +96,7 @@ public virtual object Create()
{
throw new InvalidOperationException(
CoreStrings.NavigationCannotCreateType(
_propertyName, typeof(TStructural).ShortDisplayName(), typeof(TCollection).ShortDisplayName()));
_propertyName, typeof(TEntity).ShortDisplayName(), typeof(TCollection).ShortDisplayName()));
}

return _createCollection();
Expand All @@ -121,17 +122,17 @@ private ICollection<TElement> GetOrCreateCollection(object instance, bool forMat

if (setCollection == null)
{
throw new InvalidOperationException(CoreStrings.NavigationNoSetter(_propertyName, typeof(TStructural).ShortDisplayName()));
throw new InvalidOperationException(CoreStrings.NavigationNoSetter(_propertyName, typeof(TEntity).ShortDisplayName()));
}

if (_createAndSetCollection == null)
{
throw new InvalidOperationException(
CoreStrings.NavigationCannotCreateType(
_propertyName, typeof(TStructural).ShortDisplayName(), typeof(TCollection).ShortDisplayName()));
_propertyName, typeof(TEntity).ShortDisplayName(), typeof(TCollection).ShortDisplayName()));
}

collection = (ICollection<TElement>)_createAndSetCollection((TStructural)instance, setCollection);
collection = (ICollection<TElement>)_createAndSetCollection((TEntity)instance, setCollection);
}

return collection;
Expand All @@ -146,7 +147,7 @@ private ICollection<TElement> GetOrCreateCollection(object instance, bool forMat
return (ICollection<TElement>?)_createCollection?.Invoke();
}

var enumerable = _getCollection!((TStructural)instance);
var enumerable = _getCollection!((TEntity)instance);
var collection = enumerable as ICollection<TElement>;

if (enumerable != null
Expand All @@ -155,7 +156,7 @@ private ICollection<TElement> GetOrCreateCollection(object instance, bool forMat
throw new InvalidOperationException(
CoreStrings.NavigationBadType(
_propertyName,
typeof(TStructural).ShortDisplayName(),
typeof(TEntity).ShortDisplayName(),
enumerable.GetType().ShortDisplayName(),
typeof(TElement).ShortDisplayName()));
}
Expand All @@ -170,7 +171,7 @@ private ICollection<TElement> GetOrCreateCollection(object instance, bool forMat
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public virtual bool Contains(object entity, object value)
=> Contains(GetCollection((TStructural)entity), value);
=> Contains(GetCollection((TEntity)entity), value);

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
Expand All @@ -188,7 +189,7 @@ public virtual bool ContainsStandalone(object collection, object value)
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public virtual bool Remove(object entity, object value)
=> RemoveStandalone(GetCollection((TStructural)entity), value);
=> RemoveStandalone(GetCollection((TEntity)entity), value);

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
Expand Down
59 changes: 32 additions & 27 deletions src/EFCore/Metadata/Internal/ClrCollectionAccessorFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,19 +109,20 @@ private ClrCollectionAccessorFactory()
}

[UsedImplicitly]
private static IClrCollectionAccessor CreateGeneric<TStructural, TCollection, TElement>(IPropertyBase relationship)
private static IClrCollectionAccessor CreateGeneric<TEntity, TCollection, TElement>(IPropertyBase relationship)
where TEntity : class
where TCollection : class, IEnumerable<TElement>
where TElement : class
{
CreateExpressions<TStructural, TCollection, TElement>(
CreateExpressions<TEntity, TCollection, TElement>(
relationship,
out var getCollection,
out var setCollection,
out var setCollectionForMaterialization,
out var createAndSetCollection,
out var createCollection);

return new ClrCollectionAccessor<TStructural, TCollection, TElement>(
return new ClrCollectionAccessor<TEntity, TCollection, TElement>(
relationship.Name,
relationship.IsShadowProperty(),
getCollection?.Compile(),
Expand Down Expand Up @@ -179,13 +180,14 @@ private static readonly MethodInfo GenericCreateExpressions
= typeof(ClrCollectionAccessorFactory).GetMethod(nameof(CreateExpressions), BindingFlags.Static | BindingFlags.NonPublic)!;

[UsedImplicitly]
private static void CreateExpressions<TStructural, TCollection, TElement>(
private static void CreateExpressions<TEntity, TCollection, TElement>(
IPropertyBase relationship,
out Expression<Func<TStructural, TCollection>>? getCollection,
out Expression<Action<TStructural, TCollection>>? setCollection,
out Expression<Action<TStructural, TCollection>>? setCollectionForMaterialization,
out Expression<Func<TStructural, Action<TStructural, TCollection>, TCollection>>? createAndSetCollection,
out Expression<Func<TEntity, TCollection>>? getCollection,
out Expression<Action<TEntity, TCollection>>? setCollection,
out Expression<Action<TEntity, TCollection>>? setCollectionForMaterialization,
out Expression<Func<TEntity, Action<TEntity, TCollection>, TCollection>>? createAndSetCollection,
out Expression<Func<TCollection>>? createCollection)
where TEntity : class
where TCollection : class, IEnumerable<TElement>
where TElement : class
{
Expand All @@ -195,7 +197,7 @@ private static void CreateExpressions<TStructural, TCollection, TElement>(
createAndSetCollection = null;
createCollection = null;

var entityParameter = Expression.Parameter(typeof(TStructural), "entity");
var entityParameter = Expression.Parameter(typeof(TEntity), "entity");
var valueParameter = Expression.Parameter(typeof(TCollection), "collection");

if (!relationship.IsShadowProperty())
Expand All @@ -209,7 +211,7 @@ private static void CreateExpressions<TStructural, TCollection, TElement>(
memberAccessForRead = Expression.Convert(memberAccessForRead, typeof(TCollection));
}

getCollection = Expression.Lambda<Func<TStructural, TCollection>>(
getCollection = Expression.Lambda<Func<TEntity, TCollection>>(
memberAccessForRead,
entityParameter);

Expand All @@ -225,7 +227,7 @@ private static void CreateExpressions<TStructural, TCollection, TElement>(
}

var concreteType = CollectionTypeFactory.Instance.TryFindTypeToInstantiate(
typeof(TStructural),
typeof(TEntity),
typeof(TCollection),
relationship.DeclaringType.Model[CoreAnnotationNames.FullChangeTrackingNotificationsRequired] != null);
if (concreteType != null)
Expand All @@ -234,18 +236,18 @@ private static void CreateExpressions<TStructural, TCollection, TElement>(
if (setCollection != null
|| setCollectionForMaterialization != null)
{
var setterParameter = Expression.Parameter(typeof(Action<TStructural, TCollection>), "setter");
var setterParameter = Expression.Parameter(typeof(Action<TEntity, TCollection>), "setter");

var createAndSetCollectionMethod = isHashSet
? CreateAndSetHashSetMethod
.MakeGenericMethod(typeof(TStructural), typeof(TCollection), typeof(TElement))
.MakeGenericMethod(typeof(TEntity), typeof(TCollection), typeof(TElement))
: IsObservableHashSet(concreteType)
? CreateAndSetObservableHashSetMethod
.MakeGenericMethod(typeof(TStructural), typeof(TCollection), typeof(TElement))
.MakeGenericMethod(typeof(TEntity), typeof(TCollection), typeof(TElement))
: CreateAndSetMethod
.MakeGenericMethod(typeof(TStructural), typeof(TCollection), concreteType);
.MakeGenericMethod(typeof(TEntity), typeof(TCollection), concreteType);

createAndSetCollection = Expression.Lambda<Func<TStructural, Action<TStructural, TCollection>, TCollection>>(
createAndSetCollection = Expression.Lambda<Func<TEntity, Action<TEntity, TCollection>, TCollection>>(
Expression.Call(createAndSetCollectionMethod, entityParameter, setterParameter),
entityParameter,
setterParameter);
Expand All @@ -258,11 +260,11 @@ private static void CreateExpressions<TStructural, TCollection, TElement>(
: Expression.Lambda<Func<TCollection>>(Expression.New(concreteType));
}

static Expression<Action<TStructural, TCollection>> CreateSetterDelegate(
static Expression<Action<TEntity, TCollection>> CreateSetterDelegate(
ParameterExpression parameterExpression,
MemberInfo memberInfo,
ParameterExpression valueParameter1)
=> Expression.Lambda<Action<TStructural, TCollection>>(
=> Expression.Lambda<Action<TEntity, TCollection>>(
Expression.MakeMemberAccess(
parameterExpression,
memberInfo).Assign(
Expand Down Expand Up @@ -296,9 +298,10 @@ private static bool IsObservableHashSet(Type type)
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public static TCollection CreateAndSet<TStructural, TCollection, TConcreteCollection>(
TStructural entity,
Action<TStructural, TCollection> setterDelegate)
public static TCollection CreateAndSet<TEntity, TCollection, TConcreteCollection>(
TEntity entity,
Action<TEntity, TCollection> setterDelegate)
where TEntity : class
where TCollection : class
where TConcreteCollection : TCollection, new()
{
Expand All @@ -313,9 +316,10 @@ public static TCollection CreateAndSet<TStructural, TCollection, TConcreteCollec
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public static TCollection CreateAndSetHashSet<TStructural, TCollection, TElement>(
TStructural entity,
Action<TStructural, TCollection> setterDelegate)
public static TCollection CreateAndSetHashSet<TEntity, TCollection, TElement>(
TEntity entity,
Action<TEntity, TCollection> setterDelegate)
where TEntity : class
where TCollection : class
where TElement : class
{
Expand All @@ -330,9 +334,10 @@ public static TCollection CreateAndSetHashSet<TStructural, TCollection, TElement
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public static TCollection CreateAndSetObservableHashSet<TStructural, TCollection, TElement>(
TStructural entity,
Action<TStructural, TCollection> setterDelegate)
public static TCollection CreateAndSetObservableHashSet<TEntity, TCollection, TElement>(
TEntity entity,
Action<TEntity, TCollection> setterDelegate)
where TEntity : class
where TCollection : class
where TElement : class
{
Expand Down
Loading