From fc3a815e57376aefa91adf219d64fbc6538821b3 Mon Sep 17 00:00:00 2001 From: Doug Bunting Date: Tue, 20 Mar 2018 15:37:12 -0700 Subject: [PATCH 1/2] Restore `ModelMetadata.PropertyName != null` behaviour - #7413 part 2 of 2 - add `ModelMetadata.Name` and `ParameterName` - use `Name` instead of `PropertyName` in most cases - update `ModelMetadata.ContainerType` and other property use - choose using `MetadataKind` almost everywhere; support all possibilties - usually parameter metadata was possible but not handled - worst case was one or two potential NREs, especially `ContainerType.*` dereferences - improve `MvcCoreLoggerExtensions` metadata handling - add three new debug messages, one for type metadata and two for parameter metadata - update `ModelMetadata.ContainerMetadata`, `ContainerType` and `PropertyName` doc comments - no changes needed in Microsoft.AspNetCore.Mvc.ViewFeatures because parameters aren't viewed nits: - add missing `TestModelMetadataProvider.ForParameter(...)` method - remove unused `EmptyModelMetadataProvider` instances in `ModelMetadataTest` - refactor `ModelValidationResultComparer` out of DataAnnotationsModelValidatorTest` - take VS suggestions, mostly related to variable inlining and object initializers --- .../ModelBinding/ModelMetadata.cs | 39 ++- .../ModelBinding/ModelStateDictionary.cs | 2 +- .../DefaultApiDescriptionProvider.cs | 2 +- .../Internal/MvcCoreLoggerExtensions.cs | 309 ++++++++++++++---- .../Binders/ComplexTypeModelBinder.cs | 29 +- .../ModelBinding/ModelBinderFactory.cs | 28 +- .../Validation/ValidationVisitor.cs | 15 +- .../Properties/Resources.Designer.cs | 22 +- .../Resources.resx | 5 +- .../Internal/DataAnnotationsModelValidator.cs | 2 +- .../Internal/NumericClientModelValidator.cs | 2 +- .../Internal/ValidatableObjectAdapter.cs | 7 +- .../ModelBinding/ModelMetadataTest.cs | 144 ++++++-- .../ModelBinding/ModelStateDictionaryTest.cs | 65 +++- .../Binders/ComplexTypeModelBinderTest.cs | 30 +- .../Binders/SimpleTypeModelBinderTest.cs | 51 ++- .../ModelBinding/ParameterBinderTest.cs | 118 ++++++- .../DataAnnotationsModelValidatorTest.cs | 61 ++-- .../Internal/ModelValidationResultComparer.cs | 44 +++ .../NumericClientModelValidatorTest.cs | 39 ++- .../Internal/ValidatableObjectAdapterTest.cs | 221 +++++++++++++ .../TestModelMetadataProvider.cs | 11 +- 22 files changed, 1034 insertions(+), 212 deletions(-) create mode 100644 test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/ModelValidationResultComparer.cs create mode 100644 test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/ValidatableObjectAdapterTest.cs diff --git a/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelMetadata.cs b/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelMetadata.cs index 3d68fd45bb..f78815a7d1 100644 --- a/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelMetadata.cs +++ b/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelMetadata.cs @@ -38,12 +38,13 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding } /// - /// Gets the container type of this metadata if it represents a property, otherwise null. + /// Gets the type containing the property if this metadata is for a property; otherwise. /// public Type ContainerType => Identity.ContainerType; /// - /// Gets the metadata of the container type that the current instance is part of. + /// Gets the metadata for if this metadata is for a property; + /// otherwise. /// public virtual ModelMetadata ContainerMetadata { @@ -64,9 +65,20 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding public Type ModelType => Identity.ModelType; /// - /// Gets the property name represented by the current instance. + /// Gets the name of the parameter or property if this metadata is for a parameter or property; + /// otherwise i.e. if this is the metadata for a type. /// - public string PropertyName => Identity.Name; + public string Name => Identity.Name; + + /// + /// Gets the name of the parameter if this metadata is for a parameter; otherwise. + /// + public string ParameterName => MetadataKind == ModelMetadataKind.Parameter ? Identity.Name : null; + + /// + /// Gets the name of the property if this metadata is for a property; otherwise. + /// + public string PropertyName => MetadataKind == ModelMetadataKind.Property ? Identity.Name : null; /// /// Gets the key for the current instance. @@ -384,12 +396,12 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding /// /// /// will return the first of the following expressions which has a - /// non-null value: DisplayName, PropertyName, ModelType.Name. + /// non- value: , , or ModelType.Name. /// /// The display name. public string GetDisplayName() { - return DisplayName ?? PropertyName ?? ModelType.Name; + return DisplayName ?? Name ?? ModelType.Name; } /// @@ -470,13 +482,16 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding private string DebuggerToString() { - if (Identity.MetadataKind == ModelMetadataKind.Type) + switch (MetadataKind) { - return $"ModelMetadata (Type: '{ModelType.Name}')"; - } - else - { - return $"ModelMetadata (Property: '{ContainerType.Name}.{PropertyName}' Type: '{ModelType.Name}')"; + case ModelMetadataKind.Parameter: + return $"ModelMetadata (Parameter: '{ParameterName}' Type: '{ModelType.Name}')"; + case ModelMetadataKind.Property: + return $"ModelMetadata (Property: '{ContainerType.Name}.{PropertyName}' Type: '{ModelType.Name}')"; + case ModelMetadataKind.Type: + return $"ModelMetadata (Type: '{ModelType.Name}')"; + default: + return $"Unsupported MetadataKind '{MetadataKind}'."; } } diff --git a/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelStateDictionary.cs b/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelStateDictionary.cs index 647a76da5c..970e5b6975 100644 --- a/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelStateDictionary.cs +++ b/src/Microsoft.AspNetCore.Mvc.Abstractions/ModelBinding/ModelStateDictionary.cs @@ -180,7 +180,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding /// /// /// This method allows adding the to the current - /// when is not available or the exact + /// when is not available or the exact /// must be maintained for later use (even if it is for example a ). /// Where is available, use instead. /// diff --git a/src/Microsoft.AspNetCore.Mvc.ApiExplorer/DefaultApiDescriptionProvider.cs b/src/Microsoft.AspNetCore.Mvc.ApiExplorer/DefaultApiDescriptionProvider.cs index 4dda71ab01..50734ccded 100644 --- a/src/Microsoft.AspNetCore.Mvc.ApiExplorer/DefaultApiDescriptionProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.ApiExplorer/DefaultApiDescriptionProvider.cs @@ -620,7 +620,7 @@ namespace Microsoft.AspNetCore.Mvc.ApiExplorer ModelMetadata = metadata, BinderModelName = bindingInfo?.BinderModelName ?? metadata.BinderModelName, BindingSource = bindingInfo?.BindingSource ?? metadata.BindingSource, - PropertyName = propertyName ?? metadata.PropertyName + PropertyName = propertyName ?? metadata.Name, }; } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcCoreLoggerExtensions.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcCoreLoggerExtensions.cs index 8fe2fe37b5..1dd72c189f 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcCoreLoggerExtensions.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcCoreLoggerExtensions.cs @@ -17,6 +17,7 @@ using Microsoft.AspNetCore.Mvc.Filters; using Microsoft.AspNetCore.Mvc.Formatters; using Microsoft.AspNetCore.Mvc.Formatters.Internal; using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata; using Microsoft.Extensions.Internal; using Microsoft.Extensions.Logging; using Microsoft.Net.Http.Headers; @@ -104,7 +105,8 @@ namespace Microsoft.AspNetCore.Mvc.Internal private static readonly Action _unableToInferParameterSources; private static readonly Action _registeredModelBinderProviders; private static readonly Action _foundNoValueForPropertyInRequest; - private static readonly Action _foundNoValueInRequest; + private static readonly Action _foundNoValueForParameterInRequest; + private static readonly Action _foundNoValueInRequest; private static readonly Action _noPublicSettableProperties; private static readonly Action _cannotBindToComplexType; private static readonly Action _cannotBindToFilesCollectionDueToUnsupportedContentType; @@ -115,6 +117,8 @@ namespace Microsoft.AspNetCore.Mvc.Internal private static readonly Action _attemptingToBindCollectionUsingIndices; private static readonly Action _attemptingToBindCollectionOfKeyValuePair; private static readonly Action _noKeyValueFormatForDictionaryModelBinder; + private static readonly Action _attemptingToBindParameterModel; + private static readonly Action _doneAttemptingToBindParameterModel; private static readonly Action _attemptingToBindPropertyModel; private static readonly Action _doneAttemptingToBindPropertyModel; private static readonly Action _attemptingToBindModel; @@ -475,7 +479,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal 15, "Could not find a value in the request with name '{ModelName}' for binding property '{PropertyContainerType}.{ModelFieldName}' of type '{ModelType}'."); - _foundNoValueInRequest = LoggerMessage.Define( + _foundNoValueForParameterInRequest = LoggerMessage.Define( LogLevel.Debug, 16, "Could not find a value in the request with name '{ModelName}' for binding parameter '{ModelFieldName}' of type '{ModelType}'."); @@ -610,6 +614,21 @@ namespace Microsoft.AspNetCore.Mvc.Internal LogLevel.Debug, 43, "Could not create a binder for type '{ModelType}' as this binder only supports 'System.String' type or a collection of 'System.String'."); + + _attemptingToBindParameterModel = LoggerMessage.Define( + LogLevel.Debug, + 44, + "Attempting to bind parameter '{ParameterName}' of type '{ModelType}' using the name '{ModelName}' in request data ..."); + + _doneAttemptingToBindParameterModel = LoggerMessage.Define( + LogLevel.Debug, + 45, + "Done attempting to bind parameter '{ParameterName}' of type '{ModelType}'."); + + _foundNoValueInRequest = LoggerMessage.Define( + LogLevel.Debug, + 46, + "Could not find a value in the request with name '{ModelName}' of type '{ModelType}'."); } public static void RegisteredOutputFormatters(this ILogger logger, IEnumerable outputFormatters) @@ -1157,26 +1176,32 @@ namespace Microsoft.AspNetCore.Mvc.Internal } var modelMetadata = bindingContext.ModelMetadata; - var isProperty = modelMetadata.ContainerType != null; - - if (isProperty) + switch (modelMetadata.MetadataKind) { - _foundNoValueForPropertyInRequest( - logger, - bindingContext.ModelName, - modelMetadata.ContainerType, - modelMetadata.PropertyName, - bindingContext.ModelType, - null); - } - else - { - _foundNoValueInRequest( - logger, - bindingContext.ModelName, - modelMetadata.PropertyName, - bindingContext.ModelType, - null); + case ModelMetadataKind.Parameter: + _foundNoValueForParameterInRequest( + logger, + bindingContext.ModelName, + modelMetadata.ParameterName, + bindingContext.ModelType, + null); + break; + case ModelMetadataKind.Property: + _foundNoValueForPropertyInRequest( + logger, + bindingContext.ModelName, + modelMetadata.ContainerType, + modelMetadata.PropertyName, + bindingContext.ModelType, + null); + break; + case ModelMetadataKind.Type: + _foundNoValueInRequest( + logger, + bindingContext.ModelName, + bindingContext.ModelType, + null); + break; } } @@ -1218,89 +1243,237 @@ namespace Microsoft.AspNetCore.Mvc.Internal } var modelMetadata = bindingContext.ModelMetadata; - var isProperty = modelMetadata.ContainerType != null; - - if (isProperty) + switch (modelMetadata.MetadataKind) { - _attemptingToBindPropertyModel( - logger, - modelMetadata.ContainerType, - modelMetadata.PropertyName, - modelMetadata.ModelType, - bindingContext.ModelName, - null); - } - else - { - _attemptingToBindModel(logger, bindingContext.ModelType, bindingContext.ModelName, null); + case ModelMetadataKind.Parameter: + _attemptingToBindParameterModel( + logger, + modelMetadata.ParameterName, + modelMetadata.ModelType, + bindingContext.ModelName, + null); + break; + case ModelMetadataKind.Property: + _attemptingToBindPropertyModel( + logger, + modelMetadata.ContainerType, + modelMetadata.PropertyName, + modelMetadata.ModelType, + bindingContext.ModelName, + null); + break; + case ModelMetadataKind.Type: + _attemptingToBindModel(logger, bindingContext.ModelType, bindingContext.ModelName, null); + break; } } public static void DoneAttemptingToBindModel(this ILogger logger, ModelBindingContext bindingContext) { + if (!logger.IsEnabled(LogLevel.Debug)) + { + return; + } + var modelMetadata = bindingContext.ModelMetadata; - var isProperty = modelMetadata.ContainerType != null; - - if (isProperty) + switch (modelMetadata.MetadataKind) { - _doneAttemptingToBindPropertyModel( - logger, - modelMetadata.ContainerType, - modelMetadata.PropertyName, - modelMetadata.ModelType, - null); - } - else - { - _doneAttemptingToBindModel(logger, bindingContext.ModelType, bindingContext.ModelName, null); + case ModelMetadataKind.Parameter: + _doneAttemptingToBindParameterModel( + logger, + modelMetadata.ParameterName, + modelMetadata.ModelType, + null); + break; + case ModelMetadataKind.Property: + _doneAttemptingToBindPropertyModel( + logger, + modelMetadata.ContainerType, + modelMetadata.PropertyName, + modelMetadata.ModelType, + null); + break; + case ModelMetadataKind.Type: + _doneAttemptingToBindModel(logger, bindingContext.ModelType, bindingContext.ModelName, null); + break; } } - public static void AttemptingToBindParameterOrProperty(this ILogger logger, ParameterDescriptor parameter, ModelBindingContext bindingContext) + public static void AttemptingToBindParameterOrProperty( + this ILogger logger, + ParameterDescriptor parameter, + ModelBindingContext bindingContext) { - if (parameter is ControllerBoundPropertyDescriptor propertyDescriptor) + if (!logger.IsEnabled(LogLevel.Debug)) { - _attemptingToBindProperty(logger, propertyDescriptor.PropertyInfo.DeclaringType, parameter.Name, bindingContext.ModelType, null); + return; } - else + + var modelMetadata = bindingContext.ModelMetadata; + switch (modelMetadata.MetadataKind) { - _attemptingToBindParameter(logger, parameter.Name, bindingContext.ModelType, null); + case ModelMetadataKind.Parameter: + _attemptingToBindParameter(logger, modelMetadata.ParameterName, modelMetadata.ModelType, null); + break; + case ModelMetadataKind.Property: + _attemptingToBindProperty( + logger, + modelMetadata.ContainerType, + modelMetadata.PropertyName, + modelMetadata.ModelType, + null); + break; + case ModelMetadataKind.Type: + if (parameter is ControllerParameterDescriptor parameterDescriptor) + { + _attemptingToBindParameter( + logger, + parameterDescriptor.ParameterInfo.Name, + modelMetadata.ModelType, + null); + } + else + { + // Likely binding a page handler parameter. Due to various special cases, parameter.Name may + // be empty. No way to determine actual name. + _attemptingToBindParameter(logger, parameter.Name, modelMetadata.ModelType, null); + } + break; } } - public static void DoneAttemptingToBindParameterOrProperty(this ILogger logger, ParameterDescriptor parameter, ModelBindingContext bindingContext) + public static void DoneAttemptingToBindParameterOrProperty( + this ILogger logger, + ParameterDescriptor parameter, + ModelBindingContext bindingContext) { - if (parameter is ControllerBoundPropertyDescriptor propertyDescriptor) + if (!logger.IsEnabled(LogLevel.Debug)) { - _doneAttemptingToBindProperty(logger, propertyDescriptor.PropertyInfo.DeclaringType, parameter.Name, bindingContext.ModelType, null); + return; } - else + + var modelMetadata = bindingContext.ModelMetadata; + switch (modelMetadata.MetadataKind) { - _doneAttemptingToBindParameter(logger, parameter.Name, bindingContext.ModelType, null); + case ModelMetadataKind.Parameter: + _doneAttemptingToBindParameter(logger, modelMetadata.ParameterName, modelMetadata.ModelType, null); + break; + case ModelMetadataKind.Property: + _doneAttemptingToBindProperty( + logger, + modelMetadata.ContainerType, + modelMetadata.PropertyName, + modelMetadata.ModelType, + null); + break; + case ModelMetadataKind.Type: + if (parameter is ControllerParameterDescriptor parameterDescriptor) + { + _doneAttemptingToBindParameter( + logger, + parameterDescriptor.ParameterInfo.Name, + modelMetadata.ModelType, + null); + } + else + { + // Likely binding a page handler parameter. Due to various special cases, parameter.Name may + // be empty. No way to determine actual name. + _doneAttemptingToBindParameter(logger, parameter.Name, modelMetadata.ModelType, null); + } + break; } } - public static void AttemptingToValidateParameterOrProperty(this ILogger logger, ParameterDescriptor parameter, ModelBindingContext bindingContext) + public static void AttemptingToValidateParameterOrProperty( + this ILogger logger, + ParameterDescriptor parameter, + ModelBindingContext bindingContext) { - if (parameter is ControllerBoundPropertyDescriptor propertyDescriptor) + if (!logger.IsEnabled(LogLevel.Debug)) { - _attemptingToValidateProperty(logger, propertyDescriptor.PropertyInfo.DeclaringType, parameter.Name, bindingContext.ModelType, null); + return; } - else + + var modelMetadata = bindingContext.ModelMetadata; + switch (modelMetadata.MetadataKind) { - _attemptingToValidateParameter(logger, parameter.Name, bindingContext.ModelType, null); + case ModelMetadataKind.Parameter: + _attemptingToValidateParameter(logger, modelMetadata.ParameterName, modelMetadata.ModelType, null); + break; + case ModelMetadataKind.Property: + _attemptingToValidateProperty( + logger, + modelMetadata.ContainerType, + modelMetadata.PropertyName, + modelMetadata.ModelType, + null); + break; + case ModelMetadataKind.Type: + if (parameter is ControllerParameterDescriptor parameterDescriptor) + { + _attemptingToValidateParameter( + logger, + parameterDescriptor.ParameterInfo.Name, + modelMetadata.ModelType, + null); + } + else + { + // Likely binding a page handler parameter. Due to various special cases, parameter.Name may + // be empty. No way to determine actual name. This case is less likely than for binding logging + // (above). Should occur only with a legacy IModelMetadataProvider implementation. + _attemptingToValidateParameter(logger, parameter.Name, modelMetadata.ModelType, null); + } + break; } } - public static void DoneAttemptingToValidateParameterOrProperty(this ILogger logger, ParameterDescriptor parameter, ModelBindingContext bindingContext) + public static void DoneAttemptingToValidateParameterOrProperty( + this ILogger logger, + ParameterDescriptor parameter, + ModelBindingContext bindingContext) { - if (parameter is ControllerBoundPropertyDescriptor propertyDescriptor) + if (!logger.IsEnabled(LogLevel.Debug)) { - _doneAttemptingToValidateProperty(logger, propertyDescriptor.PropertyInfo.DeclaringType, parameter.Name, bindingContext.ModelType, null); + return; } - else + + var modelMetadata = bindingContext.ModelMetadata; + switch (modelMetadata.MetadataKind) { - _doneAttemptingToValidateParameter(logger, parameter.Name, bindingContext.ModelType, null); + case ModelMetadataKind.Parameter: + _doneAttemptingToValidateParameter( + logger, + modelMetadata.ParameterName, + modelMetadata.ModelType, + null); + break; + case ModelMetadataKind.Property: + _doneAttemptingToValidateProperty( + logger, + modelMetadata.ContainerType, + modelMetadata.PropertyName, + modelMetadata.ModelType, + null); + break; + case ModelMetadataKind.Type: + if (parameter is ControllerParameterDescriptor parameterDescriptor) + { + _doneAttemptingToValidateParameter( + logger, + parameterDescriptor.ParameterInfo.Name, + modelMetadata.ModelType, + null); + } + else + { + // Likely binding a page handler parameter. Due to various special cases, parameter.Name may + // be empty. No way to determine actual name. This case is less likely than for binding logging + // (above). Should occur only with a legacy IModelMetadataProvider implementation. + _doneAttemptingToValidateParameter(logger, parameter.Name, modelMetadata.ModelType, null); + } + break; } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs index 55a5e41055..dd02365c96 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs @@ -8,6 +8,7 @@ using System.Reflection; using System.Threading.Tasks; using Microsoft.AspNetCore.Mvc.Core; using Microsoft.AspNetCore.Mvc.Internal; +using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; @@ -357,24 +358,32 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders // application developer should know that this was an invalid type to try to bind to. if (_modelCreator == null) { - // The following check causes the ComplexTypeModelBinder to NOT participate in binding structs as + // The following check causes the ComplexTypeModelBinder to NOT participate in binding structs as // reflection does not provide information about the implicit parameterless constructor for a struct. // This binder would eventually fail to construct an instance of the struct as the Linq's NewExpression // compile fails to construct it. var modelTypeInfo = bindingContext.ModelType.GetTypeInfo(); if (modelTypeInfo.IsAbstract || modelTypeInfo.GetConstructor(Type.EmptyTypes) == null) { - if (bindingContext.IsTopLevelObject) + var metadata = bindingContext.ModelMetadata; + switch (metadata.MetadataKind) { - throw new InvalidOperationException( - Resources.FormatComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject(modelTypeInfo.FullName)); + case ModelMetadataKind.Parameter: + throw new InvalidOperationException( + Resources.FormatComplexTypeModelBinder_NoParameterlessConstructor_ForParameter( + modelTypeInfo.FullName, + metadata.ParameterName)); + case ModelMetadataKind.Property: + throw new InvalidOperationException( + Resources.FormatComplexTypeModelBinder_NoParameterlessConstructor_ForProperty( + modelTypeInfo.FullName, + metadata.PropertyName, + bindingContext.ModelMetadata.ContainerType.FullName)); + case ModelMetadataKind.Type: + throw new InvalidOperationException( + Resources.FormatComplexTypeModelBinder_NoParameterlessConstructor_ForType( + modelTypeInfo.FullName)); } - - throw new InvalidOperationException( - Resources.FormatComplexTypeModelBinder_NoParameterlessConstructor_ForProperty( - modelTypeInfo.FullName, - bindingContext.ModelName, - bindingContext.ModelMetadata.ContainerType.FullName)); } _modelCreator = Expression diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ModelBinderFactory.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ModelBinderFactory.cs index a6735bdaa4..82f74e6e58 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ModelBinderFactory.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ModelBinderFactory.cs @@ -79,8 +79,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding typeof(IModelBinderProvider).FullName)); } - IModelBinder binder; - if (TryGetCachedBinder(context.Metadata, context.CacheToken, out binder)) + if (TryGetCachedBinder(context.Metadata, context.CacheToken, out var binder)) { return binder; } @@ -106,8 +105,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding // so that all intermediate results can be cached. private IModelBinder CreateBinderCoreCached(DefaultModelBinderProviderContext providerContext, object token) { - IModelBinder binder; - if (TryGetCachedBinder(providerContext.Metadata, token, out binder)) + if (TryGetCachedBinder(providerContext.Metadata, token, out var binder)) { return binder; } @@ -145,8 +143,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding // PlaceholderBinder because that would result in lots of unnecessary indirection and allocations. var visited = providerContext.Visited; - IModelBinder binder; - if (visited.TryGetValue(key, out binder)) + if (visited.TryGetValue(key, out var binder)) { if (binder != null) { @@ -178,8 +175,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding } // If the PlaceholderBinder was created, then it means we recursed. Hook it up to the 'real' binder. - var placeholderBinder = visited[key] as PlaceholderBinder; - if (placeholderBinder != null) + if (visited[key] is PlaceholderBinder placeholderBinder) { // It's also possible that user code called into `CreateBinder` but then returned null, we don't // want to create something that will null-ref later so just hook this up to the no-op binder. @@ -347,13 +343,17 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding public override string ToString() { - if (_metadata.MetadataKind == ModelMetadataKind.Type) + switch (_metadata.MetadataKind) { - return $"{_token} (Type: '{_metadata.ModelType.Name}')"; - } - else - { - return $"{_token} (Property: '{_metadata.ContainerType.Name}.{_metadata.PropertyName}' Type: '{_metadata.ModelType.Name}')"; + case ModelMetadataKind.Parameter: + return $"{_token} (Parameter: '{_metadata.ParameterName}' Type: '{_metadata.ModelType.Name}')"; + case ModelMetadataKind.Property: + return $"{_token} (Property: '{_metadata.ContainerType.Name}.{_metadata.PropertyName}' " + + $"Type: '{_metadata.ModelType.Name}')"; + case ModelMetadataKind.Type: + return $"{_token} (Type: '{_metadata.ModelType.Name}')"; + default: + return $"Unsupported MetadataKind '{_metadata.MetadataKind}'."; } } } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs index d128b04dd6..9f621f3b9e 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/ValidationVisitor.cs @@ -55,7 +55,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation ModelState = actionContext.ModelState; CurrentPath = new ValidationStack(); } - + protected IModelValidatorProvider ValidatorProvider { get; } protected IModelMetadataProvider MetadataProvider { get; } protected ValidatorCache Cache { get; } @@ -71,7 +71,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation protected IValidationStrategy Strategy { get; set; } /// - /// Indicates whether validation of a complex type should be performed if validation fails for any of its children. The default behavior is false. + /// Indicates whether validation of a complex type should be performed if validation fails for any of its children. The default behavior is false. /// public bool ValidateComplexTypesIfChildValidationFails { get; set; } /// @@ -146,11 +146,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation var result = results[i]; var key = ModelNames.CreatePropertyModelName(Key, result.MemberName); - // If this is a top-level parameter/property, the key would be empty, - // so use the name of the top-level property - if (string.IsNullOrEmpty(key) && Metadata.PropertyName != null) + // If this is a top-level parameter/property, the key would be empty. + // So, use the name of the top-level property/property. + if (string.IsNullOrEmpty(key) && Metadata.Name != null) { - key = Metadata.PropertyName; + key = Metadata.Name; } ModelState.TryAddModelError(key, result.Message); @@ -306,8 +306,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation return null; } - ValidationStateEntry entry; - ValidationState.TryGetValue(model, out entry); + ValidationState.TryGetValue(model, out var entry); return entry; } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs b/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs index 8e42894851..ec92b11640 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs @@ -1273,16 +1273,16 @@ namespace Microsoft.AspNetCore.Mvc.Core /// /// Could not create an instance of type '{0}'. Model bound complex types must not be abstract or value types and must have a parameterless constructor. /// - internal static string ComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject + internal static string ComplexTypeModelBinder_NoParameterlessConstructor_ForType { - get => GetString("ComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject"); + get => GetString("ComplexTypeModelBinder_NoParameterlessConstructor_ForType"); } /// /// Could not create an instance of type '{0}'. Model bound complex types must not be abstract or value types and must have a parameterless constructor. /// - internal static string FormatComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject(object p0) - => string.Format(CultureInfo.CurrentCulture, GetString("ComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject"), p0); + internal static string FormatComplexTypeModelBinder_NoParameterlessConstructor_ForType(object p0) + => string.Format(CultureInfo.CurrentCulture, GetString("ComplexTypeModelBinder_NoParameterlessConstructor_ForType"), p0); /// /// Could not create an instance of type '{0}'. Model bound complex types must not be abstract or value types and must have a parameterless constructor. Alternatively, set the '{1}' property to a non-null value in the '{2}' constructor. @@ -1438,6 +1438,20 @@ namespace Microsoft.AspNetCore.Mvc.Core internal static string FormatApplicationAssembliesProvider_RelatedAssemblyCannotDefineAdditional(object p0, object p1) => string.Format(CultureInfo.CurrentCulture, GetString("ApplicationAssembliesProvider_RelatedAssemblyCannotDefineAdditional"), p0, p1); + /// + /// Could not create an instance of type '{0}'. Model bound complex types must not be abstract or value types and must have a parameterless constructor. Alternatively, give the '{1}' parameter a non-null default value. + /// + internal static string ComplexTypeModelBinder_NoParameterlessConstructor_ForParameter + { + get => GetString("ComplexTypeModelBinder_NoParameterlessConstructor_ForParameter"); + } + + /// + /// Could not create an instance of type '{0}'. Model bound complex types must not be abstract or value types and must have a parameterless constructor. Alternatively, give the '{1}' parameter a non-null default value. + /// + internal static string FormatComplexTypeModelBinder_NoParameterlessConstructor_ForParameter(object p0, object p1) + => string.Format(CultureInfo.CurrentCulture, GetString("ComplexTypeModelBinder_NoParameterlessConstructor_ForParameter"), p0, p1); + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx b/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx index e4326aba1d..671190495e 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx +++ b/src/Microsoft.AspNetCore.Mvc.Core/Resources.resx @@ -400,7 +400,7 @@ '{0}' and '{1}' are out of bounds for the string. '{0}' and '{1}' are the parameters which combine to be out of bounds. - + Could not create an instance of type '{0}'. Model bound complex types must not be abstract or value types and must have a parameterless constructor. @@ -436,4 +436,7 @@ Assembly '{0}' declared as a related assembly by assembly '{1}' cannot define additional related assemblies. + + Could not create an instance of type '{0}'. Model bound complex types must not be abstract or value types and must have a parameterless constructor. Alternatively, give the '{1}' parameter a non-null default value. + \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Mvc.DataAnnotations/Internal/DataAnnotationsModelValidator.cs b/src/Microsoft.AspNetCore.Mvc.DataAnnotations/Internal/DataAnnotationsModelValidator.cs index 790356eba3..563fa078a7 100644 --- a/src/Microsoft.AspNetCore.Mvc.DataAnnotations/Internal/DataAnnotationsModelValidator.cs +++ b/src/Microsoft.AspNetCore.Mvc.DataAnnotations/Internal/DataAnnotationsModelValidator.cs @@ -80,7 +80,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal } var metadata = validationContext.ModelMetadata; - var memberName = metadata.PropertyName; + var memberName = metadata.Name; var container = validationContext.Container; var context = new ValidationContext( diff --git a/src/Microsoft.AspNetCore.Mvc.DataAnnotations/Internal/NumericClientModelValidator.cs b/src/Microsoft.AspNetCore.Mvc.DataAnnotations/Internal/NumericClientModelValidator.cs index d10c310e28..edc0286909 100644 --- a/src/Microsoft.AspNetCore.Mvc.DataAnnotations/Internal/NumericClientModelValidator.cs +++ b/src/Microsoft.AspNetCore.Mvc.DataAnnotations/Internal/NumericClientModelValidator.cs @@ -42,7 +42,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal } var messageProvider = modelMetadata.ModelBindingMessageProvider; - var name = modelMetadata.DisplayName ?? modelMetadata.PropertyName; + var name = modelMetadata.DisplayName ?? modelMetadata.Name; if (name == null) { return messageProvider.NonPropertyValueMustBeANumberAccessor(); diff --git a/src/Microsoft.AspNetCore.Mvc.DataAnnotations/Internal/ValidatableObjectAdapter.cs b/src/Microsoft.AspNetCore.Mvc.DataAnnotations/Internal/ValidatableObjectAdapter.cs index dc0a594120..55a9ca23a1 100644 --- a/src/Microsoft.AspNetCore.Mvc.DataAnnotations/Internal/ValidatableObjectAdapter.cs +++ b/src/Microsoft.AspNetCore.Mvc.DataAnnotations/Internal/ValidatableObjectAdapter.cs @@ -19,8 +19,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal return Enumerable.Empty(); } - var validatable = model as IValidatableObject; - if (validatable == null) + if (!(model is IValidatableObject validatable)) { var message = Resources.FormatValidatableObjectAdapter_IncompatibleType( typeof(IValidatableObject).Name, @@ -39,7 +38,7 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal items: null) { DisplayName = context.ModelMetadata.GetDisplayName(), - MemberName = context.ModelMetadata.PropertyName, + MemberName = context.ModelMetadata.Name, }; return ConvertResults(validatable.Validate(validationContext)); @@ -66,4 +65,4 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal } } } -} \ No newline at end of file +} diff --git a/test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs b/test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs index 7be03ba94d..b637350e3f 100644 --- a/test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelMetadataTest.cs @@ -5,6 +5,7 @@ using System; using System.Collections; using System.Collections.Generic; using System.Collections.ObjectModel; +using System.Reflection; using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata; using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; using Xunit; @@ -24,10 +25,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding [InlineData(typeof(int))] public void IsComplexType_ReturnsFalseForSimpleTypes(Type type) { - // Arrange - var provider = new EmptyModelMetadataProvider(); - - // Act + // Arrange & Act var modelMetadata = new TestModelMetadata(type); // Assert @@ -41,10 +39,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding [InlineData(typeof(Nullable))] public void IsComplexType_ReturnsTrueForComplexTypes(Type type) { - // Arrange - var provider = new EmptyModelMetadataProvider(); - - // Act + // Arrange & Act var modelMetadata = new TestModelMetadata(type); // Assert @@ -106,10 +101,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding [InlineData(typeof(JustEnumerable))] public void IsCollectionType_ReturnsFalseForNonCollectionTypes(Type type) { - // Arrange - var provider = new EmptyModelMetadataProvider(); - - // Act + // Arrange & Act var modelMetadata = new TestModelMetadata(type); // Assert @@ -120,10 +112,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding [MemberData(nameof(CollectionAndEnumerableData))] public void IsCollectionType_ReturnsTrueForCollectionTypes(Type type) { - // Arrange - var provider = new EmptyModelMetadataProvider(); - - // Act + // Arrange & Act var modelMetadata = new TestModelMetadata(type); // Assert @@ -134,10 +123,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding [MemberData(nameof(NonCollectionNonEnumerableData))] public void IsEnumerableType_ReturnsFalseForNonEnumerableTypes(Type type) { - // Arrange - var provider = new EmptyModelMetadataProvider(); - - // Act + // Arrange & Act var modelMetadata = new TestModelMetadata(type); // Assert @@ -151,10 +137,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding [InlineData(typeof(JustEnumerable))] public void IsEnumerableType_ReturnsTrueForEnumerableTypes(Type type) { - // Arrange - var provider = new EmptyModelMetadataProvider(); - - // Act + // Arrange & Act var modelMetadata = new TestModelMetadata(type); // Assert @@ -173,10 +156,10 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding [InlineData(typeof(Nullable), true)] public void IsNullableValueType_ReturnsExpectedValue(Type modelType, bool expected) { - // Arrange + // Arrange & Act var modelMetadata = new TestModelMetadata(modelType); - // Act & Assert + // Assert Assert.Equal(expected, modelMetadata.IsNullableValueType); } @@ -192,10 +175,10 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding [InlineData(typeof(Nullable), true)] public void IsReferenceOrNullableType_ReturnsExpectedValue(Type modelType, bool expected) { - // Arrange + // Arrange & Act var modelMetadata = new TestModelMetadata(modelType); - // Act & Assert + // Assert Assert.Equal(expected, modelMetadata.IsReferenceOrNullableType); } @@ -211,13 +194,15 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding [InlineData(typeof(Nullable), typeof(IsComplexTypeModel))] public void UnderlyingOrModelType_ReturnsExpectedValue(Type modelType, Type expected) { - // Arrange + // Arrange & Act var modelMetadata = new TestModelMetadata(modelType); - // Act & Assert + // Assert Assert.Equal(expected, modelMetadata.UnderlyingOrModelType); } + // ElementType + [Theory] [InlineData(typeof(object))] [InlineData(typeof(int))] @@ -257,13 +242,86 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Assert.Equal(expected, elementType); } + // ContainerType + + [Fact] + public void ContainerType_IsNull_ForType() + { + // Arrange & Act + var metadata = new TestModelMetadata(typeof(int)); + + // Assert + Assert.Null(metadata.ContainerType); + } + + [Fact] + public void ContainerType_IsNull_ForParameter() + { + // Arrange & Act + var method = typeof(CollectionImplementation).GetMethod(nameof(CollectionImplementation.Add)); + var parameter = method.GetParameters()[0]; // Add(string item) + var metadata = new TestModelMetadata(parameter); + + // Assert + Assert.Null(metadata.ContainerType); + } + + [Fact] + public void ContainerType_ReturnExpectedMetadata_ForProperty() + { + // Arrange & Act + var metadata = new TestModelMetadata(typeof(int), nameof(string.Length), typeof(string)); + + // Assert + Assert.Equal(typeof(string), metadata.ContainerType); + } + + // Name / ParameterName / PropertyName + + [Fact] + public void Names_ReturnExpectedMetadata_ForType() + { + // Arrange & Act + var metadata = new TestModelMetadata(typeof(int)); + + // Assert + Assert.Null(metadata.Name); + Assert.Null(metadata.ParameterName); + Assert.Null(metadata.PropertyName); + } + + [Fact] + public void Names_ReturnExpectedMetadata_ForParameter() + { + // Arrange & Act + var method = typeof(CollectionImplementation).GetMethod(nameof(CollectionImplementation.Add)); + var parameter = method.GetParameters()[0]; // Add(string item) + var metadata = new TestModelMetadata(parameter); + + // Assert + Assert.Equal("item", metadata.Name); + Assert.Equal("item", metadata.ParameterName); + Assert.Null(metadata.PropertyName); + } + + [Fact] + public void Names_ReturnExpectedMetadata_ForProperty() + { + // Arrange & Act + var metadata = new TestModelMetadata(typeof(int), nameof(string.Length), typeof(string)); + + // Assert + Assert.Equal(nameof(string.Length), metadata.Name); + Assert.Null(metadata.ParameterName); + Assert.Equal(nameof(string.Length), metadata.PropertyName); + } + // GetDisplayName() [Fact] public void GetDisplayName_ReturnsDisplayName_IfSet() { // Arrange - var provider = new EmptyModelMetadataProvider(); var metadata = new TestModelMetadata(typeof(int), "Length", typeof(string)); metadata.SetDisplayName("displayName"); @@ -274,11 +332,25 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Assert.Equal("displayName", result); } + [Fact] + public void GetDisplayName_ReturnsParameterName_WhenSetAndDisplayNameIsNull() + { + // Arrange + var method = typeof(CollectionImplementation).GetMethod(nameof(CollectionImplementation.Add)); + var parameter = method.GetParameters()[0]; // Add(string item) + var metadata = new TestModelMetadata(parameter); + + // Act + var result = metadata.GetDisplayName(); + + // Assert + Assert.Equal("item", result); + } + [Fact] public void GetDisplayName_ReturnsPropertyName_WhenSetAndDisplayNameIsNull() { // Arrange - var provider = new EmptyModelMetadataProvider(); var metadata = new TestModelMetadata(typeof(int), "Length", typeof(string)); // Act @@ -292,7 +364,6 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding public void GetDisplayName_ReturnsTypeName_WhenPropertyNameAndDisplayNameAreNull() { // Arrange - var provider = new EmptyModelMetadataProvider(); var metadata = new TestModelMetadata(typeof(string)); // Act @@ -302,6 +373,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Assert.Equal("String", result); } + // Virtual methods and properties that throw NotImplementedException in the abstract class. + [Fact] public void GetContainerMetadata_ThrowsNotImplementedException_ByDefault() { @@ -341,6 +414,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding { } + public TestModelMetadata(ParameterInfo parameter) + : base(ModelMetadataIdentity.ForParameter(parameter)) + { + } + public TestModelMetadata(Type modelType, string propertyName, Type containerType) : base(ModelMetadataIdentity.ForProperty(modelType, propertyName, containerType)) { diff --git a/test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelStateDictionaryTest.cs b/test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelStateDictionaryTest.cs index 7fb7de7464..75a01945d2 100644 --- a/test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelStateDictionaryTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Abstractions.Test/ModelBinding/ModelStateDictionaryTest.cs @@ -971,7 +971,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding } [Fact] - public void ModelStateDictionary_AddsCustomErrorMessage_WhenModelStateNotSet_WithNonProperty() + public void ModelStateDictionary_AddsCustomErrorMessage_WhenModelStateNotSet_WithParameter() { // Arrange var expected = "Hmm, the supplied value is not valid."; @@ -981,7 +981,35 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding var compositeProvider = new DefaultCompositeMetadataDetailsProvider(new[] { bindingMetadataProvider }); var optionsAccessor = new OptionsAccessor(); optionsAccessor.Value.ModelBindingMessageProvider.SetNonPropertyUnknownValueIsInvalidAccessor( - () => $"Hmm, the supplied value is not valid."); + () => "Hmm, the supplied value is not valid."); + + var method = typeof(string).GetMethod(nameof(string.Copy)); + var parameter = method.GetParameters()[0]; // Copy(string str) + var provider = new DefaultModelMetadataProvider(compositeProvider, optionsAccessor); + var metadata = provider.GetMetadataForParameter(parameter); + + // Act + dictionary.TryAddModelError("key", new FormatException(), metadata); + + // Assert + var entry = Assert.Single(dictionary); + Assert.Equal("key", entry.Key); + var error = Assert.Single(entry.Value.Errors); + Assert.Equal(expected, error.ErrorMessage); + } + + [Fact] + public void ModelStateDictionary_AddsCustomErrorMessage_WhenModelStateNotSet_WithType() + { + // Arrange + var expected = "Hmm, the supplied value is not valid."; + var dictionary = new ModelStateDictionary(); + + var bindingMetadataProvider = new DefaultBindingMetadataProvider(); + var compositeProvider = new DefaultCompositeMetadataDetailsProvider(new[] { bindingMetadataProvider }); + var optionsAccessor = new OptionsAccessor(); + optionsAccessor.Value.ModelBindingMessageProvider.SetNonPropertyUnknownValueIsInvalidAccessor( + () => "Hmm, the supplied value is not valid."); var provider = new DefaultModelMetadataProvider(compositeProvider, optionsAccessor); var metadata = provider.GetMetadataForType(typeof(int)); @@ -1058,7 +1086,36 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding } [Fact] - public void ModelStateDictionary_AddsCustomErrorMessage_WhenModelStateSet_WithNonProperty() + public void ModelStateDictionary_AddsCustomErrorMessage_WhenModelStateSet_WithParameter() + { + // Arrange + var expected = "Hmm, the value 'some value' is not valid."; + var dictionary = new ModelStateDictionary(); + dictionary.SetModelValue("key", new string[] { "some value" }, "some value"); + + var bindingMetadataProvider = new DefaultBindingMetadataProvider(); + var compositeProvider = new DefaultCompositeMetadataDetailsProvider(new[] { bindingMetadataProvider }); + var optionsAccessor = new OptionsAccessor(); + optionsAccessor.Value.ModelBindingMessageProvider.SetNonPropertyAttemptedValueIsInvalidAccessor( + value => $"Hmm, the value '{ value }' is not valid."); + + var method = typeof(string).GetMethod(nameof(string.Copy)); + var parameter = method.GetParameters()[0]; // Copy(string str) + var provider = new DefaultModelMetadataProvider(compositeProvider, optionsAccessor); + var metadata = provider.GetMetadataForParameter(parameter); + + // Act + dictionary.TryAddModelError("key", new FormatException(), metadata); + + // Assert + var entry = Assert.Single(dictionary); + Assert.Equal("key", entry.Key); + var error = Assert.Single(entry.Value.Errors); + Assert.Equal(expected, error.ErrorMessage); + } + + [Fact] + public void ModelStateDictionary_AddsCustomErrorMessage_WhenModelStateSet_WithType() { // Arrange var expected = "Hmm, the value 'some value' is not valid."; @@ -1477,4 +1534,4 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding public override string Message { get; } } -} \ No newline at end of file +} diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderTest.cs index cc9caed07e..c76eb27c0a 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderTest.cs @@ -6,7 +6,6 @@ using System.Collections.Generic; using System.ComponentModel; using System.ComponentModel.DataAnnotations; using System.Linq; -using System.Reflection; using System.Runtime.Serialization; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; @@ -371,6 +370,25 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders exception.Message); } + [Fact] + public void CreateModel_ForClassWithNoParameterlessConstructor_AsElement_ThrowsException() + { + // Arrange + var expectedMessage = "Could not create an instance of type " + + $"'{typeof(ClassWithNoParameterlessConstructor)}'. Model bound complex types must not be abstract " + + "or value types and must have a parameterless constructor."; + var metadata = GetMetadataForType(typeof(ClassWithNoParameterlessConstructor)); + var bindingContext = new DefaultModelBindingContext + { + ModelMetadata = metadata, + }; + var binder = CreateBinder(metadata); + + // Act & Assert + var exception = Assert.Throws(() => binder.CreateModelPublic(bindingContext)); + Assert.Equal(expectedMessage, exception.Message); + } + [Fact] public void CreateModel_ForStructModelType_AsProperty_ThrowsException() { @@ -1096,6 +1114,16 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders public double Y { get; } } + private class ClassWithNoParameterlessConstructor + { + public ClassWithNoParameterlessConstructor(string name) + { + Name = name; + } + + public string Name { get; set; } + } + private class BindingOptionalProperty { [BindingBehavior(BindingBehavior.Optional)] diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/SimpleTypeModelBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/SimpleTypeModelBinderTest.cs index 4831151d13..dc4dbc9aa3 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/SimpleTypeModelBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/SimpleTypeModelBinderTest.cs @@ -6,6 +6,7 @@ using System.Globalization; using System.Threading.Tasks; using Microsoft.AspNetCore.Testing; using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Logging.Testing; using Xunit; namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders @@ -157,12 +158,34 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Assert.Equal(message, error.ErrorMessage); } - [Fact] - public async Task BindModel_EmptyValueProviderResult_ReturnsFailed() + public static TheoryData IntegerModelMetadataDataSet + { + get + { + var metadataProvider = new EmptyModelMetadataProvider(); + var method = typeof(MetadataClass).GetMethod(nameof(MetadataClass.IsLovely)); + var parameter = method.GetParameters()[0]; // IsLovely(int parameter) + + return new TheoryData + { + metadataProvider.GetMetadataForParameter(parameter), + metadataProvider.GetMetadataForProperty(typeof(MetadataClass), nameof(MetadataClass.Property)), + metadataProvider.GetMetadataForType(typeof(int)), + }; + } + } + + [Theory] + [MemberData(nameof(IntegerModelMetadataDataSet))] + public async Task BindModel_EmptyValueProviderResult_ReturnsFailedAndLogsSuccessfully(ModelMetadata metadata) { // Arrange var bindingContext = GetBindingContext(typeof(int)); - var binder = new SimpleTypeModelBinder(typeof(int), NullLoggerFactory.Instance); + bindingContext.ModelMetadata = metadata; + + var sink = new TestSink(); + var loggerFactory = new TestLoggerFactory(sink, enabled: true); + var binder = new SimpleTypeModelBinder(typeof(int), loggerFactory); // Act await binder.BindModelAsync(bindingContext); @@ -170,6 +193,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders // Assert Assert.Equal(ModelBindingResult.Failed(), bindingContext.Result); Assert.Empty(bindingContext.ModelState); + Assert.Equal(2, sink.Writes.Count); } [Theory] @@ -236,17 +260,21 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Assert.True(bindingContext.ModelState.ContainsKey("theModelName")); } - [Fact] - public async Task BindModel_ValidValueProviderResult_ReturnsModel() + [Theory] + [MemberData(nameof(IntegerModelMetadataDataSet))] + public async Task BindModel_ValidValueProviderResult_ReturnsModelAndLogsSuccessfully(ModelMetadata metadata) { // Arrange var bindingContext = GetBindingContext(typeof(int)); + bindingContext.ModelMetadata = metadata; bindingContext.ValueProvider = new SimpleValueProvider { { "theModelName", "42" } }; - var binder = new SimpleTypeModelBinder(typeof(int), NullLoggerFactory.Instance); + var sink = new TestSink(); + var loggerFactory = new TestLoggerFactory(sink, enabled: true); + var binder = new SimpleTypeModelBinder(typeof(int), loggerFactory); // Act await binder.BindModelAsync(bindingContext); @@ -255,6 +283,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Assert.True(bindingContext.Result.IsModelSet); Assert.Equal(42, bindingContext.Result.Model); Assert.True(bindingContext.ModelState.ContainsKey("theModelName")); + Assert.Equal(2, sink.Writes.Count); } public static TheoryData BiggerNumericTypes @@ -488,5 +517,15 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders Value2 = 2, MaxValue = int.MaxValue } + + private class MetadataClass + { + public int Property { get; set; } + + public bool IsLovely(int parameter) + { + return true; + } + } } } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ParameterBinderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ParameterBinderTest.cs index 4838cf3f62..461a3c8bca 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ParameterBinderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/ParameterBinderTest.cs @@ -2,11 +2,13 @@ // 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.ComponentModel.DataAnnotations; using System.Linq; using System.Threading.Tasks; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.Abstractions; +using Microsoft.AspNetCore.Mvc.Controllers; using Microsoft.AspNetCore.Mvc.DataAnnotations; using Microsoft.AspNetCore.Mvc.DataAnnotations.Internal; using Microsoft.AspNetCore.Mvc.Internal; @@ -15,6 +17,7 @@ using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Logging.Testing; using Microsoft.Extensions.Options; using Moq; using Xunit; @@ -309,6 +312,107 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding Assert.Empty(actionContext.ModelState); } + public static TheoryData EnforcesTopLevelRequiredDataSet + { + get + { + var attribute = new RequiredAttribute(); + var bindingInfo = new BindingInfo + { + BinderModelName = string.Empty, + }; + var parameterDescriptor = new ParameterDescriptor + { + Name = string.Empty, + BindingInfo = bindingInfo, + ParameterType = typeof(Person), + }; + + var method = typeof(Person).GetMethod(nameof(Person.Equals), new[] { typeof(Person) }); + var parameter = method.GetParameters()[0]; // Equals(Person other) + var controllerParameterDescriptor = new ControllerParameterDescriptor + { + Name = string.Empty, + BindingInfo = bindingInfo, + ParameterInfo = parameter, + ParameterType = typeof(Person), + }; + + var provider1 = new TestModelMetadataProvider(); + provider1 + .ForParameter(parameter) + .ValidationDetails(d => + { + d.IsRequired = true; + d.ValidatorMetadata.Add(attribute); + }); + provider1 + .ForProperty(typeof(Family), nameof(Family.Mom)) + .ValidationDetails(d => + { + d.IsRequired = true; + d.ValidatorMetadata.Add(attribute); + }); + + var provider2 = new TestModelMetadataProvider(); + provider2 + .ForType(typeof(Person)) + .ValidationDetails(d => + { + d.IsRequired = true; + d.ValidatorMetadata.Add(attribute); + }); + + return new TheoryData + { + { attribute, parameterDescriptor, provider1.GetMetadataForParameter(parameter) }, + { attribute, parameterDescriptor, provider1.GetMetadataForProperty(typeof(Family), nameof(Family.Mom)) }, + { attribute, parameterDescriptor, provider2.GetMetadataForType(typeof(Person)) }, + { attribute, controllerParameterDescriptor, provider2.GetMetadataForType(typeof(Person)) }, + }; + } + } + + [Theory] + [MemberData(nameof(EnforcesTopLevelRequiredDataSet))] + public async Task BindModelAsync_EnforcesTopLevelRequiredAndLogsSuccessfully_WithEmptyPrefix( + RequiredAttribute attribute, + ParameterDescriptor parameterDescriptor, + ModelMetadata metadata) + { + // Arrange + var expectedKey = metadata.Name ?? string.Empty; + var expectedFieldName = metadata.Name ?? nameof(Person); + + var actionContext = GetControllerContext(); + var validator = new DataAnnotationsModelValidator( + new ValidationAttributeAdapterProvider(), + attribute, + stringLocalizer: null); + + var sink = new TestSink(); + var loggerFactory = new TestLoggerFactory(sink, enabled: true); + var parameterBinder = CreateParameterBinder(metadata, validator, loggerFactory: loggerFactory); + var modelBindingResult = ModelBindingResult.Success(null); + + // Act + var result = await parameterBinder.BindModelAsync( + actionContext, + CreateMockModelBinder(modelBindingResult), + CreateMockValueProvider(), + parameterDescriptor, + metadata, + "ignoredvalue"); + + // Assert + Assert.False(actionContext.ModelState.IsValid); + var modelState = Assert.Single(actionContext.ModelState); + Assert.Equal(expectedKey, modelState.Key); + var error = Assert.Single(modelState.Value.Errors); + Assert.Equal(attribute.FormatErrorMessage(expectedFieldName), error.ErrorMessage); + Assert.Equal(4, sink.Writes.Count); + } + [Fact] public async Task BindModelAsync_EnforcesTopLevelDataAnnotationsAttribute() { @@ -426,7 +530,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding private static ParameterBinder CreateParameterBinder( ModelMetadata modelMetadata, IModelValidator validator = null, - IOptions optionsAccessor = null) + IOptions optionsAccessor = null, + ILoggerFactory loggerFactory = null) { var mockModelMetadataProvider = new Mock(MockBehavior.Strict); mockModelMetadataProvider @@ -442,7 +547,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding mockModelMetadataProvider.Object, new[] { GetModelValidatorProvider(validator) }), optionsAccessor, - NullLoggerFactory.Instance); + loggerFactory ?? NullLoggerFactory.Instance); } private static IModelValidatorProvider GetModelValidatorProvider(IModelValidator validator = null) @@ -527,6 +632,15 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding } } + private class Family + { + public Person Dad { get; set; } + + public Person Mom { get; set; } + + public IList Kids { get; } = new List(); + } + public abstract class FakeModelMetadata : ModelMetadata { public FakeModelMetadata() diff --git a/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/DataAnnotationsModelValidatorTest.cs b/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/DataAnnotationsModelValidatorTest.cs index 8930283bce..17669fd0d4 100644 --- a/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/DataAnnotationsModelValidatorTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/DataAnnotationsModelValidatorTest.cs @@ -1,7 +1,6 @@ // 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.ComponentModel.DataAnnotations; using System.Linq; @@ -9,7 +8,6 @@ using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc.ModelBinding; using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.Internal; using Microsoft.Extensions.Localization; using Moq; using Xunit; @@ -18,7 +16,8 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal { public class DataAnnotationsModelValidatorTest { - private static IModelMetadataProvider _metadataProvider = TestModelMetadataProvider.CreateDefaultProvider(); + private static readonly ModelMetadataProvider _metadataProvider + = TestModelMetadataProvider.CreateDefaultProvider(); [Fact] public void Constructor_SetsAttribute() @@ -36,11 +35,15 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal Assert.Same(attribute, validator.Attribute); } - public static TheoryData Validate_SetsMemberName_AsExpectedData + public static TheoryData Validate_SetsMemberName_AsExpectedData { get { var array = new[] { new SampleModel { Name = "one" }, new SampleModel { Name = "two" } }; + var method = typeof(ModelValidationResultComparer).GetMethod( + nameof(ModelValidationResultComparer.GetHashCode), + new[] { typeof(ModelValidationResult) }); + var parameter = method.GetParameters()[0]; // GetHashCode(ModelValidationResult obj) // metadata, container, model, expected MemberName return new TheoryData @@ -52,7 +55,21 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal nameof(string.Length) }, { - // Validating a top-level model + // Validating a top-level property. + _metadataProvider.GetMetadataForProperty(typeof(SampleModel), nameof(SampleModel.Name)), + null, + "Fred", + nameof(SampleModel.Name) + }, + { + // Validating a parameter. + _metadataProvider.GetMetadataForParameter(parameter), + null, + new ModelValidationResult(memberName: string.Empty, message: string.Empty), + "obj" + }, + { + // Validating a top-level parameter as if using old-fashioned metadata provider. _metadataProvider.GetMetadataForType(typeof(SampleModel)), null, 15, @@ -542,39 +559,5 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal { void DoSomething(); } - - private class ModelValidationResultComparer : IEqualityComparer - { - public static readonly ModelValidationResultComparer Instance = new ModelValidationResultComparer(); - - private ModelValidationResultComparer() - { - } - - public bool Equals(ModelValidationResult x, ModelValidationResult y) - { - if (x == null || y == null) - { - return x == null && y == null; - } - - return string.Equals(x.MemberName, y.MemberName, StringComparison.Ordinal) && - string.Equals(x.Message, y.Message, StringComparison.Ordinal); - } - - public int GetHashCode(ModelValidationResult obj) - { - if (obj == null) - { - throw new ArgumentNullException(nameof(obj)); - } - - var hashCodeCombiner = HashCodeCombiner.Start(); - hashCodeCombiner.Add(obj.MemberName, StringComparer.Ordinal); - hashCodeCombiner.Add(obj.Message, StringComparer.Ordinal); - - return hashCodeCombiner.CombinedHash; - } - } } } diff --git a/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/ModelValidationResultComparer.cs b/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/ModelValidationResultComparer.cs new file mode 100644 index 0000000000..289f30ec35 --- /dev/null +++ b/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/ModelValidationResultComparer.cs @@ -0,0 +1,44 @@ +// 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 Microsoft.AspNetCore.Mvc.ModelBinding.Validation; +using Microsoft.Extensions.Internal; + +namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal +{ + public class ModelValidationResultComparer : IEqualityComparer + { + public static readonly ModelValidationResultComparer Instance = new ModelValidationResultComparer(); + + private ModelValidationResultComparer() + { + } + + public bool Equals(ModelValidationResult x, ModelValidationResult y) + { + if (x == null || y == null) + { + return x == null && y == null; + } + + return string.Equals(x.MemberName, y.MemberName, StringComparison.Ordinal) && + string.Equals(x.Message, y.Message, StringComparison.Ordinal); + } + + public int GetHashCode(ModelValidationResult obj) + { + if (obj == null) + { + throw new ArgumentNullException(nameof(obj)); + } + + var hashCodeCombiner = HashCodeCombiner.Start(); + hashCodeCombiner.Add(obj.MemberName, StringComparer.Ordinal); + hashCodeCombiner.Add(obj.Message, StringComparer.Ordinal); + + return hashCodeCombiner.CombinedHash; + } + } +} diff --git a/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/NumericClientModelValidatorTest.cs b/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/NumericClientModelValidatorTest.cs index 1921085893..71e0b6adfb 100644 --- a/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/NumericClientModelValidatorTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/NumericClientModelValidatorTest.cs @@ -70,7 +70,39 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal } [Fact] - public void AddValidation_CorrectValidationTypeAndOverriddenErrorMessage_WithNonProperty() + public void AddValidation_CorrectValidationTypeAndOverriddenErrorMessage_WithParameter() + { + // Arrange + var expectedMessage = "Error message about 'number' from override."; + + var method = typeof(TypeWithNumericProperty).GetMethod(nameof(TypeWithNumericProperty.IsLovely)); + var parameter = method.GetParameters()[0]; // IsLovely(double number) + var provider = new TestModelMetadataProvider(); + provider + .ForParameter(parameter) + .BindingDetails(d => + { + d.ModelBindingMessageProvider.SetValueMustBeANumberAccessor( + name => $"Error message about '{ name }' from override."); + }); + var metadata = provider.GetMetadataForParameter(parameter); + + var adapter = new NumericClientModelValidator(); + var actionContext = new ActionContext(); + var context = new ClientModelValidationContext(actionContext, metadata, provider, new AttributeDictionary()); + + // Act + adapter.AddValidation(context); + + // Assert + Assert.Collection( + context.Attributes, + kvp => { Assert.Equal("data-val", kvp.Key); Assert.Equal("true", kvp.Value); }, + kvp => { Assert.Equal("data-val-number", kvp.Key); Assert.Equal(expectedMessage, kvp.Value); }); + } + + [Fact] + public void AddValidation_CorrectValidationTypeAndOverriddenErrorMessage_WithType() { // Arrange var expectedMessage = "Error message from override."; @@ -125,6 +157,11 @@ namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal { [Display(Name = "DisplayId")] public float Id { get; set; } + + public bool IsLovely(double number) + { + return true; + } } } } diff --git a/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/ValidatableObjectAdapterTest.cs b/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/ValidatableObjectAdapterTest.cs new file mode 100644 index 0000000000..56a127ea25 --- /dev/null +++ b/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test/Internal/ValidatableObjectAdapterTest.cs @@ -0,0 +1,221 @@ +// 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 Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.AspNetCore.Mvc.ModelBinding.Validation; +using Xunit; + +namespace Microsoft.AspNetCore.Mvc.DataAnnotations.Internal +{ + public class ValidatableObjectAdapterTest + { + private static readonly ModelMetadataProvider _metadataProvider + = TestModelMetadataProvider.CreateDefaultProvider(); + + // Inspired by DataAnnotationsModelValidatorTest.Validate_SetsMemberName_AsExpectedData but using a type that + // implements IValidatableObject. Values are metadata, expected DisplayName, and expected MemberName. + public static TheoryData Validate_PassesExpectedNamesData + { + get + { + var method = typeof(SampleModel).GetMethod(nameof(SampleModel.IsLovely)); + var parameter = method.GetParameters()[0]; // IsLovely(SampleModel other) + return new TheoryData + { + { + // Validating a property. + _metadataProvider.GetMetadataForProperty( + typeof(SampleModelContainer), + nameof(SampleModelContainer.SampleModel)), + nameof(SampleModelContainer.SampleModel), + nameof(SampleModelContainer.SampleModel) + }, + { + // Validating a property with [Display(Name = "...")]. + _metadataProvider.GetMetadataForProperty( + typeof(SampleModelContainer), + nameof(SampleModelContainer.SampleModelWithDisplay)), + "sample model", + nameof(SampleModelContainer.SampleModelWithDisplay) + }, + { + // Validating a parameter. + _metadataProvider.GetMetadataForParameter(parameter), + "other", + "other" + }, + { + // Validating a top-level parameter when using old-fashioned metadata provider. + // Or, validating an element of a collection. + _metadataProvider.GetMetadataForType(typeof(SampleModel)), + nameof(SampleModel), + null + }, + }; + } + } + + public static TheoryData Validate_ReturnsExpectedResultsData + { + get + { + return new TheoryData + { + { + new[] { new ValidationResult("Error message") }, + new[] { new ModelValidationResult(memberName: null, message: "Error message") } + }, + { + new[] { new ValidationResult("Error message", new[] { nameof(SampleModel.FirstName) }) }, + new[] { new ModelValidationResult(nameof(SampleModel.FirstName), "Error message") } + }, + { + new[] + { + new ValidationResult("Error message1"), + new ValidationResult("Error message2", new[] { nameof(SampleModel.FirstName) }), + new ValidationResult("Error message3", new[] { nameof(SampleModel.LastName) }), + new ValidationResult("Error message4", new[] { nameof(SampleModel) }), + }, + new[] + { + new ModelValidationResult(memberName: null, message: "Error message1"), + new ModelValidationResult(nameof(SampleModel.FirstName), "Error message2"), + new ModelValidationResult(nameof(SampleModel.LastName), "Error message3"), + // No special case for ValidationContext.MemberName==ValidationResult.MemberName + new ModelValidationResult(nameof(SampleModel), "Error message4"), + } + }, + { + new[] + { + new ValidationResult("Error message1", new[] + { + nameof(SampleModel.FirstName), + nameof(SampleModel.LastName), + }), + new ValidationResult("Error message2"), + }, + new[] + { + new ModelValidationResult(nameof(SampleModel.FirstName), "Error message1"), + new ModelValidationResult(nameof(SampleModel.LastName), "Error message1"), + new ModelValidationResult(memberName: null, message: "Error message2"), + } + }, + }; + } + } + + + [Theory] + [MemberData(nameof(Validate_PassesExpectedNamesData))] + public void Validate_PassesExpectedNames( + ModelMetadata metadata, + string expectedDisplayName, + string expectedMemberName) + { + // Arrange + var adapter = new ValidatableObjectAdapter(); + var model = new SampleModel(); + var validationContext = new ModelValidationContext( + new ActionContext(), + metadata, + _metadataProvider, + container: new SampleModelContainer(), + model: model); + + // Act + var results = adapter.Validate(validationContext); + + // Assert + Assert.NotNull(results); + Assert.Empty(results); + + Assert.Equal(expectedDisplayName, model.DisplayName); + Assert.Equal(expectedMemberName, model.MemberName); + Assert.Equal(model, model.ObjectInstance); + } + + [Theory] + [MemberData(nameof(Validate_ReturnsExpectedResultsData))] + public void Validate_ReturnsExpectedResults( + ValidationResult[] innerResults, + ModelValidationResult[] expectedResults) + { + // Arrange + var adapter = new ValidatableObjectAdapter(); + var model = new SampleModel(); + foreach (var result in innerResults) + { + model.ValidationResults.Add(result); + } + + var metadata = _metadataProvider.GetMetadataForProperty( + typeof(SampleModelContainer), + nameof(SampleModelContainer.SampleModel)); + var validationContext = new ModelValidationContext( + new ActionContext(), + metadata, + _metadataProvider, + container: null, + model: model); + + // Act + var results = adapter.Validate(validationContext); + + // Assert + Assert.NotNull(results); + Assert.Equal(expectedResults, results, ModelValidationResultComparer.Instance); + } + + private class SampleModel : IValidatableObject + { + // "Real" properties. + + public string FirstName { get; set; } + + public string LastName { get; set; } + + // ValidationContext members passed to Validate(...) + + public string DisplayName { get; private set; } + + public string MemberName { get; private set; } + + public object ObjectInstance { get; private set; } + + // What Validate(...) should return. + + public IList ValidationResults { get; } = new List(); + + // Test method. + + public bool IsLovely(SampleModel other) + { + return true; + } + + // IValidatableObject for realz. + + public IEnumerable Validate(ValidationContext validationContext) + { + DisplayName = validationContext.DisplayName; + MemberName = validationContext.MemberName; + ObjectInstance = validationContext.ObjectInstance; + + return ValidationResults; + } + } + + private class SampleModelContainer + { + [Display(Name = "sample model")] + public SampleModel SampleModelWithDisplay { get; set; } + + public SampleModel SampleModel { get; set; } + } + } +} diff --git a/test/Microsoft.AspNetCore.Mvc.TestCommon/TestModelMetadataProvider.cs b/test/Microsoft.AspNetCore.Mvc.TestCommon/TestModelMetadataProvider.cs index fd743e60e4..62952ab4c6 100644 --- a/test/Microsoft.AspNetCore.Mvc.TestCommon/TestModelMetadataProvider.cs +++ b/test/Microsoft.AspNetCore.Mvc.TestCommon/TestModelMetadataProvider.cs @@ -112,6 +112,15 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding return builder; } + public IMetadataBuilder ForParameter(ParameterInfo parameter) + { + var key = ModelMetadataIdentity.ForParameter(parameter); + var builder = new MetadataBuilder(key); + _detailsProvider.Builders.Add(builder); + + return builder; + } + public IMetadataBuilder ForProperty(string propertyName) { return ForProperty(typeof(TContainer), propertyName); @@ -223,4 +232,4 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding } } } -} \ No newline at end of file +} From d360886b788421a1375670c2aec433787524f716 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Wed, 28 Mar 2018 20:21:25 -0700 Subject: [PATCH 2/2] Fix #7558 infer [FromRoute] with parameter in ANY route This changes the logic for when we infer [FromRoute] on an action parameter from *ALL* to *ANY*. This means that if a parameter occurs in any route on an ApiController, we will treat it as [FromRoute]. We think this is the best decision because it's less ambiguous. If a parameter appears in a route, it won't be eligible to be bound from query. I think that's good. If for some reason you want this kind of behavior (route or query) then we suggest breaking up the actions. This isn't very documentation friendly (swagger) so we don't suggest it. --- .../ApiBehaviorApplicationModelProvider.cs | 14 +++++--------- .../ApiBehaviorApplicationModelProviderTest.cs | 10 +++++----- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs index e91b46c995..7bc6c6892d 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ApiBehaviorApplicationModelProvider.cs @@ -230,7 +230,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal // Internal for unit testing. internal BindingSource InferBindingSourceForParameter(ParameterModel parameter) { - if (ParameterExistsInAllRoutes(parameter.Action, parameter.ParameterName)) + if (ParameterExistsInAnyRoute(parameter.Action, parameter.ParameterName)) { return BindingSource.Path; } @@ -242,9 +242,8 @@ namespace Microsoft.AspNetCore.Mvc.Internal return bindingSource; } - private bool ParameterExistsInAllRoutes(ActionModel actionModel, string parameterName) + private bool ParameterExistsInAnyRoute(ActionModel actionModel, string parameterName) { - var parameterExistsInSomeRoute = false; foreach (var (route, _, _) in ActionAttributeRouteModel.GetAttributeRoutes(actionModel)) { if (route == null) @@ -253,16 +252,13 @@ namespace Microsoft.AspNetCore.Mvc.Internal } var parsedTemplate = TemplateParser.Parse(route.Template); - if (parsedTemplate.GetParameter(parameterName) == null) + if (parsedTemplate.GetParameter(parameterName) != null) { - return false; + return true; } - - // Ensure at least one route exists. - parameterExistsInSomeRoute = true; } - return parameterExistsInSomeRoute; + return false; } private bool IsComplexTypeParameter(ParameterModel parameter) diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorApplicationModelProviderTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorApplicationModelProviderTest.cs index 935e0094e9..a9a01f5aba 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorApplicationModelProviderTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ApiBehaviorApplicationModelProviderTest.cs @@ -219,7 +219,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal } [Fact] - public void InferBindingSourceForParameter_ReturnsPath_IfParameterAppearsInAllRoutes() + public void InferBindingSourceForParameter_ReturnsPath_IfParameterAppearsInAnyRoutes_MulitpleRoutes() { // Arrange var actionName = nameof(ParameterBindingController.ParameterInMultipleRoutes); @@ -234,7 +234,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal } [Fact] - public void InferBindingSourceForParameter_DoesNotReturnPath_IfParameterDoesNotAppearInAllRoutes() + public void InferBindingSourceForParameter_ReturnsPath_IfParameterAppearsInAnyRoute() { // Arrange var actionName = nameof(ParameterBindingController.ParameterNotInAllRoutes); @@ -245,7 +245,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal var result = provider.InferBindingSourceForParameter(parameter); // Assert - Assert.Same(BindingSource.Query, result); + Assert.Same(BindingSource.Path, result); } [Fact] @@ -309,7 +309,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal } [Fact] - public void InferBindingSourceForParameter_DoesNotReturnPath_IfOneActionRouteOverridesControllerRoute() + public void InferBindingSourceForParameter_ReturnsPath_IfParameterPresentInNonOverriddenControllerRoute() { // Arrange var actionName = nameof(ParameterInController.MultipleRouteWithOverride); @@ -320,7 +320,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal var result = provider.InferBindingSourceForParameter(parameter); // Assert - Assert.Same(BindingSource.Query, result); + Assert.Same(BindingSource.Path, result); } [Fact]