diff options
author | Ryan Nowak <nowakra@gmail.com> | 2019-06-03 03:11:43 +0300 |
---|---|---|
committer | GitHub <noreply@github.com> | 2019-06-03 03:11:43 +0300 |
commit | 91e6839c8d0b3c1723f80ba839749a23ce52870c (patch) | |
tree | 32fb2cf8645b2f4cd3bd46fa255b30c24f7a0c03 | |
parent | 620c673705bb17b33cbc5ff32872d85a5fbf82b9 (diff) |
Adds inferred [Required] for non-null ref types (#9978)
* Adds inferred [Required] for non-null ref types
Follow up from #9194
This change adds the automatic inference of [Required] for non-nullable
properties and parameters. This means that if you opt into nullable
context in C#8, we'll start treating those types as-if you put
[Required] on them.
This provides a nice invariant to rely on, namely that MVC will honor
your declared nullability contract OR report a validation error. This
reinforces the guidance already published by the C# team for using
POCOs/DTOs with nullability. See
https://github.com/aspnet/specs/blob/master/notes/3_0/nullable.md for my
analysis on the topic.
* preemptively fix PR feedback xD
* PR feedback and functional test
* more
* Fix test failures
* fix
* more
* Do a barrel roll
16 files changed, 700 insertions, 126 deletions
diff --git a/src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs b/src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs index 54a8699f3f..f55eeb1ca2 100644 --- a/src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs +++ b/src/Mvc/Mvc.Core/ref/Microsoft.AspNetCore.Mvc.Core.netcoreapp3.0.cs @@ -884,6 +884,7 @@ namespace Microsoft.AspNetCore.Mvc public System.Text.Json.Serialization.JsonSerializerOptions SerializerOptions { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } } public int? SslPort { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } public bool SuppressAsyncSuffixInActionNames { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } + public bool SuppressImplicitRequiredAttributeForNonNullableReferenceTypes { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } public bool SuppressInputFormatterBuffering { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } public bool SuppressOutputFormatterBuffering { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } public bool ValidateComplexTypesIfChildValidationFails { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } [System.Runtime.CompilerServices.CompilerGeneratedAttribute]set { } } @@ -2817,6 +2818,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata public ValidationMetadataProviderContext(Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.ModelMetadataIdentity key, Microsoft.AspNetCore.Mvc.ModelBinding.ModelAttributes attributes) { } public System.Collections.Generic.IReadOnlyList<object> Attributes { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } } public Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.ModelMetadataIdentity Key { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } } + public System.Collections.Generic.IReadOnlyList<object> ParameterAttributes { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } } public System.Collections.Generic.IReadOnlyList<object> PropertyAttributes { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } } public System.Collections.Generic.IReadOnlyList<object> TypeAttributes { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } } public Microsoft.AspNetCore.Mvc.ModelBinding.Metadata.ValidationMetadata ValidationMetadata { [System.Runtime.CompilerServices.CompilerGeneratedAttribute]get { throw null; } } diff --git a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/ValidationMetadataProviderContext.cs b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/ValidationMetadataProviderContext.cs index 84cfdd76bf..dde77f5bd0 100644 --- a/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/ValidationMetadataProviderContext.cs +++ b/src/Mvc/Mvc.Core/src/ModelBinding/Metadata/ValidationMetadataProviderContext.cs @@ -27,6 +27,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata Key = key; Attributes = attributes.Attributes; + ParameterAttributes = attributes.ParameterAttributes; PropertyAttributes = attributes.PropertyAttributes; TypeAttributes = attributes.TypeAttributes; @@ -44,6 +45,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata public ModelMetadataIdentity Key { get; } /// <summary> + /// Gets the parameter attributes. + /// </summary> + public IReadOnlyList<object> ParameterAttributes { get; } + + /// <summary> /// Gets the property attributes. /// </summary> public IReadOnlyList<object> PropertyAttributes { get; } diff --git a/src/Mvc/Mvc.Core/src/MvcOptions.cs b/src/Mvc/Mvc.Core/src/MvcOptions.cs index d3add19c02..4933974b22 100644 --- a/src/Mvc/Mvc.Core/src/MvcOptions.cs +++ b/src/Mvc/Mvc.Core/src/MvcOptions.cs @@ -4,6 +4,7 @@ using System; using System.Collections; using System.Collections.Generic; +using System.ComponentModel.DataAnnotations; using System.Text.Json; using System.Text.Json.Serialization; using Microsoft.AspNetCore.Mvc.ApplicationModels; @@ -103,6 +104,30 @@ namespace Microsoft.AspNetCore.Mvc public FormatterCollection<IInputFormatter> InputFormatters { get; } /// <summary> + /// Gets or sets a value that detemines if the inference of <see cref="RequiredAttribute"/> for + /// for properties and parameters of non-nullable reference types is suppressed. If <c>false</c> + /// (the default), then all non-nullable reference types will behave as-if <c>[Required]</c> has + /// been applied. If <c>true</c>, this behavior will be suppressed; nullable reference types and + /// non-nullable reference types will behave the same for the purposes of validation. + /// </summary> + /// <remarks> + /// <para> + /// This option controls whether MVC model binding and validation treats nullable and non-nullable + /// reference types differently. + /// </para> + /// <para> + /// By default, MVC will treat a non-nullable reference type parameters and properties as-if + /// <c>[Required]</c> has been applied, resulting in validation errors when no value was bound. + /// </para> + /// <para> + /// MVC does not support non-nullable reference type annotations on type arguments and type parameter + /// contraints. The framework will not infer any validation attributes for generic-typed properties + /// or collection elements. + /// </para> + /// </remarks> + public bool SuppressImplicitRequiredAttributeForNonNullableReferenceTypes { get; set; } + + /// <summary> /// Gets or sets a value that determines if buffering is disabled for input formatters that /// synchronously read from the HTTP request body. /// </summary> diff --git a/src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsMetadataProvider.cs b/src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsMetadataProvider.cs index 9ffa816a5b..1299d6f778 100644 --- a/src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsMetadataProvider.cs +++ b/src/Mvc/Mvc.DataAnnotations/src/DataAnnotationsMetadataProvider.cs @@ -23,11 +23,17 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations IDisplayMetadataProvider, IValidationMetadataProvider { + // The [Nullable] attribute is synthesized by the compiler. It's best to just compare the type name. + private const string NullableAttributeFullTypeName = "System.Runtime.CompilerServices.NullableAttribute"; + private const string NullableFlagsFieldName = "NullableFlags"; + private readonly IStringLocalizerFactory _stringLocalizerFactory; + private readonly MvcOptions _options; private readonly MvcDataAnnotationsLocalizationOptions _localizationOptions; public DataAnnotationsMetadataProvider( - IOptions<MvcDataAnnotationsLocalizationOptions> options, + MvcOptions options, + IOptions<MvcDataAnnotationsLocalizationOptions> localizationOptions, IStringLocalizerFactory stringLocalizerFactory) { if (options == null) @@ -35,7 +41,13 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations throw new ArgumentNullException(nameof(options)); } - _localizationOptions = options.Value; + if (localizationOptions == null) + { + throw new ArgumentNullException(nameof(localizationOptions)); + } + + _options = options; + _localizationOptions = localizationOptions.Value; _stringLocalizerFactory = stringLocalizerFactory; } @@ -328,6 +340,29 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations // RequiredAttribute marks a property as required by validation - this means that it // must have a non-null value on the model during validation. var requiredAttribute = attributes.OfType<RequiredAttribute>().FirstOrDefault(); + + // For non-nullable reference types, treat them as-if they had an implicit [Required]. + // This allows the developer to specify [Required] to customize the error message, so + // if they already have [Required] then there's no need for us to do this check. + if (!_options.SuppressImplicitRequiredAttributeForNonNullableReferenceTypes && + requiredAttribute == null && + !context.Key.ModelType.IsValueType && + + // Look specifically at attributes on the property/parameter. [Nullable] on + // the type has a different meaning. + IsNonNullable(context.ParameterAttributes ?? context.PropertyAttributes ?? Array.Empty<object>())) + { + // Since this behavior specifically relates to non-null-ness, we will use the non-default + // option to tolerate empty/whitespace strings. empty/whitespace INPUT will still result in + // a validation error by default because we convert empty/whitespace strings to null + // unless you say otherwise. + requiredAttribute = new RequiredAttribute() + { + AllowEmptyStrings = true, + }; + attributes.Add(requiredAttribute); + } + if (requiredAttribute != null) { context.ValidationMetadata.IsRequired = true; @@ -380,5 +415,35 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations return string.Empty; } + + // Internal for testing + internal static bool IsNonNullable(IEnumerable<object> attributes) + { + // [Nullable] is compiler synthesized, comparing by name. + var nullableAttribute = attributes + .Where(a => string.Equals(a.GetType().FullName, NullableAttributeFullTypeName, StringComparison.Ordinal)) + .FirstOrDefault(); + if (nullableAttribute == null) + { + return false; + } + + // We don't handle cases where generics and NNRT are used. This runs into a + // fundamental limitation of ModelMetadata - we use a single Type and Property/Parameter + // to look up the metadata. However when generics are involved and NNRT is in use + // the distance between the [Nullable] and member we're looking at is potentially + // unbounded. + // + // See: https://github.com/dotnet/roslyn/blob/master/docs/features/nullable-reference-types.md#annotations + if (nullableAttribute.GetType().GetField(NullableFlagsFieldName) is FieldInfo field && + field.GetValue(nullableAttribute) is byte[] flags && + flags.Length >= 0 && + flags[0] == 1) // First element is the property/parameter type. + { + return true; + } + + return false; + } } } diff --git a/src/Mvc/Mvc.DataAnnotations/src/DependencyInjection/MvcDataAnnotationsMvcOptionsSetup.cs b/src/Mvc/Mvc.DataAnnotations/src/DependencyInjection/MvcDataAnnotationsMvcOptionsSetup.cs index f43d3cca66..1de8755e97 100644 --- a/src/Mvc/Mvc.DataAnnotations/src/DependencyInjection/MvcDataAnnotationsMvcOptionsSetup.cs +++ b/src/Mvc/Mvc.DataAnnotations/src/DependencyInjection/MvcDataAnnotationsMvcOptionsSetup.cs @@ -53,6 +53,7 @@ namespace Microsoft.Extensions.DependencyInjection } options.ModelMetadataDetailsProviders.Add(new DataAnnotationsMetadataProvider( + options, _dataAnnotationLocalizationOptions, _stringLocalizerFactory)); diff --git a/src/Mvc/Mvc.DataAnnotations/test/DataAnnotationsMetadataProviderTest.cs b/src/Mvc/Mvc.DataAnnotations/test/DataAnnotationsMetadataProviderTest.cs index 58516c4b6b..e3fa20042a 100644 --- a/src/Mvc/Mvc.DataAnnotations/test/DataAnnotationsMetadataProviderTest.cs +++ b/src/Mvc/Mvc.DataAnnotations/test/DataAnnotationsMetadataProviderTest.cs @@ -6,6 +6,7 @@ using System.Collections.Generic; using System.ComponentModel; using System.ComponentModel.DataAnnotations; using System.Globalization; +using System.Linq; using Microsoft.AspNetCore.Mvc.ModelBinding; using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata; using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; @@ -90,9 +91,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations object expected) { // Arrange - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory: null); + var provider = CreateProvider(); var key = ModelMetadataIdentity.ForType(typeof(string)); var context = new DisplayMetadataProviderContext(key, GetModelAttributes(new object[] { attribute })); @@ -109,9 +108,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations public void CreateDisplayMetadata_FindsDisplayFormat_FromDataType() { // Arrange - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory: null); + var provider = CreateProvider(); var dataType = new DataTypeAttribute(DataType.Currency); var displayFormat = dataType.DisplayFormat; // Non-null for DataType.Currency. @@ -131,9 +128,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations public void CreateDisplayMetadata_FindsDisplayFormat_OverridingDataType() { // Arrange - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory: null); + var provider = CreateProvider(); var dataType = new DataTypeAttribute(DataType.Time); // Has a non-null DisplayFormat. var displayFormat = new DisplayFormatAttribute() // But these values override the values from DataType @@ -156,9 +151,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations public void CreateBindingMetadata_EditableAttributeFalse_SetsReadOnlyTrue() { // Arrange - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory: null); + var provider = CreateProvider(); var editable = new EditableAttribute(allowEdit: false); @@ -177,9 +170,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations public void CreateBindingMetadata_EditableAttributeTrue_SetsReadOnlyFalse() { // Arrange - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory: null); + var provider = CreateProvider(); var editable = new EditableAttribute(allowEdit: true); @@ -198,11 +189,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations public void CreateDisplayMetadata_DisplayAttribute_OverridesDisplayNameAttribute() { // Arrange - var localizationOptions = Options.Create(new MvcDataAnnotationsLocalizationOptions()); - - var provider = new DataAnnotationsMetadataProvider( - localizationOptions, - stringLocalizerFactory: null); + var provider = CreateProvider(); var displayName = new DisplayNameAttribute("DisplayNameAttributeValue"); var display = new DisplayAttribute() @@ -225,11 +212,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations public void CreateDisplayMetadata_DisplayAttribute_OverridesDisplayNameAttribute_IfNameEmpty() { // Arrange - var localizationOptions = Options.Create(new MvcDataAnnotationsLocalizationOptions()); - - var provider = new DataAnnotationsMetadataProvider( - localizationOptions, - stringLocalizerFactory: null); + var provider = CreateProvider(); var displayName = new DisplayNameAttribute("DisplayNameAttributeValue"); var display = new DisplayAttribute() @@ -252,11 +235,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations public void CreateDisplayMetadata_DisplayAttribute_DoesNotOverrideDisplayNameAttribute_IfNameNull() { // Arrange - var localizationOptions = Options.Create(new MvcDataAnnotationsLocalizationOptions()); - - var provider = new DataAnnotationsMetadataProvider( - localizationOptions, - stringLocalizerFactory: null); + var provider = CreateProvider(); var displayName = new DisplayNameAttribute("DisplayNameAttributeValue"); var display = new DisplayAttribute() @@ -289,15 +268,13 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations .Setup(s => s.Create(typeof(EmptyClass))) .Returns(() => sharedLocalizer.Object); - var localizationOptions = Options.Create(new MvcDataAnnotationsLocalizationOptions()); - localizationOptions.Value.DataAnnotationLocalizerProvider = (type, stringLocalizerFactory) => + var localizationOptions = new MvcDataAnnotationsLocalizationOptions(); + localizationOptions.DataAnnotationLocalizerProvider = (type, stringLocalizerFactory) => { return stringLocalizerFactory.Create(typeof(EmptyClass)); }; - var provider = new DataAnnotationsMetadataProvider( - localizationOptions, - stringLocalizerFactory: stringLocalizerFactoryMock.Object); + var provider = CreateProvider(options: null, localizationOptions, stringLocalizerFactoryMock.Object); var displayName = new DisplayNameAttribute("DisplayNameValue"); @@ -329,15 +306,13 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations .Setup(s => s.Create(typeof(EmptyClass))) .Returns(() => sharedLocalizer.Object); - var localizationOptions = Options.Create(new MvcDataAnnotationsLocalizationOptions()); - localizationOptions.Value.DataAnnotationLocalizerProvider = (type, stringLocalizerFactory) => + var localizationOptions = new MvcDataAnnotationsLocalizationOptions(); + localizationOptions.DataAnnotationLocalizerProvider = (type, stringLocalizerFactory) => { return stringLocalizerFactory.Create(typeof(EmptyClass)); }; - var provider = new DataAnnotationsMetadataProvider( - localizationOptions, - stringLocalizerFactory: stringLocalizerFactoryMock.Object); + var provider = CreateProvider(options: null, localizationOptions, stringLocalizerFactoryMock.Object); var displayName = new DisplayNameAttribute("DisplayNameValue"); @@ -363,15 +338,15 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations .Setup(s => s.Create(typeof(EmptyClass))) .Returns(() => sharedLocalizer.Object); - var options = Options.Create(new MvcDataAnnotationsLocalizationOptions()); + var localizationOptions = new MvcDataAnnotationsLocalizationOptions(); var dataAnnotationLocalizerProviderWasUsed = false; - options.Value.DataAnnotationLocalizerProvider = (type, stringLocalizerFactory) => + localizationOptions.DataAnnotationLocalizerProvider = (type, stringLocalizerFactory) => { dataAnnotationLocalizerProviderWasUsed = true; return stringLocalizerFactory.Create(typeof(EmptyClass)); }; - var provider = new DataAnnotationsMetadataProvider(options, stringLocalizerFactoryMock.Object); + var provider = CreateProvider(options: null, localizationOptions, stringLocalizerFactoryMock.Object); var display = new DisplayAttribute() { @@ -395,9 +370,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations public void CreateDisplayMetadata_DisplayAttribute_NameFromResources_NullLocalizer() { // Arrange - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory: null); + var provider = CreateProvider(); var display = new DisplayAttribute() { @@ -432,9 +405,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations stringLocalizerFactory .Setup(s => s.Create(It.IsAny<Type>())) .Returns(() => stringLocalizer.Object); - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory.Object); + var provider = CreateProvider(stringLocalizerFactory: stringLocalizerFactory.Object); var display = new DisplayAttribute() { @@ -469,9 +440,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations stringLocalizerFactory .Setup(s => s.Create(It.IsAny<Type>())) .Returns(() => stringLocalizer.Object); - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory.Object); + var provider = CreateProvider(stringLocalizerFactory: stringLocalizerFactory.Object); var display = new DisplayAttribute() { @@ -500,9 +469,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations public void CreateDisplayMetadata_DisplayAttribute_DescriptionFromResources_NullLocalizer() { // Arrange - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory: null); + var provider = CreateProvider(); var display = new DisplayAttribute() { @@ -537,9 +504,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations stringLocalizerFactory .Setup(s => s.Create(It.IsAny<Type>())) .Returns(() => stringLocalizer.Object); - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory.Object); + var provider = CreateProvider(stringLocalizerFactory: stringLocalizerFactory.Object); var display = new DisplayAttribute() { @@ -568,9 +533,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations public void CreateDisplayMetadata_DisplayAttribute_PromptFromResources_NullLocalizer() { // Arrange - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory: null); + var provider = CreateProvider(); var display = new DisplayAttribute() { @@ -613,13 +576,13 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations stringLocalizerFactoryMock .Setup(f => f.Create(It.IsAny<Type>())) .Returns(stringLocalizer.Object); - var options = Options.Create(new MvcDataAnnotationsLocalizationOptions()); - options.Value.DataAnnotationLocalizerProvider = (type, stringLocalizerFactory) => + var localizationOptions = new MvcDataAnnotationsLocalizationOptions(); + localizationOptions.DataAnnotationLocalizerProvider = (type, stringLocalizerFactory) => { return stringLocalizerFactory.Create(type); }; - var provider = new DataAnnotationsMetadataProvider(options, stringLocalizerFactoryMock.Object); + var provider = CreateProvider(options: null, localizationOptions, stringLocalizerFactoryMock.Object); var display = new DisplayAttribute() { @@ -671,9 +634,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations public void CreateDisplayMetadata_IsEnum_ReflectsModelType(Type type, bool expectedIsEnum) { // Arrange - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory: null); + var provider = CreateProvider(); var key = ModelMetadataIdentity.ForType(type); var attributes = new object[0]; @@ -707,9 +668,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations public void CreateDisplayMetadata_IsFlagsEnum_ReflectsModelType(Type type, bool expectedIsFlagsEnum) { // Arrange - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory: null); + var provider = CreateProvider(); var key = ModelMetadataIdentity.ForType(type); var attributes = new object[0]; @@ -841,9 +800,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations IReadOnlyDictionary<string, string> expectedDictionary) { // Arrange - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory: null); + var provider = CreateProvider(); var key = ModelMetadataIdentity.ForType(type); var attributes = new object[0]; @@ -887,9 +844,9 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations .Setup(f => f.Create(It.IsAny<Type>())) .Returns(stringLocalizer.Object); - var options = Options.Create(new MvcDataAnnotationsLocalizationOptions()); - options.Value.DataAnnotationLocalizerProvider = (modelType, stringLocalizerFactory) => stringLocalizerFactory.Create(modelType); - var provider = new DataAnnotationsMetadataProvider(options, stringLocalizerFactoryMock.Object); + var localizationOptions = new MvcDataAnnotationsLocalizationOptions(); + localizationOptions.DataAnnotationLocalizerProvider = (modelType, stringLocalizerFactory) => stringLocalizerFactory.Create(modelType); + var provider = CreateProvider(options: null, localizationOptions, stringLocalizerFactoryMock.Object); // Act provider.CreateDisplayMetadata(context); @@ -1021,9 +978,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations IEnumerable<KeyValuePair<EnumGroupAndName, string>> expectedKeyValuePairs) { // Arrange - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory: null); + var provider = CreateProvider(); var key = ModelMetadataIdentity.ForType(type); var attributes = new object[0]; @@ -1051,9 +1006,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations new KeyValuePair<EnumGroupAndName, string>(new EnumGroupAndName(string.Empty, nameof(EnumWithDisplayOrder.Null)), "3"), }; - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory: null); + var provider = CreateProvider(); var key = ModelMetadataIdentity.ForType(typeof(EnumWithDisplayOrder)); var attributes = new object[0]; @@ -1153,9 +1106,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations public void CreateValidationMetadata_RequiredAttribute_SetsIsRequiredToTrue() { // Arrange - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory: null); + var provider = CreateProvider(); var required = new RequiredAttribute(); @@ -1177,9 +1128,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations public void CreateValidationMetadata_NoRequiredAttribute_IsRequiredLeftAlone(bool? initialValue) { // Arrange - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory: null); + var provider = CreateProvider(); var attributes = new Attribute[] { }; var key = ModelMetadataIdentity.ForProperty(typeof(int), "Length", typeof(string)); @@ -1194,12 +1143,82 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations } [Fact] + public void CreateValidationMetadata_InfersRequiredAttribute_NoNonNullableProperty() + { + // Arrange + var provider = CreateProvider(); + + var attributes = ModelAttributes.GetAttributesForProperty( + typeof(NullableReferenceTypes), + typeof(NullableReferenceTypes).GetProperty(nameof(NullableReferenceTypes.NonNullableReferenceType))); + var key = ModelMetadataIdentity.ForProperty( + typeof(NullableReferenceTypes), + nameof(NullableReferenceTypes.NonNullableReferenceType), typeof(string)); + var context = new ValidationMetadataProviderContext(key, attributes); + + // Act + provider.CreateValidationMetadata(context); + + // Assert + Assert.True(context.ValidationMetadata.IsRequired); + var attribute = Assert.Single(context.ValidationMetadata.ValidatorMetadata, m => m is RequiredAttribute); + Assert.True(((RequiredAttribute)attribute).AllowEmptyStrings); // non-Default for [Required] + } + + [Fact] + public void CreateValidationMetadata_InfersRequiredAttribute_NoNonNullableProperty_PrefersExistingRequiredAttribute() + { + // Arrange + var provider = CreateProvider(); + + var attributes = ModelAttributes.GetAttributesForProperty( + typeof(NullableReferenceTypes), + typeof(NullableReferenceTypes).GetProperty(nameof(NullableReferenceTypes.NonNullableReferenceTypeWithRequired))); + var key = ModelMetadataIdentity.ForProperty( + typeof(NullableReferenceTypes), + nameof(NullableReferenceTypes.NonNullableReferenceTypeWithRequired), typeof(string)); + var context = new ValidationMetadataProviderContext(key, attributes); + + // Act + provider.CreateValidationMetadata(context); + + // Assert + Assert.True(context.ValidationMetadata.IsRequired); + var attribute = Assert.Single(context.ValidationMetadata.ValidatorMetadata, m => m is RequiredAttribute a); + Assert.Equal("Test", ((RequiredAttribute)attribute).ErrorMessage); + Assert.False(((RequiredAttribute)attribute).AllowEmptyStrings); // Default for [Required] + } + + [Fact] + public void CreateValidationMetadata_SuppressRequiredInference_Noops() + { + // Arrange + var provider = CreateProvider(options: new MvcOptions() + { + SuppressImplicitRequiredAttributeForNonNullableReferenceTypes = true, + }); + + var attributes = ModelAttributes.GetAttributesForProperty( + typeof(NullableReferenceTypes), + typeof(NullableReferenceTypes).GetProperty(nameof(NullableReferenceTypes.NonNullableReferenceType))); + var key = ModelMetadataIdentity.ForProperty( + typeof(NullableReferenceTypes), + nameof(NullableReferenceTypes.NonNullableReferenceType), typeof(string)); + var context = new ValidationMetadataProviderContext(key, attributes); + + // Act + provider.CreateValidationMetadata(context); + + // Assert + Assert.Null(context.ValidationMetadata.IsRequired); + Assert.DoesNotContain(context.ValidationMetadata.ValidatorMetadata, m => m is RequiredAttribute); + } + + [Fact] public void CreateValidationMetadata_WillAddValidationAttributes_From_ValidationProviderAttribute() { // Arrange - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory: null); + var provider = CreateProvider(); var validationProviderAttribute = new FooCompositeValidationAttribute( attributes: new List<ValidationAttribute> { @@ -1232,9 +1251,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations public void CreateBindingMetadata_RequiredAttribute_IsBindingRequiredLeftAlone(bool initialValue) { // Arrange - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory: null); + var provider = CreateProvider(); var attributes = new Attribute[] { new RequiredAttribute() }; var key = ModelMetadataIdentity.ForProperty(typeof(int), "Length", typeof(string)); @@ -1255,9 +1272,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations public void CreateBindingDetails_NoEditableAttribute_IsReadOnlyLeftAlone(bool? initialValue) { // Arrange - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory: null); + var provider = CreateProvider(); var attributes = new Attribute[] { }; var key = ModelMetadataIdentity.ForProperty(typeof(int), "Length", typeof(string)); @@ -1275,9 +1290,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations public void CreateValidationDetails_ValidatableObject_ReturnsObject() { // Arrange - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory: null); + var provider = CreateProvider(); var attribute = new TestValidationAttribute(); var attributes = new Attribute[] { attribute }; @@ -1296,9 +1309,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations public void CreateValidationDetails_ValidatableObject_AlreadyInContext_Ignores() { // Arrange - var provider = new DataAnnotationsMetadataProvider( - Options.Create(new MvcDataAnnotationsLocalizationOptions()), - stringLocalizerFactory: null); + var provider = CreateProvider(); var attribute = new TestValidationAttribute(); var attributes = new Attribute[] { attribute }; @@ -1314,6 +1325,64 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations Assert.Same(attribute, validatorMetadata); } + [Fact] + public void IsNonNullable_FindsNonNullableProperty() + { + // Arrange + var type = typeof(NullableReferenceTypes); + var property = type.GetProperty(nameof(NullableReferenceTypes.NonNullableReferenceType)); + + // Act + var result = DataAnnotationsMetadataProvider.IsNonNullable(property.GetCustomAttributes(inherit: true)); + + // Assert + Assert.True(result); + } + + [Fact] + public void IsNonNullable_FindsNullableProperty() + { + // Arrange + var type = typeof(NullableReferenceTypes); + var property = type.GetProperty(nameof(NullableReferenceTypes.NullableReferenceType)); + + // Act + var result = DataAnnotationsMetadataProvider.IsNonNullable(property.GetCustomAttributes(inherit: true)); + + // Assert + Assert.False(result); + } + + [Fact] + public void IsNonNullable_FindsNonNullableParameter() + { + // Arrange + var type = typeof(NullableReferenceTypes); + var method = type.GetMethod(nameof(NullableReferenceTypes.Method)); + var parameter = method.GetParameters().Where(p => p.Name == "nonNullableParameter").Single(); + + // Act + var result = DataAnnotationsMetadataProvider.IsNonNullable(parameter.GetCustomAttributes(inherit: true)); + + // Assert + Assert.True(result); + } + + [Fact] + public void IsNonNullable_FindsNullableParameter() + { + // Arrange + var type = typeof(NullableReferenceTypes); + var method = type.GetMethod(nameof(NullableReferenceTypes.Method)); + var parameter = method.GetParameters().Where(p => p.Name == "nullableParameter").Single(); + + // Act + var result = DataAnnotationsMetadataProvider.IsNonNullable(parameter.GetCustomAttributes(inherit: true)); + + // Assert + Assert.False(result); + } + private IEnumerable<KeyValuePair<EnumGroupAndName, string>> GetLocalizedEnumGroupedDisplayNamesAndValues( bool useStringLocalizer) { @@ -1328,6 +1397,17 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations return context.DisplayMetadata.EnumGroupedDisplayNamesAndValues; } + private DataAnnotationsMetadataProvider CreateProvider( + MvcOptions options = null, + MvcDataAnnotationsLocalizationOptions localizationOptions = null, + IStringLocalizerFactory stringLocalizerFactory = null) + { + return new DataAnnotationsMetadataProvider( + options ?? new MvcOptions(), + Options.Create(localizationOptions ?? new MvcDataAnnotationsLocalizationOptions()), + stringLocalizerFactory); + } + private DataAnnotationsMetadataProvider CreateIStringLocalizerProvider(bool useStringLocalizer) { var stringLocalizer = new Mock<IStringLocalizer>(MockBehavior.Strict); @@ -1343,12 +1423,10 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations .Setup(factory => factory.Create(typeof(EnumWithLocalizedDisplayNames))) .Returns(stringLocalizer.Object); - var options = Options.Create(new MvcDataAnnotationsLocalizationOptions()); - options.Value.DataAnnotationLocalizerProvider = (modelType, localizerFactory) => localizerFactory.Create(modelType); + var localizationOptions = new MvcDataAnnotationsLocalizationOptions(); + localizationOptions.DataAnnotationLocalizerProvider = (modelType, localizerFactory) => localizerFactory.Create(modelType); - return new DataAnnotationsMetadataProvider( - options, - useStringLocalizer ? stringLocalizerFactory.Object : null); + return CreateProvider(options: null, localizationOptions, useStringLocalizer ? stringLocalizerFactory.Object : null); } private ModelAttributes GetModelAttributes(IEnumerable<object> typeAttributes) @@ -1532,5 +1610,21 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations return _attributes; } } + +#nullable enable + private class NullableReferenceTypes + { + public string NonNullableReferenceType { get; set; } = default!; + + [Required(ErrorMessage = "Test")] + public string NonNullableReferenceTypeWithRequired { get; set; } = default!; + + public string? NullableReferenceType { get; set; } = default!; + + public void Method(string nonNullableParameter, string? nullableParameter) + { + } + } +#nullable restore } } diff --git a/src/Mvc/Mvc.DataAnnotations/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test.csproj b/src/Mvc/Mvc.DataAnnotations/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test.csproj index e841d748dd..c205a676cd 100644 --- a/src/Mvc/Mvc.DataAnnotations/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test.csproj +++ b/src/Mvc/Mvc.DataAnnotations/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test.csproj @@ -2,6 +2,7 @@ <PropertyGroup> <TargetFramework>netcoreapp3.0</TargetFramework> + <UseSharedCompilation>false</UseSharedCompilation> </PropertyGroup> <ItemGroup> diff --git a/src/Mvc/Mvc.DataAnnotations/test/ModelMetadataProviderTest.cs b/src/Mvc/Mvc.DataAnnotations/test/ModelMetadataProviderTest.cs index a103fefb96..2a40de238d 100644 --- a/src/Mvc/Mvc.DataAnnotations/test/ModelMetadataProviderTest.cs +++ b/src/Mvc/Mvc.DataAnnotations/test/ModelMetadataProviderTest.cs @@ -1052,6 +1052,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations { new DefaultBindingMetadataProvider(), new DataAnnotationsMetadataProvider( + new MvcOptions(), Options.Create(new MvcDataAnnotationsLocalizationOptions()), stringLocalizerFactory: null), }), diff --git a/src/Mvc/shared/Mvc.Core.TestCommon/TestModelMetadataProvider.cs b/src/Mvc/shared/Mvc.Core.TestCommon/TestModelMetadataProvider.cs index d72bc80148..d284e4197d 100644 --- a/src/Mvc/shared/Mvc.Core.TestCommon/TestModelMetadataProvider.cs +++ b/src/Mvc/shared/Mvc.Core.TestCommon/TestModelMetadataProvider.cs @@ -17,10 +17,10 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding { private static DataAnnotationsMetadataProvider CreateDefaultDataAnnotationsProvider(IStringLocalizerFactory stringLocalizerFactory) { - var options = Options.Create(new MvcDataAnnotationsLocalizationOptions()); - options.Value.DataAnnotationLocalizerProvider = (modelType, localizerFactory) => localizerFactory.Create(modelType); + var localizationOptions = Options.Create(new MvcDataAnnotationsLocalizationOptions()); + localizationOptions.Value.DataAnnotationLocalizerProvider = (modelType, localizerFactory) => localizerFactory.Create(modelType); - return new DataAnnotationsMetadataProvider(options, stringLocalizerFactory); + return new DataAnnotationsMetadataProvider(new MvcOptions(), localizationOptions, stringLocalizerFactory); } // Creates a provider with all the defaults - includes data annotations @@ -50,6 +50,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding new DefaultBindingMetadataProvider(), new DefaultValidationMetadataProvider(), new DataAnnotationsMetadataProvider( + new MvcOptions(), Options.Create(new MvcDataAnnotationsLocalizationOptions()), stringLocalizerFactory: null), new DataMemberRequiredBindingMetadataProvider(), @@ -92,6 +93,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding new DefaultBindingMetadataProvider(), new DefaultValidationMetadataProvider(), new DataAnnotationsMetadataProvider( + new MvcOptions(), Options.Create(new MvcDataAnnotationsLocalizationOptions()), stringLocalizerFactory: null), detailsProvider diff --git a/src/Mvc/test/Mvc.FunctionalTests/NonNullableReferenceTypesTest.cs b/src/Mvc/test/Mvc.FunctionalTests/NonNullableReferenceTypesTest.cs new file mode 100644 index 0000000000..e22462ce34 --- /dev/null +++ b/src/Mvc/test/Mvc.FunctionalTests/NonNullableReferenceTypesTest.cs @@ -0,0 +1,110 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Collections.Generic; +using System.Net; +using System.Net.Http; +using System.Net.Http.Headers; +using System.Reflection; +using System.Threading.Tasks; +using AngleSharp.Dom.Html; +using AngleSharp.Parser.Html; +using BasicWebSite.Models; +using Newtonsoft.Json; +using Newtonsoft.Json.Linq; +using Xunit; + +namespace Microsoft.AspNetCore.Mvc.FunctionalTests +{ + public class NonNullableReferenceTypesTest : IClassFixture<MvcTestFixture<BasicWebSite.StartupWithoutEndpointRouting>> + { + public NonNullableReferenceTypesTest(MvcTestFixture<BasicWebSite.StartupWithoutEndpointRouting> fixture) + { + Client = fixture.CreateDefaultClient(); + } + + private HttpClient Client { get; set; } + + [Fact] + public async Task CanUseNonNullableReferenceType_WithController_OmitData_ValidationErrors() + { + // Arrange + var parser = new HtmlParser(); + + // Act 1 + var response = await Client.GetAsync("http://localhost/NonNullable"); + + // Assert 1 + await response.AssertStatusCodeAsync(HttpStatusCode.OK); + var content = await response.Content.ReadAsStringAsync(); + + var document = parser.Parse(content); + var errors = document.QuerySelectorAll("#errors > ul > li"); + var li = Assert.Single(errors); + Assert.Empty(li.TextContent); + + var cookieToken = AntiforgeryTestHelper.RetrieveAntiforgeryCookie(response); + var formToken = document.RetrieveAntiforgeryToken(); + + var request = new HttpRequestMessage(HttpMethod.Post, "http://localhost/NonNullable"); + request.Headers.Add("Cookie", cookieToken.Key + "=" + cookieToken.Value); + request.Content = new FormUrlEncodedContent(new[] + { + new KeyValuePair<string, string>("__RequestVerificationToken", formToken), + }); + + // Act 2 + response = await Client.SendAsync(request); + + // Assert 2 + // + // OK means there were validation errors. + await response.AssertStatusCodeAsync(HttpStatusCode.OK); + content = await response.Content.ReadAsStringAsync(); + + document = parser.Parse(content); + errors = errors = document.QuerySelectorAll("#errors > ul > li"); + Assert.Equal(2, errors.Length); // Not validating BCL error messages + } + + [Fact] + public async Task CanUseNonNullableReferenceType_WithController_SubmitData_NoError() + { + // Arrange + var parser = new HtmlParser(); + + // Act 1 + var response = await Client.GetAsync("http://localhost/NonNullable"); + + // Assert 1 + await response.AssertStatusCodeAsync(HttpStatusCode.OK); + var content = await response.Content.ReadAsStringAsync(); + + var document = parser.Parse(content); + var errors = document.QuerySelectorAll("#errors > ul > li"); + var li = Assert.Single(errors); + Assert.Empty(li.TextContent); + + var cookieToken = AntiforgeryTestHelper.RetrieveAntiforgeryCookie(response); + var formToken = document.RetrieveAntiforgeryToken(); + + var request = new HttpRequestMessage(HttpMethod.Post, "http://localhost/NonNullable"); + request.Headers.Add("Cookie", cookieToken.Key + "=" + cookieToken.Value); + request.Content = new FormUrlEncodedContent(new[] + { + new KeyValuePair<string, string>("__RequestVerificationToken", formToken), + new KeyValuePair<string, string>("Name", "Pranav"), + new KeyValuePair<string, string>("description", "Meme") + }); + + // Act 2 + response = await Client.SendAsync(request); + + // Assert 2 + // + // Redirect means there were no validation errors. + await response.AssertStatusCodeAsync(HttpStatusCode.Redirect); + } + } +} diff --git a/src/Mvc/test/Mvc.IntegrationTests/ComplexTypeModelBinderIntegrationTest.cs b/src/Mvc/test/Mvc.IntegrationTests/ComplexTypeModelBinderIntegrationTest.cs index 7d58c007ef..19d65e864c 100644 --- a/src/Mvc/test/Mvc.IntegrationTests/ComplexTypeModelBinderIntegrationTest.cs +++ b/src/Mvc/test/Mvc.IntegrationTests/ComplexTypeModelBinderIntegrationTest.cs @@ -1715,7 +1715,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests private class Order8 { - public string Name { get; set; } + public string Name { get; set; } = default!; public KeyValuePair<string, int> ProductId { get; set; } } @@ -1869,13 +1869,15 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal("bill", model.Name); Assert.Equal(default, model.ProductId); - Assert.Single(modelState); - Assert.Equal(0, modelState.ErrorCount); - Assert.True(modelState.IsValid); + Assert.Equal(1, modelState.ErrorCount); + Assert.False(modelState.IsValid); var entry = Assert.Single(modelState, e => e.Key == "parameter.Name").Value; Assert.Equal("bill", entry.AttemptedValue); Assert.Equal("bill", entry.RawValue); + + entry = Assert.Single(modelState, e => e.Key == "parameter.ProductId.Key").Value; + Assert.Single(entry.Errors); } [Fact] @@ -1916,9 +1918,11 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Null(model.Name); Assert.Equal(default, model.ProductId); - Assert.Empty(modelState); - Assert.Equal(0, modelState.ErrorCount); - Assert.True(modelState.IsValid); + Assert.Equal(1, modelState.ErrorCount); + Assert.False(modelState.IsValid); + + var entry = Assert.Single(modelState, e => e.Key == "ProductId.Key").Value; + Assert.Single(entry.Errors); } private class Car4 diff --git a/src/Mvc/test/Mvc.IntegrationTests/KeyValuePairModelBinderIntegrationTest.cs b/src/Mvc/test/Mvc.IntegrationTests/KeyValuePairModelBinderIntegrationTest.cs index 42b6b827f9..974cce8bf0 100644 --- a/src/Mvc/test/Mvc.IntegrationTests/KeyValuePairModelBinderIntegrationTest.cs +++ b/src/Mvc/test/Mvc.IntegrationTests/KeyValuePairModelBinderIntegrationTest.cs @@ -337,9 +337,11 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal(new KeyValuePair<string, int>(), modelBindingResult.Model); - Assert.Empty(modelState); - Assert.Equal(0, modelState.ErrorCount); - Assert.True(modelState.IsValid); + Assert.Equal(1, modelState.ErrorCount); + Assert.False(modelState.IsValid); + + var entry = Assert.Single(modelState, kvp => kvp.Key == "Key").Value; + Assert.Single(entry.Errors); } private class Person @@ -500,9 +502,14 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests Assert.Equal(new KeyValuePair<string, Person>(), modelBindingResult.Model); - Assert.Empty(modelState); - Assert.Equal(0, modelState.ErrorCount); - Assert.True(modelState.IsValid); + Assert.Equal(2, modelState.ErrorCount); + Assert.False(modelState.IsValid); + + var entry = Assert.Single(modelState, kvp => kvp.Key == "Key").Value; + Assert.Single(entry.Errors); + + entry = Assert.Single(modelState, kvp => kvp.Key == "Value").Value; + Assert.Single(entry.Errors); } [Fact] diff --git a/src/Mvc/test/Mvc.IntegrationTests/Microsoft.AspNetCore.Mvc.IntegrationTests.csproj b/src/Mvc/test/Mvc.IntegrationTests/Microsoft.AspNetCore.Mvc.IntegrationTests.csproj index 3cd3ac71e5..0e33905acc 100644 --- a/src/Mvc/test/Mvc.IntegrationTests/Microsoft.AspNetCore.Mvc.IntegrationTests.csproj +++ b/src/Mvc/test/Mvc.IntegrationTests/Microsoft.AspNetCore.Mvc.IntegrationTests.csproj @@ -2,6 +2,7 @@ <PropertyGroup> <TargetFramework>netcoreapp3.0</TargetFramework> + <UseSharedCompilation>false</UseSharedCompilation> </PropertyGroup> <ItemGroup> diff --git a/src/Mvc/test/Mvc.IntegrationTests/NullableReferenceTypeIntegrationTest.cs b/src/Mvc/test/Mvc.IntegrationTests/NullableReferenceTypeIntegrationTest.cs new file mode 100644 index 0000000000..5fd7a89e89 --- /dev/null +++ b/src/Mvc/test/Mvc.IntegrationTests/NullableReferenceTypeIntegrationTest.cs @@ -0,0 +1,212 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Collections.Generic; +using System.ComponentModel.DataAnnotations; +using System.IO; +using System.Linq; +using System.Reflection; +using System.Text; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Internal; +using Microsoft.AspNetCore.Mvc.Abstractions; +using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.Extensions.Primitives; +using Xunit; + +namespace Microsoft.AspNetCore.Mvc.IntegrationTests +{ + public class NullableReferenceTypeIntegrationTest + { +#nullable enable + private class Person1 + { + public string FirstName { get; set; } = default!; + } +#nullable restore + + [Fact] + public async Task BindProperty_WithNonNullableReferenceType_NoData_ValidationError() + { + // Arrange + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(); + var parameter = new ParameterDescriptor() + { + Name = "Parameter1", + BindingInfo = new BindingInfo(), + ParameterType = typeof(Person1) + }; + + var testContext = ModelBindingTestHelper.GetTestContext(); + var modelState = testContext.ModelState; + + // Act + var modelBindingResult = await parameterBinder.BindModelAsync(parameter, testContext); + + // Assert + + // ModelBindingResult + Assert.True(modelBindingResult.IsModelSet); + + // Model + var boundPerson = Assert.IsType<Person1>(modelBindingResult.Model); + Assert.Null(boundPerson.FirstName); + + // ModelState + Assert.False(modelState.IsValid); + Assert.Collection( + modelState.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("FirstName", kvp.Key); + Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState); + + // Not validating framework error message. + Assert.Single(kvp.Value.Errors); + }); + } + +#nullable enable + private class Person2 + { + public string? FirstName { get; set; } + } +#nullable restore + + [Fact] + public async Task BindProperty_WithNullableReferenceType_NoData_NoValidationError() + { + // Arrange + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(); + var parameter = new ParameterDescriptor() + { + Name = "Parameter1", + BindingInfo = new BindingInfo(), + ParameterType = typeof(Person2) + }; + + var testContext = ModelBindingTestHelper.GetTestContext(); + var modelState = testContext.ModelState; + + // Act + var modelBindingResult = await parameterBinder.BindModelAsync(parameter, testContext); + + // Assert + + // ModelBindingResult + Assert.True(modelBindingResult.IsModelSet); + + // Model + var boundPerson = Assert.IsType<Person2>(modelBindingResult.Model); + Assert.Null(boundPerson.FirstName); + + // ModelState + Assert.True(modelState.IsValid); + } + +#nullable enable + private class Person3 + { + [Required(ErrorMessage = "Test")] + public string FirstName { get; set; } = default!; + } +#nullable restore + + [Fact] + public async Task BindProperty_WithNonNullableReferenceType_NoData_ValidationError_CustomMessage() + { + // Arrange + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(); + var parameter = new ParameterDescriptor() + { + Name = "Parameter1", + BindingInfo = new BindingInfo(), + ParameterType = typeof(Person3) + }; + + var testContext = ModelBindingTestHelper.GetTestContext(); + var modelState = testContext.ModelState; + + // Act + var modelBindingResult = await parameterBinder.BindModelAsync(parameter, testContext); + + // Assert + + // ModelBindingResult + Assert.True(modelBindingResult.IsModelSet); + + // Model + var boundPerson = Assert.IsType<Person3>(modelBindingResult.Model); + Assert.Null(boundPerson.FirstName); + + // ModelState + Assert.False(modelState.IsValid); + Assert.Collection( + modelState.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("FirstName", kvp.Key); + Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState); + + var error = Assert.Single(kvp.Value.Errors); + Assert.Equal("Test", error.ErrorMessage); + }); + } + +#nullable enable + private void NonNullableParameter(string param1) + { + } +#nullable restore + + [Fact] + public async Task BindParameter_WithNonNullableReferenceType_NoData_ValidationError() + { + // Arrange + var parameterBinder = ModelBindingTestHelper.GetParameterBinder(); + var parameter = new ParameterDescriptor() + { + Name = "param1", + BindingInfo = new BindingInfo(), + ParameterType = typeof(string) + }; + + var method = GetType().GetMethod(nameof(NonNullableParameter), BindingFlags.NonPublic | BindingFlags.Instance); + var parameterInfo = method.GetParameters().Single(); + var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); + var modelMetadata = modelMetadataProvider.GetMetadataForParameter(parameterInfo); + + var testContext = ModelBindingTestHelper.GetTestContext(); + var modelState = testContext.ModelState; + + // Act + var modelBindingResult = await parameterBinder.BindModelAsync( + parameter, + testContext, + modelMetadataProvider, + modelMetadata); + + // Assert + + // ModelBindingResult + Assert.False(modelBindingResult.IsModelSet); + + // Model + Assert.Null(modelBindingResult.Model); + + // ModelState + Assert.False(modelState.IsValid); + Assert.Collection( + modelState.OrderBy(kvp => kvp.Key), + kvp => + { + Assert.Equal("param1", kvp.Key); + Assert.Equal(ModelValidationState.Invalid, kvp.Value.ValidationState); + + // Not validating framework error message. + Assert.Single(kvp.Value.Errors); + }); + } + } +}
\ No newline at end of file diff --git a/src/Mvc/test/WebSites/BasicWebSite/Controllers/NonNullableController.cs b/src/Mvc/test/WebSites/BasicWebSite/Controllers/NonNullableController.cs new file mode 100644 index 0000000000..b5f3d6c3ac --- /dev/null +++ b/src/Mvc/test/WebSites/BasicWebSite/Controllers/NonNullableController.cs @@ -0,0 +1,33 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +#nullable enable +using BasicWebSite.Models; +using Microsoft.AspNetCore.Mvc; + +namespace BasicWebSite.Controllers +{ + public class NonNullableController : Controller + { + public ActionResult Index() + { + return View(); + } + + [HttpPost] + public ActionResult Index(NonNullablePerson person, string description) + { + if (ModelState.IsValid) + { + return RedirectToAction(); + } + + return View(person); + } + + public class NonNullablePerson + { + public string Name { get; set; } = default!; + } + } +} diff --git a/src/Mvc/test/WebSites/BasicWebSite/Views/NonNullable/Index.cshtml b/src/Mvc/test/WebSites/BasicWebSite/Views/NonNullable/Index.cshtml new file mode 100644 index 0000000000..518b1504b2 --- /dev/null +++ b/src/Mvc/test/WebSites/BasicWebSite/Views/NonNullable/Index.cshtml @@ -0,0 +1,10 @@ +@model BasicWebSite.Controllers.NonNullableController.NonNullablePerson +@addTagHelper "*, Microsoft.AspNetCore.Mvc.TagHelpers" + +<div asp-validation-summary="All" id="errors"></div> + +<form asp-action="Index"> + <p>Name:</p> + <input asp-for="Name" /> + <button type="submit">Submit</button> +</form>
\ No newline at end of file |