From fce57e11991b2814d4f2dd6d3bf5f6d72b9d0ead Mon Sep 17 00:00:00 2001 From: Bart Koelman Date: Thu, 26 Nov 2020 15:13:37 +0100 Subject: [PATCH] Refactored JsonApiException to contain a list of errors. This makes it possible to catch one base exception type, regardless of how many errors are returned in the response. --- .../Errors/IHasMultipleErrors.cs | 10 ----- .../Errors/InvalidModelStateException.cs | 16 ++++---- .../Errors/JsonApiException.cs | 27 ++++++++++--- ...sourcesInRelationshipsNotFoundException.cs | 8 ++-- .../Middleware/ExceptionHandler.cs | 38 ++++++++++--------- .../Extensibility/CustomErrorHandlingTests.cs | 2 +- .../BaseJsonApiController_Tests.cs | 14 +++---- .../DefaultsParseTests.cs | 9 +++-- .../QueryStringParameters/FilterParseTests.cs | 9 +++-- .../IncludeParseTests.cs | 9 +++-- .../LegacyFilterParseTests.cs | 9 +++-- .../QueryStringParameters/NullsParseTests.cs | 9 +++-- .../PaginationParseTests.cs | 18 +++++---- .../QueryStringParameters/SortParseTests.cs | 9 +++-- .../SparseFieldSetParseTests.cs | 9 +++-- 15 files changed, 105 insertions(+), 91 deletions(-) delete mode 100644 src/JsonApiDotNetCore/Errors/IHasMultipleErrors.cs diff --git a/src/JsonApiDotNetCore/Errors/IHasMultipleErrors.cs b/src/JsonApiDotNetCore/Errors/IHasMultipleErrors.cs deleted file mode 100644 index 87e91922e4..0000000000 --- a/src/JsonApiDotNetCore/Errors/IHasMultipleErrors.cs +++ /dev/null @@ -1,10 +0,0 @@ -using System.Collections.Generic; -using JsonApiDotNetCore.Serialization.Objects; - -namespace JsonApiDotNetCore.Errors -{ - public interface IHasMultipleErrors - { - public IReadOnlyCollection Errors { get; } - } -} diff --git a/src/JsonApiDotNetCore/Errors/InvalidModelStateException.cs b/src/JsonApiDotNetCore/Errors/InvalidModelStateException.cs index 51ddea6a09..33e2f5be14 100644 --- a/src/JsonApiDotNetCore/Errors/InvalidModelStateException.cs +++ b/src/JsonApiDotNetCore/Errors/InvalidModelStateException.cs @@ -13,23 +13,21 @@ namespace JsonApiDotNetCore.Errors /// /// The error that is thrown when model state validation fails. /// - public class InvalidModelStateException : Exception, IHasMultipleErrors + public class InvalidModelStateException : JsonApiException { - public IReadOnlyCollection Errors { get; } - public InvalidModelStateException(ModelStateDictionary modelState, Type resourceType, bool includeExceptionStackTraceInErrors, NamingStrategy namingStrategy) + : base(FromModelState(modelState, resourceType, includeExceptionStackTraceInErrors, namingStrategy)) { - if (modelState == null) throw new ArgumentNullException(nameof(modelState)); - if (resourceType == null) throw new ArgumentNullException(nameof(resourceType)); - if (namingStrategy == null) throw new ArgumentNullException(nameof(namingStrategy)); - - Errors = FromModelState(modelState, resourceType, includeExceptionStackTraceInErrors, namingStrategy); } private static IReadOnlyCollection FromModelState(ModelStateDictionary modelState, Type resourceType, bool includeExceptionStackTraceInErrors, NamingStrategy namingStrategy) { + if (modelState == null) throw new ArgumentNullException(nameof(modelState)); + if (resourceType == null) throw new ArgumentNullException(nameof(resourceType)); + if (namingStrategy == null) throw new ArgumentNullException(nameof(namingStrategy)); + List errors = new List(); foreach (var (propertyName, entry) in modelState.Where(x => x.Value.Errors.Any())) @@ -40,7 +38,7 @@ private static IReadOnlyCollection FromModelState(ModelStateDictionary mo { if (modelError.Exception is JsonApiException jsonApiException) { - errors.Add(jsonApiException.Error); + errors.AddRange(jsonApiException.Errors); } else { diff --git a/src/JsonApiDotNetCore/Errors/JsonApiException.cs b/src/JsonApiDotNetCore/Errors/JsonApiException.cs index 81609a740f..52e531a164 100644 --- a/src/JsonApiDotNetCore/Errors/JsonApiException.cs +++ b/src/JsonApiDotNetCore/Errors/JsonApiException.cs @@ -1,11 +1,13 @@ using System; +using System.Collections.Generic; +using System.Linq; using JsonApiDotNetCore.Serialization.Objects; using Newtonsoft.Json; namespace JsonApiDotNetCore.Errors { /// - /// The base class for an that represents a json:api error object in an unsuccessful response. + /// The base class for an that represents one or more json:api error objects in an unsuccessful response. /// public class JsonApiException : Exception { @@ -15,14 +17,29 @@ public class JsonApiException : Exception Formatting = Formatting.Indented }; - public Error Error { get; } + public IReadOnlyList Errors { get; } public JsonApiException(Error error, Exception innerException = null) - : base(error.Title, innerException) + : base(null, innerException) { - Error = error; + if (error == null) throw new ArgumentNullException(nameof(error)); + + Errors = new[] {error}; + } + + public JsonApiException(IEnumerable errors, Exception innerException = null) + : base(null, innerException) + { + if (errors == null) throw new ArgumentNullException(nameof(errors)); + + Errors = errors.ToList(); + + if (!Errors.Any()) + { + throw new ArgumentException("At least one error is required.", nameof(errors)); + } } - public override string Message => "Error = " + JsonConvert.SerializeObject(Error, _errorSerializerSettings); + public override string Message => "Errors = " + JsonConvert.SerializeObject(Errors, _errorSerializerSettings); } } diff --git a/src/JsonApiDotNetCore/Errors/ResourcesInRelationshipsNotFoundException.cs b/src/JsonApiDotNetCore/Errors/ResourcesInRelationshipsNotFoundException.cs index 7c7a9e5a2a..82d433324e 100644 --- a/src/JsonApiDotNetCore/Errors/ResourcesInRelationshipsNotFoundException.cs +++ b/src/JsonApiDotNetCore/Errors/ResourcesInRelationshipsNotFoundException.cs @@ -9,16 +9,14 @@ namespace JsonApiDotNetCore.Errors /// /// The error that is thrown when referencing one or more non-existing resources in one or more relationships. /// - public sealed class ResourcesInRelationshipsNotFoundException : Exception, IHasMultipleErrors + public sealed class ResourcesInRelationshipsNotFoundException : JsonApiException { - public IReadOnlyCollection Errors { get; } - public ResourcesInRelationshipsNotFoundException(IEnumerable missingResources) + : base(missingResources.Select(CreateError)) { - Errors = missingResources.Select(CreateError).ToList(); } - private Error CreateError(MissingResourceInRelationship missingResourceInRelationship) + private static Error CreateError(MissingResourceInRelationship missingResourceInRelationship) { return new Error(HttpStatusCode.NotFound) { diff --git a/src/JsonApiDotNetCore/Middleware/ExceptionHandler.cs b/src/JsonApiDotNetCore/Middleware/ExceptionHandler.cs index 451af73e63..99731e7b49 100644 --- a/src/JsonApiDotNetCore/Middleware/ExceptionHandler.cs +++ b/src/JsonApiDotNetCore/Middleware/ExceptionHandler.cs @@ -51,7 +51,7 @@ protected virtual LogLevel GetLogLevel(Exception exception) return LogLevel.None; } - if (exception is JsonApiException || exception is InvalidModelStateException) + if (exception is JsonApiException) { return LogLevel.Information; } @@ -63,36 +63,38 @@ protected virtual string GetLogMessage(Exception exception) { if (exception == null) throw new ArgumentNullException(nameof(exception)); - return exception is JsonApiException jsonApiException - ? jsonApiException.Error.Title - : exception.Message; + return exception.Message; } protected virtual ErrorDocument CreateErrorDocument(Exception exception) { if (exception == null) throw new ArgumentNullException(nameof(exception)); - if (exception is IHasMultipleErrors exceptionWithMultipleErrors) - { - return new ErrorDocument(exceptionWithMultipleErrors.Errors); - } - - Error error = exception is JsonApiException jsonApiException - ? jsonApiException.Error + var errors = exception is JsonApiException jsonApiException + ? jsonApiException.Errors : exception is TaskCanceledException - ? new Error((HttpStatusCode) 499) + ? new[] { - Title = "Request execution was canceled." + new Error((HttpStatusCode) 499) + { + Title = "Request execution was canceled." + } } - : new Error(HttpStatusCode.InternalServerError) + : new[] { - Title = "An unhandled error occurred while processing this request.", - Detail = exception.Message + new Error(HttpStatusCode.InternalServerError) + { + Title = "An unhandled error occurred while processing this request.", + Detail = exception.Message + } }; - ApplyOptions(error, exception); + foreach (var error in errors) + { + ApplyOptions(error, exception); + } - return new ErrorDocument(error); + return new ErrorDocument(errors); } private void ApplyOptions(Error error, Exception exception) diff --git a/test/JsonApiDotNetCoreExampleTests/Acceptance/Extensibility/CustomErrorHandlingTests.cs b/test/JsonApiDotNetCoreExampleTests/Acceptance/Extensibility/CustomErrorHandlingTests.cs index 91143812b9..ff49489904 100644 --- a/test/JsonApiDotNetCoreExampleTests/Acceptance/Extensibility/CustomErrorHandlingTests.cs +++ b/test/JsonApiDotNetCoreExampleTests/Acceptance/Extensibility/CustomErrorHandlingTests.cs @@ -55,7 +55,7 @@ protected override ErrorDocument CreateErrorDocument(Exception exception) { if (exception is NoPermissionException noPermissionException) { - noPermissionException.Error.Meta.Data.Add("support", + noPermissionException.Errors[0].Meta.Data.Add("support", "For support, email to: support@company.com?subject=" + noPermissionException.CustomerCode); } diff --git a/test/UnitTests/Controllers/BaseJsonApiController_Tests.cs b/test/UnitTests/Controllers/BaseJsonApiController_Tests.cs index 5d6fddc6a0..b11004d236 100644 --- a/test/UnitTests/Controllers/BaseJsonApiController_Tests.cs +++ b/test/UnitTests/Controllers/BaseJsonApiController_Tests.cs @@ -76,7 +76,7 @@ public async Task GetAsync_Throws_405_If_No_Service() var exception = await Assert.ThrowsAsync(() => controller.GetAsync(CancellationToken.None)); // Assert - Assert.Equal(HttpStatusCode.MethodNotAllowed, exception.Error.StatusCode); + Assert.Equal(HttpStatusCode.MethodNotAllowed, exception.Errors[0].StatusCode); Assert.Equal(HttpMethod.Get, exception.Method); } @@ -106,7 +106,7 @@ public async Task GetAsyncById_Throws_405_If_No_Service() var exception = await Assert.ThrowsAsync(() => controller.GetAsync(id, CancellationToken.None)); // Assert - Assert.Equal(HttpStatusCode.MethodNotAllowed, exception.Error.StatusCode); + Assert.Equal(HttpStatusCode.MethodNotAllowed, exception.Errors[0].StatusCode); Assert.Equal(HttpMethod.Get, exception.Method); } @@ -136,7 +136,7 @@ public async Task GetRelationshipsAsync_Throws_405_If_No_Service() var exception = await Assert.ThrowsAsync(() => controller.GetRelationshipAsync(id, string.Empty, CancellationToken.None)); // Assert - Assert.Equal(HttpStatusCode.MethodNotAllowed, exception.Error.StatusCode); + Assert.Equal(HttpStatusCode.MethodNotAllowed, exception.Errors[0].StatusCode); Assert.Equal(HttpMethod.Get, exception.Method); } @@ -166,7 +166,7 @@ public async Task GetRelationshipAsync_Throws_405_If_No_Service() var exception = await Assert.ThrowsAsync(() => controller.GetSecondaryAsync(id, string.Empty, CancellationToken.None)); // Assert - Assert.Equal(HttpStatusCode.MethodNotAllowed, exception.Error.StatusCode); + Assert.Equal(HttpStatusCode.MethodNotAllowed, exception.Errors[0].StatusCode); Assert.Equal(HttpMethod.Get, exception.Method); } @@ -199,7 +199,7 @@ public async Task PatchAsync_Throws_405_If_No_Service() var exception = await Assert.ThrowsAsync(() => controller.PatchAsync(id, resource, CancellationToken.None)); // Assert - Assert.Equal(HttpStatusCode.MethodNotAllowed, exception.Error.StatusCode); + Assert.Equal(HttpStatusCode.MethodNotAllowed, exception.Errors[0].StatusCode); Assert.Equal(HttpMethod.Patch, exception.Method); } @@ -247,7 +247,7 @@ public async Task PatchRelationshipsAsync_Throws_405_If_No_Service() var exception = await Assert.ThrowsAsync(() => controller.PatchRelationshipAsync(id, string.Empty, null, CancellationToken.None)); // Assert - Assert.Equal(HttpStatusCode.MethodNotAllowed, exception.Error.StatusCode); + Assert.Equal(HttpStatusCode.MethodNotAllowed, exception.Errors[0].StatusCode); Assert.Equal(HttpMethod.Patch, exception.Method); } @@ -277,7 +277,7 @@ public async Task DeleteAsync_Throws_405_If_No_Service() var exception = await Assert.ThrowsAsync(() => controller.DeleteAsync(id, CancellationToken.None)); // Assert - Assert.Equal(HttpStatusCode.MethodNotAllowed, exception.Error.StatusCode); + Assert.Equal(HttpStatusCode.MethodNotAllowed, exception.Errors[0].StatusCode); Assert.Equal(HttpMethod.Delete, exception.Method); } } diff --git a/test/UnitTests/QueryStringParameters/DefaultsParseTests.cs b/test/UnitTests/QueryStringParameters/DefaultsParseTests.cs index 90459eaa36..0a6e88449c 100644 --- a/test/UnitTests/QueryStringParameters/DefaultsParseTests.cs +++ b/test/UnitTests/QueryStringParameters/DefaultsParseTests.cs @@ -75,10 +75,11 @@ public void Reader_Read_Fails(string parameterName, string parameterValue, strin var exception = action.Should().ThrowExactly().And; exception.QueryParameterName.Should().Be(parameterName); - exception.Error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - exception.Error.Title.Should().Be("The specified defaults is invalid."); - exception.Error.Detail.Should().Be(errorMessage); - exception.Error.Source.Parameter.Should().Be(parameterName); + exception.Errors.Should().HaveCount(1); + exception.Errors[0].StatusCode.Should().Be(HttpStatusCode.BadRequest); + exception.Errors[0].Title.Should().Be("The specified defaults is invalid."); + exception.Errors[0].Detail.Should().Be(errorMessage); + exception.Errors[0].Source.Parameter.Should().Be(parameterName); } [Theory] diff --git a/test/UnitTests/QueryStringParameters/FilterParseTests.cs b/test/UnitTests/QueryStringParameters/FilterParseTests.cs index 3f1f272066..7ebdfd5a0c 100644 --- a/test/UnitTests/QueryStringParameters/FilterParseTests.cs +++ b/test/UnitTests/QueryStringParameters/FilterParseTests.cs @@ -98,10 +98,11 @@ public void Reader_Read_Fails(string parameterName, string parameterValue, strin var exception = action.Should().ThrowExactly().And; exception.QueryParameterName.Should().Be(parameterName); - exception.Error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - exception.Error.Title.Should().Be("The specified filter is invalid."); - exception.Error.Detail.Should().Be(errorMessage); - exception.Error.Source.Parameter.Should().Be(parameterName); + exception.Errors.Should().HaveCount(1); + exception.Errors[0].StatusCode.Should().Be(HttpStatusCode.BadRequest); + exception.Errors[0].Title.Should().Be("The specified filter is invalid."); + exception.Errors[0].Detail.Should().Be(errorMessage); + exception.Errors[0].Source.Parameter.Should().Be(parameterName); } [Theory] diff --git a/test/UnitTests/QueryStringParameters/IncludeParseTests.cs b/test/UnitTests/QueryStringParameters/IncludeParseTests.cs index a52dc5ddc1..281dc92cf4 100644 --- a/test/UnitTests/QueryStringParameters/IncludeParseTests.cs +++ b/test/UnitTests/QueryStringParameters/IncludeParseTests.cs @@ -64,10 +64,11 @@ public void Reader_Read_Fails(string parameterName, string parameterValue, strin var exception = action.Should().ThrowExactly().And; exception.QueryParameterName.Should().Be(parameterName); - exception.Error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - exception.Error.Title.Should().Be("The specified include is invalid."); - exception.Error.Detail.Should().Be(errorMessage); - exception.Error.Source.Parameter.Should().Be(parameterName); + exception.Errors.Should().HaveCount(1); + exception.Errors[0].StatusCode.Should().Be(HttpStatusCode.BadRequest); + exception.Errors[0].Title.Should().Be("The specified include is invalid."); + exception.Errors[0].Detail.Should().Be(errorMessage); + exception.Errors[0].Source.Parameter.Should().Be(parameterName); } [Theory] diff --git a/test/UnitTests/QueryStringParameters/LegacyFilterParseTests.cs b/test/UnitTests/QueryStringParameters/LegacyFilterParseTests.cs index 58a9c7ce06..a5f9e3c6bb 100644 --- a/test/UnitTests/QueryStringParameters/LegacyFilterParseTests.cs +++ b/test/UnitTests/QueryStringParameters/LegacyFilterParseTests.cs @@ -48,10 +48,11 @@ public void Reader_Read_Fails(string parameterName, string parameterValue, strin var exception = action.Should().ThrowExactly().And; exception.QueryParameterName.Should().Be(parameterName); - exception.Error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - exception.Error.Title.Should().Be("The specified filter is invalid."); - exception.Error.Detail.Should().Be(errorMessage); - exception.Error.Source.Parameter.Should().Be(parameterName); + exception.Errors.Should().HaveCount(1); + exception.Errors[0].StatusCode.Should().Be(HttpStatusCode.BadRequest); + exception.Errors[0].Title.Should().Be("The specified filter is invalid."); + exception.Errors[0].Detail.Should().Be(errorMessage); + exception.Errors[0].Source.Parameter.Should().Be(parameterName); } [Theory] diff --git a/test/UnitTests/QueryStringParameters/NullsParseTests.cs b/test/UnitTests/QueryStringParameters/NullsParseTests.cs index 5d754cb17b..009950c644 100644 --- a/test/UnitTests/QueryStringParameters/NullsParseTests.cs +++ b/test/UnitTests/QueryStringParameters/NullsParseTests.cs @@ -75,10 +75,11 @@ public void Reader_Read_Fails(string parameterName, string parameterValue, strin var exception = action.Should().ThrowExactly().And; exception.QueryParameterName.Should().Be(parameterName); - exception.Error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - exception.Error.Title.Should().Be("The specified nulls is invalid."); - exception.Error.Detail.Should().Be(errorMessage); - exception.Error.Source.Parameter.Should().Be(parameterName); + exception.Errors.Should().HaveCount(1); + exception.Errors[0].StatusCode.Should().Be(HttpStatusCode.BadRequest); + exception.Errors[0].Title.Should().Be("The specified nulls is invalid."); + exception.Errors[0].Detail.Should().Be(errorMessage); + exception.Errors[0].Source.Parameter.Should().Be(parameterName); } [Theory] diff --git a/test/UnitTests/QueryStringParameters/PaginationParseTests.cs b/test/UnitTests/QueryStringParameters/PaginationParseTests.cs index d62b379587..c70a38f814 100644 --- a/test/UnitTests/QueryStringParameters/PaginationParseTests.cs +++ b/test/UnitTests/QueryStringParameters/PaginationParseTests.cs @@ -76,10 +76,11 @@ public void Reader_Read_Page_Number_Fails(string parameterValue, string errorMes var exception = action.Should().ThrowExactly().And; exception.QueryParameterName.Should().Be("page[number]"); - exception.Error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - exception.Error.Title.Should().Be("The specified paging is invalid."); - exception.Error.Detail.Should().Be(errorMessage); - exception.Error.Source.Parameter.Should().Be("page[number]"); + exception.Errors.Should().HaveCount(1); + exception.Errors[0].StatusCode.Should().Be(HttpStatusCode.BadRequest); + exception.Errors[0].Title.Should().Be("The specified paging is invalid."); + exception.Errors[0].Detail.Should().Be(errorMessage); + exception.Errors[0].Source.Parameter.Should().Be("page[number]"); } [Theory] @@ -108,10 +109,11 @@ public void Reader_Read_Page_Size_Fails(string parameterValue, string errorMessa var exception = action.Should().ThrowExactly().And; exception.QueryParameterName.Should().Be("page[size]"); - exception.Error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - exception.Error.Title.Should().Be("The specified paging is invalid."); - exception.Error.Detail.Should().Be(errorMessage); - exception.Error.Source.Parameter.Should().Be("page[size]"); + exception.Errors.Should().HaveCount(1); + exception.Errors[0].StatusCode.Should().Be(HttpStatusCode.BadRequest); + exception.Errors[0].Title.Should().Be("The specified paging is invalid."); + exception.Errors[0].Detail.Should().Be(errorMessage); + exception.Errors[0].Source.Parameter.Should().Be("page[size]"); } [Theory] diff --git a/test/UnitTests/QueryStringParameters/SortParseTests.cs b/test/UnitTests/QueryStringParameters/SortParseTests.cs index c3b9c7ce09..6fce86f153 100644 --- a/test/UnitTests/QueryStringParameters/SortParseTests.cs +++ b/test/UnitTests/QueryStringParameters/SortParseTests.cs @@ -79,10 +79,11 @@ public void Reader_Read_Fails(string parameterName, string parameterValue, strin var exception = action.Should().ThrowExactly().And; exception.QueryParameterName.Should().Be(parameterName); - exception.Error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - exception.Error.Title.Should().Be("The specified sort is invalid."); - exception.Error.Detail.Should().Be(errorMessage); - exception.Error.Source.Parameter.Should().Be(parameterName); + exception.Errors.Should().HaveCount(1); + exception.Errors[0].StatusCode.Should().Be(HttpStatusCode.BadRequest); + exception.Errors[0].Title.Should().Be("The specified sort is invalid."); + exception.Errors[0].Detail.Should().Be(errorMessage); + exception.Errors[0].Source.Parameter.Should().Be(parameterName); } [Theory] diff --git a/test/UnitTests/QueryStringParameters/SparseFieldSetParseTests.cs b/test/UnitTests/QueryStringParameters/SparseFieldSetParseTests.cs index 82decd2696..c3f0f45abd 100644 --- a/test/UnitTests/QueryStringParameters/SparseFieldSetParseTests.cs +++ b/test/UnitTests/QueryStringParameters/SparseFieldSetParseTests.cs @@ -70,10 +70,11 @@ public void Reader_Read_Fails(string parameterName, string parameterValue, strin var exception = action.Should().ThrowExactly().And; exception.QueryParameterName.Should().Be(parameterName); - exception.Error.StatusCode.Should().Be(HttpStatusCode.BadRequest); - exception.Error.Title.Should().Be("The specified fieldset is invalid."); - exception.Error.Detail.Should().Be(errorMessage); - exception.Error.Source.Parameter.Should().Be(parameterName); + exception.Errors.Should().HaveCount(1); + exception.Errors[0].StatusCode.Should().Be(HttpStatusCode.BadRequest); + exception.Errors[0].Title.Should().Be("The specified fieldset is invalid."); + exception.Errors[0].Detail.Should().Be(errorMessage); + exception.Errors[0].Source.Parameter.Should().Be(parameterName); } [Theory]