Merge pull request #1641 from json-api-dotnet/resource-inheritance-fixes

Resource inheritance fixes
json-api-dotnet · Nov 26, 2024 · 0c79d35 · 0c79d35
2 parents 55f671c + cfe159b
commit 0c79d35
Show file tree

Hide file tree

Showing 26 changed files with 302 additions and 118 deletions.
diff --git a/Directory.Build.props b/Directory.Build.props
@@ -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>

diff --git a/JsonApiDotNetCore.sln.DotSettings b/JsonApiDotNetCore.sln.DotSettings
@@ -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>

diff --git a/src/Examples/DapperExample/Repositories/DapperRepository.cs b/src/Examples/DapperExample/Repositories/DapperRepository.cs
@@ -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;
 
@@ -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;
@@ -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);
 
@@ -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);
 

diff --git a/src/Examples/DapperExample/Repositories/ResourceChangeDetector.cs b/src/Examples/DapperExample/Repositories/ResourceChangeDetector.cs
@@ -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 = [];
@@ -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;
         }

diff --git a/src/JsonApiDotNetCore.Annotations/CollectionConverter.cs b/src/JsonApiDotNetCore.Annotations/CollectionConverter.cs
@@ -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)
     {
@@ -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)
     {
@@ -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)
     {
@@ -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)
     {

diff --git a/src/JsonApiDotNetCore.Annotations/Resources/Annotations/HasManyAttribute.cs b/src/JsonApiDotNetCore.Annotations/Resources/Annotations/HasManyAttribute.cs
@@ -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;
         }
 
@@ -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);
         }
     }

diff --git a/src/JsonApiDotNetCore.Annotations/Resources/Annotations/HasOneAttribute.cs b/src/JsonApiDotNetCore.Annotations/Resources/Annotations/HasOneAttribute.cs
@@ -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;
         }
 

diff --git a/src/JsonApiDotNetCore.Annotations/Resources/Annotations/RelationshipAttribute.cs b/src/JsonApiDotNetCore.Annotations/Resources/Annotations/RelationshipAttribute.cs
@@ -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;
 

diff --git a/src/JsonApiDotNetCore/AtomicOperations/Processors/SetRelationshipProcessor.cs b/src/JsonApiDotNetCore/AtomicOperations/Processors/SetRelationshipProcessor.cs
@@ -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)
@@ -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);
         }
 

diff --git a/src/JsonApiDotNetCore/Errors/InvalidModelStateException.cs b/src/JsonApiDotNetCore/Errors/InvalidModelStateException.cs
@@ -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()
@@ -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)
                 {

diff --git a/src/JsonApiDotNetCore/Queries/Parsing/IncludeParser.cs b/src/JsonApiDotNetCore/Queries/Parsing/IncludeParser.cs
@@ -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)
             {
@@ -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)
     {

diff --git a/src/JsonApiDotNetCore/Queries/QueryLayerComposer.cs b/src/JsonApiDotNetCore/Queries/QueryLayerComposer.cs
@@ -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;
@@ -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))
             {
@@ -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)
             {

diff --git a/src/JsonApiDotNetCore/Queries/QueryableBuilding/QueryClauseBuilderContext.cs b/src/JsonApiDotNetCore/Queries/QueryableBuilding/QueryClauseBuilderContext.cs
@@ -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;
@@ -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);

diff --git a/src/JsonApiDotNetCore/Queries/QueryableBuilding/QueryableBuilder.cs b/src/JsonApiDotNetCore/Queries/QueryableBuilding/QueryableBuilder.cs
@@ -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;
 
@@ -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);