Log warning at startup when [ApiController] found on a JSON:API controller

Author: bkoelman

src/JsonApiDotNetCore/Middleware/JsonApiRoutingConvention.cs

@@ -7,6 +7,7 @@
 using JsonApiDotNetCore.Resources;
 using Microsoft.AspNetCore.Mvc;
 using Microsoft.AspNetCore.Mvc.ApplicationModels;
+using Microsoft.Extensions.Logging;
 
 namespace JsonApiDotNetCore.Middleware;
 
@@ -30,17 +31,20 @@ public sealed class JsonApiRoutingConvention : IJsonApiRoutingConvention
 {
     private readonly IJsonApiOptions _options;
     private readonly IResourceGraph _resourceGraph;
+    private readonly ILogger<JsonApiRoutingConvention> _logger;
     private readonly Dictionary<string, string> _registeredControllerNameByTemplate = new();
     private readonly Dictionary<Type, ResourceType> _resourceTypePerControllerTypeMap = new();
     private readonly Dictionary<ResourceType, ControllerModel> _controllerPerResourceTypeMap = new();
 
-    public JsonApiRoutingConvention(IJsonApiOptions options, IResourceGraph resourceGraph)
+    public JsonApiRoutingConvention(IJsonApiOptions options, IResourceGraph resourceGraph, ILogger<JsonApiRoutingConvention> logger)
     {
         ArgumentGuard.NotNull(options);
         ArgumentGuard.NotNull(resourceGraph);
+        ArgumentGuard.NotNull(logger);
 
         _options = options;
         _resourceGraph = resourceGraph;
+        _logger = logger;
     }
 
     /// <inheritdoc />
@@ -64,36 +68,51 @@ public void Apply(ApplicationModel application)
 
         foreach (ControllerModel controller in application.Controllers)
         {
-            bool isOperationsController = IsOperationsController(controller.ControllerType);
+            if (!IsJsonApiController(controller))
+            {
+                continue;
+            }
+
+            if (HasApiControllerAttribute(controller))
+            {
+                // Although recommended by Microsoft for hard-written controllers, the opinionated behavior of [ApiController] violates the JSON:API specification.
+                // See https://learn.microsoft.com/en-us/aspnet/core/web-api/?view=aspnetcore-7.0#apicontroller-attribute for its effects.
+                // JsonApiDotNetCore already handles all of these concerns, but in a JSON:API-compliant way. So the attribute doesn't do any good.
 
-            if (!isOperationsController)
+                // While we try our best when [ApiController] is used, we can't completely avoid a degraded experience. ModelState validation errors are turned into
+                // ProblemDetails, where the origin of the error gets lost. As a result, we can't populate the source pointer in JSON:API error responses.
+                // For backwards-compatibility, we log a warning instead of throwing. But we can't think of any use cases where having [ApiController] makes sense.
+
+                _logger.LogWarning(
+                    $"Found JSON:API controller '{controller.ControllerType}' with [ApiController]. Please remove this attribute for optimal JSON:API compliance.");
+            }
+
+            if (!IsOperationsController(controller.ControllerType))
             {
                 Type? resourceClrType = ExtractResourceClrTypeFromController(controller.ControllerType);
 
                 if (resourceClrType != null)
                 {
                     ResourceType? resourceType = _resourceGraph.FindResourceType(resourceClrType);
 
-                    if (resourceType != null)
-                    {
-                        if (_controllerPerResourceTypeMap.ContainsKey(resourceType))
-                        {
-                            throw new InvalidConfigurationException(
-                                $"Multiple controllers found for resource type '{resourceType}': '{_controllerPerResourceTypeMap[resourceType].ControllerType}' and '{controller.ControllerType}'.");
-                        }
-
-                        _resourceTypePerControllerTypeMap.Add(controller.ControllerType, resourceType);
-                        _controllerPerResourceTypeMap.Add(resourceType, controller);
-                    }
-                    else
+                    if (resourceType == null)
                     {
                         throw new InvalidConfigurationException($"Controller '{controller.ControllerType}' depends on " +
                             $"resource type '{resourceClrType}', which does not exist in the resource graph.");
                     }
+
+                    if (_controllerPerResourceTypeMap.ContainsKey(resourceType))
+                    {
+                        throw new InvalidConfigurationException(
+                            $"Multiple controllers found for resource type '{resourceType}': '{_controllerPerResourceTypeMap[resourceType].ControllerType}' and '{controller.ControllerType}'.");
+                    }
+
+                    _resourceTypePerControllerTypeMap.Add(controller.ControllerType, resourceType);
+                    _controllerPerResourceTypeMap.Add(resourceType, controller);
                 }
             }
 
-            if (!IsRoutingConventionEnabled(controller))
+            if (IsRoutingConventionDisabled(controller))
             {
                 continue;
             }
@@ -115,10 +134,19 @@ public void Apply(ApplicationModel application)
         }
     }
 
-    private bool IsRoutingConventionEnabled(ControllerModel controller)
+    private static bool IsJsonApiController(ControllerModel controller)
+    {
+        return controller.ControllerType.IsSubclassOf(typeof(CoreJsonApiController));
+    }
+
+    private static bool HasApiControllerAttribute(ControllerModel controller)
+    {
+        return controller.ControllerType.GetCustomAttribute<ApiControllerAttribute>() != null;
+    }
+
+    private static bool IsRoutingConventionDisabled(ControllerModel controller)
     {
-        return controller.ControllerType.IsSubclassOf(typeof(CoreJsonApiController)) &&
-            controller.ControllerType.GetCustomAttribute<DisableRoutingConventionAttribute>(true) == null;
+        return controller.ControllerType.GetCustomAttribute<DisableRoutingConventionAttribute>(true) != null;
     }
 
     /// <summary>

test/JsonApiDotNetCoreTests/IntegrationTests/CustomRoutes/ApiControllerAttributeLogTests.cs

@@ -0,0 +1,47 @@
+using FluentAssertions;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging;
+using TestBuildingBlocks;
+using Xunit;
+
+namespace JsonApiDotNetCoreTests.IntegrationTests.CustomRoutes;
+
+public sealed class ApiControllerAttributeLogTests : IntegrationTestContext<TestableStartup<CustomRouteDbContext>, CustomRouteDbContext>
+{
+    private readonly FakeLoggerFactory _loggerFactory;
+
+    public ApiControllerAttributeLogTests()
+    {
+        UseController<CiviliansController>();
+
+        _loggerFactory = new FakeLoggerFactory(LogLevel.Warning);
+
+        ConfigureLogging(options =>
+        {
+            options.ClearProviders();
+            options.AddProvider(_loggerFactory);
+        });
+
+        ConfigureServicesBeforeStartup(services =>
+        {
+            services.AddSingleton(_loggerFactory);
+        });
+    }
+
+    [Fact]
+    public void Logs_warning_at_startup_when_ApiControllerAttribute_found()
+    {
+        // Arrange
+        _loggerFactory.Logger.Clear();
+
+        // Act
+        _ = Factory;
+
+        // Assert
+        _loggerFactory.Logger.Messages.ShouldHaveCount(1);
+        _loggerFactory.Logger.Messages.Single().LogLevel.Should().Be(LogLevel.Warning);
+
+        _loggerFactory.Logger.Messages.Single().Text.Should().Be(
+            $"Found JSON:API controller '{typeof(CiviliansController)}' with [ApiController]. Please remove this attribute for optimal JSON:API compliance.");
+    }
+}