Resource Graph validations (#1101)

* Added extra validations of the resource graph and unified unit tests

* Fail at startup on controller that derives from BaseJsonApiController, but uses a resource type that is not registered in the resource graph

* Check off roadmap entry

Author: Bart Koelman

ROADMAP.md

@@ -23,11 +23,11 @@ The need for breaking changes has blocked several efforts in the v4.x release, s
 - [x] Optimize IIdentifiable to ResourceObject conversion [#1028](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/1028) [#1024](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/1024) [#233](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/233)
 - [x] Nullable reference types [#1029](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/1029)
 - [x] Improved paging links [#1010](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/1010)
+- [x] Configuration validation [#170](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/170)
 
 Aside from the list above, we have interest in the following topics. It's too soon yet to decide whether they'll make it into v5.x or in a later major version.
 
 - Auto-generated controllers [#732](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/732) [#365](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/365)
-- Configuration validation [#170](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/170)
 - Optimistic concurrency [#1004](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/1004)
 - Extract annotations into separate package [#730](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/730)
 - OpenAPI (Swagger) [#1046](https://github.com/json-api-dotnet/JsonApiDotNetCore/issues/1046)

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 < 2)
+            {
+                _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)
         {

src/JsonApiDotNetCore/Middleware/JsonApiRoutingConvention.cs

@@ -84,6 +84,11 @@ public void Apply(ApplicationModel application)
                             _resourceTypePerControllerTypeMap.Add(controller.ControllerType, resourceType);
                             _controllerPerResourceTypeMap.Add(resourceType, controller);
                         }
+                        else
+                        {
+                            throw new InvalidConfigurationException($"Controller '{controller.ControllerType}' depends on " +
+                                $"resource type '{resourceClrType}', which does not exist in the resource graph.");
+                        }
                     }
                 }
 

test/JsonApiDotNetCoreTests/IntegrationTests/NonJsonApiControllers/UnknownResource.cs

@@ -0,0 +1,11 @@
+using JetBrains.Annotations;
+using JsonApiDotNetCore.Resources;
+
+namespace JsonApiDotNetCoreTests.IntegrationTests.NonJsonApiControllers
+{
+    [UsedImplicitly(ImplicitUseTargetFlags.Members)]
+    public sealed class UnknownResource : Identifiable<int>
+    {
+        public string? Value { get; set; }
+    }
+}

test/JsonApiDotNetCoreTests/IntegrationTests/NonJsonApiControllers/UnknownResourceControllerTests.cs

@@ -0,0 +1,32 @@
+using System;
+using FluentAssertions;
+using JsonApiDotNetCore.Errors;
+using TestBuildingBlocks;
+using Xunit;
+
+namespace JsonApiDotNetCoreTests.IntegrationTests.NonJsonApiControllers
+{
+    public sealed class UnknownResourceControllerTests : IntegrationTestContext<TestableStartup<NonJsonApiDbContext>, NonJsonApiDbContext>
+    {
+        public UnknownResourceControllerTests()
+        {
+            UseController<UnknownResourcesController>();
+        }
+
+        [Fact]
+        public void Fails_at_startup_when_using_controller_for_resource_type_that_is_not_registered_in_resource_graph()
+        {
+            // Act
+            Action action = () => _ = Factory;
+
+            // Assert
+            action.Should().ThrowExactly<InvalidConfigurationException>().WithMessage($"Controller '{typeof(UnknownResourcesController)}' " +
+                $"depends on resource type '{typeof(UnknownResource)}', which does not exist in the resource graph.");
+        }
+
+        public override void Dispose()
+        {
+            // Prevents crash when test cleanup tries to access lazily constructed Factory.
+        }
+    }
+}

test/JsonApiDotNetCoreTests/IntegrationTests/NonJsonApiControllers/UnknownResourcesController.cs

@@ -0,0 +1,16 @@
+using JsonApiDotNetCore.Configuration;
+using JsonApiDotNetCore.Controllers;
+using JsonApiDotNetCore.Services;
+using Microsoft.Extensions.Logging;
+
+namespace JsonApiDotNetCoreTests.IntegrationTests.NonJsonApiControllers
+{
+    public sealed class UnknownResourcesController : JsonApiController<UnknownResource, int>
+    {
+        public UnknownResourcesController(IJsonApiOptions options, IResourceGraph resourceGraph, ILoggerFactory loggerFactory,
+            IResourceService<UnknownResource, int> resourceService)
+            : base(options, resourceGraph, loggerFactory, resourceService)
+        {
+        }
+    }
+}

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>