Merge pull request #1246 from json-api-dotnet/api-controller-attribute

Better handling of [ApiController] usage

Author: maurei

src/JsonApiDotNetCore/Errors/UnsuccessfulActionResultException.cs

@@ -1,6 +1,7 @@
 using System.Net;
 using JetBrains.Annotations;
 using JsonApiDotNetCore.Serialization.Objects;
+using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc;
 
 namespace JsonApiDotNetCore.Errors;
@@ -20,20 +21,35 @@ public UnsuccessfulActionResultException(HttpStatusCode status)
     }
 
     public UnsuccessfulActionResultException(ProblemDetails problemDetails)
-        : base(ToError(problemDetails))
+        : base(ToErrorObjects(problemDetails))
     {
     }
 
-    private static ErrorObject ToError(ProblemDetails problemDetails)
+    private static IEnumerable<ErrorObject> ToErrorObjects(ProblemDetails problemDetails)
     {
         ArgumentGuard.NotNull(problemDetails);
 
         HttpStatusCode status = problemDetails.Status != null ? (HttpStatusCode)problemDetails.Status.Value : HttpStatusCode.InternalServerError;
 
+        if (problemDetails is HttpValidationProblemDetails validationProblemDetails && validationProblemDetails.Errors.Any())
+        {
+            foreach (string errorMessage in validationProblemDetails.Errors.SelectMany(pair => pair.Value))
+            {
+                yield return ToErrorObject(status, validationProblemDetails, errorMessage);
+            }
+        }
+        else
+        {
+            yield return ToErrorObject(status, problemDetails, problemDetails.Detail);
+        }
+    }
+
+    private static ErrorObject ToErrorObject(HttpStatusCode status, ProblemDetails problemDetails, string? detail)
+    {
         var error = new ErrorObject(status)
         {
             Title = problemDetails.Title,
-            Detail = problemDetails.Detail
+            Detail = detail
         };
 
         if (!string.IsNullOrWhiteSpace(problemDetails.Instance))

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.");
+    }
+}

test/JsonApiDotNetCoreTests/IntegrationTests/CustomRoutes/ApiControllerAttributeTests.cs

@@ -35,4 +35,48 @@ public async Task ApiController_attribute_transforms_NotFound_action_result_with
         error.Links.ShouldNotBeNull();
         error.Links.About.Should().Be("https://tools.ietf.org/html/rfc7231#section-6.5.4");
     }
+
+    [Fact]
+    public async Task ProblemDetails_from_invalid_ModelState_is_translated_into_error_response()
+    {
+        // Arrange
+        var requestBody = new
+        {
+            data = new
+            {
+                type = "civilians",
+                attributes = new
+                {
+                    name = (string?)null,
+                    yearOfBirth = 1850
+                }
+            }
+        };
+
+        const string route = "/world-civilians";
+
+        // Act
+        (HttpResponseMessage httpResponse, Document responseDocument) = await _testContext.ExecutePostAsync<Document>(route, requestBody);
+
+        // Assert
+        httpResponse.ShouldHaveStatusCode(HttpStatusCode.BadRequest);
+
+        responseDocument.Errors.ShouldHaveCount(2);
+
+        ErrorObject error1 = responseDocument.Errors[0];
+        error1.StatusCode.Should().Be(HttpStatusCode.BadRequest);
+        error1.Links.ShouldNotBeNull();
+        error1.Links.About.Should().Be("https://tools.ietf.org/html/rfc7231#section-6.5.1");
+        error1.Title.Should().Be("One or more validation errors occurred.");
+        error1.Detail.Should().Be("The Name field is required.");
+        error1.Source.Should().BeNull();
+
+        ErrorObject error2 = responseDocument.Errors[1];
+        error2.StatusCode.Should().Be(HttpStatusCode.BadRequest);
+        error2.Links.ShouldNotBeNull();
+        error2.Links.About.Should().Be("https://tools.ietf.org/html/rfc7231#section-6.5.1");
+        error2.Title.Should().Be("One or more validation errors occurred.");
+        error2.Detail.Should().Be("The field YearOfBirth must be between 1900 and 2050.");
+        error2.Source.Should().BeNull();
+    }
 }

test/JsonApiDotNetCoreTests/IntegrationTests/CustomRoutes/Civilian.cs

@@ -1,3 +1,4 @@
+using System.ComponentModel.DataAnnotations;
 using JetBrains.Annotations;
 using JsonApiDotNetCore.Resources;
 using JsonApiDotNetCore.Resources.Annotations;
@@ -10,4 +11,8 @@ public sealed class Civilian : Identifiable<int>
 {
     [Attr]
     public string Name { get; set; } = null!;
+
+    [Attr]
+    [Range(1900, 2050)]
+    public int YearOfBirth { get; set; }
 }