From 7d08a55c1fa6b54454c10f84d6c3a06061f39205 Mon Sep 17 00:00:00 2001 From: Bart Koelman <10324372+bkoelman@users.noreply.github.com> Date: Sat, 4 Feb 2023 15:38:50 +0100 Subject: [PATCH] Bugfix in ModelState validation: before this change, the JADNC filter would only kick in for required attributes, which is incorrect. We didn't have a test for an optional property containing a range validation, where the property default value is not in the required range. In that case, using such a resource as non-toplevel (for example: update relationship) would incorrectly produce a validation error. --- .../JsonApiModelMetadataProvider.cs | 6 +--- .../Configuration/JsonApiValidationFilter.cs | 9 +++--- .../ModelState/ModelStateFakers.cs | 6 ++-- .../ModelState/ModelStateValidationTests.cs | 31 +++++++++---------- .../ModelState/SystemDirectory.cs | 4 --- .../InputValidation/ModelState/SystemFile.cs | 7 +++-- 6 files changed, 27 insertions(+), 36 deletions(-) diff --git a/src/JsonApiDotNetCore/Configuration/JsonApiModelMetadataProvider.cs b/src/JsonApiDotNetCore/Configuration/JsonApiModelMetadataProvider.cs index 9ffe2f7641..0f9cbf1fd2 100644 --- a/src/JsonApiDotNetCore/Configuration/JsonApiModelMetadataProvider.cs +++ b/src/JsonApiDotNetCore/Configuration/JsonApiModelMetadataProvider.cs @@ -32,11 +32,7 @@ public JsonApiModelMetadataProvider(ICompositeMetadataDetailsProvider detailsPro protected override ModelMetadata CreateModelMetadata(DefaultMetadataDetails entry) { var metadata = (DefaultModelMetadata)base.CreateModelMetadata(entry); - - if (metadata.ValidationMetadata.IsRequired == true) - { - metadata.ValidationMetadata.PropertyValidationFilter = _jsonApiValidationFilter; - } + metadata.ValidationMetadata.PropertyValidationFilter = _jsonApiValidationFilter; return metadata; } diff --git a/src/JsonApiDotNetCore/Configuration/JsonApiValidationFilter.cs b/src/JsonApiDotNetCore/Configuration/JsonApiValidationFilter.cs index a27acf8ebd..3fd29a9c65 100644 --- a/src/JsonApiDotNetCore/Configuration/JsonApiValidationFilter.cs +++ b/src/JsonApiDotNetCore/Configuration/JsonApiValidationFilter.cs @@ -1,6 +1,7 @@ using JsonApiDotNetCore.Middleware; using JsonApiDotNetCore.Resources; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata; using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; using Microsoft.Extensions.DependencyInjection; @@ -23,15 +24,13 @@ public JsonApiValidationFilter(IHttpContextAccessor httpContextAccessor) /// public bool ShouldValidateEntry(ValidationEntry entry, ValidationEntry parentEntry) { - IServiceProvider serviceProvider = GetScopedServiceProvider(); - - var request = serviceProvider.GetRequiredService(); - - if (IsId(entry.Key)) + if (entry.Metadata.MetadataKind == ModelMetadataKind.Type || IsId(entry.Key)) { return true; } + IServiceProvider serviceProvider = GetScopedServiceProvider(); + var request = serviceProvider.GetRequiredService(); bool isTopResourceInPrimaryRequest = string.IsNullOrEmpty(parentEntry.Key) && IsAtPrimaryEndpoint(request); if (!isTopResourceInPrimaryRequest) diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/InputValidation/ModelState/ModelStateFakers.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/InputValidation/ModelState/ModelStateFakers.cs index 75183eaf9f..7d7d7d1acf 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/InputValidation/ModelState/ModelStateFakers.cs +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/InputValidation/ModelState/ModelStateFakers.cs @@ -17,14 +17,14 @@ internal sealed class ModelStateFakers : FakerContainer new Faker() .UseSeed(GetFakerSeed()) .RuleFor(systemFile => systemFile.FileName, faker => faker.System.FileName()) + .RuleFor(systemFile => systemFile.Attributes, faker => faker.Random.Enum(FileAttributes.Normal, FileAttributes.Hidden, FileAttributes.ReadOnly)) .RuleFor(systemFile => systemFile.SizeInBytes, faker => faker.Random.Long(0, 1_000_000))); private readonly Lazy> _lazySystemDirectoryFaker = new(() => new Faker() .UseSeed(GetFakerSeed()) - .RuleFor(systemDirectory => systemDirectory.Name, faker => faker.Address.City()) - .RuleFor(systemDirectory => systemDirectory.IsCaseSensitive, faker => faker.Random.Bool()) - .RuleFor(systemDirectory => systemDirectory.SizeInBytes, faker => faker.Random.Long(0, 1_000_000))); + .RuleFor(systemDirectory => systemDirectory.Name, faker => Path.GetFileNameWithoutExtension(faker.System.FileName())) + .RuleFor(systemDirectory => systemDirectory.IsCaseSensitive, faker => faker.Random.Bool())); public Faker SystemVolume => _lazySystemVolumeFaker.Value; public Faker SystemFile => _lazySystemFileFaker.Value; diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/InputValidation/ModelState/ModelStateValidationTests.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/InputValidation/ModelState/ModelStateValidationTests.cs index 7df5b15a74..b114962bd3 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/InputValidation/ModelState/ModelStateValidationTests.cs +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/InputValidation/ModelState/ModelStateValidationTests.cs @@ -166,8 +166,6 @@ public async Task Cannot_create_resource_with_multiple_violations() type = "systemDirectories", attributes = new { - isCaseSensitive = false, - sizeInBytes = -1 } } }; @@ -192,9 +190,9 @@ public async Task Cannot_create_resource_with_multiple_violations() ErrorObject error2 = responseDocument.Errors[1]; error2.StatusCode.Should().Be(HttpStatusCode.UnprocessableEntity); error2.Title.Should().Be("Input validation failed."); - error2.Detail.Should().Be("The field SizeInBytes must be between 0 and 9223372036854775807."); + error2.Detail.Should().Be("The IsCaseSensitive field is required."); error2.Source.ShouldNotBeNull(); - error2.Source.Pointer.Should().Be("/data/attributes/sizeInBytes"); + error2.Source.Pointer.Should().Be("/data/attributes/isCaseSensitive"); } [Fact] @@ -205,15 +203,14 @@ public async Task Does_not_exceed_MaxModelValidationErrors() { data = new { - type = "systemDirectories", + type = "systemFiles", attributes = new { - sizeInBytes = -1 } } }; - const string route = "/systemDirectories"; + const string route = "/systemFiles"; // Act (HttpResponseMessage httpResponse, Document responseDocument) = await _testContext.ExecutePostAsync(route, requestBody); @@ -232,16 +229,16 @@ public async Task Does_not_exceed_MaxModelValidationErrors() ErrorObject error2 = responseDocument.Errors[1]; error2.StatusCode.Should().Be(HttpStatusCode.UnprocessableEntity); error2.Title.Should().Be("Input validation failed."); - error2.Detail.Should().Be("The Name field is required."); + error2.Detail.Should().Be("The FileName field is required."); error2.Source.ShouldNotBeNull(); - error2.Source.Pointer.Should().Be("/data/attributes/directoryName"); + error2.Source.Pointer.Should().Be("/data/attributes/fileName"); ErrorObject error3 = responseDocument.Errors[2]; error3.StatusCode.Should().Be(HttpStatusCode.UnprocessableEntity); error3.Title.Should().Be("Input validation failed."); - error3.Detail.Should().Be("The IsCaseSensitive field is required."); + error3.Detail.Should().Be("The Attributes field is required."); error3.Source.ShouldNotBeNull(); - error3.Source.Pointer.Should().Be("/data/attributes/isCaseSensitive"); + error3.Source.Pointer.Should().Be("/data/attributes/attributes"); } [Fact] @@ -360,13 +357,13 @@ await _testContext.RunOnDatabaseAsync(async dbContext => public async Task Can_update_resource_with_omitted_required_attribute_value() { // Arrange - SystemDirectory existingDirectory = _fakers.SystemDirectory.Generate(); + SystemFile existingFile = _fakers.SystemFile.Generate(); - long newSizeInBytes = _fakers.SystemDirectory.Generate().SizeInBytes; + long? newSizeInBytes = _fakers.SystemFile.Generate().SizeInBytes; await _testContext.RunOnDatabaseAsync(async dbContext => { - dbContext.Directories.Add(existingDirectory); + dbContext.Files.Add(existingFile); await dbContext.SaveChangesAsync(); }); @@ -374,8 +371,8 @@ await _testContext.RunOnDatabaseAsync(async dbContext => { data = new { - type = "systemDirectories", - id = existingDirectory.StringId, + type = "systemFiles", + id = existingFile.StringId, attributes = new { sizeInBytes = newSizeInBytes @@ -383,7 +380,7 @@ await _testContext.RunOnDatabaseAsync(async dbContext => } }; - string route = $"/systemDirectories/{existingDirectory.StringId}"; + string route = $"/systemFiles/{existingFile.StringId}"; // Act (HttpResponseMessage httpResponse, string responseDocument) = await _testContext.ExecutePatchAsync(route, requestBody); diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/InputValidation/ModelState/SystemDirectory.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/InputValidation/ModelState/SystemDirectory.cs index 1712ad103e..4265dc688e 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/InputValidation/ModelState/SystemDirectory.cs +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/InputValidation/ModelState/SystemDirectory.cs @@ -20,10 +20,6 @@ public sealed class SystemDirectory : Identifiable [Required] public bool? IsCaseSensitive { get; set; } - [Attr] - [Range(typeof(long), "0", "9223372036854775807")] - public long SizeInBytes { get; set; } - [HasMany] public ICollection Subdirectories { get; set; } = new List(); diff --git a/test/JsonApiDotNetCoreTests/IntegrationTests/InputValidation/ModelState/SystemFile.cs b/test/JsonApiDotNetCoreTests/IntegrationTests/InputValidation/ModelState/SystemFile.cs index 56bd50d1e7..a5515922e4 100644 --- a/test/JsonApiDotNetCoreTests/IntegrationTests/InputValidation/ModelState/SystemFile.cs +++ b/test/JsonApiDotNetCoreTests/IntegrationTests/InputValidation/ModelState/SystemFile.cs @@ -15,6 +15,9 @@ public sealed class SystemFile : Identifiable [Attr] [Required] - [Range(typeof(long), "0", "9223372036854775807")] - public long? SizeInBytes { get; set; } + public FileAttributes? Attributes { get; set; } + + [Attr] + [Range(typeof(long), "1", "9223372036854775807")] + public long SizeInBytes { get; set; } }