diff --git a/src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsMetadataProvider.cs b/src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsMetadataProvider.cs index e63b02edce..0e975e697f 100644 --- a/src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsMetadataProvider.cs +++ b/src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsMetadataProvider.cs @@ -333,7 +333,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations var contextAttributes = context.Attributes; var contextAttributesCount = contextAttributes.Count; var attributes = new List(contextAttributesCount); - + for (var i = 0; i < contextAttributesCount; i++) { var attribute = contextAttributes[i]; @@ -367,15 +367,15 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations else if (context.Key.MetadataKind == ModelMetadataKind.Property) { addInferredRequiredAttribute = IsNullableReferenceType( - context.Key.ContainerType, - member: null, + context.Key.ContainerType, + member: null, context.PropertyAttributes); } else if (context.Key.MetadataKind == ModelMetadataKind.Parameter) { addInferredRequiredAttribute = IsNullableReferenceType( - context.Key.ParameterInfo?.Member.ReflectedType, - context.Key.ParameterInfo.Member, + context.Key.ParameterInfo?.Member.ReflectedType, + context.Key.ParameterInfo.Member, context.ParameterAttributes); } else @@ -494,6 +494,15 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations internal static bool IsNullableBasedOnContext(Type containingType, MemberInfo member) { + // For generic types, inspecting the nullability requirement additionally requires + // inspecting the nullability constraint on generic type parameters. This is fairly non-triviial + // so we'll just avoid calculating it. Users should still be able to apply an explicit [Required] + // attribute on these members. + if (containingType.IsGenericType) + { + return false; + } + // The [Nullable] and [NullableContext] attributes are not inherited. // // The [NullableContext] attribute can appear on a method or on the module. @@ -516,7 +525,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations } type = type.DeclaringType; - } + } while (type != null); // If we don't find the attribute on the declaring type then repeat at the module level diff --git a/src/Mvc/Mvc.DataAnnotations/test/DataAnnotationsMetadataProviderTest.cs b/src/Mvc/Mvc.DataAnnotations/test/DataAnnotationsMetadataProviderTest.cs index 677c452d2d..f93ad363cc 100644 --- a/src/Mvc/Mvc.DataAnnotations/test/DataAnnotationsMetadataProviderTest.cs +++ b/src/Mvc/Mvc.DataAnnotations/test/DataAnnotationsMetadataProviderTest.cs @@ -1339,6 +1339,38 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations Assert.True(result); } + [Fact] + public void IsNullableReferenceType_ReturnsFalse_ForKeyValuePairWithoutNullableConstraints() + { + // Arrange + var type = typeof(KeyValuePair); + var property = type.GetProperty(nameof(KeyValuePair.Key)); + + // Act + var result = DataAnnotationsMetadataProvider.IsNullableReferenceType(type, member: null, property.GetCustomAttributes(inherit: true)); + + // Assert + Assert.False(result); + } + +#nullable enable + [Fact] + public void IsNullableReferenceType_ReturnsTrue_ForKeyValuePairWithNullableConstraints() + { + // Arrange + var type = typeof(KeyValuePair); + var property = type.GetProperty(nameof(KeyValuePair.Key))!; + + // Act + var result = DataAnnotationsMetadataProvider.IsNullableReferenceType(type, member: null, property.GetCustomAttributes(inherit: true)); + + // Assert + // While we'd like for result to be 'true', we don't have a very good way of actually calculating it correctly. + // This test is primarily here to document the behavior. + Assert.False(result); + } +#nullable restore + [Fact] public void IsNonNullable_FindsNullableProperty() { diff --git a/src/Mvc/test/Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs b/src/Mvc/test/Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs index ebe8c73ce9..34a0e4fca8 100644 --- a/src/Mvc/test/Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs +++ b/src/Mvc/test/Mvc.IntegrationTests/CollectionModelBinderIntegrationTest.cs @@ -1126,6 +1126,105 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal(expectedMessage, exception.Message); } + [Fact] + public async Task CollectionModelBinder_CollectionOfSimpleTypes_DoesNotResultInValidationError() + { + // Regression test for https://github.com/aspnet/AspNetCore/issues/13512 + // Arrange + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(Collection), + }; + + var testContext = ModelBindingTestHelper.GetTestContext( + request => + { + request.QueryString = new QueryString("?[0]=hello&[1]="); + }); + + var modelState = testContext.ModelState; + var metadata = testContext.MetadataProvider.GetMetadataForType(parameter.ParameterType); + var valueProvider = await CompositeValueProvider.CreateAsync(testContext); + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(testContext); + + // Act + var result = await parameterBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(modelState.IsValid); + Assert.Equal(0, modelState.ErrorCount); + + Assert.True(result.IsModelSet); + var model = Assert.IsType>(result.Model); + Assert.Collection( + model, + item => Assert.Equal("hello", item), + item => Assert.Null(item)); + + Assert.Collection( + modelState, + kvp => + { + Assert.Equal("[0]", kvp.Key); + Assert.Equal(ModelValidationState.Valid, kvp.Value.ValidationState); + }, + kvp => + { + Assert.Equal("[1]", kvp.Key); + Assert.Equal(ModelValidationState.Valid, kvp.Value.ValidationState); + }); + } + + [Fact] + public async Task CollectionModelBinder_CollectionOfNonNullableTypes_AppliesImplicitRequired() + { + // Arrange + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(Collection), + }; + + var testContext = ModelBindingTestHelper.GetTestContext( + request => + { + request.QueryString = new QueryString("?[0]=hello&[1]="); + }); + + var modelState = testContext.ModelState; + var metadata = testContext.MetadataProvider.GetMetadataForType(parameter.ParameterType); + var valueProvider = await CompositeValueProvider.CreateAsync(testContext); + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(testContext); + + // Act + var result = await parameterBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(modelState.IsValid); + Assert.Equal(0, modelState.ErrorCount); + + Assert.True(result.IsModelSet); + var model = Assert.IsType>(result.Model); + Assert.Collection( + model, + item => Assert.Equal("hello", item), + item => Assert.Null(item)); + + Assert.Collection( + modelState, + kvp => + { + Assert.Equal("[0]", kvp.Key); + Assert.Equal(ModelValidationState.Valid, kvp.Value.ValidationState); + }, + kvp => + { + Assert.Equal("[1]", kvp.Key); + Assert.Equal(ModelValidationState.Valid, kvp.Value.ValidationState); + }); + } + private class ClosedGenericCollection : Collection { } diff --git a/src/Mvc/test/Mvc.IntegrationTests/DictionaryModelBinderIntegrationTest.cs b/src/Mvc/test/Mvc.IntegrationTests/DictionaryModelBinderIntegrationTest.cs index e48a33586f..5bdcafa6c5 100644 --- a/src/Mvc/test/Mvc.IntegrationTests/DictionaryModelBinderIntegrationTest.cs +++ b/src/Mvc/test/Mvc.IntegrationTests/DictionaryModelBinderIntegrationTest.cs @@ -5,6 +5,7 @@ using System; using System.Collections; using System.Collections.Generic; using System.ComponentModel.DataAnnotations; +using System.Linq; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Abstractions; @@ -1162,6 +1163,211 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal(expectedMessage, exception.Message); } + [Fact] + public async Task DictionaryModelBinder_DictionaryOfSimpleType_NullValue_DoesNotResultInRequiredValidation() + { + // Regression test for https://github.com/aspnet/AspNetCore/issues/13512 + // Arrange + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(); + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(Dictionary) + }; + + var testContext = ModelBindingTestHelper.GetTestContext(request => + { + request.QueryString = new QueryString("?parameter[key0]="); + }); + + var modelState = testContext.ModelState; + + // Act + var modelBindingResult = await parameterBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(modelBindingResult.IsModelSet); + + var model = Assert.IsType>(modelBindingResult.Model); + Assert.Collection( + model.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("key0", kvp.Key); + Assert.Null(kvp.Value); + }); + + Assert.Collection( + modelState.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("parameter[key0]", kvp.Key); + Assert.Equal(ModelValidationState.Valid, kvp.Value.ValidationState); + }); + Assert.Equal(0, modelState.ErrorCount); + Assert.True(modelState.IsValid); + } + +#nullable enable + public class NonNullPerson + { + public int Age { get; set; } + + // This should be implicitly required + public string Name { get; set; } = default!; + } +#nullable restore + + [Fact] + public async Task DictionaryModelBinder_ValuesIsNonNullableType_AppliesImplicitRequired() + { + // Arrange + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(); + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(Dictionary) + }; + + var testContext = ModelBindingTestHelper.GetTestContext(request => + { + request.QueryString = new QueryString("?parameter[key0].Age=¶meter[key0].Name=name0¶meter[key1].Age=27¶meter[key1].Name="); + }); + + var modelState = testContext.ModelState; + + // Act + var modelBindingResult = await parameterBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(modelBindingResult.IsModelSet); + + var model = Assert.IsType>(modelBindingResult.Model); + Assert.Collection( + model.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("key0", kvp.Key); + var person = kvp.Value; + Assert.Equal(0, person.Age); + Assert.Equal("name0", person.Name); + }, + kvp => + { + Assert.Equal("key1", kvp.Key); + var person = kvp.Value; + Assert.Equal(27, person.Age); + Assert.Null(person.Name); + }); + + Assert.Collection( + modelState.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("parameter[key0].Age", kvp.Key); + Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState); + Assert.Equal("The value '' is invalid.", Assert.Single(kvp.Value.Errors).ErrorMessage); + }, + kvp => + { + Assert.Equal("parameter[key0].Name", kvp.Key); + Assert.Equal(ModelValidationState.Valid, kvp.Value.ValidationState); + }, + kvp => + { + Assert.Equal("parameter[key1].Age", kvp.Key); + Assert.Equal(ModelValidationState.Valid, kvp.Value.ValidationState); + }, + kvp => + { + Assert.Equal("parameter[key1].Name", kvp.Key); + Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState); + Assert.Equal("The Name field is required.", Assert.Single(kvp.Value.Errors).ErrorMessage); + }); + Assert.Equal(2, modelState.ErrorCount); + Assert.False(modelState.IsValid); + } + +#nullable enable + public class NonNullPersonWithRequiredProperties + { + public int Age { get; set; } + + [Required] + public string? Name { get; set; } + } +#nullable restore + + [Fact] + public async Task DictionaryModelBinder_ValuesNullableTypeWithRequiredAttributes_AppliesValidation() + { + // Arrange + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(); + var parameter = new ParameterDescriptor() + { + Name = "parameter", + ParameterType = typeof(Dictionary) + }; + + var testContext = ModelBindingTestHelper.GetTestContext(request => + { + request.QueryString = new QueryString("?parameter[key0].Age=¶meter[key0].Name=name0¶meter[key1].Age=27¶meter[key1].Name="); + }); + + var modelState = testContext.ModelState; + + // Act + var modelBindingResult = await parameterBinder.BindModelAsync(parameter, testContext); + + // Assert + Assert.True(modelBindingResult.IsModelSet); + + var model = Assert.IsType>(modelBindingResult.Model); + Assert.Collection( + model.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("key0", kvp.Key); + var person = kvp.Value; + Assert.Equal(0, person.Age); + Assert.Equal("name0", person.Name); + }, + kvp => + { + Assert.Equal("key1", kvp.Key); + var person = kvp.Value; + Assert.Equal(27, person.Age); + Assert.Null(person.Name); + }); + + Assert.Collection( + modelState.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("parameter[key0].Age", kvp.Key); + Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState); + Assert.Equal("The value '' is invalid.", Assert.Single(kvp.Value.Errors).ErrorMessage); + }, + kvp => + { + Assert.Equal("parameter[key0].Name", kvp.Key); + Assert.Equal(ModelValidationState.Valid, kvp.Value.ValidationState); + }, + kvp => + { + Assert.Equal("parameter[key1].Age", kvp.Key); + Assert.Equal(ModelValidationState.Valid, kvp.Value.ValidationState); + }, + kvp => + { + Assert.Equal("parameter[key1].Name", kvp.Key); + Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState); + Assert.Equal("The Name field is required.", Assert.Single(kvp.Value.Errors).ErrorMessage); + }); + Assert.Equal(2, modelState.ErrorCount); + Assert.False(modelState.IsValid); + } + private class ClosedGenericDictionary : Dictionary { }