diff --git a/src/JsonApiDotNetCore/Errors/UnsuccessfulActionResultException.cs b/src/JsonApiDotNetCore/Errors/UnsuccessfulActionResultException.cs index e739ec9cbf..b5ab3859cc 100644 --- a/src/JsonApiDotNetCore/Errors/UnsuccessfulActionResultException.cs +++ b/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 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)) diff --git a/src/JsonApiDotNetCore/Middleware/JsonApiRoutingConvention.cs b/src/JsonApiDotNetCore/Middleware/JsonApiRoutingConvention.cs index 0f596326f7..5c6b84cba7 100644 --- a/src/JsonApiDotNetCore/Middleware/JsonApiRoutingConvention.cs +++ b/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 _logger; private readonly Dictionary _registeredControllerNameByTemplate = new(); private readonly Dictionary _resourceTypePerControllerTypeMap = new(); private readonly Dictionary _controllerPerResourceTypeMap = new(); - public JsonApiRoutingConvention(IJsonApiOptions options, IResourceGraph resourceGraph) + public JsonApiRoutingConvention(IJsonApiOptions options, IResourceGraph resourceGraph, ILogger logger) { ArgumentGuard.NotNull(options); ArgumentGuard.NotNull(resourceGraph); + ArgumentGuard.NotNull(logger); _options = options; _resourceGraph = resourceGraph; + _logger = logger; } /// @@ -64,9 +68,26 @@ 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); @@ -74,26 +95,24 @@ public void Apply(ApplicationModel application) { 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() != null; + } + + private static bool IsRoutingConventionDisabled(ControllerModel controller) { - return controller.ControllerType.IsSubclassOf(typeof(CoreJsonApiController)) && - controller.ControllerType.GetCustomAttribute(true) == null; + return controller.ControllerType.GetCustomAttribute(true) != null; } /// diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/CustomRoutes/ApiControllerAttributeLogTests.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/CustomRoutes/ApiControllerAttributeLogTests.cs new file mode 100644 index 0000000000..2b4a8c70d6 --- /dev/null +++ b/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, CustomRouteDbContext> +{ + private readonly FakeLoggerFactory _loggerFactory; + + public ApiControllerAttributeLogTests() + { + UseController(); + + _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."); + } +} diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/CustomRoutes/ApiControllerAttributeTests.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/CustomRoutes/ApiControllerAttributeTests.cs index 25404e25d2..28e6ba2439 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/CustomRoutes/ApiControllerAttributeTests.cs +++ b/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(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(); + } } diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/CustomRoutes/Civilian.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/CustomRoutes/Civilian.cs index 09fceeca60..00baaaa16c 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/CustomRoutes/Civilian.cs +++ b/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 { [Attr] public string Name { get; set; } = null!; + + [Attr] + [Range(1900, 2050)] + public int YearOfBirth { get; set; } }