Skip to content
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.

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

Resource inheritance fixes #1641

Merged
merged 3 commits into from
Nov 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions Directory.Build.props
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
<CodeAnalysisRuleSet>$(MSBuildThisFileDirectory)CodingGuidelines.ruleset</CodeAnalysisRuleSet>
<RunSettingsFilePath>$(MSBuildThisFileDirectory)tests.runsettings</RunSettingsFilePath>
<JsonApiDotNetCoreVersionPrefix>5.6.1</JsonApiDotNetCoreVersionPrefix>
<NuGetAuditMode>direct</NuGetAuditMode>
<NoWarn>$(NoWarn);NETSDK1215</NoWarn>
</PropertyGroup>

<PropertyGroup>
Expand Down
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
11 changes: 6 additions & 5 deletions src/Examples/DapperExample/Repositories/DapperRepository.cs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ public sealed partial class DapperRepository<TResource, TId> : IResourceReposito
private readonly SqlCaptureStore _captureStore;
private readonly ILoggerFactory _loggerFactory;
private readonly ILogger<DapperRepository<TResource, TId>> _logger;
private readonly CollectionConverter _collectionConverter = new();
private readonly ParameterFormatter _parameterFormatter = new();
private readonly DapperFacade _dapperFacade;

Expand Down Expand Up @@ -270,12 +269,12 @@ private async Task ApplyTargetedFieldsAsync(TResource resourceFromRequest, TReso

if (relationship is HasManyAttribute hasManyRelationship)
{
HashSet<IIdentifiable> rightResourceIds = _collectionConverter.ExtractResources(rightValue).ToHashSet(IdentifiableComparer.Instance);
HashSet<IIdentifiable> rightResourceIds = CollectionConverter.Instance.ExtractResources(rightValue).ToHashSet(IdentifiableComparer.Instance);

await _resourceDefinitionAccessor.OnSetToManyRelationshipAsync(leftResource, hasManyRelationship, rightResourceIds, writeOperation,
cancellationToken);

return _collectionConverter.CopyToTypedCollection(rightResourceIds, relationship.Property.PropertyType);
return CollectionConverter.Instance.CopyToTypedCollection(rightResourceIds, relationship.Property.PropertyType);
}

return rightValue;
Expand Down Expand Up @@ -464,7 +463,9 @@ public async Task AddToToManyRelationshipAsync(TResource? leftResource, [Disallo
leftPlaceholderResource.Id = leftId;

await _resourceDefinitionAccessor.OnAddToRelationshipAsync(leftPlaceholderResource, relationship, rightResourceIds, cancellationToken);
relationship.SetValue(leftPlaceholderResource, _collectionConverter.CopyToTypedCollection(rightResourceIds, relationship.Property.PropertyType));

relationship.SetValue(leftPlaceholderResource,
CollectionConverter.Instance.CopyToTypedCollection(rightResourceIds, relationship.Property.PropertyType));

await _resourceDefinitionAccessor.OnWritingAsync(leftPlaceholderResource, WriteOperationKind.AddToRelationship, cancellationToken);

Expand Down Expand Up @@ -500,7 +501,7 @@ public async Task RemoveFromToManyRelationshipAsync(TResource leftResource, ISet
var relationship = (HasManyAttribute)_targetedFields.Relationships.Single();

await _resourceDefinitionAccessor.OnRemoveFromRelationshipAsync(leftResource, relationship, rightResourceIds, cancellationToken);
relationship.SetValue(leftResource, _collectionConverter.CopyToTypedCollection(rightResourceIds, relationship.Property.PropertyType));
relationship.SetValue(leftResource, CollectionConverter.Instance.CopyToTypedCollection(rightResourceIds, relationship.Property.PropertyType));

await _resourceDefinitionAccessor.OnWritingAsync(leftResource, WriteOperationKind.RemoveFromRelationship, cancellationToken);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ namespace DapperExample.Repositories;
/// </summary>
internal sealed class ResourceChangeDetector
{
private readonly CollectionConverter _collectionConverter = new();
private readonly IDataModelService _dataModelService;

private Dictionary<string, object?> _currentColumnValues = [];
Expand Down Expand Up @@ -69,7 +68,7 @@ private Dictionary<RelationshipAttribute, HashSet<IIdentifiable>> CaptureRightRe
foreach (RelationshipAttribute relationship in ResourceType.Relationships)
{
object? rightValue = relationship.GetValue(resource);
HashSet<IIdentifiable> rightResources = _collectionConverter.ExtractResources(rightValue).ToHashSet(IdentifiableComparer.Instance);
HashSet<IIdentifiable> rightResources = CollectionConverter.Instance.ExtractResources(rightValue).ToHashSet(IdentifiableComparer.Instance);

relationshipValues[relationship] = rightResources;
}
Expand Down
34 changes: 29 additions & 5 deletions src/JsonApiDotNetCore.Annotations/CollectionConverter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,24 @@ internal sealed class CollectionConverter
typeof(IEnumerable<>)
];

public static CollectionConverter Instance { get; } = new();

private CollectionConverter()
{
}

/// <summary>
/// Creates a collection instance based on the specified collection type and copies the specified elements into it.
/// </summary>
/// <param name="source">
/// Source to copy from.
/// </param>
/// <param name="collectionType">
/// Target collection type, for example: typeof(List{Article}) or typeof(ISet{Person}).
/// Target collection type, for example: <code><![CDATA[
/// typeof(List<Article>)
/// ]]></code> or <code><![CDATA[
/// typeof(ISet<Person>)
/// ]]></code>.
/// </param>
public IEnumerable CopyToTypedCollection(IEnumerable source, Type collectionType)
{
Expand All @@ -41,7 +51,12 @@ public IEnumerable CopyToTypedCollection(IEnumerable source, Type collectionType
}

/// <summary>
/// Returns a compatible collection type that can be instantiated, for example IList{Article} -> List{Article} or ISet{Article} -> HashSet{Article}
/// Returns a compatible collection type that can be instantiated, for example: <code><![CDATA[
/// IList<Article> -> List<Article>
/// ]]></code> or
/// <code><![CDATA[
/// ISet<Article> -> HashSet<Article>
/// ]]></code>.
/// </summary>
private Type ToConcreteCollectionType(Type collectionType)
{
Expand Down Expand Up @@ -80,7 +95,12 @@ public IReadOnlyCollection<IIdentifiable> ExtractResources(object? value)
}

/// <summary>
/// Returns the element type if the specified type is a generic collection, for example: IList{string} -> string or IList -> null.
/// Returns the element type if the specified type is a generic collection, for example: <code><![CDATA[
/// IList<string> -> string
/// ]]></code> or
/// <code><![CDATA[
/// IList -> null
/// ]]></code>.
/// </summary>
public Type? FindCollectionElementType(Type? type)
{
Expand All @@ -96,8 +116,12 @@ public IReadOnlyCollection<IIdentifiable> ExtractResources(object? value)
}

/// <summary>
/// Indicates whether a <see cref="HashSet{T}" /> instance can be assigned to the specified type, for example IList{Article} -> false or ISet{Article} ->
/// true.
/// Indicates whether a <see cref="HashSet{T}" /> instance can be assigned to the specified type, for example:
/// <code><![CDATA[
/// IList<Article> -> false
/// ]]></code> or <code><![CDATA[
/// ISet<Article> -> true
/// ]]></code>.
/// </summary>
public bool TypeCanContainHashSet(Type collectionType)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ private bool EvaluateIsManyToMany()
{
if (InverseNavigationProperty != null)
{
Type? elementType = CollectionConverter.FindCollectionElementType(InverseNavigationProperty.PropertyType);
Type? elementType = CollectionConverter.Instance.FindCollectionElementType(InverseNavigationProperty.PropertyType);
return elementType != null;
}

Expand Down Expand Up @@ -103,14 +103,14 @@ public void AddValue(object resource, IIdentifiable resourceToAdd)
ArgumentGuard.NotNull(resourceToAdd);

object? rightValue = GetValue(resource);
List<IIdentifiable> rightResources = CollectionConverter.ExtractResources(rightValue).ToList();
List<IIdentifiable> rightResources = CollectionConverter.Instance.ExtractResources(rightValue).ToList();

if (!rightResources.Exists(nextResource => nextResource == resourceToAdd))
{
rightResources.Add(resourceToAdd);

Type collectionType = rightValue?.GetType() ?? Property.PropertyType;
IEnumerable typedCollection = CollectionConverter.CopyToTypedCollection(rightResources, collectionType);
IEnumerable typedCollection = CollectionConverter.Instance.CopyToTypedCollection(rightResources, collectionType);
base.SetValue(resource, typedCollection);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ private bool EvaluateIsOneToOne()
{
if (InverseNavigationProperty != null)
{
Type? elementType = CollectionConverter.FindCollectionElementType(InverseNavigationProperty.PropertyType);
Type? elementType = CollectionConverter.Instance.FindCollectionElementType(InverseNavigationProperty.PropertyType);
return elementType == null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ namespace JsonApiDotNetCore.Resources.Annotations;
[PublicAPI]
public abstract class RelationshipAttribute : ResourceFieldAttribute
{
private protected static readonly CollectionConverter CollectionConverter = new();

// This field is definitely assigned after building the resource graph, which is why its public equivalent is declared as non-nullable.
private ResourceType? _rightType;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ namespace JsonApiDotNetCore.AtomicOperations.Processors;
public class SetRelationshipProcessor<TResource, TId> : ISetRelationshipProcessor<TResource, TId>
where TResource : class, IIdentifiable<TId>
{
private readonly CollectionConverter _collectionConverter = new();
private readonly ISetRelationshipService<TResource, TId> _service;

public SetRelationshipProcessor(ISetRelationshipService<TResource, TId> service)
Expand Down Expand Up @@ -40,7 +39,7 @@ public SetRelationshipProcessor(ISetRelationshipService<TResource, TId> service)

if (relationship is HasManyAttribute)
{
IReadOnlyCollection<IIdentifiable> rightResources = _collectionConverter.ExtractResources(rightValue);
IReadOnlyCollection<IIdentifiable> rightResources = CollectionConverter.Instance.ExtractResources(rightValue);
return rightResources.ToHashSet(IdentifiableComparer.Instance);
}

Expand Down
4 changes: 1 addition & 3 deletions src/JsonApiDotNetCore/Errors/InvalidModelStateException.cs
Original file line number Diff line number Diff line change
Expand Up @@ -319,8 +319,6 @@ private sealed class ArrayIndexerSegment(
Func<Type, int, Type?>? getCollectionElementTypeCallback)
: ModelStateKeySegment(modelType, isInComplexType, nextKey, sourcePointer, parent, getCollectionElementTypeCallback)
{
private static readonly CollectionConverter CollectionConverter = new();

public int ArrayIndex { get; } = arrayIndex;

public Type GetCollectionElementType()
Expand All @@ -333,7 +331,7 @@ private Type GetDeclaredCollectionElementType()
{
if (ModelType != typeof(string))
{
Type? elementType = CollectionConverter.FindCollectionElementType(ModelType);
Type? elementType = CollectionConverter.Instance.FindCollectionElementType(ModelType);

if (elementType != null)
{
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
5 changes: 2 additions & 3 deletions src/JsonApiDotNetCore/Queries/QueryLayerComposer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ namespace JsonApiDotNetCore.Queries;
[PublicAPI]
public class QueryLayerComposer : IQueryLayerComposer
{
private readonly CollectionConverter _collectionConverter = new();
private readonly IQueryConstraintProvider[] _constraintProviders;
private readonly IResourceDefinitionAccessor _resourceDefinitionAccessor;
private readonly IJsonApiOptions _options;
Expand Down Expand Up @@ -213,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 Expand Up @@ -413,7 +412,7 @@ public QueryLayer ComposeForUpdate<TId>([DisallowNull] TId id, ResourceType prim
foreach (RelationshipAttribute relationship in _targetedFields.Relationships)
{
object? rightValue = relationship.GetValue(primaryResource);
HashSet<IIdentifiable> rightResourceIds = _collectionConverter.ExtractResources(rightValue).ToHashSet(IdentifiableComparer.Instance);
HashSet<IIdentifiable> rightResourceIds = CollectionConverter.Instance.ExtractResources(rightValue).ToHashSet(IdentifiableComparer.Instance);

if (rightResourceIds.Count > 0)
{
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
Loading
Loading