Skip to content

Commit

Permalink
To render selectors for derived types, a selector for each non-abstra…
Browse files Browse the repository at this point in the history
…ct derived type must be available in `QueryLayer.Selection`, which wasn't the case.

Parsing of include chains is changed to include relationships on all concrete derived types.
For example, given an inclusion chain of "wheels" at `/vehicles`, the parser now returns `Car.Wheels` and `Truck.Wheels`. This used to be `Vehicle.Wheels`. There's no point in adding abstract types, because they can't be instantiated in selectors.

`QueryLayerComposer` was fixed to generate selectors for the left-type of these relationships, instead of for the `QueryLayer` resource type (so `Car.Wheels` and `Truck.Wheels` instead of `Vehicle.Wheels`).

`SelectClauseBuilder` was missing a cast to the derived type when descending into relationship selectors, which is fixed now.

Being unable to include relationships of sibling types after a POST/PATCH resource request at a base endpoint was because a `QueryLayer` for fetch-after-post was built against the accurized resource type, and then sent to the original (non-accurized) repository. For example, `POST /vehicles?include=TruckRelationship,CarRelationship` only works if the query executes against the non-accurized table (so `Vehicle` instead of `Car`, because `Car` doesn't contain `TruckRelationship`). The fix is to discard the accurized resource type and use the original `TResource`.

Other improvements:
- During the process of building expression trees, consistency checks have been added to prevent downstream crashes that are difficult to diagnose.
- `SelectClauseBuilder` internally passed along a `LambdaScope` that overruled the one present in context, so care had to be taken to use the right one. To eliminate this pitfall, now a new context is forked which contains the appropriate `LambdaScope`, so the overruling parameter is no longer needed.

Fixes #1639, fixes #1640.
  • Loading branch information
bkoelman committed Nov 26, 2024
1 parent a257c08 commit 7bf6be8
Show file tree
Hide file tree
Showing 12 changed files with 236 additions and 62 deletions.
1 change: 1 addition & 0 deletions JsonApiDotNetCore.sln.DotSettings
Original file line number Diff line number Diff line change
Expand Up @@ -668,6 +668,7 @@ $left$ = $right$;</s:String>
<s:String x:Key="/Default/PatternsAndTemplates/StructuralSearch/Pattern/=D29C3A091CD9E74BBFDF1B8974F5A977/SearchPattern/@EntryValue">if ($argument$ is null) throw new ArgumentNullException(nameof($argument$));</s:String>
<s:String x:Key="/Default/PatternsAndTemplates/StructuralSearch/Pattern/=D29C3A091CD9E74BBFDF1B8974F5A977/Severity/@EntryValue">WARNING</s:String>
<s:Boolean x:Key="/Default/UserDictionary/Words/=Accurize/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=accurized/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=appsettings/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=Assignee/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=Contoso/@EntryIndexedValue">True</s:Boolean>
Expand Down
28 changes: 27 additions & 1 deletion src/JsonApiDotNetCore/Queries/Parsing/IncludeParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ private static ReadOnlyCollection<IncludeTreeNode> LookupRelationshipName(string
{
// Depending on the left side of the include chain, we may match relationships anywhere in the resource type hierarchy.
// This is compensated for when rendering the response, which substitutes relationships on base types with the derived ones.
IReadOnlySet<RelationshipAttribute> relationships = parent.Relationship.RightType.GetRelationshipsInTypeOrDerived(relationshipName);
HashSet<RelationshipAttribute> relationships = GetRelationshipsInConcreteTypes(parent.Relationship.RightType, relationshipName);

if (relationships.Count > 0)
{
Expand All @@ -140,6 +140,32 @@ private static ReadOnlyCollection<IncludeTreeNode> LookupRelationshipName(string
return children.AsReadOnly();
}

private static HashSet<RelationshipAttribute> GetRelationshipsInConcreteTypes(ResourceType resourceType, string relationshipName)
{
HashSet<RelationshipAttribute> relationshipsToInclude = [];

foreach (RelationshipAttribute relationship in resourceType.GetRelationshipsInTypeOrDerived(relationshipName))
{
if (!relationship.LeftType.ClrType.IsAbstract)
{
relationshipsToInclude.Add(relationship);
}

IncludeRelationshipsFromConcreteDerivedTypes(relationship, relationshipsToInclude);
}

return relationshipsToInclude;
}

private static void IncludeRelationshipsFromConcreteDerivedTypes(RelationshipAttribute relationship, HashSet<RelationshipAttribute> relationshipsToInclude)
{
foreach (ResourceType derivedType in relationship.LeftType.GetAllConcreteDerivedTypes())
{
RelationshipAttribute relationshipInDerived = derivedType.GetRelationshipByPublicName(relationship.PublicName);
relationshipsToInclude.Add(relationshipInDerived);
}
}

private static void AssertRelationshipsFound(HashSet<RelationshipAttribute> relationshipsFound, string relationshipName,
IReadOnlyCollection<IncludeTreeNode> parents, int position)
{
Expand Down
2 changes: 1 addition & 1 deletion src/JsonApiDotNetCore/Queries/QueryLayerComposer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ private IImmutableSet<IncludeElementExpression> ProcessIncludeSet(IImmutableSet<
foreach (IncludeElementExpression includeElement in includeElementsEvaluated)
{
parentLayer.Selection ??= new FieldSelection();
FieldSelectors selectors = parentLayer.Selection.GetOrCreateSelectors(parentLayer.ResourceType);
FieldSelectors selectors = parentLayer.Selection.GetOrCreateSelectors(includeElement.Relationship.LeftType);

if (!selectors.ContainsField(includeElement.Relationship))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ public QueryClauseBuilderContext(Expression source, ResourceType resourceType, T
ArgumentGuard.NotNull(lambdaScopeFactory);
ArgumentGuard.NotNull(lambdaScope);
ArgumentGuard.NotNull(queryableBuilder);
AssertSameType(source.Type, resourceType);

Source = source;
ResourceType = resourceType;
Expand All @@ -72,6 +73,17 @@ public QueryClauseBuilderContext(Expression source, ResourceType resourceType, T
State = state;
}

private static void AssertSameType(Type sourceType, ResourceType resourceType)
{
Type? sourceElementType = CollectionConverter.Instance.FindCollectionElementType(sourceType);

if (sourceElementType != resourceType.ClrType)
{
throw new InvalidOperationException(
$"Internal error: Mismatch between expression type '{sourceElementType?.Name}' and resource type '{resourceType.ClrType.Name}'.");
}
}

public QueryClauseBuilderContext WithSource(Expression source)
{
ArgumentGuard.NotNull(source);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ public virtual Expression ApplyQuery(QueryLayer layer, QueryableBuilderContext c
{
ArgumentGuard.NotNull(layer);
ArgumentGuard.NotNull(context);
AssertSameType(layer.ResourceType, context.ElementType);

Expression expression = context.Source;

Expand Down Expand Up @@ -66,6 +67,15 @@ public virtual Expression ApplyQuery(QueryLayer layer, QueryableBuilderContext c
return expression;
}

private static void AssertSameType(ResourceType resourceType, Type elementType)
{
if (elementType != resourceType.ClrType)
{
throw new InvalidOperationException(
$"Internal error: Mismatch between query layer type '{resourceType.ClrType.Name}' and query element type '{elementType.Name}'.");
}
}

protected virtual Expression ApplyInclude(Expression source, IncludeExpression include, ResourceType resourceType, QueryableBuilderContext context)
{
ArgumentGuard.NotNull(source);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,36 +29,47 @@ public virtual Expression ApplySelect(FieldSelection selection, QueryClauseBuild
{
ArgumentGuard.NotNull(selection);

Expression bodyInitializer = CreateLambdaBodyInitializer(selection, context.ResourceType, context.LambdaScope, false, context);
Expression bodyInitializer = CreateLambdaBodyInitializer(selection, context.ResourceType, false, context);

LambdaExpression lambda = Expression.Lambda(bodyInitializer, context.LambdaScope.Parameter);

return SelectExtensionMethodCall(context.ExtensionType, context.Source, context.LambdaScope.Parameter.Type, lambda);
}

private Expression CreateLambdaBodyInitializer(FieldSelection selection, ResourceType resourceType, LambdaScope lambdaScope,
bool lambdaAccessorRequiresTestForNull, QueryClauseBuilderContext context)
private Expression CreateLambdaBodyInitializer(FieldSelection selection, ResourceType resourceType, bool lambdaAccessorRequiresTestForNull,
QueryClauseBuilderContext context)
{
AssertSameType(context.LambdaScope.Accessor.Type, resourceType);

IReadOnlyEntityType entityType = context.EntityModel.FindEntityType(resourceType.ClrType)!;
IReadOnlyEntityType[] concreteEntityTypes = entityType.GetConcreteDerivedTypesInclusive().ToArray();

Expression bodyInitializer = concreteEntityTypes.Length > 1
? CreateLambdaBodyInitializerForTypeHierarchy(selection, resourceType, concreteEntityTypes, lambdaScope, context)
: CreateLambdaBodyInitializerForSingleType(selection, resourceType, lambdaScope, context);
? CreateLambdaBodyInitializerForTypeHierarchy(selection, resourceType, concreteEntityTypes, context)
: CreateLambdaBodyInitializerForSingleType(selection, resourceType, context);

if (!lambdaAccessorRequiresTestForNull)
{
return bodyInitializer;
}

return TestForNull(lambdaScope.Accessor, bodyInitializer);
return TestForNull(context.LambdaScope.Accessor, bodyInitializer);
}

private static void AssertSameType(Type lambdaAccessorType, ResourceType resourceType)
{
if (lambdaAccessorType != resourceType.ClrType)
{
throw new InvalidOperationException(
$"Internal error: Mismatch between lambda accessor type '{lambdaAccessorType.Name}' and resource type '{resourceType.ClrType.Name}'.");
}
}

private Expression CreateLambdaBodyInitializerForTypeHierarchy(FieldSelection selection, ResourceType baseResourceType,
IEnumerable<IReadOnlyEntityType> concreteEntityTypes, LambdaScope lambdaScope, QueryClauseBuilderContext context)
IEnumerable<IReadOnlyEntityType> concreteEntityTypes, QueryClauseBuilderContext context)
{
IReadOnlySet<ResourceType> resourceTypes = selection.GetResourceTypes();
Expression rootCondition = lambdaScope.Accessor;
Expression rootCondition = context.LambdaScope.Accessor;

foreach (IReadOnlyEntityType entityType in concreteEntityTypes)
{
Expand All @@ -73,14 +84,14 @@ private Expression CreateLambdaBodyInitializerForTypeHierarchy(FieldSelection se
Dictionary<PropertyInfo, PropertySelector>.ValueCollection propertySelectors =
ToPropertySelectors(fieldSelectors, resourceType, entityType.ClrType, context.EntityModel);

MemberBinding[] propertyAssignments = propertySelectors.Select(selector => CreatePropertyAssignment(selector, lambdaScope, context))
MemberBinding[] propertyAssignments = propertySelectors.Select(selector => CreatePropertyAssignment(selector, context))
.Cast<MemberBinding>().ToArray();

NewExpression createInstance = _resourceFactory.CreateNewExpression(entityType.ClrType);
MemberInitExpression memberInit = Expression.MemberInit(createInstance, propertyAssignments);
UnaryExpression castToBaseType = Expression.Convert(memberInit, baseResourceType.ClrType);

BinaryExpression typeCheck = CreateRuntimeTypeCheck(lambdaScope, entityType.ClrType);
BinaryExpression typeCheck = CreateRuntimeTypeCheck(context.LambdaScope, entityType.ClrType);
rootCondition = Expression.Condition(typeCheck, castToBaseType, rootCondition);
}
}
Expand All @@ -100,18 +111,16 @@ private static BinaryExpression CreateRuntimeTypeCheck(LambdaScope lambdaScope,
return Expression.MakeBinary(ExpressionType.Equal, getTypeCall, concreteTypeConstant, false, TypeOpEqualityMethod);
}

private MemberInitExpression CreateLambdaBodyInitializerForSingleType(FieldSelection selection, ResourceType resourceType, LambdaScope lambdaScope,
private MemberInitExpression CreateLambdaBodyInitializerForSingleType(FieldSelection selection, ResourceType resourceType,
QueryClauseBuilderContext context)
{
FieldSelectors fieldSelectors = selection.GetOrCreateSelectors(resourceType);

Dictionary<PropertyInfo, PropertySelector>.ValueCollection propertySelectors =
ToPropertySelectors(fieldSelectors, resourceType, lambdaScope.Accessor.Type, context.EntityModel);

MemberBinding[] propertyAssignments = propertySelectors.Select(selector => CreatePropertyAssignment(selector, lambdaScope, context))
.Cast<MemberBinding>().ToArray();
ToPropertySelectors(fieldSelectors, resourceType, context.LambdaScope.Accessor.Type, context.EntityModel);

NewExpression createInstance = _resourceFactory.CreateNewExpression(lambdaScope.Accessor.Type);
MemberBinding[] propertyAssignments = propertySelectors.Select(selector => CreatePropertyAssignment(selector, context)).Cast<MemberBinding>().ToArray();
NewExpression createInstance = _resourceFactory.CreateNewExpression(context.LambdaScope.Accessor.Type);
return Expression.MemberInit(createInstance, propertyAssignments);
}

Expand Down Expand Up @@ -182,50 +191,56 @@ private static void IncludeEagerLoads(ResourceType resourceType, Dictionary<Prop
}
}

private MemberAssignment CreatePropertyAssignment(PropertySelector propertySelector, LambdaScope lambdaScope, QueryClauseBuilderContext context)
private MemberAssignment CreatePropertyAssignment(PropertySelector propertySelector, QueryClauseBuilderContext context)
{
bool requiresUpCast = lambdaScope.Accessor.Type != propertySelector.Property.DeclaringType &&
lambdaScope.Accessor.Type.IsAssignableFrom(propertySelector.Property.DeclaringType);
bool requiresUpCast = context.LambdaScope.Accessor.Type != propertySelector.Property.DeclaringType &&
context.LambdaScope.Accessor.Type.IsAssignableFrom(propertySelector.Property.DeclaringType);

UnaryExpression? derivedAccessor = requiresUpCast ? Expression.Convert(context.LambdaScope.Accessor, propertySelector.Property.DeclaringType!) : null;

MemberExpression propertyAccess = requiresUpCast
? Expression.MakeMemberAccess(Expression.Convert(lambdaScope.Accessor, propertySelector.Property.DeclaringType!), propertySelector.Property)
: Expression.Property(lambdaScope.Accessor, propertySelector.Property);
MemberExpression propertyAccess = derivedAccessor != null
? Expression.MakeMemberAccess(derivedAccessor, propertySelector.Property)
: Expression.Property(context.LambdaScope.Accessor, propertySelector.Property);

Expression assignmentRightHandSide = propertyAccess;

if (propertySelector.NextLayer != null)
{
assignmentRightHandSide =
CreateAssignmentRightHandSideForLayer(propertySelector.NextLayer, lambdaScope, propertyAccess, propertySelector.Property, context);
QueryClauseBuilderContext rightHandSideContext =
derivedAccessor != null ? context.WithLambdaScope(context.LambdaScope.WithAccessor(derivedAccessor)) : context;

assignmentRightHandSide = CreateAssignmentRightHandSideForLayer(propertySelector.NextLayer, propertyAccess,
propertySelector.Property, rightHandSideContext);
}

return Expression.Bind(propertySelector.Property, assignmentRightHandSide);
}

private Expression CreateAssignmentRightHandSideForLayer(QueryLayer layer, LambdaScope outerLambdaScope, MemberExpression propertyAccess,
PropertyInfo selectorPropertyInfo, QueryClauseBuilderContext context)
private Expression CreateAssignmentRightHandSideForLayer(QueryLayer layer, MemberExpression propertyAccess, PropertyInfo selectorPropertyInfo,
QueryClauseBuilderContext context)
{
Type? collectionElementType = CollectionConverter.Instance.FindCollectionElementType(selectorPropertyInfo.PropertyType);
Type bodyElementType = collectionElementType ?? selectorPropertyInfo.PropertyType;

if (collectionElementType != null)
{
return CreateCollectionInitializer(outerLambdaScope, selectorPropertyInfo, bodyElementType, layer, context);
return CreateCollectionInitializer(selectorPropertyInfo, bodyElementType, layer, context);
}

if (layer.Selection == null || layer.Selection.IsEmpty)
{
return propertyAccess;
}

using LambdaScope scope = context.LambdaScopeFactory.CreateScope(bodyElementType, propertyAccess);
return CreateLambdaBodyInitializer(layer.Selection, layer.ResourceType, scope, true, context);
using LambdaScope initializerScope = context.LambdaScopeFactory.CreateScope(bodyElementType, propertyAccess);
QueryClauseBuilderContext initializerContext = context.WithLambdaScope(initializerScope);
return CreateLambdaBodyInitializer(layer.Selection, layer.ResourceType, true, initializerContext);
}

private static MethodCallExpression CreateCollectionInitializer(LambdaScope lambdaScope, PropertyInfo collectionProperty, Type elementType,
QueryLayer layer, QueryClauseBuilderContext context)
private static MethodCallExpression CreateCollectionInitializer(PropertyInfo collectionProperty, Type elementType, QueryLayer layer,
QueryClauseBuilderContext context)
{
MemberExpression propertyExpression = Expression.Property(lambdaScope.Accessor, collectionProperty);
MemberExpression propertyExpression = Expression.Property(context.LambdaScope.Accessor, collectionProperty);

var nestedContext = new QueryableBuilderContext(propertyExpression, elementType, typeof(Enumerable), context.EntityModel, context.LambdaScopeFactory,
context.State);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ namespace JsonApiDotNetCore.Repositories;
/// </summary>
public interface IResourceRepositoryAccessor
{
/// <summary>
/// Uses the <see cref="IResourceGraph" /> to lookup the corresponding <see cref="ResourceType" /> for the specified CLR type.
/// </summary>
ResourceType LookupResourceType(Type resourceClrType);

/// <summary>
/// Invokes <see cref="IResourceReadRepository{TResource,TId}.GetAsync" /> for the specified resource type.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,14 @@ public ResourceRepositoryAccessor(IServiceProvider serviceProvider, IResourceGra
_request = request;
}

/// <inheritdoc />
public ResourceType LookupResourceType(Type resourceClrType)
{
ArgumentGuard.NotNull(resourceClrType);

return _resourceGraph.GetResourceType(resourceClrType);
}

/// <inheritdoc />
public async Task<IReadOnlyCollection<TResource>> GetAsync<TResource>(QueryLayer queryLayer, CancellationToken cancellationToken)
where TResource : class, IIdentifiable
Expand Down
Loading

0 comments on commit 7bf6be8

Please sign in to comment.