Added extra validations of the resource graph and unified unit tests

Author: Bart Koelman

src/JsonApiDotNetCore/Configuration/ResourceGraphBuilder.cs

@@ -21,7 +21,7 @@ public class ResourceGraphBuilder
     {
         private readonly IJsonApiOptions _options;
         private readonly ILogger<ResourceGraphBuilder> _logger;
-        private readonly HashSet<ResourceType> _resourceTypes = new();
+        private readonly Dictionary<Type, ResourceType> _resourceTypesByClrType = new();
         private readonly TypeLocator _typeLocator = new();
 
         public ResourceGraphBuilder(IJsonApiOptions options, ILoggerFactory loggerFactory)
@@ -38,12 +38,27 @@ public ResourceGraphBuilder(IJsonApiOptions options, ILoggerFactory loggerFactor
         /// </summary>
         public IResourceGraph Build()
         {
-            var resourceGraph = new ResourceGraph(_resourceTypes);
+            HashSet<ResourceType> resourceTypes = _resourceTypesByClrType.Values.ToHashSet();
 
-            foreach (RelationshipAttribute relationship in _resourceTypes.SelectMany(resourceType => resourceType.Relationships))
+            if (!resourceTypes.Any())
+            {
+                _logger.LogWarning("The resource graph is empty.");
+            }
+
+            var resourceGraph = new ResourceGraph(resourceTypes);
+
+            foreach (RelationshipAttribute relationship in resourceTypes.SelectMany(resourceType => resourceType.Relationships))
             {
                 relationship.LeftType = resourceGraph.GetResourceType(relationship.LeftClrType!);
-                relationship.RightType = resourceGraph.GetResourceType(relationship.RightClrType!);
+                ResourceType? rightType = resourceGraph.FindResourceType(relationship.RightClrType!);
+
+                if (rightType == null)
+                {
+                    throw new InvalidConfigurationException($"Resource type '{relationship.LeftClrType}' depends on " +
+                        $"'{relationship.RightClrType}', which was not added to the resource graph.");
+                }
+
+                relationship.RightType = rightType;
             }
 
             return resourceGraph;
@@ -123,7 +138,7 @@ public ResourceGraphBuilder Add(Type resourceClrType, Type? idClrType = null, st
         {
             ArgumentGuard.NotNull(resourceClrType, nameof(resourceClrType));
 
-            if (_resourceTypes.Any(resourceType => resourceType.ClrType == resourceClrType))
+            if (_resourceTypesByClrType.ContainsKey(resourceClrType))
             {
                 return this;
             }
@@ -139,7 +154,10 @@ public ResourceGraphBuilder Add(Type resourceClrType, Type? idClrType = null, st
                 }
 
                 ResourceType resourceType = CreateResourceType(effectivePublicName, resourceClrType, effectiveIdType);
-                _resourceTypes.Add(resourceType);
+
+                AssertNoDuplicatePublicName(resourceType, effectivePublicName);
+
+                _resourceTypesByClrType.Add(resourceClrType, resourceType);
             }
             else
             {
@@ -155,6 +173,8 @@ private ResourceType CreateResourceType(string publicName, Type resourceClrType,
             IReadOnlyCollection<RelationshipAttribute> relationships = GetRelationships(resourceClrType);
             IReadOnlyCollection<EagerLoadAttribute> eagerLoads = GetEagerLoads(resourceClrType);
 
+            AssertNoDuplicatePublicName(attributes, relationships);
+
             var linksAttribute = (ResourceLinksAttribute?)resourceClrType.GetCustomAttribute(typeof(ResourceLinksAttribute));
 
             return linksAttribute == null
@@ -165,7 +185,7 @@ private ResourceType CreateResourceType(string publicName, Type resourceClrType,
 
         private IReadOnlyCollection<AttrAttribute> GetAttributes(Type resourceClrType)
         {
-            var attributes = new List<AttrAttribute>();
+            var attributesByName = new Dictionary<string, AttrAttribute>();
 
             foreach (PropertyInfo property in resourceClrType.GetProperties())
             {
@@ -181,7 +201,7 @@ private IReadOnlyCollection<AttrAttribute> GetAttributes(Type resourceClrType)
                         Capabilities = _options.DefaultAttrCapabilities
                     };
 
-                    attributes.Add(idAttr);
+                    IncludeField(attributesByName, idAttr);
                     continue;
                 }
 
@@ -200,15 +220,20 @@ private IReadOnlyCollection<AttrAttribute> GetAttributes(Type resourceClrType)
                     attribute.Capabilities = _options.DefaultAttrCapabilities;
                 }
 
-                attributes.Add(attribute);
+                IncludeField(attributesByName, attribute);
             }
 
-            return attributes;
+            if (attributesByName.Count == 1)
+            {
+                _logger.LogWarning($"Type '{resourceClrType}' does not contain any attributes.");
+            }
+
+            return attributesByName.Values;
         }
 
         private IReadOnlyCollection<RelationshipAttribute> GetRelationships(Type resourceClrType)
         {
-            var relationships = new List<RelationshipAttribute>();
+            var relationshipsByName = new Dictionary<string, RelationshipAttribute>();
             PropertyInfo[] properties = resourceClrType.GetProperties();
 
             foreach (PropertyInfo property in properties)
@@ -222,11 +247,11 @@ private IReadOnlyCollection<RelationshipAttribute> GetRelationships(Type resourc
                     relationship.LeftClrType = resourceClrType;
                     relationship.RightClrType = GetRelationshipType(relationship, property);
 
-                    relationships.Add(relationship);
+                    IncludeField(relationshipsByName, relationship);
                 }
             }
 
-            return relationships;
+            return relationshipsByName.Values;
         }
 
         private void SetPublicName(ResourceFieldAttribute field, PropertyInfo property)
@@ -269,6 +294,51 @@ private IReadOnlyCollection<EagerLoadAttribute> GetEagerLoads(Type resourceClrTy
             return attributes;
         }
 
+        private static void IncludeField<TField>(Dictionary<string, TField> fieldsByName, TField field)
+            where TField : ResourceFieldAttribute
+        {
+            if (fieldsByName.TryGetValue(field.PublicName, out var existingField))
+            {
+                throw CreateExceptionForDuplicatePublicName(field.Property.DeclaringType!, existingField, field);
+            }
+
+            fieldsByName.Add(field.PublicName, field);
+        }
+
+        private void AssertNoDuplicatePublicName(ResourceType resourceType, string effectivePublicName)
+        {
+            var (existingClrType, _) = _resourceTypesByClrType.FirstOrDefault(type => type.Value.PublicName == resourceType.PublicName);
+
+            if (existingClrType != null)
+            {
+                throw new InvalidConfigurationException(
+                    $"Resource '{existingClrType}' and '{resourceType.ClrType}' both use public name '{effectivePublicName}'.");
+            }
+        }
+
+        private void AssertNoDuplicatePublicName(IReadOnlyCollection<AttrAttribute> attributes, IReadOnlyCollection<RelationshipAttribute> relationships)
+        {
+            IEnumerable<(AttrAttribute attribute, RelationshipAttribute relationship)> query =
+                from attribute in attributes
+                from relationship in relationships
+                where attribute.PublicName == relationship.PublicName
+                select (attribute, relationship);
+
+            (AttrAttribute? duplicateAttribute, RelationshipAttribute? duplicateRelationship) = query.FirstOrDefault();
+
+            if (duplicateAttribute != null && duplicateRelationship != null)
+            {
+                throw CreateExceptionForDuplicatePublicName(duplicateAttribute.Property.DeclaringType!, duplicateAttribute, duplicateRelationship);
+            }
+        }
+
+        private static InvalidConfigurationException CreateExceptionForDuplicatePublicName(Type containingClrType, ResourceFieldAttribute existingField,
+            ResourceFieldAttribute field)
+        {
+            return new InvalidConfigurationException(
+                $"Properties '{containingClrType}.{existingField.Property.Name}' and '{containingClrType}.{field.Property.Name}' both use public name '{field.PublicName}'.");
+        }
+
         [AssertionMethod]
         private static void AssertNoInfiniteRecursion(int recursionDepth)
         {

test/JsonApiDotNetCoreTests/JsonApiDotNetCoreTests.csproj

@@ -18,6 +18,7 @@
     <PackageReference Include="Macross.Json.Extensions" Version="2.0.0" />
     <PackageReference Include="Microsoft.AspNetCore.Mvc.Testing" Version="$(AspNetCoreVersion)" />
     <PackageReference Include="Microsoft.EntityFrameworkCore.InMemory" Version="$(EFCoreVersion)" />
+    <PackageReference Include="Microsoft.EntityFrameworkCore.Proxies" Version="$(EFCoreVersion)" />
     <PackageReference Include="Microsoft.NET.Test.Sdk" Version="$(TestSdkVersion)" />
   </ItemGroup>
 </Project>