Do not infer Required attributes based on context for non-nullable generic types (#13551)

Fixes https://github.com/aspnet/AspNetCore/issues/13512

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<object>(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

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<string, object>);
+            var property = type.GetProperty(nameof(KeyValuePair<string, object>.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<string, object>);
+            var property = type.GetProperty(nameof(KeyValuePair<string, object>.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()
         {

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<string>),
+            };
+
+            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<Collection<string>>(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<string>),
+            };
+
+            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<Collection<string>>(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<string>
         {
         }

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<string, string>)
+            };
+
+            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<Dictionary<string, string>>(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<string, NonNullPerson>)
+            };
+
+            var testContext = ModelBindingTestHelper.GetTestContext(request =>
+            {
+                request.QueryString = new QueryString("?parameter[key0].Age=&parameter[key0].Name=name0&parameter[key1].Age=27&parameter[key1].Name=");
+            });
+
+            var modelState = testContext.ModelState;
+
+            // Act
+            var modelBindingResult = await parameterBinder.BindModelAsync(parameter, testContext);
+
+            // Assert
+            Assert.True(modelBindingResult.IsModelSet);
+
+            var model = Assert.IsType<Dictionary<string, NonNullPerson>>(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<string, NonNullPersonWithRequiredProperties>)
+            };
+
+            var testContext = ModelBindingTestHelper.GetTestContext(request =>
+            {
+                request.QueryString = new QueryString("?parameter[key0].Age=&parameter[key0].Name=name0&parameter[key1].Age=27&parameter[key1].Name=");
+            });
+
+            var modelState = testContext.ModelState;
+
+            // Act
+            var modelBindingResult = await parameterBinder.BindModelAsync(parameter, testContext);
+
+            // Assert
+            Assert.True(modelBindingResult.IsModelSet);
+
+            var model = Assert.IsType<Dictionary<string, NonNullPersonWithRequiredProperties>>(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<string, string>
         {
         }